On 11/20/23 13:42, Wu, Jiaxin wrote: > For core id in cpu features library, I agree it should be not easy or > simple change to 0x1f. > > > > But in SMM CPU, there is no usage case depends on the number of cores > retrieved from cupid 0x0b return value, only PackageId will be used. So, > this patch doesn’t do bad things, should no regression issue. I agree > with Ray’s explanation that “CPUID.0B.PackageId == CPUID.1F.PackageId”, > thus no need update for the PackageId update. > > > > I checked the latest SDM: > > > > “The sub-leaves of CPUID leaf 0BH describe an ordered hierarchy of > logical processors starting from the smallest-scoped domain of a Logical > Processor (sub-leaf index 0) to the Core domain (sub-leaf index 1) to > the largest-scoped domain (the last valid sub-leaf index) **that is > implicitly subordinate to the unenumerated highest-scoped domain of the > processor package (socket)**” > > > > Looks it already updated to indicate the largest-scoped domain is package. > > > > With all above, I agree to drop this path, but other 2 patches in this > set should be ok. Thanks Ray help clarify this.
This is precisely the reason why I originally requested the now-last patch to be split off from the rest. I certainly didn't / couldn't go into such depths of CPUID.0B versus CPUID.1F discussion, but still that change looked very distinct from *populating* Location2 in the SMM-add-processor protocol member function (upon CPU hotplug). So, FWIW, I'm fine if the last patch in the series gets dropped. Thanks Laszlo > > > > Thanks, > > Jiaxin > > > > *From:* Ni, Ray <ray...@intel.com> > *Sent:* Monday, November 20, 2023 9:45 AM > *To:* Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Wu, Jiaxin > <jiaxin...@intel.com> > *Cc:* Dong, Eric <eric.d...@intel.com>; Kumar, Rahul R > <rahul.r.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Zeng, Star > <star.z...@intel.com> > *Subject:* Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: > Use processor extended information > > > > let me add more to explain: > > > > 1. CPUID.0B.PackageId == CPUID.1F.PackageId > > > > SDM clearly states the scope of every MSR (public): package, core, or > thread. > > But SDM doesn't emphasize that if a MSR is package scope, it's within > the package defined by CPUID.0B or CPUID.1F. > > That implies, CPUID.0B and CPUID.1F should return the same value for > package ID. > > > > Also, SDM has following statement to explain result of EAX for CPUID.0B > and CPUID.1F: > > Bits 04-00: The number of bits that the x2APIC ID must be shifted to > the right to address instances of the "*next higher-scoped"* domain. > > > > That means when CPUID.0B returns the EAX[04:00], it returns the total > bits of "thread", "core", "module", "tie", "die" because "package" is > the next higher-scoped domain. > > > > That also supports the idea: CPUID.0B.PackageId == CPUID.1F.PackageId. > > > > 2. CPU Feature Initialization > > > > In UefiCpuPkg/Include/RegisterCpuFeaturesLib.h, the following macros > were added to support consumers of RegisterCpuFeaturesLib specify > dependencies among different features. > > For example, when feature #a PACKAGE_BEFORE feature #b, #b is performed > in one thread of a package and after all threads have performed #a. > > That means internally multi-thread-sync is used to guarantee the > dependencies. > > #define CPU_FEATURE_THREAD_BEFORE BIT25 > > #define CPU_FEATURE_THREAD_AFTER BIT26 > > #define CPU_FEATURE_CORE_BEFORE BIT27 > > #define CPU_FEATURE_CORE_AFTER BIT28 > > #define CPU_FEATURE_PACKAGE_BEFORE BIT29 > > #define CPU_FEATURE_PACKAGE_AFTER BIT30 > > > > But above 3 sets of macro only define the dependencies between 3 scopes: > thread, core and package. > > Other scopes were not supported as there is no MSR which belongs to > other scopes at that moment, even today. > > So, the cpu features library implementation also only depends on CPUID.0B. > > If we update the code to get package id from CPUID.1F, to be consistent, > we should also get the core id from CPUID.1F. > > But if we do that, the number of cores which belong to the same domain > could be less in CPUID.1F. As CPUID.1F returns > the number of cores per module, instead of per package. > > That will break the MP sync logic which depends on the number of cores > per each domain. > > > > Conclusion: we should not update code to use CPUID.1F as it will break > the MP-sync logic in RegisterCpuFeaturesLib which is not aware of more > than 3 layers of scopes. > > > > Thanks, > > Ray > > > > ------------------------------------------------------------------------ > > *From:* Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com>> > *Sent:* Saturday, November 18, 2023 5:05 AM > *To:* devel@edk2.groups.io > <mailto:devel@edk2.groups.io><devel@edk2.groups.io > <mailto:devel@edk2.groups.io>>; Ni, Ray <ray...@intel.com > <mailto:ray...@intel.com>>; Wu, Jiaxin <jiaxin...@intel.com > <mailto:jiaxin...@intel.com>> > *Cc:* Dong, Eric <eric.d...@intel.com <mailto:eric.d...@intel.com>>; > Kumar, Rahul R <rahul.r.ku...@intel.com > <mailto:rahul.r.ku...@intel.com>>; Gerd Hoffmann <kra...@redhat.com > <mailto:kra...@redhat.com>>; Zeng, Star <star.z...@intel.com > <mailto:star.z...@intel.com>> > *Subject:* Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: > Use processor extended information > > > > On 11/16/23 02:30, Ni, Ray wrote: >> I cannot remember if CPUID.0B and CPUID.1F return the same value for >> package ID. >> >> And I am not sure about the benefit if we get the package id from > location2. > > Isn't the benefit that Location2 / CPUID leaf 1F is fully specified, > while leaf 0B isn't? From the commit message it seems we should always > prefer leaf 1F and Location2, even if we're not aware of concrete > problems with leaf 0B. > > Do you think we should only merge patches #1 and #2? > > Thanks, > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111557): https://edk2.groups.io/g/devel/message/111557 Mute This Topic: https://groups.io/mt/102602853/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
