On 11/7/23 19:40, Laszlo Ersek wrote:
> On 11/7/23 03:43, Wu, Jiaxin wrote:
>> Processor extended information is filled when
>> CPU_V2_EXTENDED_TOPOLOGY is set in parameter ProcessorNumber
>> from GetProcessorInfo() (See commit: 1fadd18d).
>>
>> This filed value is retrieved from CPUID leaf 1FH, which is
>> a preferred superset to leaf 0BH.
>>
>> Since Intel recommends first use the CPUID leaf 1FH instead of
>> leaf 0BH, this patch change to use the processor extended
>> information, which can reflect the value from CPUID leaf 1FH.
>>
>> Cc: Eric Dong <eric.d...@intel.com>
>> Cc: Ray Ni <ray...@intel.com>
>> Cc: Rahul Kumar <rahul1.ku...@intel.com>
>> Cc: Gerd Hoffmann <kra...@redhat.com>
>> Cc: Star Zeng <star.z...@intel.com>
>> Signed-off-by: Jiaxin Wu <jiaxin...@intel.com>
>> ---
>> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 10 ++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 +++---
>> 2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>> index 391b64e9f2..c0485b0519 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>> @@ -169,10 +169,20 @@ SmmAddProcessor (
>> &gSmmCpuPrivate->ProcessorInfo[Index].Location.Package,
>> &gSmmCpuPrivate->ProcessorInfo[Index].Location.Core,
>> &gSmmCpuPrivate->ProcessorInfo[Index].Location.Thread
>> );
>>
>> + GetProcessorLocation2ByApicId (
>> + (UINT32)ProcessorId,
>> +
>> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package,
>> +
>> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Die,
>> +
>> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Tile,
>> +
>> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Module,
>> +
>> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Core,
>> +
>> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Thread
>> + );
>> +
>> *ProcessorNumber = Index;
>> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
>> return EFI_SUCCESS;
>> }
>> }
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> index 25d058c5b9..c61562c867 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> @@ -177,11 +177,11 @@ IsPackageFirstThread (
>> IN UINTN CpuIndex
>> )
>> {
>> UINT32 PackageIndex;
>>
>> - PackageIndex = gSmmCpuPrivate->ProcessorInfo[CpuIndex].Location.Package;
>> + PackageIndex =
>> gSmmCpuPrivate->ProcessorInfo[CpuIndex].ExtendedInformation.Location2.Package;
>>
>> ASSERT (mPackageFirstThreadIndex != NULL);
>>
>> //
>> // Set the value of mPackageFirstThreadIndex[PackageIndex].
>> @@ -1834,12 +1834,12 @@ InitPackageFirstThreadIndexInfo (
>>
>> //
>> // Count the number of package, set to max PackageId + 1
>> //
>> for (Index = 0; Index < mNumberOfCpus; Index++) {
>> - if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].Location.Package) {
>> - PackageId = gSmmCpuPrivate->ProcessorInfo[Index].Location.Package;
>> + if (PackageId <
>> gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package) {
>> + PackageId =
>> gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package;
>> }
>> }
>>
>> PackageCount = PackageId + 1;
>>
>
> The patch looks OK to me, but:
>
> - I would like to test it with CPU hotplug (later, likely under v2), and
>
> - 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.
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.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110873): https://edk2.groups.io/g/devel/message/110873
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]
-=-=-=-=-=-=-=-=-=-=-=-
