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

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

On 11/8/23 05:11, Wu, Jiaxin wrote:
> Hi Laszlo,
> 
>>>
>>> The patch looks OK to me, but:
>>>
>>> - I would like to test it with CPU hotplug (later, likely under v2), and
>>>
> 
> Sure, I can wait the update from you.
> 
>>> - I think this should be two patches.
>>>
>>> First, the SmmAddProcessor() function should be extended just to
>>> complete commit 1fadd18d. (BTW I highly appreciate the reference to
>>> commit 1fadd18d; otherwise I couldn't find where the *coldplugged* CPUs'
>>> locations were retrieved!)
>>>
>>> Then the Package calculations should be updated separately -- mostly
>>> because I would appreciate a concrete description in that separate
>>> commit message why the difference matters. Clearly you have a use case
>>> where the v1 and v2 package numbers differ, and recording that in the
>>> commit history would be great.
> 
> Sure, let me explain more, there are 2 reason I did this change:
> 
> 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 what's the next level mean? 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. That's the reason we suggest use the leaf 1Fh.
>    
> 2. And 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. 
> 
> That's the main reasons we switch to EFI_CPU_PHYSICAL_LOCATION2.
> 
>>
>> Side note, just for completeness: the x2apic lib instance performs the
>> v2 feature detection correctly since Gerd's commit 170d4ce8e90a
>> ("UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY
>> detection", 2023-10-25). Furthermore, OVMF uses the x2apic lib instance
>> since commit decb365b0016 ("OvmfPkg: select LocalApicLib instance with
>> x2apic support", 2015-11-30). Therefore, this patch looks fine for OVMF.
>>
>> However, for platforms that use the old xapic lib instance, there could
>> be problems, as the v2 feature detection in *that* instance is not fixed
>> -- it does not check EBX.
>>
> 
> Great catch this! I can create the patch 3 for this porting to old xapic lib instance if you no objection.

Sure, sounds good, although I have no way of testing that.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111144): https://edk2.groups.io/g/devel/message/111144
Mute This Topic: https://groups.io/mt/102436095/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-