[PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor

Paolo Bonzini posted 4 patches 5 months, 1 week ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Zhao Liu <zhao1.liu@intel.com>, Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Marcelo Tosatti <mtosatti@redhat.com>, Reinoud Zandijk <reinoud@netbsd.org>, Sunil Muthuswamy <sunilmut@microsoft.com>
[PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Paolo Bonzini 5 months, 1 week ago
NVMM and WHPX are virtualizers, and therefore they need to use
(at least by default) the host vendor for the guest CPUID.
Add a cpu_instance_init implementation to these accelerators.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.c           |  8 +++++++-
 target/i386/nvmm/nvmm-all.c | 23 +++++++++++++++++++++++
 target/i386/whpx/whpx-all.c | 23 +++++++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 624cebc3ff7..69bdffbfe46 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -43,6 +43,7 @@
 #include "hw/boards.h"
 #include "hw/i386/sgx-epc.h"
 #endif
+#include "system/qtest.h"
 #include "tcg/tcg-cpu.h"
 
 #include "disas/capstone.h"
@@ -1943,7 +1944,7 @@ uint32_t xsave_area_size(uint64_t mask, bool compacted)
 
 static inline bool accel_uses_host_cpuid(void)
 {
-    return kvm_enabled() || hvf_enabled();
+    return !tcg_enabled() && !qtest_enabled();
 }
 
 static inline uint64_t x86_cpu_xsave_xcr0_components(X86CPU *cpu)
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index b4a4d50e860..69125230abb 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -1207,10 +1207,33 @@ static const TypeInfo nvmm_accel_type = {
     .class_init = nvmm_accel_class_init,
 };
 
+static void nvmm_cpu_instance_init(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    host_cpu_instance_init(cpu);
+}
+
+static void nvmm_cpu_accel_class_init(ObjectClass *oc, const void *data)
+{
+    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
+
+    acc->cpu_instance_init = nvmm_cpu_instance_init;
+}
+
+static const TypeInfo nvmm_cpu_accel_type = {
+    .name = ACCEL_CPU_NAME("nvmm"),
+
+    .parent = TYPE_ACCEL_CPU,
+    .class_init = nvmm_cpu_accel_class_init,
+    .abstract = true,
+};
+
 static void
 nvmm_type_init(void)
 {
     type_register_static(&nvmm_accel_type);
+    type_register_static(&nvmm_cpu_accel_type);
 }
 
 type_init(nvmm_type_init);
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index faf56e19722..b380459d6fe 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -2500,6 +2500,28 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
     }
 }
 
+static void whpx_cpu_instance_init(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    host_cpu_instance_init(cpu);
+}
+
+static void whpx_cpu_accel_class_init(ObjectClass *oc, const void *data)
+{
+    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
+
+    acc->cpu_instance_init = whpx_cpu_instance_init;
+}
+
+static const TypeInfo whpx_cpu_accel_type = {
+    .name = ACCEL_CPU_NAME("whpx"),
+
+    .parent = TYPE_ACCEL_CPU,
+    .class_init = whpx_cpu_accel_class_init,
+    .abstract = true,
+};
+
 /*
  * Partition support
  */
@@ -2726,6 +2748,7 @@ static const TypeInfo whpx_accel_type = {
 static void whpx_type_init(void)
 {
     type_register_static(&whpx_accel_type);
+    type_register_static(&whpx_cpu_accel_type);
 }
 
 bool init_whp_dispatch(void)
-- 
2.50.0
Re: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Zhao Liu 5 months, 1 week ago
On Fri, Jul 11, 2025 at 02:06:01AM +0200, Paolo Bonzini wrote:
> Date: Fri, 11 Jul 2025 02:06:01 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets
>  host vendor
> X-Mailer: git-send-email 2.50.0
> 
> NVMM and WHPX are virtualizers, and therefore they need to use
> (at least by default) the host vendor for the guest CPUID.

Here's a comment of about why the vendor needs to be overridden in KVM:

(in x86_cpu_load_model())

    /* sysenter isn't supported in compatibility mode on AMD,
     * syscall isn't supported in compatibility mode on Intel.
     * Normally we advertise the actual CPU vendor, but you can
     * override this using the 'vendor' property if you want to use
     * KVM's sysenter/syscall emulation in compatibility mode and
     * when doing cross vendor migration
     */

This is a KVM default-vendor hack since the 1st KVM commit [*].

I guess that this hack might have been related to the immaturity of
vDSO at the time (it's been so long, I just took a quick look at the
general time, maybe linux v2.6), or just to reduce overhead.

Now, both KVM's emulation and vDSO seem to be quite stable. Do you
think QEMU KVM still needs to keep this hack today?

Maybe it's difficult to change for QEMU KVM because it's been a
long-time practice, but other accels don't seem to need to inherit KVM's
history. What do you think?

[*]: 1201818980-27534-7-git-send-email-aliguori@us.ibm.com

> Add a cpu_instance_init implementation to these accelerators.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c           |  8 +++++++-
>  target/i386/nvmm/nvmm-all.c | 23 +++++++++++++++++++++++
>  target/i386/whpx/whpx-all.c | 23 +++++++++++++++++++++++
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 624cebc3ff7..69bdffbfe46 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -43,6 +43,7 @@
>  #include "hw/boards.h"
>  #include "hw/i386/sgx-epc.h"
>  #endif
> +#include "system/qtest.h"
>  #include "tcg/tcg-cpu.h"
>  
>  #include "disas/capstone.h"
> @@ -1943,7 +1944,7 @@ uint32_t xsave_area_size(uint64_t mask, bool compacted)
>  
>  static inline bool accel_uses_host_cpuid(void)
>  {
> -    return kvm_enabled() || hvf_enabled();
> +    return !tcg_enabled() && !qtest_enabled();
>  }

I was considerreing whether we could check this helper and call
host_cpu_instance_init(cpu) directly in x86_cpu_load_model().

However, this goes against the original intent of moving this hack to
the KVM-specific code. But when it can cover almost all accels, it
becomes a general case...

...

So in summary, the benefits of having all accels override the vendor now
include:

the behavior of -cpu NAMED_CPU is consistent across all accels (except
TCG), and all showing the same vendor as the Host.

The possible issue would be: 
 * This changes the previous behavior of these accels, which might not
   have required setting the vendor before, but now the vendor has
   changed... (I'm unsure if these accels are used in migration
   scenarios, but it's better to add a compat option?)
 * expand the scope of historical KVM hack (if it's still a hack?)

Thanks,
Zhao
Re: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Xiaoyao Li 5 months, 1 week ago
On 7/11/2025 8:06 AM, Paolo Bonzini wrote:
> NVMM and WHPX are virtualizers, and therefore they need to use
> (at least by default) the host vendor for the guest CPUID.
> Add a cpu_instance_init implementation to these accelerators.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/cpu.c           |  8 +++++++-
>   target/i386/nvmm/nvmm-all.c | 23 +++++++++++++++++++++++
>   target/i386/whpx/whpx-all.c | 23 +++++++++++++++++++++++
>   3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 624cebc3ff7..69bdffbfe46 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -43,6 +43,7 @@
>   #include "hw/boards.h"
>   #include "hw/i386/sgx-epc.h"
>   #endif
> +#include "system/qtest.h"
>   #include "tcg/tcg-cpu.h"
>   
>   #include "disas/capstone.h"
> @@ -1943,7 +1944,7 @@ uint32_t xsave_area_size(uint64_t mask, bool compacted)
>   
>   static inline bool accel_uses_host_cpuid(void)
>   {
> -    return kvm_enabled() || hvf_enabled();
> +    return !tcg_enabled() && !qtest_enabled();
>   }
>   
>   static inline uint64_t x86_cpu_xsave_xcr0_components(X86CPU *cpu)
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index b4a4d50e860..69125230abb 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -1207,10 +1207,33 @@ static const TypeInfo nvmm_accel_type = {
>       .class_init = nvmm_accel_class_init,
>   };
>   
> +static void nvmm_cpu_instance_init(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +
> +    host_cpu_instance_init(cpu);
Besides, do we need to call host_cpu_max_instance_init() for the case of 
xcc->max_features, like what has been done for kvm and hvf?
Re: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Paolo Bonzini 5 months, 1 week ago
Il ven 11 lug 2025, 04:18 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> Besides, do we need to call host_cpu_max_instance_init() for the case of
> xcc->max_features, like what has been done for kvm and hvf?
>

I am intentionally skipping that because it would not have any effect and
there is no equivalent to KVM_GET_SUPPORTED_CPUID2 implemented for those
accelerators.

Paolo


>
Re: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Xiaoyao Li 5 months, 1 week ago
On 7/11/2025 2:35 PM, Paolo Bonzini wrote:
> Il ven 11 lug 2025, 04:18 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:
> 
>> Besides, do we need to call host_cpu_max_instance_init() for the case of
>> xcc->max_features, like what has been done for kvm and hvf?
>>
> 
> I am intentionally skipping that because it would not have any effect and
> there is no equivalent to KVM_GET_SUPPORTED_CPUID2 implemented for those
> accelerators.

I meant host_cpu_max_instance_init(), not the upper function like 
kvm_cpu_max_instance_init() or hvf_cpu_max_instance_init().

host_cpu_max_instance_init() is for the case "-cpu max/host", which not 
only sets "vendor" to the host value, but also the "host-phys-bits", 
"family" "model" "stepping" and "model-id"

> Paolo
> 
> 
>>
>
Re: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Paolo Bonzini 5 months, 1 week ago
On 7/11/25 08:40, Xiaoyao Li wrote:
> On 7/11/2025 2:35 PM, Paolo Bonzini wrote:
>> Il ven 11 lug 2025, 04:18 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:
>>
>>> Besides, do we need to call host_cpu_max_instance_init() for the case of
>>> xcc->max_features, like what has been done for kvm and hvf?
>>
>> I am intentionally skipping that because it would not have any effect and
>> there is no equivalent to KVM_GET_SUPPORTED_CPUID2 implemented for those
>> accelerators.
> 
> I meant host_cpu_max_instance_init(), not the upper function like 
> kvm_cpu_max_instance_init() or hvf_cpu_max_instance_init().
> 
> host_cpu_max_instance_init() is for the case "-cpu max/host", which not 
> only sets "vendor" to the host value, but also the "host-phys-bits", 
> "family" "model" "stepping" and "model-id"

Ah, thanks - it also does not have any effect so I didn't think about 
it.  But the separation between host_cpu_instance_init() and 
host_cpu_max_instance_init() is confusing.  I'll send a patch to merge 
them into one function, which should resolve your doubt.

Paolo
Re: [PATCH 2/4] target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
Posted by Xiaoyao Li 5 months, 1 week ago
On 7/11/2025 8:06 AM, Paolo Bonzini wrote:
> NVMM and WHPX are virtualizers, and therefore they need to use
> (at least by default) the host vendor for the guest CPUID.
> Add a cpu_instance_init implementation to these accelerators.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/cpu.c           |  8 +++++++-
>   target/i386/nvmm/nvmm-all.c | 23 +++++++++++++++++++++++
>   target/i386/whpx/whpx-all.c | 23 +++++++++++++++++++++++
>   3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 624cebc3ff7..69bdffbfe46 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -43,6 +43,7 @@
>   #include "hw/boards.h"
>   #include "hw/i386/sgx-epc.h"
>   #endif
> +#include "system/qtest.h"
>   #include "tcg/tcg-cpu.h"
>   
>   #include "disas/capstone.h"
> @@ -1943,7 +1944,7 @@ uint32_t xsave_area_size(uint64_t mask, bool compacted)
>   
>   static inline bool accel_uses_host_cpuid(void)
>   {
> -    return kvm_enabled() || hvf_enabled();
> +    return !tcg_enabled() && !qtest_enabled();
>   }
>   
>   static inline uint64_t x86_cpu_xsave_xcr0_components(X86CPU *cpu)
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index b4a4d50e860..69125230abb 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -1207,10 +1207,33 @@ static const TypeInfo nvmm_accel_type = {
>       .class_init = nvmm_accel_class_init,
>   };
>   
> +static void nvmm_cpu_instance_init(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +
> +    host_cpu_instance_init(cpu);

host_cpu_instance_init() only overwrites the "vendor" for "xcc->model" case.

So for "-cpu max/host" case, it will still use the one set in 
max_x86_cpu_class_init(), i.e., CPUID_VENDOR_AMD

I'm not sure if you saw my patch
https://lore.kernel.org/qemu-devel/20250701075738.3451873-2-xiaoyao.li@intel.com/

On top of it, we can simply make it

static void nvmm_cpu_instance_init(CPUState *cs)
{
     X86CPU *cpu = X86_CPU(cs);

     apply_host_vendor(cpu);
}

> +}
> +
> +static void nvmm_cpu_accel_class_init(ObjectClass *oc, const void *data)
> +{
> +    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> +
> +    acc->cpu_instance_init = nvmm_cpu_instance_init;
> +}
> +
> +static const TypeInfo nvmm_cpu_accel_type = {
> +    .name = ACCEL_CPU_NAME("nvmm"),
> +
> +    .parent = TYPE_ACCEL_CPU,
> +    .class_init = nvmm_cpu_accel_class_init,
> +    .abstract = true,
> +};
> +
>   static void
>   nvmm_type_init(void)
>   {
>       type_register_static(&nvmm_accel_type);
> +    type_register_static(&nvmm_cpu_accel_type);
>   }
>   
>   type_init(nvmm_type_init);
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index faf56e19722..b380459d6fe 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -2500,6 +2500,28 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
>       }
>   }
>   
> +static void whpx_cpu_instance_init(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +
> +    host_cpu_instance_init(cpu);
> +}
> +
> +static void whpx_cpu_accel_class_init(ObjectClass *oc, const void *data)
> +{
> +    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> +
> +    acc->cpu_instance_init = whpx_cpu_instance_init;
> +}
> +
> +static const TypeInfo whpx_cpu_accel_type = {
> +    .name = ACCEL_CPU_NAME("whpx"),
> +
> +    .parent = TYPE_ACCEL_CPU,
> +    .class_init = whpx_cpu_accel_class_init,
> +    .abstract = true,
> +};
> +
>   /*
>    * Partition support
>    */
> @@ -2726,6 +2748,7 @@ static const TypeInfo whpx_accel_type = {
>   static void whpx_type_init(void)
>   {
>       type_register_static(&whpx_accel_type);
> +    type_register_static(&whpx_cpu_accel_type);
>   }
>   
>   bool init_whp_dispatch(void)