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

Message

Philippe Mathieu-Daudé Sept. 11, 2023, 9:13 p.m. UTC

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(-)

Comments

Paolo Bonzini Sept. 12, 2023, 2:07 p.m. UTC | #1

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

Philippe Mathieu-Daudé Sept. 12, 2023, 4:44 p.m. UTC | #2

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/[e-mail]/

Philippe Mathieu-Daudé Sept. 13, 2023, 9:26 a.m. UTC | #3

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/[e-mail]/ <https://lore.kernel.org/qemu-devel/[e-mail]/>
> 
> 
> 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