1. Patchew
  2. QEMU
  3. [PATCH v2 0/4] Add support for Zhaoxin Yongfeng CPU model and other improvements
  4. View patch

[PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs

EwanHai posted 4 patches 4 months, 3 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
[PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by EwanHai 4 months, 3 weeks ago 
Zhaoxin CPUs handle the CMPLegacy bit in the same way
as Intel CPUs. This patch simplifies the existing logic by
using the IS_XXX_CPU macro and includes checks for Zhaoxin
vendor to align their behavior with Intel.

Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
---
 target/i386/cpu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a3747fc487..c52a4cf3ba 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6945,9 +6945,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
          * So don't set it here for Intel to make Linux guests happy.
          */
         if (threads_per_pkg > 1) {
-            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
-                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
-                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
+            if (!IS_INTEL_CPU(env) && !IS_ZHAOXIN_CPU(env)) {
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
-- 
2.34.1
Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by Zhao Liu 3 months, 2 weeks ago 
Hi EwanHai,

On Thu, Jul 04, 2024 at 07:25:11AM -0400, EwanHai wrote:
> Date: Thu, 4 Jul 2024 07:25:11 -0400
> From: EwanHai <ewanhai-oc@zhaoxin.com>
> Subject: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin
>  CPUs
> X-Mailer: git-send-email 2.34.1
> 
> Zhaoxin CPUs handle the CMPLegacy bit in the same way
> as Intel CPUs. This patch simplifies the existing logic by
> using the IS_XXX_CPU macro and includes checks for Zhaoxin
> vendor to align their behavior with Intel.
> 
> Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
> ---
>  target/i386/cpu.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a3747fc487..c52a4cf3ba 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6945,9 +6945,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           * So don't set it here for Intel to make Linux guests happy.
>           */
>          if (threads_per_pkg > 1) {
> -            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
> -                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
> -                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
> +            if (!IS_INTEL_CPU(env) && !IS_ZHAOXIN_CPU(env)) {

This change implicitly changes the behavior of existing VIA CPU.

Is this a bug for the original VIA? If so, I suggest a separate patch to
fix it and explain the effect on the VIA (Zhaoxin1) CPU.

Regards,
Zhao

>                  *ecx |= 1 << 1;    /* CmpLegacy bit */
>              }
>          }
> -- 
> 2.34.1
>
Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by Ewan Hai 3 months, 2 weeks ago 
Hi Zhao Liu,

Thank you for your feedback.

On 8/8/24 06:30, Zhao Liu wrote:
> Hi EwanHai,
>
> On Thu, Jul 04, 2024 at 07:25:11AM -0400, EwanHai wrote:
>> Date: Thu, 4 Jul 2024 07:25:11 -0400
>> From: EwanHai <ewanhai-oc@zhaoxin.com>
>> Subject: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin
>>   CPUs
>> X-Mailer: git-send-email 2.34.1
>>
>> Zhaoxin CPUs handle the CMPLegacy bit in the same way
>> as Intel CPUs. This patch simplifies the existing logic by
>> using the IS_XXX_CPU macro and includes checks for Zhaoxin
>> vendor to align their behavior with Intel.
>>
>> Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
>> ---
>>   target/i386/cpu.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index a3747fc487..c52a4cf3ba 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6945,9 +6945,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>            * So don't set it here for Intel to make Linux guests happy.
>>            */
>>           if (threads_per_pkg > 1) {
>> -            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
>> -                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
>> -                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
>> +            if (!IS_INTEL_CPU(env) && !IS_ZHAOXIN_CPU(env)) {
> This change implicitly changes the behavior of existing VIA CPU.
>
> Is this a bug for the original VIA? If so, I suggest a separate patch to
> fix it and explain the effect on the VIA (Zhaoxin1) CPU.
>
> Regards,
> Zhao
The reason for this change is not due to a discovered bug, but rather
because both Centaurhauls and Shanghai CPUs follow Intel’s behavior
regarding the CMPLegacy bit. Specifically, AMD CPUs enumerate the
threads per package information in the CPUID leaf 0x80000001 output
ECX register, while Intel (and **other processors following Intel’s
behavior**) do not. Therefore, this modification is simply intended to
logically supplement the existing code.

Given this, do you think it would be appropriate for me to submit
a separate patch to explain this behavior and its effect on
VIA (Zhaoxin1) CPUs? If so, I will submmit this change in a separate
patch.
>>                   *ecx |= 1 << 1;    /* CmpLegacy bit */
>>               }
>>           }
>> --
>> 2.34.1
>>
Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by Zhao Liu 3 months, 2 weeks ago 
On Thu, Aug 08, 2024 at 09:44:18PM -0400, Ewan Hai wrote:
> Date: Thu, 8 Aug 2024 21:44:18 -0400
> From: Ewan Hai <ewanhai-oc@zhaoxin.com>
> Subject: Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for
>  Zhaoxin CPUs
> 
> 
> Hi Zhao Liu,
> 
> Thank you for your feedback.
> 
> On 8/8/24 06:30, Zhao Liu wrote:
> > Hi EwanHai,
> > 
> > On Thu, Jul 04, 2024 at 07:25:11AM -0400, EwanHai wrote:
> > > Date: Thu, 4 Jul 2024 07:25:11 -0400
> > > From: EwanHai <ewanhai-oc@zhaoxin.com>
> > > Subject: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin
> > >   CPUs
> > > X-Mailer: git-send-email 2.34.1
> > > 
> > > Zhaoxin CPUs handle the CMPLegacy bit in the same way
> > > as Intel CPUs.

Here it could be clearer to say "Don't set up CMPLegacy bit in
CPUID[0x80000001].ecx for VIA/Zhaoxin CPUs".

> This patch simplifies the existing logic by
> > > using the IS_XXX_CPU macro and includes checks for Zhaoxin
> > > vendor to align their behavior with Intel.
> > > 
> > > Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
> > > ---
> > >   target/i386/cpu.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index a3747fc487..c52a4cf3ba 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -6945,9 +6945,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> > >            * So don't set it here for Intel to make Linux guests happy.
> > >            */
> > >           if (threads_per_pkg > 1) {
> > > -            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
> > > -                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
> > > -                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
> > > +            if (!IS_INTEL_CPU(env) && !IS_ZHAOXIN_CPU(env)) {
> > This change implicitly changes the behavior of existing VIA CPU.
> > 
> > Is this a bug for the original VIA? If so, I suggest a separate patch to
> > fix it and explain the effect on the VIA (Zhaoxin1) CPU.
> > 
> > Regards,
> > Zhao
>
> The reason for this change is not due to a discovered bug, but rather
> because both Centaurhauls and Shanghai CPUs follow Intel’s behavior
> regarding the CMPLegacy bit. Specifically, AMD CPUs enumerate the
> threads per package information in the CPUID leaf 0x80000001 output
> ECX register, while Intel (and **other processors following Intel’s
> behavior**) do not. Therefore, this modification is simply intended to
> logically supplement the existing code.

I see, thanks.

> Given this, do you think it would be appropriate for me to submit
> a separate patch to explain this behavior and its effect on
> VIA (Zhaoxin1) CPUs? If so, I will submmit this change in a separate
> patch.

I think there's no need to split this.

However, I think it's necessary to state the effect of the change in
the changelog/commit message. It's also worth stating if it won't have
any effect on the OS/software. Afterall, the comment of this bit said
it affects Linux kernel.

Also, changes to the old VIA behavior are worth stating in the commit
message, i.e., this patch's changes to Zhaoxin CPUs include the previous
VIA CPUs.

Additionally, considering this change is to fix the CPUID which doesn't
match the bare metal, then what about changing the subject to

"target/i386: Mask CMPLegacy bit in CPUID[0x80000001].ecx for Zhaoxin/VIA
CPUs"?

Thanks,
Zhao

> > >                   *ecx |= 1 << 1;    /* CmpLegacy bit */
> > >               }
> > >           }
> > > --
> > > 2.34.1
> > > 
>
Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by Ewan Hai 3 months, 2 weeks ago 

On 8/8/24 23:22, Zhao Liu wrote:
>>> Hi EwanHai,
>>>
>>> On Thu, Jul 04, 2024 at 07:25:11AM -0400, EwanHai wrote:
>>>> Date: Thu, 4 Jul 2024 07:25:11 -0400
>>>> From: EwanHai <ewanhai-oc@zhaoxin.com>
>>>> Subject: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin
>>>>    CPUs
>>>> X-Mailer: git-send-email 2.34.1
>>>>
>>>> Zhaoxin CPUs handle the CMPLegacy bit in the same way
>>>> as Intel CPUs.
> Here it could be clearer to say "Don't set up CMPLegacy bit in
> CPUID[0x80000001].ecx for VIA/Zhaoxin CPUs".
Ok, I will change this statement.
>> This patch simplifies the existing logic by
>>>> using the IS_XXX_CPU macro and includes checks for Zhaoxin
>>>> vendor to align their behavior with Intel.
>>>>
>>>> Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
>>>> ---
>>>>    target/i386/cpu.c | 4 +---
>>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>>> index a3747fc487..c52a4cf3ba 100644
>>>> --- a/target/i386/cpu.c
>>>> +++ b/target/i386/cpu.c
>>>> @@ -6945,9 +6945,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>>>             * So don't set it here for Intel to make Linux guests happy.
>>>>             */
>>>>            if (threads_per_pkg > 1) {
>>>> -            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
>>>> -                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
>>>> -                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
>>>> +            if (!IS_INTEL_CPU(env) && !IS_ZHAOXIN_CPU(env)) {
>>> This change implicitly changes the behavior of existing VIA CPU.
>>>
>>> Is this a bug for the original VIA? If so, I suggest a separate patch to
>>> fix it and explain the effect on the VIA (Zhaoxin1) CPU.
>>>
>>> Regards,
>>> Zhao
>> The reason for this change is not due to a discovered bug, but rather
>> because both Centaurhauls and Shanghai CPUs follow Intel’s behavior
>> regarding the CMPLegacy bit. Specifically, AMD CPUs enumerate the
>> threads per package information in the CPUID leaf 0x80000001 output
>> ECX register, while Intel (and **other processors following Intel’s
>> behavior**) do not. Therefore, this modification is simply intended to
>> logically supplement the existing code.
> I see, thanks.
>
>> Given this, do you think it would be appropriate for me to submit
>> a separate patch to explain this behavior and its effect on
>> VIA (Zhaoxin1) CPUs? If so, I will submmit this change in a separate
>> patch.
> I think there's no need to split this.
>
> However, I think it's necessary to state the effect of the change in
> the changelog/commit message. It's also worth stating if it won't have
> any effect on the OS/software. Afterall, the comment of this bit said
> it affects Linux kernel.
>
> Also, changes to the old VIA behavior are worth stating in the commit
> message, i.e., this patch's changes to Zhaoxin CPUs include the previous
> VIA CPUs.
>
> Additionally, considering this change is to fix the CPUID which doesn't
> match the bare metal, then what about changing the subject to
>
> "target/i386: Mask CMPLegacy bit in CPUID[0x80000001].ecx for Zhaoxin/VIA
> CPUs"?
>
> Thanks,
> Zhao
Thank you for your suggestion; the changes will indeed make it clearer.
I have a question: since you’ve already added your reviewed-by tag to
the first three patches, if I want to modify these descriptions, should
I submit a v3 patchset containing all four patches, or should I only send a
new patch titled "target/i386: Mask CMPLegacy bit in CPUID[0x80000001].ecx
for Zhaoxin/Centaur CPUs"?
>>>>                    *ecx |= 1 << 1;    /* CmpLegacy bit */
>>>>                }
>>>>            }
>>>> --
>>>> 2.34.1
>>>>
Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by Zhao Liu 3 months, 2 weeks ago 
On Thu, Aug 08, 2024 at 11:25:45PM -0400, Ewan Hai wrote:

[snip]

> Thank you for your suggestion; the changes will indeed make it clearer.
> I have a question: since you’ve already added your reviewed-by tag to
> the first three patches, if I want to modify these descriptions, should
> I submit a v3 patchset containing all four patches, or should I only send a
> new patch titled "target/i386: Mask CMPLegacy bit in CPUID[0x80000001].ecx
> for Zhaoxin/Centaur CPUs"?

The v3 should contain all 4 patches, and you can add my R/b tag in the
first three patches.
Re: [PATCH v2 4/4] target/i386: Update CMPLegacy handling for Zhaoxin CPUs
Posted by Ewan Hai 3 months, 2 weeks ago 
On 8/8/24 23:47, Zhao Liu wrote:
> On Thu, Aug 08, 2024 at 11:25:45PM -0400, Ewan Hai wrote:
>
> [snip]
>
>> Thank you for your suggestion; the changes will indeed make it clearer.
>> I have a question: since you’ve already added your reviewed-by tag to
>> the first three patches, if I want to modify these descriptions, should
>> I submit a v3 patchset containing all four patches, or should I only send a
>> new patch titled "target/i386: Mask CMPLegacy bit in CPUID[0x80000001].ecx
>> for Zhaoxin/Centaur CPUs"?
> The v3 should contain all 4 patches, and you can add my R/b tag in the
> first three patches.
>
Thanks! See you in v3!

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.