This patch changes to use the processor extended information,
which can reflect the value from CPUID leaf 1FH.

The reasons are listed as below:
1. The processor package ID retrieved from CPUID 0x0Bh may be
not correct/accurate if CPU has the module & die info, it depends
on the CPUID implementation. See SDM statement:

EAX Bits 04 - 00: Number of bits to shift right on x2APIC ID to
get a unique topology ID of the next level type.
ECX Bits 15 - 08: level type

Level type field has the following encoding:
0: Invalid.
1: SMT.
2: Core.
3-255: Reserved

So, if level type returned from ECX Bits 15 - 08 is 2 (Core),
then it's not clear about the next level. It can be Module or
Die or Package. SDM doesn't has explanation for the next level
of Core. If so, the value will be decided by implementation.
The value can be package info for compatibility consideration,
but it's not standardized.

2. According SDM declaration, "CPUID leaf 1FH is a preferred
superset to leaf 0BH. Intel recommends first checking for the
existence of CPUID leaf 1FH before using leaf 0BH." This is
perfect match the existing GetProcessorLocation2ByApicId()
implementation.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 25d058c5b9..c61562c867 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -177,11 +177,11 @@ IsPackageFirstThread (
   IN UINTN  CpuIndex
   )
 {
   UINT32  PackageIndex;
 
-  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].Location.Package;
+  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].ExtendedInformation.Location2.Package;
 
   ASSERT (mPackageFirstThreadIndex != NULL);
 
   //
   // Set the value of mPackageFirstThreadIndex[PackageIndex].
@@ -1834,12 +1834,12 @@ InitPackageFirstThreadIndexInfo (
 
   //
   // Count the number of package, set to max PackageId + 1
   //
   for (Index = 0; Index < mNumberOfCpus; Index++) {
-    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].Location.Package) {
-      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].Location.Package;
+    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package) {
+      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package;
     }
   }
 
   PackageCount = PackageId + 1;
 
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111259): https://edk2.groups.io/g/devel/message/111259
Mute This Topic: https://groups.io/mt/102602853/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Wednesday, November 15, 2023 7:15 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

This patch changes to use the processor extended information,
which can reflect the value from CPUID leaf 1FH.

The reasons are listed as below:
1. The processor package ID retrieved from CPUID 0x0Bh may be
not correct/accurate if CPU has the module & die info, it depends
on the CPUID implementation. See SDM statement:

EAX Bits 04 - 00: Number of bits to shift right on x2APIC ID to
get a unique topology ID of the next level type.
ECX Bits 15 - 08: level type

Level type field has the following encoding:
0: Invalid.
1: SMT.
2: Core.
3-255: Reserved

So, if level type returned from ECX Bits 15 - 08 is 2 (Core),
then it's not clear about the next level. It can be Module or
Die or Package. SDM doesn't has explanation for the next level
of Core. If so, the value will be decided by implementation.
The value can be package info for compatibility consideration,
but it's not standardized.

2. According SDM declaration, "CPUID leaf 1FH is a preferred
superset to leaf 0BH. Intel recommends first checking for the
existence of CPUID leaf 1FH before using leaf 0BH." This is
perfect match the existing GetProcessorLocation2ByApicId()
implementation.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 25d058c5b9..c61562c867 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -177,11 +177,11 @@ IsPackageFirstThread (
   IN UINTN  CpuIndex
   )
 {
   UINT32  PackageIndex;

-  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].Location.Package;
+  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].ExtendedInformation.Location2.Package;

   ASSERT (mPackageFirstThreadIndex != NULL);

   //
   // Set the value of mPackageFirstThreadIndex[PackageIndex].
@@ -1834,12 +1834,12 @@ InitPackageFirstThreadIndexInfo (

   //
   // Count the number of package, set to max PackageId + 1
   //
   for (Index = 0; Index < mNumberOfCpus; Index++) {
-    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].Location.Package) {
-      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].Location.Package;
+    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package) {
+      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package;
     }
   }

   PackageCount = PackageId + 1;

--
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111297): https://edk2.groups.io/g/devel/message/111297
Mute This Topic: https://groups.io/mt/102602853/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 (#111414): https://edk2.groups.io/g/devel/message/111414
Mute This Topic: https://groups.io/mt/102602853/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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>
Sent: Saturday, November 18, 2023 5:05 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; 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

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 (#111432): https://edk2.groups.io/g/devel/message/111432
Mute This Topic: https://groups.io/mt/102602853/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

Merged the first two patches in the series as commits
ad0b1cc144b56fcbd8d369eaff6eaf5f3020efe7 and
7eb504060787c9c37d5b3c33f5d65021d553ea3f, via
<https://github.com/tianocore/edk2/pull/5125>.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112229): https://edk2.groups.io/g/devel/message/112229
Mute This Topic: https://groups.io/mt/102602853/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.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 (#111557): https://edk2.groups.io/g/devel/message/111557
Mute This Topic: https://groups.io/mt/102602853/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-