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

[Ewan Hai]

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