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.
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 (#111472): https://edk2.groups.io/g/devel/message/111472
Mute This Topic: https://groups.io/mt/102602853/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
