1. Patchew
  2. QEMU
  3. [PATCH 0/2] Clean up of target/i386/host-cpu.c
  4. View patch

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

Xiaoyao Li posted 2 patches 4 months, 2 weeks 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>
[PATCH 1/2] i386/cpu: Rename host_cpu_instance_init() to apply_host_vendor()
Posted by Xiaoyao Li 4 months, 2 weeks ago 
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()
Posted by Claudio Fontana 4 months, 2 weeks ago 
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()
Posted by Xiaoyao Li 4 months, 2 weeks ago 
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");
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.