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

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

[Philippe Mathieu-Daudé]

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

[PATCH v4 1/3] target/i386: Check kvm_hyperv_expand_features() return value

[Philippe Mathieu-Daudé]

Move kvm_hyperv_expand_features() call earlier (this will simplify
reviewing the next commit) and check its return value, since it can
fail.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 24ee67b42d..760428d4dc 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7107,6 +7107,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         }
     }
 
+    if (kvm_enabled() && !kvm_hyperv_expand_features(cpu, errp)) {
+        return;
+    }
+
     /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
     if (env->cpuid_level_func7 == UINT32_MAX) {
         env->cpuid_level_func7 = env->cpuid_min_level_func7;
@@ -7120,10 +7124,6 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
     if (env->cpuid_xlevel2 == UINT32_MAX) {
         env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
     }
-
-    if (kvm_enabled()) {
-        kvm_hyperv_expand_features(cpu, errp);
-    }
 }
 
 /*
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

Since commits 3adce820cf ("target/i386: Remove unused KVM
stubs") and ef1cf6890f ("target/i386: Allow elision of
kvm_hv_vpindex_settable()"), when building on a x86 host
configured as:

  $ ./configure --cc=clang \
    --target-list=x86_64-linux-user,x86_64-softmmu \
    --enable-debug

we get:

  [71/71] Linking target qemu-x86_64
  FAILED: qemu-x86_64
  /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `cpu_x86_cpuid':
  cpu.c:(.text+0x1374): undefined reference to `kvm_arch_get_supported_cpuid'
  /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `x86_cpu_filter_features':
  cpu.c:(.text+0x81c2): undefined reference to `kvm_arch_get_supported_cpuid'
  /usr/bin/ld: cpu.c:(.text+0x81da): undefined reference to `kvm_arch_get_supported_cpuid'
  /usr/bin/ld: cpu.c:(.text+0x81f2): undefined reference to `kvm_arch_get_supported_cpuid'
  /usr/bin/ld: cpu.c:(.text+0x820a): undefined reference to `kvm_arch_get_supported_cpuid'
  /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o:cpu.c:(.text+0x8225): more undefined references to `kvm_arch_get_supported_cpuid' follow
  clang: error: linker command failed with exit code 1 (use -v to see invocation)
  ninja: build stopped: subcommand failed.

libqemu-x86_64-linux-user.fa is user emulation specific, so
having system emulation code called there is dubious.

'--enable-debug' disables optimizations (CFLAGS=-O0).

While at this (un)optimization level GCC eliminate the
following dead code (CPP output of mentioned build):

 static void x86_cpu_get_supported_cpuid(uint32_t func, uint32_t index,
                                         uint32_t *eax, uint32_t *ebx,
                                         uint32_t *ecx, uint32_t *edx)
 {
     if ((0)) {
         *eax = kvm_arch_get_supported_cpuid(kvm_state, func, index, R_EAX);
         *ebx = kvm_arch_get_supported_cpuid(kvm_state, func, index, R_EBX);
         *ecx = kvm_arch_get_supported_cpuid(kvm_state, func, index, R_ECX);
         *edx = kvm_arch_get_supported_cpuid(kvm_state, func, index, R_EDX);
     } else if (0) {
         *eax = 0;
         *ebx = 0;
         *ecx = 0;
         *edx = 0;
     } else {
         *eax = 0;
         *ebx = 0;
         *ecx = 0;
         *edx = 0;
     }
 }

Clang does not.

Instead of trying to deal with compiler specific checks around
__OPTIMIZE__ (see commit 2140cfa51d "i386: Fix build by providing
stub kvm_arch_get_supported_cpuid()"), simply restrict code
belonging to system emulation, easing user emulation linking.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: I might have been overzealous in x86_cpu_expand_features(),
please double check.
---
 target/i386/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 760428d4dc..8b57708604 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1664,6 +1664,7 @@ static inline uint64_t x86_cpu_xsave_xss_components(X86CPU *cpu)
            cpu->env.features[FEAT_XSAVE_XSS_LO];
 }
 
+#ifndef CONFIG_USER_ONLY
 /*
  * Returns the set of feature flags that are supported and migratable by
  * QEMU, for a given FeatureWord.
@@ -1686,6 +1687,7 @@ static uint64_t x86_cpu_get_migratable_flags(FeatureWord w)
     }
     return r;
 }
+#endif /* !CONFIG_USER_ONLY */
 
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
@@ -5677,8 +5679,6 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-#endif /* !CONFIG_USER_ONLY */
-
 uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                             bool migratable_only)
 {
@@ -5781,6 +5781,38 @@ static void x86_cpu_get_cache_cpuid(uint32_t func, uint32_t index,
     }
 }
 
+#else /* CONFIG_USER_ONLY */
+
+uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
+                                            bool migratable_only)
+{
+    FeatureWordInfo *wi = &feature_word_info[w];
+
+    return wi->tcg_features;
+}
+
+static void x86_cpu_get_supported_cpuid(uint32_t func, uint32_t index,
+                                        uint32_t *eax, uint32_t *ebx,
+                                        uint32_t *ecx, uint32_t *edx)
+{
+    *eax = 0;
+    *ebx = 0;
+    *ecx = 0;
+    *edx = 0;
+}
+
+static void x86_cpu_get_cache_cpuid(uint32_t func, uint32_t index,
+                                    uint32_t *eax, uint32_t *ebx,
+                                    uint32_t *ecx, uint32_t *edx)
+{
+    *eax = 0;
+    *ebx = 0;
+    *ecx = 0;
+    *edx = 0;
+}
+
+#endif /* !CONFIG_USER_ONLY */
+
 /*
  * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
  */
@@ -6163,6 +6195,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             }
             *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
 
+#ifndef CONFIG_USER_ONLY
             /*
              * SGX cannot be emulated in software.  If hardware does not
              * support enabling SGX and/or SGX flexible launch control,
@@ -6181,6 +6214,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                     CPUID_7_0_ECX_SGX_LC))) {
                 *ecx &= ~CPUID_7_0_ECX_SGX_LC;
             }
+#endif
         } else if (count == 1) {
             *eax = env->features[FEAT_7_1_EAX];
             *edx = env->features[FEAT_7_1_EDX];
@@ -6876,6 +6910,8 @@ static void mce_init(X86CPU *cpu)
     }
 }
 
+#ifndef CONFIG_USER_ONLY
+
 static void x86_cpu_adjust_level(X86CPU *cpu, uint32_t *min, uint32_t value)
 {
     if (*min < value) {
@@ -6948,6 +6984,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
     env->features[FEAT_XSAVE_XSS_HI] = mask >> 32;
 }
 
+#endif /* !CONFIG_USER_ONLY */
+
 /***** Steps involved on loading and filtering CPUID data
  *
  * When initializing and realizing a CPU object, the steps
@@ -7040,6 +7078,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         }
     }
 
+#ifndef CONFIG_USER_ONLY
     if (!kvm_enabled() || !cpu->expose_kvm) {
         env->features[FEAT_KVM] = 0;
     }
@@ -7111,6 +7150,8 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         return;
     }
 
+#endif /* !CONFIG_USER_ONLY */
+
     /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
     if (env->cpuid_level_func7 == UINT32_MAX) {
         env->cpuid_level_func7 = env->cpuid_min_level_func7;
@@ -7152,6 +7193,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
         mark_unavailable_features(cpu, w, unavailable_features, prefix);
     }
 
+#ifndef CONFIG_USER_ONLY
     if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
         kvm_enabled()) {
         KVMState *s = CPU(cpu)->kvm_state;
@@ -7179,6 +7221,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
             mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
         }
     }
+#endif
 }
 
 static void x86_cpu_hyperv_realize(X86CPU *cpu)
-- 
2.41.0

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

[Paolo Bonzini]

On 9/11/23 23:13, Philippe Mathieu-Daudé wrote:
>   /*
>    * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
>    */
> @@ -6163,6 +6195,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>               }
>               *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
>   
> +#ifndef CONFIG_USER_ONLY
>               /*
>                * SGX cannot be emulated in software.  If hardware does not
>                * support enabling SGX and/or SGX flexible launch control,
> @@ -6181,6 +6214,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                       CPUID_7_0_ECX_SGX_LC))) {
>                   *ecx &= ~CPUID_7_0_ECX_SGX_LC;
>               }
> +#endif

This can use a variant of x86_cpu_get_supported_cpuid that returns a 
single register; or it can be rewritten to use x86_cpu_get_supported_cpuid.

In general, a lot of checks for accel_uses_host_cpuid() are unnecessary, 
and the code can be modified to not depend on either KVM or HVF.

>           } else if (count == 1) {
>               *eax = env->features[FEAT_7_1_EAX];
>               *edx = env->features[FEAT_7_1_EDX];
> @@ -6876,6 +6910,8 @@ static void mce_init(X86CPU *cpu)
>       }
>   }
>   
> +#ifndef CONFIG_USER_ONLY
> +
>   static void x86_cpu_adjust_level(X86CPU *cpu, uint32_t *min, uint32_t value)
>   {
>       if (*min < value) {
> @@ -6948,6 +6984,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>       env->features[FEAT_XSAVE_XSS_HI] = mask >> 32;
>   }
>   
> +#endif /* !CONFIG_USER_ONLY */

These functions should all be used in user-mode emulation as well.

>   /***** Steps involved on loading and filtering CPUID data
>    *
>    * When initializing and realizing a CPU object, the steps
> @@ -7040,6 +7078,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>           }
>       }
>   
> +#ifndef CONFIG_USER_ONLY
>       if (!kvm_enabled() || !cpu->expose_kvm) {
>           env->features[FEAT_KVM] = 0;
>       }

This is "!kvm_enabled()" so it should be kept for user-mode emulation.

> @@ -7111,6 +7150,8 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>           return;
>       }
>   
> +#endif /* !CONFIG_USER_ONLY */
>       /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
>       if (env->cpuid_level_func7 == UINT32_MAX) {
>           env->cpuid_level_func7 = env->cpuid_min_level_func7;
> @@ -7152,6 +7193,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>           mark_unavailable_features(cpu, w, unavailable_features, prefix);
>       }
>   
> +#ifndef CONFIG_USER_ONLY
>       if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
>           kvm_enabled()) {
>           KVMState *s = CPU(cpu)->kvm_state;
> @@ -7179,6 +7221,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>               mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
>           }
>       }
> +#endif

This need not be limited to KVM, it can likewise use 
x86_cpu_get_supported_cpuid.

Paolo

>   }
>   
>   static void x86_cpu_hyperv_realize(X86CPU *cpu)

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

[Philippe Mathieu-Daudé]

On 12/9/23 16:05, Paolo Bonzini wrote:
> On 9/11/23 23:13, Philippe Mathieu-Daudé wrote:
>>   /*
>>    * Only for builtin_x86_defs models initialized with 
>> x86_register_cpudef_types.
>>    */
>> @@ -6163,6 +6195,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>> index, uint32_t count,
>>               }
>>               *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
>> +#ifndef CONFIG_USER_ONLY
>>               /*
>>                * SGX cannot be emulated in software.  If hardware does 
>> not
>>                * support enabling SGX and/or SGX flexible launch control,
>> @@ -6181,6 +6214,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>> index, uint32_t count,
>>                       CPUID_7_0_ECX_SGX_LC))) {
>>                   *ecx &= ~CPUID_7_0_ECX_SGX_LC;
>>               }
>> +#endif
> 
> This can use a variant of x86_cpu_get_supported_cpuid that returns a 
> single register; or it can be rewritten to use x86_cpu_get_supported_cpuid.

Great suggestion, thanks!

> In general, a lot of checks for accel_uses_host_cpuid() are unnecessary, 
> and the code can be modified to not depend on either KVM or HVF.

OK.

[PATCH v4 3/3] target/i386: Prohibit target specific KVM prototypes on user emulation

[Philippe Mathieu-Daudé]

None of these target-specific prototypes should be used
by user emulation. Remove their declaration there, so we
get a compile failure if ever used (instead of having to
deal with linker and its possible optimizations, such
dead code removal).

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/kvm/kvm_i386.h | 4 ++++
 target/i386/cpu.c          | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
index 55d4e68c34..5ef73f0a1c 100644
--- a/target/i386/kvm/kvm_i386.h
+++ b/target/i386/kvm/kvm_i386.h
@@ -13,6 +13,10 @@
 
 #include "sysemu/kvm.h"
 
+#ifdef CONFIG_USER_ONLY
+#error Cannot include kvm_i386.h from user emulation
+#endif
+
 #ifdef CONFIG_KVM
 
 #define kvm_pit_in_kernel() \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 8b57708604..6be59c7b2b 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -26,7 +26,7 @@
 #include "tcg/helper-tcg.h"
 #include "sysemu/reset.h"
 #include "sysemu/hvf.h"
-#include "kvm/kvm_i386.h"
+#include "sysemu/kvm.h"
 #include "sev.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -40,6 +40,7 @@
 #include "exec/address-spaces.h"
 #include "hw/boards.h"
 #include "hw/i386/sgx-epc.h"
+#include "kvm/kvm_i386.h"
 #endif
 
 #include "disas/capstone.h"
-- 
2.41.0

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

[Paolo Bonzini]

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

[Philippe Mathieu-Daudé]

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 1655 bytes --]

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

[Philippe Mathieu-Daudé]

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