[PATCH 0/2] Clean up of target/i386/host-cpu.c

[PATCH 0/2] Clean up of target/i386/host-cpu.c

[Xiaoyao Li]

Everytime I look at the flow of how x86 cpu is initialized, I get
confused by the name of host_cpu_instance_init() and
host_cpu_max_instance_init(). They look like related to "-cpu host" and
the .instance_init() callback of "-cpu host".

However, host_cpu_instance_init() even has noting to do with "-cpu
host". So patch 1, to rename host_cpu_instance_init(). I also want to
rename host_cpu_max_instance_init(), but I cannot think of a new better
name so I just leave it as-is.

Patch 2 is the cleanup found when I look at host-cpu.c

Note, this series is based on my another cleanup [1]. It might be able
to apply cleanly on master with [1].

[1] https://lore.kernel.org/qemu-devel/20250630080610.3151956-1-xiaoyao.li@intel.com/

Xiaoyao Li (2):
  i386/cpu: Rename host_cpu_instance_init() to apply_host_vendor()
  i386/cpu: Move the implementation of is_host_cpu_intel() host-cpu.c

 target/i386/host-cpu.c        | 21 +++++++++++++--------
 target/i386/host-cpu.h        |  3 ++-
 target/i386/hvf/hvf-cpu.c     |  5 ++++-
 target/i386/kvm/kvm-cpu.c     |  4 ++--
 target/i386/kvm/vmsr_energy.c |  9 ---------
 target/i386/kvm/vmsr_energy.h |  1 -
 6 files changed, 21 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH 1/2] i386/cpu: Rename host_cpu_instance_init() to apply_host_vendor()

[Xiaoyao Li]

The name of host_cpu_instance_init is really confusing. It misleads
people to think it as the .instance_init() callback of "host" x86 cpu
type.

Rename it to match what it does and move the xcc->model check to
callers since it's better to let host-cpu.c concentrate only on the host
related functionalities.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/host-cpu.c    | 12 ++++--------
 target/i386/host-cpu.h    |  2 +-
 target/i386/hvf/hvf-cpu.c |  5 ++++-
 target/i386/kvm/kvm-cpu.c |  4 ++--
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index 383c42d4ae3d..c86b8227b974 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -127,16 +127,12 @@ void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping)
     }
 }
 
-void host_cpu_instance_init(X86CPU *cpu)
+void apply_host_vendor(X86CPU *cpu)
 {
-    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
+    char vendor[CPUID_VENDOR_SZ + 1];
 
-    if (xcc->model) {
-        char vendor[CPUID_VENDOR_SZ + 1];
-
-        host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
-        object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
-    }
+    host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
+    object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
 }
 
 void host_cpu_max_instance_init(X86CPU *cpu)
diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
index b97ec01c9bec..779f0f2f4123 100644
--- a/target/i386/host-cpu.h
+++ b/target/i386/host-cpu.h
@@ -11,7 +11,7 @@
 #define HOST_CPU_H
 
 uint32_t host_cpu_phys_bits(void);
-void host_cpu_instance_init(X86CPU *cpu);
+void apply_host_vendor(X86CPU *cpu);
 void host_cpu_max_instance_init(X86CPU *cpu);
 bool host_cpu_realizefn(CPUState *cs, Error **errp);
 
diff --git a/target/i386/hvf/hvf-cpu.c b/target/i386/hvf/hvf-cpu.c
index dfdda701268e..16647482aba0 100644
--- a/target/i386/hvf/hvf-cpu.c
+++ b/target/i386/hvf/hvf-cpu.c
@@ -61,8 +61,11 @@ 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);
+    if (xcc->model) {
+        apply_host_vendor(cpu);
+    }
 
     /* Special cases not set in the X86CPUDefinition structs: */
     /* TODO: in-kernel irqchip for hvf */
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 6df92dc6d703..99e4357d5efe 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -202,9 +202,9 @@ static void kvm_cpu_instance_init(CPUState *cs)
     X86CPU *cpu = X86_CPU(cs);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
 
-    host_cpu_instance_init(cpu);
-
     if (xcc->model) {
+        apply_host_vendor(cpu);
+
         /* only applies to builtin_x86_defs cpus */
         if (!kvm_irqchip_in_kernel()) {
             x86_cpu_change_kvm_default("x2apic", "off");
-- 
2.43.0

Re: [PATCH 1/2] i386/cpu: Rename host_cpu_instance_init() to apply_host_vendor()

[Claudio Fontana]

Hello Xiaoyao,

I did not find a better name at the time. The meaning of 'host' there has nothing to do with the cpu type called "host",
but rather identifies common code between kvm and hvf, which both use the host cpuid(), See accel_uses_host_cpuid(), host_cpuid(), host_cpu_vendor_fms().

Maybe the right way is to split the code in two files,

one dealing with these functions common between hvf and kvm,
and one file that implements the "host" cpu type.

I am concerned that "apply_host_vendor" would need to be renamed again if more code will need to be added that is common in the initialization of hvf and kvm.

I am not sure what could be a better name for the function host_cpu_instance_init(),
but maybe its name would not confuse so much anymore if it is contained in a file that specifically includes this common code,
excluding all "host" cpu type related code.

Bye,

Claudio


On 7/1/25 09:57, Xiaoyao Li wrote:
> The name of host_cpu_instance_init is really confusing. It misleads
> people to think it as the .instance_init() callback of "host" x86 cpu
> type.
> 
> Rename it to match what it does and move the xcc->model check to
> callers since it's better to let host-cpu.c concentrate only on the host
> related functionalities.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/host-cpu.c    | 12 ++++--------
>  target/i386/host-cpu.h    |  2 +-
>  target/i386/hvf/hvf-cpu.c |  5 ++++-
>  target/i386/kvm/kvm-cpu.c |  4 ++--
>  4 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> index 383c42d4ae3d..c86b8227b974 100644
> --- a/target/i386/host-cpu.c
> +++ b/target/i386/host-cpu.c
> @@ -127,16 +127,12 @@ void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping)
>      }
>  }
>  
> -void host_cpu_instance_init(X86CPU *cpu)
> +void apply_host_vendor(X86CPU *cpu)
>  {
> -    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
> +    char vendor[CPUID_VENDOR_SZ + 1];
>  
> -    if (xcc->model) {
> -        char vendor[CPUID_VENDOR_SZ + 1];
> -
> -        host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
> -        object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
> -    }
> +    host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
> +    object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
>  }
>  
>  void host_cpu_max_instance_init(X86CPU *cpu)
> diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
> index b97ec01c9bec..779f0f2f4123 100644
> --- a/target/i386/host-cpu.h
> +++ b/target/i386/host-cpu.h
> @@ -11,7 +11,7 @@
>  #define HOST_CPU_H
>  
>  uint32_t host_cpu_phys_bits(void);
> -void host_cpu_instance_init(X86CPU *cpu);
> +void apply_host_vendor(X86CPU *cpu);
>  void host_cpu_max_instance_init(X86CPU *cpu);
>  bool host_cpu_realizefn(CPUState *cs, Error **errp);
>  
> diff --git a/target/i386/hvf/hvf-cpu.c b/target/i386/hvf/hvf-cpu.c
> index dfdda701268e..16647482aba0 100644
> --- a/target/i386/hvf/hvf-cpu.c
> +++ b/target/i386/hvf/hvf-cpu.c
> @@ -61,8 +61,11 @@ 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);
> +    if (xcc->model) {
> +        apply_host_vendor(cpu);
> +    }
>  
>      /* Special cases not set in the X86CPUDefinition structs: */
>      /* TODO: in-kernel irqchip for hvf */
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 6df92dc6d703..99e4357d5efe 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -202,9 +202,9 @@ static void kvm_cpu_instance_init(CPUState *cs)
>      X86CPU *cpu = X86_CPU(cs);
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
>  
> -    host_cpu_instance_init(cpu);
> -
>      if (xcc->model) {
> +        apply_host_vendor(cpu);
> +
>          /* only applies to builtin_x86_defs cpus */
>          if (!kvm_irqchip_in_kernel()) {
>              x86_cpu_change_kvm_default("x2apic", "off");

Re: [PATCH 1/2] i386/cpu: Rename host_cpu_instance_init() to apply_host_vendor()

[Xiaoyao Li]

On 7/1/2025 6:29 PM, Claudio Fontana wrote:
> Hello Xiaoyao,
> 
> I did not find a better name at the time. The meaning of 'host' there has nothing to do with the cpu type called "host",
> but rather identifies common code between kvm and hvf, which both use the host cpuid(), See accel_uses_host_cpuid(), host_cpuid(), host_cpu_vendor_fms().

yeah. I konw this.

> Maybe the right way is to split the code in two files,
> 
> one dealing with these functions common between hvf and kvm,
> and one file that implements the "host" cpu type.

It can help, but the confusion doesn't disappear.

> I am concerned that "apply_host_vendor" would need to be renamed again if more code will need to be added that is common in the initialization of hvf and kvm.

At that time, we can introduce another specific function instead of 
putting everything in one function.

> I am not sure what could be a better name for the function host_cpu_instance_init(),
> but maybe its name would not confuse so much anymore if it is contained in a file that specifically includes this common code,
> excluding all "host" cpu type related code.

It can help, but it doesn't stop me from trying to associate it with 
"host" cpu type.

Anyway, if most people leans towards separating the files, I'm also OK.

> Bye,
> 
> Claudio
> 
> 
> On 7/1/25 09:57, Xiaoyao Li wrote:
>> The name of host_cpu_instance_init is really confusing. It misleads
>> people to think it as the .instance_init() callback of "host" x86 cpu
>> type.
>>
>> Rename it to match what it does and move the xcc->model check to
>> callers since it's better to let host-cpu.c concentrate only on the host
>> related functionalities.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/host-cpu.c    | 12 ++++--------
>>   target/i386/host-cpu.h    |  2 +-
>>   target/i386/hvf/hvf-cpu.c |  5 ++++-
>>   target/i386/kvm/kvm-cpu.c |  4 ++--
>>   4 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
>> index 383c42d4ae3d..c86b8227b974 100644
>> --- a/target/i386/host-cpu.c
>> +++ b/target/i386/host-cpu.c
>> @@ -127,16 +127,12 @@ void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping)
>>       }
>>   }
>>   
>> -void host_cpu_instance_init(X86CPU *cpu)
>> +void apply_host_vendor(X86CPU *cpu)
>>   {
>> -    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
>> +    char vendor[CPUID_VENDOR_SZ + 1];
>>   
>> -    if (xcc->model) {
>> -        char vendor[CPUID_VENDOR_SZ + 1];
>> -
>> -        host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
>> -        object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
>> -    }
>> +    host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
>> +    object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
>>   }
>>   
>>   void host_cpu_max_instance_init(X86CPU *cpu)
>> diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
>> index b97ec01c9bec..779f0f2f4123 100644
>> --- a/target/i386/host-cpu.h
>> +++ b/target/i386/host-cpu.h
>> @@ -11,7 +11,7 @@
>>   #define HOST_CPU_H
>>   
>>   uint32_t host_cpu_phys_bits(void);
>> -void host_cpu_instance_init(X86CPU *cpu);
>> +void apply_host_vendor(X86CPU *cpu);
>>   void host_cpu_max_instance_init(X86CPU *cpu);
>>   bool host_cpu_realizefn(CPUState *cs, Error **errp);
>>   
>> diff --git a/target/i386/hvf/hvf-cpu.c b/target/i386/hvf/hvf-cpu.c
>> index dfdda701268e..16647482aba0 100644
>> --- a/target/i386/hvf/hvf-cpu.c
>> +++ b/target/i386/hvf/hvf-cpu.c
>> @@ -61,8 +61,11 @@ 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);
>> +    if (xcc->model) {
>> +        apply_host_vendor(cpu);
>> +    }
>>   
>>       /* Special cases not set in the X86CPUDefinition structs: */
>>       /* TODO: in-kernel irqchip for hvf */
>> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
>> index 6df92dc6d703..99e4357d5efe 100644
>> --- a/target/i386/kvm/kvm-cpu.c
>> +++ b/target/i386/kvm/kvm-cpu.c
>> @@ -202,9 +202,9 @@ static void kvm_cpu_instance_init(CPUState *cs)
>>       X86CPU *cpu = X86_CPU(cs);
>>       X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
>>   
>> -    host_cpu_instance_init(cpu);
>> -
>>       if (xcc->model) {
>> +        apply_host_vendor(cpu);
>> +
>>           /* only applies to builtin_x86_defs cpus */
>>           if (!kvm_irqchip_in_kernel()) {
>>               x86_cpu_change_kvm_default("x2apic", "off");
> 

[PATCH 2/2] i386/cpu: Move the implementation of is_host_cpu_intel() host-cpu.c

[Xiaoyao Li]

It's more proper to put is_host_cpu_intel() in host-cpu.c instead of
vmsr_energy.c.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/host-cpu.c        | 9 +++++++++
 target/i386/host-cpu.h        | 1 +
 target/i386/kvm/vmsr_energy.c | 9 ---------
 target/i386/kvm/vmsr_energy.h | 1 -
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index c86b8227b974..6a88be8714c1 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -156,6 +156,15 @@ void host_cpu_max_instance_init(X86CPU *cpu)
                             &error_abort);
 }
 
+bool is_host_cpu_intel(void)
+{
+    char vendor[CPUID_VENDOR_SZ + 1];
+
+    host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
+
+    return g_str_equal(vendor, CPUID_VENDOR_INTEL);
+}
+
 static void host_cpu_class_init(ObjectClass *oc, const void *data)
 {
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
index 779f0f2f4123..78086a0b0164 100644
--- a/target/i386/host-cpu.h
+++ b/target/i386/host-cpu.h
@@ -17,4 +17,5 @@ bool host_cpu_realizefn(CPUState *cs, Error **errp);
 
 void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping);
 
+bool is_host_cpu_intel(void);
 #endif /* HOST_CPU_H */
diff --git a/target/i386/kvm/vmsr_energy.c b/target/i386/kvm/vmsr_energy.c
index d6aad5246b66..58ce3df53a3e 100644
--- a/target/i386/kvm/vmsr_energy.c
+++ b/target/i386/kvm/vmsr_energy.c
@@ -27,15 +27,6 @@ char *vmsr_compute_default_paths(void)
     return g_build_filename(state, "run", "qemu-vmsr-helper.sock", NULL);
 }
 
-bool is_host_cpu_intel(void)
-{
-    char vendor[CPUID_VENDOR_SZ + 1];
-
-    host_cpu_vendor_fms(vendor, NULL, NULL, NULL);
-
-    return g_str_equal(vendor, CPUID_VENDOR_INTEL);
-}
-
 int is_rapl_enabled(void)
 {
     const char *path = "/sys/class/powercap/intel-rapl/enabled";
diff --git a/target/i386/kvm/vmsr_energy.h b/target/i386/kvm/vmsr_energy.h
index 16cc1f4814f6..151bcbd64239 100644
--- a/target/i386/kvm/vmsr_energy.h
+++ b/target/i386/kvm/vmsr_energy.h
@@ -94,6 +94,5 @@ double vmsr_get_ratio(uint64_t e_delta,
                       unsigned long long delta_ticks,
                       unsigned int maxticks);
 void vmsr_init_topo_info(X86CPUTopoInfo *topo_info, const MachineState *ms);
-bool is_host_cpu_intel(void);
 int is_rapl_enabled(void);
 #endif /* VMSR_ENERGY_H */
-- 
2.43.0

Re: [PATCH 0/2] Clean up of target/i386/host-cpu.c

[Paolo Bonzini]

Queued patch 2, thanks.  I think the issue in patch 1 is also clarified
by merging host_cpu_instance_init() with host_cpu_max_instance_init().

Paolo