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

["Ni, Ray" <ray.ni@intel.com>]

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

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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