| Message ID | 20240625091905.1325205-5-ewanhai-oc@zhaoxin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Zhaoxin Yongfeng CPU model and other improvements | expand |
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 */ > } > }
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.
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.
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.
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)) { *ecx |= 1 << 1; /* CmpLegacy bit */ } }
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(-)