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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
