[PATCH 0/4] target/i386: fix position of accel_cpu_instance_init

[PATCH 0/4] target/i386: fix position of accel_cpu_instance_init

[Paolo Bonzini]

With the reordering of instance_post_init callbacks that is new in 10.1
accel_cpu_instance_init must execute in .instance_init as is already
the case for RISC-V.  Otherwise, for example, setting the vendor
property is broken when using KVM or Hypervisor.framework.

Adjust x86 code to allow this movement, and perform it in patch 4.

Paolo

Paolo Bonzini (4):
  target/i386: move max_features to class
  target/i386: nvmm, whpx: add accel/CPU class that sets host vendor
  target/i386: allow reordering max_x86_cpu_initfn vs accel CPU init
  target/i386: move accel_cpu_instance_init to .instance_init

 target/i386/cpu.h           |  2 +-
 target/i386/cpu.c           | 41 ++++++++++++++++++++++---------------
 target/i386/hvf/hvf-cpu.c   |  3 ++-
 target/i386/kvm/kvm-cpu.c   |  7 +++++--
 target/i386/nvmm/nvmm-all.c | 23 +++++++++++++++++++++
 target/i386/whpx/whpx-all.c | 23 +++++++++++++++++++++
 6 files changed, 78 insertions(+), 21 deletions(-)

-- 
2.50.0

[PATCH 1/4] target/i386: move max_features to class

[Paolo Bonzini]

max_features is always set to true for instances created by -cpu max or
-cpu host; it's always false for other classes.  Therefore it can be
turned into a field in the X86CPUClass.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h         | 2 +-
 target/i386/cpu.c         | 5 +++--
 target/i386/hvf/hvf-cpu.c | 3 ++-
 target/i386/kvm/kvm-cpu.c | 5 +++--
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 51e10139dfd..be3ae6d546e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2196,7 +2196,6 @@ struct ArchCPU {
     bool expose_tcg;
     bool migratable;
     bool migrate_smi_count;
-    bool max_features; /* Enable all supported features automatically */
     uint32_t apic_id;
 
     /* Enables publishing of TSC increment and Local APIC bus frequencies to
@@ -2349,6 +2348,7 @@ struct X86CPUClass {
      */
     const X86CPUModel *model;
 
+    bool max_features; /* Enable all supported features automatically */
     bool host_cpuid_required;
     int ordering;
     bool migration_safe;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0d35e95430f..624cebc3ff7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6187,6 +6187,7 @@ static void max_x86_cpu_class_init(ObjectClass *oc, const void *data)
 
     xcc->ordering = 9;
 
+    xcc->max_features = true;
     xcc->model_description =
         "Enables all features supported by the accelerator in the current host";
 
@@ -6201,7 +6202,6 @@ static void max_x86_cpu_initfn(Object *obj)
     /* We can't fill the features array here because we don't know yet if
      * "migratable" is true or false.
      */
-    cpu->max_features = true;
     object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
 
     /*
@@ -8282,6 +8282,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
  */
 void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
 {
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
     CPUX86State *env = &cpu->env;
     FeatureWord w;
     int i;
@@ -8306,7 +8307,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
      * plus_features & minus_features to global properties
      * inside x86_cpu_parse_featurestr() too.
      */
-    if (cpu->max_features) {
+    if (xcc->max_features) {
         for (w = 0; w < FEATURE_WORDS; w++) {
             /* Override only features that weren't set explicitly
              * by the user.
diff --git a/target/i386/hvf/hvf-cpu.c b/target/i386/hvf/hvf-cpu.c
index dfdda701268..2b991f2fc8e 100644
--- a/target/i386/hvf/hvf-cpu.c
+++ b/target/i386/hvf/hvf-cpu.c
@@ -61,13 +61,14 @@ static void hvf_cpu_xsave_init(void)
 static void hvf_cpu_instance_init(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
 
     host_cpu_instance_init(cpu);
 
     /* Special cases not set in the X86CPUDefinition structs: */
     /* TODO: in-kernel irqchip for hvf */
 
-    if (cpu->max_features) {
+    if (xcc->max_features) {
         hvf_cpu_max_instance_init(cpu);
     }
 
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 16bde4de01e..0fb88a239d4 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -41,6 +41,7 @@ static void kvm_set_guest_phys_bits(CPUState *cs)
 static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
 {
     X86CPU *cpu = X86_CPU(cs);
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
     CPUX86State *env = &cpu->env;
     bool ret;
 
@@ -63,7 +64,7 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      *   check/update ucode_rev, phys_bits, guest_phys_bits, mwait
      *   cpu_common_realizefn() (via xcc->parent_realize)
      */
-    if (cpu->max_features) {
+    if (xcc->max_features) {
         if (enable_cpu_pm) {
             if (kvm_has_waitpkg()) {
                 env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
@@ -216,7 +217,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
         x86_cpu_apply_props(cpu, kvm_default_props);
     }
 
-    if (cpu->max_features) {
+    if (xcc->max_features) {
         kvm_cpu_max_instance_init(cpu);
     }
 
-- 
2.50.0

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

[Paolo Bonzini]

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

[PATCH 3/4] target/i386: allow reordering max_x86_cpu_initfn vs accel CPU init

[Paolo Bonzini]

The PMU feature is only supported by KVM, so move it there.  And since
all accelerators other than TCG overwrite the vendor, set it in
max_x86_cpu_initfn only if it has not been initialized by the
superclass.  This makes it possible to run max_x86_cpu_initfn
after accelerator init.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.c         | 24 ++++++++++++------------
 target/i386/kvm/kvm-cpu.c |  2 ++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69bdffbfe46..46d59229200 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6204,21 +6204,21 @@ static void max_x86_cpu_class_init(ObjectClass *oc, const void *data)
 static void max_x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
-
-    /* We can't fill the features array here because we don't know yet if
-     * "migratable" is true or false.
-     */
-    object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
+    CPUX86State *env = &cpu->env;
 
     /*
-     * these defaults are used for TCG and all other accelerators
-     * besides KVM and HVF, which overwrite these values
+     * these defaults are used for TCG, other accelerators overwrite these
+     * values
      */
-    object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
-                            &error_abort);
-    object_property_set_str(OBJECT(cpu), "model-id",
-                            "QEMU TCG CPU version " QEMU_HW_VERSION,
-                            &error_abort);
+    if (!env->cpuid_vendor1) {
+        object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
+                                &error_abort);
+    }
+    if (!env->cpuid_model[0]) {
+        object_property_set_str(OBJECT(cpu), "model-id",
+                                "QEMU TCG CPU version " QEMU_HW_VERSION,
+                                &error_abort);
+    }
 }
 
 static const TypeInfo max_x86_cpu_type_info = {
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 0fb88a239d4..6fed353548e 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -111,6 +111,8 @@ static void kvm_cpu_max_instance_init(X86CPU *cpu)
 
     host_cpu_max_instance_init(cpu);
 
+    object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
+
     if (lmce_supported()) {
         object_property_set_bool(OBJECT(cpu), "lmce", true, &error_abort);
     }
-- 
2.50.0

[PATCH 4/4] target/i386: move accel_cpu_instance_init to .instance_init

[Paolo Bonzini]

With the reordering of instance_post_init callbacks that is new in 10.1
accel_cpu_instance_init must execute in .instance_init as is already
the case for RISC-V.  Otherwise, for example, setting the vendor
property is broken when using KVM or Hypervisor.framework, because
KVM sets it *after* the user's value is set by DeviceState's
intance_post_init callback.

Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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 46d59229200..5f95bb97b82 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6207,8 +6207,8 @@ static void max_x86_cpu_initfn(Object *obj)
     CPUX86State *env = &cpu->env;
 
     /*
-     * these defaults are used for TCG, other accelerators overwrite these
-     * values
+     * these defaults are used for TCG, other accelerators have overwritten
+     * these values
      */
     if (!env->cpuid_vendor1) {
         object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
@@ -9043,8 +9043,6 @@ static void x86_cpu_post_initfn(Object *obj)
         }
     }
 
-    accel_cpu_instance_init(CPU(obj));
-
 #ifndef CONFIG_USER_ONLY
     if (current_machine && current_machine->cgs) {
         x86_confidential_guest_cpu_instance_init(
@@ -9119,6 +9117,8 @@ static void x86_cpu_initfn(Object *obj)
     if (xcc->model) {
         x86_cpu_load_model(cpu, xcc->model);
     }
+
+    accel_cpu_instance_init(CPU(obj));
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
-- 
2.50.0

Re: [PATCH 1/4] target/i386: move max_features to class

[Xiaoyao Li]

On 7/11/2025 8:06 AM, Paolo Bonzini wrote:
> max_features is always set to true for instances created by -cpu max or
> -cpu host; it's always false for other classes.  Therefore it can be
> turned into a field in the X86CPUClass.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

missed one place:


----------------8<---------------
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -8302,7 +8302,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error 
**errp)
          }
      }

-    /*TODO: Now cpu->max_features doesn't overwrite features
+    /*TODO: Now xcc->max_features doesn't overwrite features
       * set using QOM properties, and we can convert
       * plus_features & minus_features to global properties
       * inside x86_cpu_parse_featurestr() too.



with above added,

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

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

[Xiaoyao Li]

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)

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

[Xiaoyao Li]

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 3/4] target/i386: allow reordering max_x86_cpu_initfn vs accel CPU init

[Xiaoyao Li]

On 7/11/2025 8:06 AM, Paolo Bonzini wrote:
> The PMU feature is only supported by KVM, so move it there.  And since
> all accelerators other than TCG overwrite the vendor, set it in
> max_x86_cpu_initfn only if it has not been initialized by the
> superclass.  This makes it possible to run max_x86_cpu_initfn
> after accelerator init.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   target/i386/cpu.c         | 24 ++++++++++++------------
>   target/i386/kvm/kvm-cpu.c |  2 ++
>   2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 69bdffbfe46..46d59229200 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6204,21 +6204,21 @@ static void max_x86_cpu_class_init(ObjectClass *oc, const void *data)
>   static void max_x86_cpu_initfn(Object *obj)
>   {
>       X86CPU *cpu = X86_CPU(obj);
> -
> -    /* We can't fill the features array here because we don't know yet if
> -     * "migratable" is true or false.
> -     */
> -    object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
> +    CPUX86State *env = &cpu->env;
>   
>       /*
> -     * these defaults are used for TCG and all other accelerators
> -     * besides KVM and HVF, which overwrite these values
> +     * these defaults are used for TCG, other accelerators overwrite these
> +     * values
>        */
> -    object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
> -                            &error_abort);
> -    object_property_set_str(OBJECT(cpu), "model-id",
> -                            "QEMU TCG CPU version " QEMU_HW_VERSION,
> -                            &error_abort);
> +    if (!env->cpuid_vendor1) {
> +        object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
> +                                &error_abort);
> +    }
> +    if (!env->cpuid_model[0]) {
> +        object_property_set_str(OBJECT(cpu), "model-id",
> +                                "QEMU TCG CPU version " QEMU_HW_VERSION,
> +                                &error_abort);
> +    }
>   }
>   
>   static const TypeInfo max_x86_cpu_type_info = {
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 0fb88a239d4..6fed353548e 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -111,6 +111,8 @@ static void kvm_cpu_max_instance_init(X86CPU *cpu)
>   
>       host_cpu_max_instance_init(cpu);
>   
> +    object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
> +
>       if (lmce_supported()) {
>           object_property_set_bool(OBJECT(cpu), "lmce", true, &error_abort);
>       }

Re: [PATCH 4/4] target/i386: move accel_cpu_instance_init to .instance_init

[Xiaoyao Li]

On 7/11/2025 8:06 AM, Paolo Bonzini wrote:
> With the reordering of instance_post_init callbacks that is new in 10.1
> accel_cpu_instance_init must execute in .instance_init as is already
> the case for RISC-V.  Otherwise, for example, setting the vendor
> property is broken when using KVM or Hypervisor.framework, because
> KVM sets it *after* the user's value is set by DeviceState's
> intance_post_init callback.
> 
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

BTW, the user's value of "pmu" and "invtsc" are still broken for TDX 
case.  tdx_cpu_instance_init() will always overwrite "pmu" and "invtsc" 
even if users explicitly request a different value via "-cpu" option.

Will we leave it as intentional? or fix it as well?

> ---
>   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 46d59229200..5f95bb97b82 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6207,8 +6207,8 @@ static void max_x86_cpu_initfn(Object *obj)
>       CPUX86State *env = &cpu->env;
>   
>       /*
> -     * these defaults are used for TCG, other accelerators overwrite these
> -     * values
> +     * these defaults are used for TCG, other accelerators have overwritten
> +     * these values
>        */
>       if (!env->cpuid_vendor1) {
>           object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
> @@ -9043,8 +9043,6 @@ static void x86_cpu_post_initfn(Object *obj)
>           }
>       }
>   
> -    accel_cpu_instance_init(CPU(obj));
> -
>   #ifndef CONFIG_USER_ONLY
>       if (current_machine && current_machine->cgs) {
>           x86_confidential_guest_cpu_instance_init(
> @@ -9119,6 +9117,8 @@ static void x86_cpu_initfn(Object *obj)
>       if (xcc->model) {
>           x86_cpu_load_model(cpu, xcc->model);
>       }
> +
> +    accel_cpu_instance_init(CPU(obj));
>   }
>   
>   static int64_t x86_cpu_get_arch_id(CPUState *cs)

Re: [PATCH 1/4] target/i386: move max_features to class

[Zhao Liu]

On Fri, Jul 11, 2025 at 02:06:00AM +0200, Paolo Bonzini wrote:
> Date: Fri, 11 Jul 2025 02:06:00 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/4] target/i386: move max_features to class
> X-Mailer: git-send-email 2.50.0
> 
> max_features is always set to true for instances created by -cpu max or
> -cpu host; it's always false for other classes.  Therefore it can be
> turned into a field in the X86CPUClass.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.h         | 2 +-
>  target/i386/cpu.c         | 5 +++--
>  target/i386/hvf/hvf-cpu.c | 3 ++-
>  target/i386/kvm/kvm-cpu.c | 5 +++--
>  4 files changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

[Zhao Liu]

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 3/4] target/i386: allow reordering max_x86_cpu_initfn vs accel CPU init

[Zhao Liu]

On Fri, Jul 11, 2025 at 02:06:02AM +0200, Paolo Bonzini wrote:
> Date: Fri, 11 Jul 2025 02:06:02 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 3/4] target/i386: allow reordering max_x86_cpu_initfn vs
>  accel CPU init
> X-Mailer: git-send-email 2.50.0
> 
> The PMU feature is only supported by KVM, so move it there.  And since
> all accelerators other than TCG overwrite the vendor, set it in
> max_x86_cpu_initfn only if it has not been initialized by the
> superclass.  This makes it possible to run max_x86_cpu_initfn
> after accelerator init.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c         | 24 ++++++++++++------------
>  target/i386/kvm/kvm-cpu.c |  2 ++
>  2 files changed, 14 insertions(+), 12 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 4/4] target/i386: move accel_cpu_instance_init to .instance_init

[Zhao Liu]

On Fri, Jul 11, 2025 at 02:06:03AM +0200, Paolo Bonzini wrote:
> Date: Fri, 11 Jul 2025 02:06:03 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 4/4] target/i386: move accel_cpu_instance_init to
>  .instance_init
> X-Mailer: git-send-email 2.50.0
> 
> With the reordering of instance_post_init callbacks that is new in 10.1
> accel_cpu_instance_init must execute in .instance_init as is already
> the case for RISC-V.  Otherwise, for example, setting the vendor
> property is broken when using KVM or Hypervisor.framework, because
> KVM sets it *after* the user's value is set by DeviceState's
> intance_post_init callback.
> 
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>

no, Intel doesn't deserve this credit. Instead, this bug is reported
from these 2 people:

"Like Xu" <like.xu.linux@gmail.com> - KUT Test
"Dongli Zhang" <dongli.zhang@oracle.com> - PMU Fix

For reference: https://lore.kernel.org/qemu-devel/aFpocfTpBLB34N3l@intel.com/

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

LGTM,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

[Paolo Bonzini]

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

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


>

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

Re: [PATCH 4/4] target/i386: move accel_cpu_instance_init to .instance_init

[Paolo Bonzini]

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

Il ven 11 lug 2025, 04:26 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> BTW, the user's value of "pmu" and "invtsc" are still broken for TDX
> case.  tdx_cpu_instance_init() will always overwrite "pmu" and "invtsc"
> even if users explicitly request a different value via "-cpu" option.
>
> Will we leave it as intentional? or fix it as well?
>

I need to check the differences with SNP but I am leaning towards treating
it as intentional... Maybe warn if there was a user option saying the
opposite.

I will include these in my soft freeze PR, thanks both for the speedy
review!!

Paolo


> > ---
> >   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 46d59229200..5f95bb97b82 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6207,8 +6207,8 @@ static void max_x86_cpu_initfn(Object *obj)
> >       CPUX86State *env = &cpu->env;
> >
> >       /*
> > -     * these defaults are used for TCG, other accelerators overwrite
> these
> > -     * values
> > +     * these defaults are used for TCG, other accelerators have
> overwritten
> > +     * these values
> >        */
> >       if (!env->cpuid_vendor1) {
> >           object_property_set_str(OBJECT(cpu), "vendor",
> CPUID_VENDOR_AMD,
> > @@ -9043,8 +9043,6 @@ static void x86_cpu_post_initfn(Object *obj)
> >           }
> >       }
> >
> > -    accel_cpu_instance_init(CPU(obj));
> > -
> >   #ifndef CONFIG_USER_ONLY
> >       if (current_machine && current_machine->cgs) {
> >           x86_confidential_guest_cpu_instance_init(
> > @@ -9119,6 +9117,8 @@ static void x86_cpu_initfn(Object *obj)
> >       if (xcc->model) {
> >           x86_cpu_load_model(cpu, xcc->model);
> >       }
> > +
> > +    accel_cpu_instance_init(CPU(obj));
> >   }
> >
> >   static int64_t x86_cpu_get_arch_id(CPUState *cs)
>
>

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

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

[Xiaoyao Li]

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

[Paolo Bonzini]

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