* [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information @ 2023-11-07 2:43 Wu, Jiaxin 2023-11-07 18:40 ` Laszlo Ersek 2023-11-13 15:38 ` Laszlo Ersek 0 siblings, 2 replies; 7+ messages in thread From: Wu, Jiaxin @ 2023-11-07 2:43 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Star Zeng 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.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Star Zeng <star.zeng@intel.com> Signed-off-by: Jiaxin Wu <jiaxin.wu@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; -- 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110786): https://edk2.groups.io/g/devel/message/110786 Mute This Topic: https://groups.io/mt/102436095/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information 2023-11-07 2:43 [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information Wu, Jiaxin @ 2023-11-07 18:40 ` Laszlo Ersek 2023-11-07 18:44 ` Laszlo Ersek 2023-11-13 15:38 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2023-11-07 18:40 UTC (permalink / raw) To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Star Zeng 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.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Star Zeng <star.zeng@intel.com> > Signed-off-by: Jiaxin Wu <jiaxin.wu@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. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110872): https://edk2.groups.io/g/devel/message/110872 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information 2023-11-07 18:40 ` Laszlo Ersek @ 2023-11-07 18:44 ` Laszlo Ersek 2023-11-08 4:11 ` Wu, Jiaxin 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2023-11-07 18:44 UTC (permalink / raw) To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Star Zeng 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.dong@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Rahul Kumar <rahul1.kumar@intel.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Star Zeng <star.zeng@intel.com> >> Signed-off-by: Jiaxin Wu <jiaxin.wu@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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information 2023-11-07 18:44 ` Laszlo Ersek @ 2023-11-08 4:11 ` Wu, Jiaxin 2023-11-13 11:31 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Wu, Jiaxin @ 2023-11-08 4:11 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Dong, Eric, Ni, Ray, Kumar, Rahul R, Gerd Hoffmann, Zeng, Star 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information 2023-11-08 4:11 ` Wu, Jiaxin @ 2023-11-13 11:31 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2023-11-13 11:31 UTC (permalink / raw) To: devel, jiaxin.wu Cc: Dong, Eric, Ni, Ray, Kumar, Rahul R, Gerd Hoffmann, Zeng, Star 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information 2023-11-07 2:43 [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information Wu, Jiaxin 2023-11-07 18:40 ` Laszlo Ersek @ 2023-11-13 15:38 ` Laszlo Ersek 2023-11-13 15:38 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2023-11-13 15:38 UTC (permalink / raw) To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Star Zeng 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.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Star Zeng <star.zeng@intel.com> > Signed-off-by: Jiaxin Wu <jiaxin.wu@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; > Regression-tested-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111161): https://edk2.groups.io/g/devel/message/111161 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information 2023-11-13 15:38 ` Laszlo Ersek @ 2023-11-13 15:38 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2023-11-13 15:38 UTC (permalink / raw) To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Star Zeng On 11/13/23 16:38, 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.dong@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Rahul Kumar <rahul1.kumar@intel.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Star Zeng <star.zeng@intel.com> >> Signed-off-by: Jiaxin Wu <jiaxin.wu@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; >> > > Regression-tested-by: Laszlo Ersek <lersek@redhat.com> > also Reviewed-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111162): https://edk2.groups.io/g/devel/message/111162 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-13 15:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-07 2:43 [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information Wu, Jiaxin 2023-11-07 18:40 ` Laszlo Ersek 2023-11-07 18:44 ` Laszlo Ersek 2023-11-08 4:11 ` Wu, Jiaxin 2023-11-13 11:31 ` Laszlo Ersek 2023-11-13 15:38 ` Laszlo Ersek 2023-11-13 15:38 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox