UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 10 ++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 +++--- 2 files changed, 13 insertions(+), 3 deletions(-)
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.