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

[Ewan Hai]

On 7/3/24 23:19, Xiaoyao Li wrote:
> On 7/4/2024 11:14 AM, Ewan Hai wrote:
> > On 7/3/24 10:49, Xiaoyao Li wrote:
> > > On 6/25/2024 5:19 PM, EwanHai wrote:
> > > > Zhaoxin and VIA 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
> > > > and VIA vendors to align their behavior with Intel.
> > > >
> > > > Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
> > > > ---
> > > >   target/i386/cpu.c | 6 +++---
> > > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index 50edff077e..0836416617 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -6945,9 +6945,9 @@ 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) &&
> > > > +                !IS_VIA_CPU(env)) {
> > >
> > > it seems you added ! by mistake.
> > >
> > > >                   *ecx |= 1 << 1; /* CmpLegacy bit */
> > > >               }
> > > >           }
> > For CPUID leaf 0x80000001 ECX bit 1, Intel defines it as "Bits 04-01:
> > Reserved,"
> > whereas AMD defines it as "CmpLegacy, Core multi-processing legacy
> > mode." For Intel
> > CPUs and those following Intel's behavior, this bit should not be set to
> > 1. Therefore,
> > I believe the "!" here is correct.
>
> Sorry, I misread the original code.
>
> I think maybe we can just use is_AMD_CPU(). But I'm not sure if any
> magic use case with customized VENDOR ID relies on it. So you code looks
> good to me.
Ok, thanks. Additionally, in this patch series, I used some VIA terms, which
might cause confusion. I will remove all VIA references in the description
of the next version of the patch.
Currently, the “Centaurhauls” Vendor ID belongs to Zhaoxin CPUs.