KVM: x86: expose CPUID 0xC000_0000 for Zhaoxin "Shanghai" vendor

[PATCH] KVM: x86: expose CPUID 0xC000_0000 for Zhaoxin "Shanghai" vendor

Posted by Ewan Hai 1 month, 3 weeks ago

Both vendor IDs used by Zhaoxin ("  Shanghai  " and "Centaurhauls") rely
on leaf 0xC000_0000 to advertise the max 0xC000_00xx function. Extend
KVM so the leaf is returned for either ID and rename the local constant
CENTAUR_CPUID_SIGNATURE to ZHAOXIN_CPUID_SIGNATURE.  The constant is
used only inside cpuid.c, so the rename is NFC outside this file.

Signed-off-by: Ewan Hai <ewanhai-oc@zhaoxin.com>
---
 arch/x86/kvm/cpuid.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index e2836a255b16..beb83eaa1868 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1811,7 +1811,7 @@ static int do_cpuid_func(struct kvm_cpuid_array *array, u32 func,
 	return __do_cpuid_func(array, func);
 }
 
-#define CENTAUR_CPUID_SIGNATURE 0xC0000000
+#define ZHAOXIN_CPUID_SIGNATURE 0xC0000000
 
 static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
 			  unsigned int type)
@@ -1819,8 +1819,9 @@ static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
 	u32 limit;
 	int r;
 
-	if (func == CENTAUR_CPUID_SIGNATURE &&
-	    boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR)
+	if (func == ZHAOXIN_CPUID_SIGNATURE &&
+		boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
+		boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
 		return 0;
 
 	r = do_cpuid_func(array, func, type);
@@ -1869,7 +1870,7 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 			    unsigned int type)
 {
 	static const u32 funcs[] = {
-		0, 0x80000000, CENTAUR_CPUID_SIGNATURE, KVM_CPUID_SIGNATURE,
+		0, 0x80000000, ZHAOXIN_CPUID_SIGNATURE, KVM_CPUID_SIGNATURE,
 	};
 
 	struct kvm_cpuid_array array = {
-- 
2.34.1

Re: [PATCH] KVM: x86: expose CPUID 0xC000_0000 for Zhaoxin "Shanghai" vendor

Posted by Sean Christopherson 1 month, 3 weeks ago

On Sun, Aug 10, 2025, Ewan Hai wrote:
> rename the local constant CENTAUR_CPUID_SIGNATURE to ZHAOXIN_CPUID_SIGNATURE. 

Why?  I'm not inclined to rename any of the Centaur references, as I don't see
any point in effectively rewriting history.  If we elect to rename things, then
it needs to be done in a separate patch, there needs to be proper justification,
and _all_ references should be converted, e.g. converting just this one macro
creates discrepancies even with cpuid.c, as there are multiple comments that
specifically talk about Centaur CPUID leaves.

> The constant is used only inside cpuid.c, so the rename is NFC outside this
> file.
> 
> Signed-off-by: Ewan Hai <ewanhai-oc@zhaoxin.com>
> ---
>  arch/x86/kvm/cpuid.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index e2836a255b16..beb83eaa1868 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1811,7 +1811,7 @@ static int do_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>  	return __do_cpuid_func(array, func);
>  }
>  
> -#define CENTAUR_CPUID_SIGNATURE 0xC0000000
> +#define ZHAOXIN_CPUID_SIGNATURE 0xC0000000
>  
>  static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>  			  unsigned int type)
> @@ -1819,8 +1819,9 @@ static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>  	u32 limit;
>  	int r;
>  
> -	if (func == CENTAUR_CPUID_SIGNATURE &&
> -	    boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR)
> +	if (func == ZHAOXIN_CPUID_SIGNATURE &&
> +		boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
> +		boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)

Align indentation.

	if (func == CENTAUR_CPUID_SIGNATURE &&
	    boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
	    boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
		return 0;

>  		return 0;
>  
>  	r = do_cpuid_func(array, func, type);
> @@ -1869,7 +1870,7 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  			    unsigned int type)
>  {
>  	static const u32 funcs[] = {
> -		0, 0x80000000, CENTAUR_CPUID_SIGNATURE, KVM_CPUID_SIGNATURE,
> +		0, 0x80000000, ZHAOXIN_CPUID_SIGNATURE, KVM_CPUID_SIGNATURE,
>  	};
>  
>  	struct kvm_cpuid_array array = {
> -- 
> 2.34.1
>

Re: [PATCH] KVM: x86: expose CPUID 0xC000_0000 for Zhaoxin "Shanghai" vendor

Posted by Ewan Hai 1 month, 3 weeks ago


On 8/12/25 11:07, Sean Christopherson wrote:
> On Sun, Aug 10, 2025, Ewan Hai wrote:
>> rename the local constant CENTAUR_CPUID_SIGNATURE to ZHAOXIN_CPUID_SIGNATURE.
> Why?  I'm not inclined to rename any of the Centaur references, as I don't see
> any point in effectively rewriting history.  If we elect to rename things, then
> it needs to be done in a separate patch, there needs to be proper justification,
> and _all_ references should be converted, e.g. converting just this one macro
> creates discrepancies even with cpuid.c, as there are multiple comments that
> specifically talk about Centaur CPUID leaves.
>
Okay, it seems I oversimplified the situation.

My initial thought was that, since there will no longer be separate handling for
"Centaurhauls," nearly all new software and hardware features will be applied to
both "Centaurhauls" and "  Shanghai  " vendors in parallel. This would gradually
lead to more and more occurrences of if (vendor == centaur || vendor ==
shanghai) in the kernel code. In that case, introducing an is_zhaoxin_vendor()
helper could significantly reduce the number of repetitive if (xx || yy) checks.

However, it appears that this "duplication issue" is not a real concern for now.
We can revisit it later when it becomes a practical problem.

For the current matter, there are two possible approaches. Which one do you
prefer? Or, if you have other suggestions, please let me know and I will
incorporate your recommendation into the v2 patch.

## Version 1 ##
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1820,7 +1820,8 @@ static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
         int r;

         if (func == CENTAUR_CPUID_SIGNATURE &&
-           boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR)
+           boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
+           boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
                 return 0;

         r = do_cpuid_func(array, func, type);

## Version 2 ##
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1812,6 +1812,7 @@ static int do_cpuid_func(struct kvm_cpuid_array *array, u32 func,
  }

  #define CENTAUR_CPUID_SIGNATURE 0xC0000000
+#define ZHAOXIN_CPUID_SIGNATURE 0xC0000000

  static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
                           unsigned int type)
@@ -1819,8 +1820,10 @@ static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
         u32 limit;
         int r;

-       if (func == CENTAUR_CPUID_SIGNATURE &&
-           boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR)
+       if ((func == CENTAUR_CPUID_SIGNATURE &&
+            boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR) ||
+           (func == ZHAOXIN_CPUID_SIGNATURE &&
+            boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN))
                 return 0;

         r = do_cpuid_func(array, func, type);

>> The constant is used only inside cpuid.c, so the rename is NFC outside this
>> file.
>>
>> Signed-off-by: Ewan Hai <ewanhai-oc@zhaoxin.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index e2836a255b16..beb83eaa1868 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1811,7 +1811,7 @@ static int do_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>>        return __do_cpuid_func(array, func);
>>   }
>>
>> -#define CENTAUR_CPUID_SIGNATURE 0xC0000000
>> +#define ZHAOXIN_CPUID_SIGNATURE 0xC0000000
>>
>>   static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>>                          unsigned int type)
>> @@ -1819,8 +1819,9 @@ static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>>        u32 limit;
>>        int r;
>>
>> -     if (func == CENTAUR_CPUID_SIGNATURE &&
>> -         boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR)
>> +     if (func == ZHAOXIN_CPUID_SIGNATURE &&
>> +             boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
>> +             boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
> Align indentation.

Will fix in v2.

>
>          if (func == CENTAUR_CPUID_SIGNATURE &&
>              boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
>              boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
>                  return 0;
>
>>                return 0;
>>
>>        r = do_cpuid_func(array, func, type);
>> @@ -1869,7 +1870,7 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>>                            unsigned int type)
>>   {
>>        static const u32 funcs[] = {
>> -             0, 0x80000000, CENTAUR_CPUID_SIGNATURE, KVM_CPUID_SIGNATURE,
>> +             0, 0x80000000, ZHAOXIN_CPUID_SIGNATURE, KVM_CPUID_SIGNATURE,
>>        };
>>
>>        struct kvm_cpuid_array array = {
>> --
>> 2.34.1
>>

Re: [PATCH] KVM: x86: expose CPUID 0xC000_0000 for Zhaoxin "Shanghai" vendor

Posted by Sean Christopherson 1 month, 2 weeks ago

On Wed, Aug 13, 2025, Ewan Hai wrote:
> 
> 
> On 8/12/25 11:07, Sean Christopherson wrote:
> > On Sun, Aug 10, 2025, Ewan Hai wrote:
> > > rename the local constant CENTAUR_CPUID_SIGNATURE to ZHAOXIN_CPUID_SIGNATURE.
> > Why?  I'm not inclined to rename any of the Centaur references, as I don't see
> > any point in effectively rewriting history.  If we elect to rename things, then
> > it needs to be done in a separate patch, there needs to be proper justification,
> > and _all_ references should be converted, e.g. converting just this one macro
> > creates discrepancies even with cpuid.c, as there are multiple comments that
> > specifically talk about Centaur CPUID leaves.
> > 
> Okay, it seems I oversimplified the situation.
> 
> My initial thought was that, since there will no longer be separate handling for
> "Centaurhauls," nearly all new software and hardware features will be applied to
> both "Centaurhauls" and "  Shanghai  " vendors in parallel. This would gradually
> lead to more and more occurrences of if (vendor == centaur || vendor ==
> shanghai) in the kernel code. In that case, introducing an is_zhaoxin_vendor()
> helper could significantly reduce the number of repetitive if (xx || yy) checks.
> 
> However, it appears that this "duplication issue" is not a real concern for now.
> We can revisit it later when it becomes a practical problem.
> 
> For the current matter, there are two possible approaches. Which one do you
> prefer? Or, if you have other suggestions, please let me know and I will
> incorporate your recommendation into the v2 patch.
> 
> ## Version 1 ##
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1820,7 +1820,8 @@ static int get_cpuid_func(struct kvm_cpuid_array *array, u32 func,
>         int r;
> 
>         if (func == CENTAUR_CPUID_SIGNATURE &&
> -           boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR)
> +           boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR &&
> +           boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN)
>                 return 0;
> 
>         r = do_cpuid_func(array, func, type);

This version, please.  As you note above, if we need to tweak things in the future
to dedup code, then I'm happy to do so.  But for now, I don't think KVM needs to
do anything more than add X86_VENDOR_ZHAOXIN to the set of compatible vendor.