Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5490 bytes --]

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.ni@intel.com>
Sent: Monday, November 20, 2023 9:45 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Zeng, Star <star.zeng@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 <lersek@redhat.com<mailto:lersek@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.ni@intel.com<mailto:ray.ni@intel.com>>; Wu, Jiaxin <jiaxin.wu@intel.com<mailto:jiaxin.wu@intel.com>>
Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 18915 bytes --]