[PATCH v4 0/3] target/i386: Restrict system-specific features from user emulation

Philippe Mathieu-Daudé posted 3 patches 7 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230911211317.28773-1-philmd@linaro.org
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>
There is a newer version of this series
target/i386/kvm/kvm_i386.h |  4 +++
target/i386/cpu.c          | 58 +++++++++++++++++++++++++++++++++-----
2 files changed, 55 insertions(+), 7 deletions(-)
[PATCH v4 0/3] target/i386: Restrict system-specific features from user emulation
Posted by Philippe Mathieu-Daudé 7 months, 2 weeks ago
Too many system-specific code (and in particular KVM related)
is pulled in user-only build. This led to adding unjustified
stubs as kludge to unagressive linker non-optimizations.

This series restrict x86 system-specific features to sysemu,
so we don't require any stub, and remove all x86 KVM declarations
from user emulation code (to trigger compile failure instead of
link one).

Philippe Mathieu-Daudé (3):
  target/i386: Check kvm_hyperv_expand_features() return value
  RFC target/i386: Restrict system-specific features from user emulation
  target/i386: Prohibit target specific KVM prototypes on user emulation

 target/i386/kvm/kvm_i386.h |  4 +++
 target/i386/cpu.c          | 58 +++++++++++++++++++++++++++++++++-----
 2 files changed, 55 insertions(+), 7 deletions(-)

-- 
2.41.0


Re: [PATCH v4 0/3] target/i386: Restrict system-specific features from user emulation
Posted by Paolo Bonzini 7 months, 2 weeks ago
On 9/11/23 23:13, Philippe Mathieu-Daudé wrote:
> Too many system-specific code (and in particular KVM related)
> is pulled in user-only build. This led to adding unjustified
> stubs as kludge to unagressive linker non-optimizations.
> 
> This series restrict x86 system-specific features to sysemu,
> so we don't require any stub, and remove all x86 KVM declarations
> from user emulation code (to trigger compile failure instead of
> link one).
> 
> Philippe Mathieu-Daudé (3):
>    target/i386: Check kvm_hyperv_expand_features() return value
>    RFC target/i386: Restrict system-specific features from user emulation
>    target/i386: Prohibit target specific KVM prototypes on user emulation

At least, patch 2 should be changed so that the #ifdef'ery is done at a 
higher level.

However, the dependency of user-mode emulation on KVM is really an 
implementation detail of QEMU.  It's very much baked into linux-user and 
hard to remove, but I'm not sure it's a good idea to add more #ifdef 
CONFIG_USER_ONLY around KVM code.

Paolo


Re: [PATCH v4 0/3] target/i386: Restrict system-specific features from user emulation
Posted by Philippe Mathieu-Daudé 7 months, 2 weeks ago
On 12/9/23 16:07, Paolo Bonzini wrote:
> On 9/11/23 23:13, Philippe Mathieu-Daudé wrote:
>> Too many system-specific code (and in particular KVM related)
>> is pulled in user-only build. This led to adding unjustified
>> stubs as kludge to unagressive linker non-optimizations.
>>
>> This series restrict x86 system-specific features to sysemu,
>> so we don't require any stub, and remove all x86 KVM declarations
>> from user emulation code (to trigger compile failure instead of
>> link one).
>>
>> Philippe Mathieu-Daudé (3):
>>    target/i386: Check kvm_hyperv_expand_features() return value
>>    RFC target/i386: Restrict system-specific features from user emulation
>>    target/i386: Prohibit target specific KVM prototypes on user emulation
> 
> At least, patch 2 should be changed so that the #ifdef'ery is done at a 
> higher level.

I can try to improve it with your comments, but I have no idea of
x86 CPU features.

> However, the dependency of user-mode emulation on KVM is really an 
> implementation detail of QEMU.  It's very much baked into linux-user and 
> hard to remove, but I'm not sure it's a good idea to add more #ifdef 
> CONFIG_USER_ONLY around KVM code.

Do you rather v3 then?

https://lore.kernel.org/qemu-devel/20230911142729.25548-1-philmd@linaro.org/


Re: [PATCH v4 0/3] target/i386: Restrict system-specific features from user emulation
Posted by Paolo Bonzini 7 months, 2 weeks ago
Il mar 12 set 2023, 18:44 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:

> I can try to improve it with your comments, but I have no idea of
> x86 CPU features.
>

I will take a look.

> However, the dependency of user-mode emulation on KVM is really an
> > implementation detail of QEMU.  It's very much baked into linux-user and
> > hard to remove, but I'm not sure it's a good idea to add more #ifdef
> > CONFIG_USER_ONLY around KVM code.
>
> Do you rather v3 then?
>
>
> https://lore.kernel.org/qemu-devel/20230911142729.25548-1-philmd@linaro.org/


No, if we want a small patch it is better to replace kvm_enabled() with
CONFIG_KVM, and also follow Kevin's suggestion to make it fail at compile
time.

Having stub prototypes was done because we expected the compiler to remove
the dead code.

Paolo
Re: [PATCH v4 0/3] target/i386: Restrict system-specific features from user emulation
Posted by Philippe Mathieu-Daudé 7 months, 2 weeks ago
On 12/9/23 19:25, Paolo Bonzini wrote:

>      > However, the dependency of user-mode emulation on KVM is really an
>      > implementation detail of QEMU.  It's very much baked into
>     linux-user and
>      > hard to remove, but I'm not sure it's a good idea to add more #ifdef
>      > CONFIG_USER_ONLY around KVM code.
> 
>     Do you rather v3 then?
> 
>     https://lore.kernel.org/qemu-devel/20230911142729.25548-1-philmd@linaro.org/ <https://lore.kernel.org/qemu-devel/20230911142729.25548-1-philmd@linaro.org/>
> 
> 
> No, if we want a small patch it is better to replace kvm_enabled() with 
> CONFIG_KVM, and also follow Kevin's suggestion to make it fail at 
> compile time.

For target common code (shared between user/system), CONFIG_KVM is not
a replacement for !CONFIG_USER_ONLY, since it can be always enabled;
which is why we defer to runtime check with kvm_enabled().

> Having stub prototypes was done because we expected the compiler to 
> remove the dead code.
> 
> Paolo