target/i386: fix position of accel_cpu_instance_init

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

Posted by Paolo Bonzini 5 months, 1 week ago

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 4/4] target/i386: move accel_cpu_instance_init to .instance_init

Posted by Zhao Liu 5 months, 1 week ago

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 4/4] target/i386: move accel_cpu_instance_init to .instance_init

Posted by Xiaoyao Li 5 months, 1 week ago

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 4/4] target/i386: move accel_cpu_instance_init to .instance_init

Posted by Paolo Bonzini 5 months, 1 week ago

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