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.
Thanks,
Jiaxin
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110893): https://edk2.groups.io/g/devel/message/110893
Mute This Topic: https://groups.io/mt/102436095/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
