| Message ID | 20240802072426.4016194-1-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386/cpu: Introduce enable_cpuid_0x1f to force exposing CPUID 0x1f | expand |
On Fri, 2 Aug 2024 03:24:26 -0400 Xiaoyao Li <xiaoyao.li@intel.com> wrote: > Currently, QEMU exposes CPUID 0x1f to guest only when necessary, i.e., > when topology level that cannot be enumerated by leaf 0xB, e.g., die or > module level, are configured for the guest. above implies that there could be a workaround to enable this leaf by tweaking -smp CLI, so I'd suggest to put it here. So users would be able to boot Windows without requiring to set this property. > However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU > topology. and 2) There is a bug in Windows that Windows expects valid > 0x1f leafs when the maximum basic leaf > 0x1f[1]. Which Windows versions are affected by this? > Introduce a bool flag enable_cpuid_0x1f in CPU for the cases that > requires CPUID leaf 0x1f to be exposed to guest. For case 2), introduce > a user settable property as well, which provides an opt-in interface > for people to run the buggy Windows as a workaround. The default value > of the property is set to false, thus making no effect on existing > setup. > Opportunistically rename x86_has_extended_topo() to > x86_has_v2_extended_topo() because per Intel SDM, leaf 0xb is for extended > topology enumeration leaf and leaf 0x1f is v2 extended topology enumration > leaf. I don't see any point in renaming, just drop it > [1] https://lore.kernel.org/qemu-devel/21ca5c19-677b-4fac-84d4-72413577f260@nutanix.com/ > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > include/hw/i386/topology.h | 9 --------- > target/i386/cpu.c | 18 ++++++++++++++++-- > target/i386/cpu.h | 4 ++++ > target/i386/kvm/kvm.c | 2 +- > 4 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h > index dff49fce1154..b63bce2f4c82 100644 > --- a/include/hw/i386/topology.h > +++ b/include/hw/i386/topology.h > @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, > return x86_apicid_from_topo_ids(topo_info, &topo_ids); > } > > -/* > - * Check whether there's extended topology level (module or die)? > - */ > -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) > -{ > - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || > - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); > -} > - > #endif /* HW_I386_TOPOLOGY_H */ > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 4688d140c2d9..b5b7ac96c272 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -300,6 +300,19 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache, > (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0); > } > > +/* > + * Check whether there's v2 extended topology level (module or die)? > + */ > +bool x86_has_v2_extended_topo(X86CPU *cpu) > +{ > + if (cpu->enable_cpuid_0x1f) { > + return true; > + } > + > + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || > + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); > +} > + > static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info, > enum CPUTopoLevel topo_level) > { > @@ -6637,7 +6650,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > break; > case 0x1F: > /* V2 Extended Topology Enumeration Leaf */ > - if (!x86_has_extended_topo(env->avail_cpu_topo)) { > + if (!x86_has_v2_extended_topo(cpu)) { > *eax = *ebx = *ecx = *edx = 0; > break; > } > @@ -7450,7 +7463,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp) > * cpu->vendor_cpuid_only has been unset for compatibility with older > * machine types. > */ > - if (x86_has_extended_topo(env->avail_cpu_topo) && > + if (x86_has_v2_extended_topo(cpu) && > (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) { > x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F); > } > @@ -8316,6 +8329,7 @@ static Property x86_cpu_properties[] = { > DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true), > DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor), > DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true), > + DEFINE_PROP_BOOL("cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false), if we ever add this knob, rename it to 'x-cpuid-0x1f' meaning: internal/unstable: use it on your own risk subject to removal without notice > DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true), > DEFINE_PROP_BOOL("x-amd-topoext-features-only", X86CPU, amd_topoext_features_only, true), > DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false), > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index c6cc035df3d8..211a42ffbfa6 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -2110,6 +2110,9 @@ struct ArchCPU { > /* Compatibility bits for old machine types: */ > bool enable_cpuid_0xb; > > + /* Force to expose cpuid 0x1f */ > + bool enable_cpuid_0x1f; > + > /* Enable auto level-increase for all CPUID leaves */ > bool full_cpuid_auto_level; > > @@ -2368,6 +2371,7 @@ void cpu_set_apic_feature(CPUX86State *env); > void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); > bool cpu_has_x2apic_feature(CPUX86State *env); > +bool x86_has_v2_extended_topo(X86CPU *cpu); > > /* helper.c */ > void x86_cpu_set_a20(X86CPU *cpu, int a20_state); > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index b4aab9a410b5..d43c1fa26746 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1835,7 +1835,7 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env, > break; > } > case 0x1f: > - if (!x86_has_extended_topo(env->avail_cpu_topo)) { > + if (!x86_has_v2_extended_topo(env_archcpu(env))) { > cpuid_i--; > break; > }
Hi Xiaoyao, Patch is generally fine for me. Just a few nits: On Fri, Aug 02, 2024 at 03:24:26AM -0400, Xiaoyao Li wrote: > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h > index dff49fce1154..b63bce2f4c82 100644 > --- a/include/hw/i386/topology.h > +++ b/include/hw/i386/topology.h > @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, > return x86_apicid_from_topo_ids(topo_info, &topo_ids); > } > > -/* > - * Check whether there's extended topology level (module or die)? > - */ > -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) > -{ > - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || > - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); > -} > - [snip] > +/* > + * Check whether there's v2 extended topology level (module or die)? > + */ > +bool x86_has_v2_extended_topo(X86CPU *cpu) > +{ > + if (cpu->enable_cpuid_0x1f) { > + return true; > + } > + > + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || > + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); > +} > + I suggest to decouple 0x1f enablement and extended topo check, since as the comment of CPUTopoLevel said: /* * CPUTopoLevel is the general i386 topology hierarchical representation, * ordered by increasing hierarchical relationship. * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F]) * or AMD (CPUID[0x80000026]). */ The topology enumeration is generic and is not bound to the vendor. [snip] > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index c6cc035df3d8..211a42ffbfa6 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -2110,6 +2110,9 @@ struct ArchCPU { > /* Compatibility bits for old machine types: */ > bool enable_cpuid_0xb; > > + /* Force to expose cpuid 0x1f */ Maybe "Force to enable cpuid 0x1f"? > + bool enable_cpuid_0x1f; > + > /* Enable auto level-increase for all CPUID leaves */ > bool full_cpuid_auto_level;i Regards, Zhao
On 8/8/2024 6:09 PM, Zhao Liu wrote: > Hi Xiaoyao, > > Patch is generally fine for me. Just a few nits: > > On Fri, Aug 02, 2024 at 03:24:26AM -0400, Xiaoyao Li wrote: >> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h >> index dff49fce1154..b63bce2f4c82 100644 >> --- a/include/hw/i386/topology.h >> +++ b/include/hw/i386/topology.h >> @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, >> return x86_apicid_from_topo_ids(topo_info, &topo_ids); >> } >> >> -/* >> - * Check whether there's extended topology level (module or die)? >> - */ >> -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) >> -{ >> - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || >> - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); >> -} >> - > > [snip] > >> +/* >> + * Check whether there's v2 extended topology level (module or die)? >> + */ >> +bool x86_has_v2_extended_topo(X86CPU *cpu) >> +{ >> + if (cpu->enable_cpuid_0x1f) { >> + return true; >> + } >> + >> + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || >> + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); >> +} >> + > > I suggest to decouple 0x1f enablement and extended topo check, since as > the comment of CPUTopoLevel said: > > /* > * CPUTopoLevel is the general i386 topology hierarchical representation, > * ordered by increasing hierarchical relationship. > * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F]) > * or AMD (CPUID[0x80000026]). > */ > > The topology enumeration is generic and is not bound to the vendor. I don't quit get your point. All the current usages of x86_has_extended_topo() are for CPUID leaf 0x1f, which is an Intel specific leaf. Are you saying x86_has_extended_topo() will be used for leaf 0x80000026 for AMD as well? or maybe I misunderstand the meaning "extend_topo". The extend_topo just means the topo level of module and die, and the topo level of smt and core are non-extended? If so, this is new to me, could I ask where the definitions come from? or just QEMU defines them itself? > [snip] > >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> index c6cc035df3d8..211a42ffbfa6 100644 >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -2110,6 +2110,9 @@ struct ArchCPU { >> /* Compatibility bits for old machine types: */ >> bool enable_cpuid_0xb; >> >> + /* Force to expose cpuid 0x1f */ > > Maybe "Force to enable cpuid 0x1f"? I can change to it. >> + bool enable_cpuid_0x1f; >> + >> /* Enable auto level-increase for all CPUID leaves */ >> bool full_cpuid_auto_level;i > > Regards, > Zhao >
On 8/8/2024 5:29 PM, Igor Mammedov wrote: > On Fri, 2 Aug 2024 03:24:26 -0400 > Xiaoyao Li <xiaoyao.li@intel.com> wrote: > >> Currently, QEMU exposes CPUID 0x1f to guest only when necessary, i.e., >> when topology level that cannot be enumerated by leaf 0xB, e.g., die or >> module level, are configured for the guest. > > above implies that there could be a workaround to enable this leaf by > tweaking -smp CLI, so I'd suggest to put it here. So users would be able > to boot Windows without requiring to set this property. sure. will add such statement. >> However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU >> topology. and 2) There is a bug in Windows that Windows expects valid >> 0x1f leafs when the maximum basic leaf > 0x1f[1]. > > Which Windows versions are affected by this? @Manish and @John, can you answer it? >> Introduce a bool flag enable_cpuid_0x1f in CPU for the cases that >> requires CPUID leaf 0x1f to be exposed to guest. For case 2), introduce >> a user settable property as well, which provides an opt-in interface >> for people to run the buggy Windows as a workaround. The default value >> of the property is set to false, thus making no effect on existing >> setup. > > >> Opportunistically rename x86_has_extended_topo() to >> x86_has_v2_extended_topo() because per Intel SDM, leaf 0xb is for extended >> topology enumeration leaf and leaf 0x1f is v2 extended topology enumration >> leaf. > I don't see any point in renaming, just drop it ok > >> [1] https://lore.kernel.org/qemu-devel/21ca5c19-677b-4fac-84d4-72413577f260@nutanix.com/ >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> include/hw/i386/topology.h | 9 --------- >> target/i386/cpu.c | 18 ++++++++++++++++-- >> target/i386/cpu.h | 4 ++++ >> target/i386/kvm/kvm.c | 2 +- >> 4 files changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h >> index dff49fce1154..b63bce2f4c82 100644 >> --- a/include/hw/i386/topology.h >> +++ b/include/hw/i386/topology.h >> @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, >> return x86_apicid_from_topo_ids(topo_info, &topo_ids); >> } >> >> -/* >> - * Check whether there's extended topology level (module or die)? >> - */ >> -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) >> -{ >> - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || >> - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); >> -} >> - >> #endif /* HW_I386_TOPOLOGY_H */ >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 4688d140c2d9..b5b7ac96c272 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -300,6 +300,19 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache, >> (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0); >> } >> >> +/* >> + * Check whether there's v2 extended topology level (module or die)? >> + */ >> +bool x86_has_v2_extended_topo(X86CPU *cpu) >> +{ >> + if (cpu->enable_cpuid_0x1f) { >> + return true; >> + } >> + >> + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || >> + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); >> +} >> + >> static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info, >> enum CPUTopoLevel topo_level) >> { >> @@ -6637,7 +6650,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, >> break; >> case 0x1F: >> /* V2 Extended Topology Enumeration Leaf */ >> - if (!x86_has_extended_topo(env->avail_cpu_topo)) { >> + if (!x86_has_v2_extended_topo(cpu)) { >> *eax = *ebx = *ecx = *edx = 0; >> break; >> } >> @@ -7450,7 +7463,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp) >> * cpu->vendor_cpuid_only has been unset for compatibility with older >> * machine types. >> */ >> - if (x86_has_extended_topo(env->avail_cpu_topo) && >> + if (x86_has_v2_extended_topo(cpu) && >> (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) { >> x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F); >> } >> @@ -8316,6 +8329,7 @@ static Property x86_cpu_properties[] = { >> DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true), >> DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor), >> DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true), >> + DEFINE_PROP_BOOL("cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false), > if we ever add this knob, rename it to 'x-cpuid-0x1f' > meaning: internal/unstable: use it on your own risk subject > to removal without notice I agree it now. Because exposing it as a user-settable interface is only for the case of Windows bug. For TDX case, QEMU can set it internally. >> DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true), >> DEFINE_PROP_BOOL("x-amd-topoext-features-only", X86CPU, amd_topoext_features_only, true), >> DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false), >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> index c6cc035df3d8..211a42ffbfa6 100644 >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -2110,6 +2110,9 @@ struct ArchCPU { >> /* Compatibility bits for old machine types: */ >> bool enable_cpuid_0xb; >> >> + /* Force to expose cpuid 0x1f */ >> + bool enable_cpuid_0x1f; >> + >> /* Enable auto level-increase for all CPUID leaves */ >> bool full_cpuid_auto_level; >> >> @@ -2368,6 +2371,7 @@ void cpu_set_apic_feature(CPUX86State *env); >> void host_cpuid(uint32_t function, uint32_t count, >> uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); >> bool cpu_has_x2apic_feature(CPUX86State *env); >> +bool x86_has_v2_extended_topo(X86CPU *cpu); >> >> /* helper.c */ >> void x86_cpu_set_a20(X86CPU *cpu, int a20_state); >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c >> index b4aab9a410b5..d43c1fa26746 100644 >> --- a/target/i386/kvm/kvm.c >> +++ b/target/i386/kvm/kvm.c >> @@ -1835,7 +1835,7 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env, >> break; >> } >> case 0x1f: >> - if (!x86_has_extended_topo(env->avail_cpu_topo)) { >> + if (!x86_has_v2_extended_topo(env_archcpu(env))) { >> cpuid_i--; >> break; >> } >
On Thu, Aug 08, 2024 at 09:59:07PM +0800, Xiaoyao Li wrote: > Date: Thu, 8 Aug 2024 21:59:07 +0800 > From: Xiaoyao Li <xiaoyao.li@intel.com> > Subject: Re: [PATCH] i386/cpu: Introduce enable_cpuid_0x1f to force > exposing CPUID 0x1f > > On 8/8/2024 6:09 PM, Zhao Liu wrote: > > Hi Xiaoyao, > > > > Patch is generally fine for me. Just a few nits: > > > > On Fri, Aug 02, 2024 at 03:24:26AM -0400, Xiaoyao Li wrote: > > > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h > > > index dff49fce1154..b63bce2f4c82 100644 > > > --- a/include/hw/i386/topology.h > > > +++ b/include/hw/i386/topology.h > > > @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, > > > return x86_apicid_from_topo_ids(topo_info, &topo_ids); > > > } > > > -/* > > > - * Check whether there's extended topology level (module or die)? > > > - */ > > > -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) > > > -{ > > > - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || > > > - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); > > > -} > > > - > > > > [snip] > > > > > +/* > > > + * Check whether there's v2 extended topology level (module or die)? > > > + */ > > > +bool x86_has_v2_extended_topo(X86CPU *cpu) > > > +{ > > > + if (cpu->enable_cpuid_0x1f) { > > > + return true; > > > + } > > > + > > > + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || > > > + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); > > > +} > > > + > > > > I suggest to decouple 0x1f enablement and extended topo check, since as > > the comment of CPUTopoLevel said: > > > > /* > > * CPUTopoLevel is the general i386 topology hierarchical representation, > > * ordered by increasing hierarchical relationship. > > * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F]) > > * or AMD (CPUID[0x80000026]). > > */ > > > > The topology enumeration is generic and is not bound to the vendor. > > I don't quit get your point. All the current usages of > x86_has_extended_topo() are for CPUID leaf 0x1f, which is an Intel specific > leaf. 0x1f is just a user of that helper, and AMD's leaf would be another potential user, even if it is not implemented yet. What this helper does is check the topology hierarchy set by -smp for x86 CPUs, and has nothing to do with enabling 0x1f or not. You cannot falsely report the presence of module/die if -smp doesn't configure such levels but 0x1f is forcibly enabled. > Are you saying x86_has_extended_topo() will be used for leaf 0x80000026 for > AMD as well? > > or maybe I misunderstand the meaning "extend_topo". The extend_topo just > means the topo level of module and die, and the topo level of smt and core > are non-extended? Any levels that 0xb doesn't cover. > If so, this is new to me, could I ask where the > definitions come from? or just QEMU defines them itself? > > > [snip] > > > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > > index c6cc035df3d8..211a42ffbfa6 100644 > > > --- a/target/i386/cpu.h > > > +++ b/target/i386/cpu.h > > > @@ -2110,6 +2110,9 @@ struct ArchCPU { > > > /* Compatibility bits for old machine types: */ > > > bool enable_cpuid_0xb; > > > + /* Force to expose cpuid 0x1f */ > > > > Maybe "Force to enable cpuid 0x1f"? > > I can change to it. > > > > + bool enable_cpuid_0x1f; > > > + > > > /* Enable auto level-increase for all CPUID leaves */ > > > bool full_cpuid_auto_level;i > > > > Regards, > > Zhao > > >
On 8/8/2024 10:46 PM, Zhao Liu wrote: > On Thu, Aug 08, 2024 at 09:59:07PM +0800, Xiaoyao Li wrote: >> Date: Thu, 8 Aug 2024 21:59:07 +0800 >> From: Xiaoyao Li <xiaoyao.li@intel.com> >> Subject: Re: [PATCH] i386/cpu: Introduce enable_cpuid_0x1f to force >> exposing CPUID 0x1f >> >> On 8/8/2024 6:09 PM, Zhao Liu wrote: >>> Hi Xiaoyao, >>> >>> Patch is generally fine for me. Just a few nits: >>> >>> On Fri, Aug 02, 2024 at 03:24:26AM -0400, Xiaoyao Li wrote: >>>> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h >>>> index dff49fce1154..b63bce2f4c82 100644 >>>> --- a/include/hw/i386/topology.h >>>> +++ b/include/hw/i386/topology.h >>>> @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, >>>> return x86_apicid_from_topo_ids(topo_info, &topo_ids); >>>> } >>>> -/* >>>> - * Check whether there's extended topology level (module or die)? >>>> - */ >>>> -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) >>>> -{ >>>> - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || >>>> - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); >>>> -} >>>> - >>> >>> [snip] >>> >>>> +/* >>>> + * Check whether there's v2 extended topology level (module or die)? >>>> + */ >>>> +bool x86_has_v2_extended_topo(X86CPU *cpu) >>>> +{ >>>> + if (cpu->enable_cpuid_0x1f) { >>>> + return true; >>>> + } >>>> + >>>> + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || >>>> + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); >>>> +} >>>> + >>> >>> I suggest to decouple 0x1f enablement and extended topo check, since as >>> the comment of CPUTopoLevel said: >>> >>> /* >>> * CPUTopoLevel is the general i386 topology hierarchical representation, >>> * ordered by increasing hierarchical relationship. >>> * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F]) >>> * or AMD (CPUID[0x80000026]). >>> */ >>> >>> The topology enumeration is generic and is not bound to the vendor. >> >> I don't quit get your point. All the current usages of >> x86_has_extended_topo() are for CPUID leaf 0x1f, which is an Intel specific >> leaf. > > 0x1f is just a user of that helper, and AMD's leaf would be another > potential user, even if it is not implemented yet. > > What this helper does is check the topology hierarchy set by -smp for > x86 CPUs, and has nothing to do with enabling 0x1f or not. You cannot > falsely report the presence of module/die if -smp doesn't configure such > levels but 0x1f is forcibly enabled. > >> Are you saying x86_has_extended_topo() will be used for leaf 0x80000026 for >> AMD as well? >> >> or maybe I misunderstand the meaning "extend_topo". The extend_topo just >> means the topo level of module and die, and the topo level of smt and core >> are non-extended? > > Any levels that 0xb doesn't cover. The name of extended_topo is so misleading. At least, it misleads me. Both Intel and AMD support leaf 0xb and the name of leaf 0xb is "Extended topology enumeration". And here, x86_has_extended_topo() is used for topo levels that cannot be covered by 0xb. >> If so, this is new to me, could I ask where the >> definitions come from? or just QEMU defines them itself? >> >>> [snip] >>> >>>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >>>> index c6cc035df3d8..211a42ffbfa6 100644 >>>> --- a/target/i386/cpu.h >>>> +++ b/target/i386/cpu.h >>>> @@ -2110,6 +2110,9 @@ struct ArchCPU { >>>> /* Compatibility bits for old machine types: */ >>>> bool enable_cpuid_0xb; >>>> + /* Force to expose cpuid 0x1f */ >>> >>> Maybe "Force to enable cpuid 0x1f"? >> >> I can change to it. >> >>>> + bool enable_cpuid_0x1f; >>>> + >>>> /* Enable auto level-increase for all CPUID leaves */ >>>> bool full_cpuid_auto_level;i >>> >>> Regards, >>> Zhao >>> >>
On Tue, Aug 13, 2024 at 10:52:27AM +0800, Xiaoyao Li wrote: [snip] > > Any levels that 0xb doesn't cover. > > The name of extended_topo is so misleading. At least, it misleads me. > > Both Intel and AMD support leaf 0xb and the name of leaf 0xb is "Extended > topology enumeration". And here, x86_has_extended_topo() is used for topo > levels that cannot be covered by 0xb. > Yes, names are really hard, Intel and AMD have different naming for topology leafs (the ones 0xb doesn't cover)... This helper has a comment, which is also a clear expression of what it is doing. Thanks, Zhao
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h index dff49fce1154..b63bce2f4c82 100644 --- a/include/hw/i386/topology.h +++ b/include/hw/i386/topology.h @@ -207,13 +207,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info, return x86_apicid_from_topo_ids(topo_info, &topo_ids); } -/* - * Check whether there's extended topology level (module or die)? - */ -static inline bool x86_has_extended_topo(unsigned long *topo_bitmap) -{ - return test_bit(CPU_TOPO_LEVEL_MODULE, topo_bitmap) || - test_bit(CPU_TOPO_LEVEL_DIE, topo_bitmap); -} - #endif /* HW_I386_TOPOLOGY_H */ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 4688d140c2d9..b5b7ac96c272 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -300,6 +300,19 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache, (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0); } +/* + * Check whether there's v2 extended topology level (module or die)? + */ +bool x86_has_v2_extended_topo(X86CPU *cpu) +{ + if (cpu->enable_cpuid_0x1f) { + return true; + } + + return test_bit(CPU_TOPO_LEVEL_MODULE, cpu->env.avail_cpu_topo) || + test_bit(CPU_TOPO_LEVEL_DIE, cpu->env.avail_cpu_topo); +} + static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info, enum CPUTopoLevel topo_level) { @@ -6637,7 +6650,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break; case 0x1F: /* V2 Extended Topology Enumeration Leaf */ - if (!x86_has_extended_topo(env->avail_cpu_topo)) { + if (!x86_has_v2_extended_topo(cpu)) { *eax = *ebx = *ecx = *edx = 0; break; } @@ -7450,7 +7463,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp) * cpu->vendor_cpuid_only has been unset for compatibility with older * machine types. */ - if (x86_has_extended_topo(env->avail_cpu_topo) && + if (x86_has_v2_extended_topo(cpu) && (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) { x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F); } @@ -8316,6 +8329,7 @@ static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true), DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor), DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true), + DEFINE_PROP_BOOL("cpuid-0x1f", X86CPU, enable_cpuid_0x1f, false), DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true), DEFINE_PROP_BOOL("x-amd-topoext-features-only", X86CPU, amd_topoext_features_only, true), DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false), diff --git a/target/i386/cpu.h b/target/i386/cpu.h index c6cc035df3d8..211a42ffbfa6 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -2110,6 +2110,9 @@ struct ArchCPU { /* Compatibility bits for old machine types: */ bool enable_cpuid_0xb; + /* Force to expose cpuid 0x1f */ + bool enable_cpuid_0x1f; + /* Enable auto level-increase for all CPUID leaves */ bool full_cpuid_auto_level; @@ -2368,6 +2371,7 @@ void cpu_set_apic_feature(CPUX86State *env); void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); bool cpu_has_x2apic_feature(CPUX86State *env); +bool x86_has_v2_extended_topo(X86CPU *cpu); /* helper.c */ void x86_cpu_set_a20(X86CPU *cpu, int a20_state); diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index b4aab9a410b5..d43c1fa26746 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1835,7 +1835,7 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env, break; } case 0x1f: - if (!x86_has_extended_topo(env->avail_cpu_topo)) { + if (!x86_has_v2_extended_topo(env_archcpu(env))) { cpuid_i--; break; }
Currently, QEMU exposes CPUID 0x1f to guest only when necessary, i.e., when topology level that cannot be enumerated by leaf 0xB, e.g., die or module level, are configured for the guest. However, 1) TDX architecture forces to require CPUID 0x1f to configure CPU topology. and 2) There is a bug in Windows that Windows expects valid 0x1f leafs when the maximum basic leaf > 0x1f[1]. Introduce a bool flag enable_cpuid_0x1f in CPU for the cases that requires CPUID leaf 0x1f to be exposed to guest. For case 2), introduce a user settable property as well, which provides an opt-in interface for people to run the buggy Windows as a workaround. The default value of the property is set to false, thus making no effect on existing setup. Opportunistically rename x86_has_extended_topo() to x86_has_v2_extended_topo() because per Intel SDM, leaf 0xb is for extended topology enumeration leaf and leaf 0x1f is v2 extended topology enumration leaf. [1] https://lore.kernel.org/qemu-devel/21ca5c19-677b-4fac-84d4-72413577f260@nutanix.com/ Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- include/hw/i386/topology.h | 9 --------- target/i386/cpu.c | 18 ++++++++++++++++-- target/i386/cpu.h | 4 ++++ target/i386/kvm/kvm.c | 2 +- 4 files changed, 21 insertions(+), 12 deletions(-)