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

["Laszlo Ersek" <lersek@redhat.com>]

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