* [PATCH] PiSmmCpuDxeSmm
@ 2016-10-27 22:29 Leo Duran
2016-10-27 22:29 ` [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library Leo Duran
0 siblings, 1 reply; 13+ messages in thread
From: Leo Duran @ 2016-10-27 22:29 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, yonghong.zhu, Leo Duran
This patch moves code that uses Intel-specific CPUID to a library.
Leo Duran (1):
UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118 ++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +--------------------
3 files changed, 136 insertions(+), 120 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-27 22:29 [PATCH] PiSmmCpuDxeSmm Leo Duran
@ 2016-10-27 22:29 ` Leo Duran
2016-10-27 23:15 ` Kinney, Michael D
0 siblings, 1 reply; 13+ messages in thread
From: Leo Duran @ 2016-10-27 22:29 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, yonghong.zhu, Leo Duran
1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib library
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leo Duran <leo.duran@amd.com>
---
UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118 ++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +--------------------
3 files changed, 136 insertions(+), 120 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
index 4478003..dd14ec5 100644
--- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
@@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
IN UINTN Pages
);
+/**
+ Get Package ID/Core ID/Thread ID of a processor.
+
+ APIC ID must be an initial APIC ID.
+
+ The algorithm assumes the target system has symmetry across physical package boundaries
+ with respect to the number of logical processors per package, number of cores per package.
+
+ @param ApicId APIC ID of the target logical processor.
+ @param Location Returns the processor location information.
+**/
+VOID
+SmmCpuFeaturesGetProcessorLocation (
+ IN UINT32 ApicId,
+ OUT EFI_CPU_PHYSICAL_LOCATION *Location
+ );
+
#endif
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 1754f2d..1e300f3 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
return NULL;
}
+/**
+ Get Package ID/Core ID/Thread ID of a processor.
+
+ APIC ID must be an initial APIC ID.
+
+ The algorithm below assumes the target system has symmetry across physical package boundaries
+ with respect to the number of logical processors per package, number of cores per package.
+
+ @param ApicId APIC ID of the target logical processor.
+ @param Location Returns the processor location information.
+**/
+VOID
+SmmCpuFeaturesGetProcessorLocation (
+ IN UINT32 ApicId,
+ OUT EFI_CPU_PHYSICAL_LOCATION *Location
+ )
+{
+ UINTN ThreadBits;
+ UINTN CoreBits;
+ UINT32 RegEax;
+ UINT32 RegEbx;
+ UINT32 RegEcx;
+ UINT32 RegEdx;
+ UINT32 MaxCpuIdIndex;
+ UINT32 SubIndex;
+ UINTN LevelType;
+ UINT32 MaxLogicProcessorsPerPackage;
+ UINT32 MaxCoresPerPackage;
+ BOOLEAN TopologyLeafSupported;
+
+ ASSERT (Location != NULL);
+
+ ThreadBits = 0;
+ CoreBits = 0;
+ TopologyLeafSupported = FALSE;
+
+ //
+ // Check if the processor is capable of supporting more than one logical processor.
+ //
+ AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
+ ASSERT ((RegEdx & BIT28) != 0);
+
+ //
+ // Assume three-level mapping of APIC ID: Package:Core:SMT.
+ //
+
+ //
+ // Get the max index of basic CPUID
+ //
+ AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
+
+ //
+ // If the extended topology enumeration leaf is available, it
+ // is the preferred mechanism for enumerating topology.
+ //
+ if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
+ AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
+ //
+ // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
+ // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
+ // supported on that processor.
+ //
+ if ((RegEbx & 0xffff) != 0) {
+ TopologyLeafSupported = TRUE;
+
+ //
+ // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
+ // the SMT sub-field of x2APIC ID.
+ //
+ LevelType = (RegEcx >> 8) & 0xff;
+ ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
+ if ((RegEbx & 0xffff) > 1 ) {
+ ThreadBits = RegEax & 0x1f;
+ } else {
+ //
+ // HT is not supported
+ //
+ ThreadBits = 0;
+ }
+
+ //
+ // Software must not assume any "level type" encoding
+ // value to be related to any sub-leaf index, except sub-leaf 0.
+ //
+ SubIndex = 1;
+ do {
+ AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
+ LevelType = (RegEcx >> 8) & 0xff;
+ if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
+ CoreBits = (RegEax & 0x1f) - ThreadBits;
+ break;
+ }
+ SubIndex++;
+ } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
+ }
+ }
+
+ if (!TopologyLeafSupported) {
+ AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
+ MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
+ if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
+ AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
+ MaxCoresPerPackage = (RegEax >> 26) + 1;
+ } else {
+ //
+ // Must be a single-core processor.
+ //
+ MaxCoresPerPackage = 1;
+ }
+
+ ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage / MaxCoresPerPackage - 1) + 1);
+ CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
+ }
+
+ Location->Thread = ApicId & ~((-1) << ThreadBits);
+ Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
+ Location->Package = (ApicId >> (ThreadBits+ CoreBits));
+}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
index 40f2a17..2bfb1e8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
@@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL mSmmCpuService = {
};
/**
- Get Package ID/Core ID/Thread ID of a processor.
-
- APIC ID must be an initial APIC ID.
-
- The algorithm below assumes the target system has symmetry across physical package boundaries
- with respect to the number of logical processors per package, number of cores per package.
-
- @param ApicId APIC ID of the target logical processor.
- @param Location Returns the processor location information.
-**/
-VOID
-SmmGetProcessorLocation (
- IN UINT32 ApicId,
- OUT EFI_CPU_PHYSICAL_LOCATION *Location
- )
-{
- UINTN ThreadBits;
- UINTN CoreBits;
- UINT32 RegEax;
- UINT32 RegEbx;
- UINT32 RegEcx;
- UINT32 RegEdx;
- UINT32 MaxCpuIdIndex;
- UINT32 SubIndex;
- UINTN LevelType;
- UINT32 MaxLogicProcessorsPerPackage;
- UINT32 MaxCoresPerPackage;
- BOOLEAN TopologyLeafSupported;
-
- ASSERT (Location != NULL);
-
- ThreadBits = 0;
- CoreBits = 0;
- TopologyLeafSupported = FALSE;
-
- //
- // Check if the processor is capable of supporting more than one logical processor.
- //
- AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
- ASSERT ((RegEdx & BIT28) != 0);
-
- //
- // Assume three-level mapping of APIC ID: Package:Core:SMT.
- //
-
- //
- // Get the max index of basic CPUID
- //
- AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
-
- //
- // If the extended topology enumeration leaf is available, it
- // is the preferred mechanism for enumerating topology.
- //
- if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
- AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
- //
- // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
- // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
- // supported on that processor.
- //
- if ((RegEbx & 0xffff) != 0) {
- TopologyLeafSupported = TRUE;
-
- //
- // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
- // the SMT sub-field of x2APIC ID.
- //
- LevelType = (RegEcx >> 8) & 0xff;
- ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
- if ((RegEbx & 0xffff) > 1 ) {
- ThreadBits = RegEax & 0x1f;
- } else {
- //
- // HT is not supported
- //
- ThreadBits = 0;
- }
-
- //
- // Software must not assume any "level type" encoding
- // value to be related to any sub-leaf index, except sub-leaf 0.
- //
- SubIndex = 1;
- do {
- AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
- LevelType = (RegEcx >> 8) & 0xff;
- if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
- CoreBits = (RegEax & 0x1f) - ThreadBits;
- break;
- }
- SubIndex++;
- } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
- }
- }
-
- if (!TopologyLeafSupported) {
- AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
- MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
- if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
- AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
- MaxCoresPerPackage = (RegEax >> 26) + 1;
- } else {
- //
- // Must be a single-core processor.
- //
- MaxCoresPerPackage = 1;
- }
-
- ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage / MaxCoresPerPackage - 1) + 1);
- CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
- }
-
- Location->Thread = ApicId & ~((-1) << ThreadBits);
- Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
- Location->Package = (ApicId >> (ThreadBits+ CoreBits));
-}
-
-/**
Gets processor information on the requested processor at the instant this call is made.
@param[in] This A pointer to the EFI_SMM_CPU_SERVICE_PROTOCOL instance.
@@ -280,7 +161,7 @@ SmmAddProcessor (
gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId == INVALID_APIC_ID) {
gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
- SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate->ProcessorInfo[Index].Location);
+ SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate->ProcessorInfo[Index].Location);
*ProcessorNumber = Index;
gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-27 22:29 ` [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library Leo Duran
@ 2016-10-27 23:15 ` Kinney, Michael D
2016-10-28 0:44 ` Fan, Jeff
2016-10-28 1:00 ` Fan, Jeff
0 siblings, 2 replies; 13+ messages in thread
From: Kinney, Michael D @ 2016-10-27 23:15 UTC (permalink / raw)
To: Leo Duran, edk2-devel@lists.01.org, Kinney, Michael D
Cc: Gao, Liming, Fan, Jeff
Leo,
This looks like a good proposed change to the SmmFeaturesLib and
PiSmmCpuDxeSmm module.
Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
There are 3 implementations of the SmmFeaturesLib in edk2/master.
This patch needs to update all 3, or some of the platforms in
edk2/master will no longer build:
* OvmfPkg\Library\SmmCpuFeaturesLib
* QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
* UefiCpuPkg\Library\SmmCpuFeaturesLib
Thanks,
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leo Duran
> Sent: Thursday, October 27, 2016 3:30 PM
> To: edk2-devel@lists.01.org
> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib
> library.
>
> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib library
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118 ++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +--------------------
> 3 files changed, 136 insertions(+), 120 deletions(-)
>
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> index 4478003..dd14ec5 100644
> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> IN UINTN Pages
> );
>
> +/**
> + Get Package ID/Core ID/Thread ID of a processor.
> +
> + APIC ID must be an initial APIC ID.
> +
> + The algorithm assumes the target system has symmetry across physical package
> boundaries
> + with respect to the number of logical processors per package, number of cores per
> package.
> +
> + @param ApicId APIC ID of the target logical processor.
> + @param Location Returns the processor location information.
> +**/
> +VOID
> +SmmCpuFeaturesGetProcessorLocation (
> + IN UINT32 ApicId,
> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 1754f2d..1e300f3 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> return NULL;
> }
>
> +/**
> + Get Package ID/Core ID/Thread ID of a processor.
> +
> + APIC ID must be an initial APIC ID.
> +
> + The algorithm below assumes the target system has symmetry across physical package
> boundaries
> + with respect to the number of logical processors per package, number of cores per
> package.
> +
> + @param ApicId APIC ID of the target logical processor.
> + @param Location Returns the processor location information.
> +**/
> +VOID
> +SmmCpuFeaturesGetProcessorLocation (
> + IN UINT32 ApicId,
> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> + )
> +{
> + UINTN ThreadBits;
> + UINTN CoreBits;
> + UINT32 RegEax;
> + UINT32 RegEbx;
> + UINT32 RegEcx;
> + UINT32 RegEdx;
> + UINT32 MaxCpuIdIndex;
> + UINT32 SubIndex;
> + UINTN LevelType;
> + UINT32 MaxLogicProcessorsPerPackage;
> + UINT32 MaxCoresPerPackage;
> + BOOLEAN TopologyLeafSupported;
> +
> + ASSERT (Location != NULL);
> +
> + ThreadBits = 0;
> + CoreBits = 0;
> + TopologyLeafSupported = FALSE;
> +
> + //
> + // Check if the processor is capable of supporting more than one logical processor.
> + //
> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> + ASSERT ((RegEdx & BIT28) != 0);
> +
> + //
> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> + //
> +
> + //
> + // Get the max index of basic CPUID
> + //
> + AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> +
> + //
> + // If the extended topology enumeration leaf is available, it
> + // is the preferred mechanism for enumerating topology.
> + //
> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
> + //
> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
> + // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
> + // supported on that processor.
> + //
> + if ((RegEbx & 0xffff) != 0) {
> + TopologyLeafSupported = TRUE;
> +
> + //
> + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
> + // the SMT sub-field of x2APIC ID.
> + //
> + LevelType = (RegEcx >> 8) & 0xff;
> + ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> + if ((RegEbx & 0xffff) > 1 ) {
> + ThreadBits = RegEax & 0x1f;
> + } else {
> + //
> + // HT is not supported
> + //
> + ThreadBits = 0;
> + }
> +
> + //
> + // Software must not assume any "level type" encoding
> + // value to be related to any sub-leaf index, except sub-leaf 0.
> + //
> + SubIndex = 1;
> + do {
> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
> + LevelType = (RegEcx >> 8) & 0xff;
> + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> + break;
> + }
> + SubIndex++;
> + } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> + }
> + }
> +
> + if (!TopologyLeafSupported) {
> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> + } else {
> + //
> + // Must be a single-core processor.
> + //
> + MaxCoresPerPackage = 1;
> + }
> +
> + ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> MaxCoresPerPackage - 1) + 1);
> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> + }
> +
> + Location->Thread = ApicId & ~((-1) << ThreadBits);
> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> + Location->Package = (ApicId >> (ThreadBits+ CoreBits));
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index 40f2a17..2bfb1e8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL mSmmCpuService = {
> };
>
> /**
> - Get Package ID/Core ID/Thread ID of a processor.
> -
> - APIC ID must be an initial APIC ID.
> -
> - The algorithm below assumes the target system has symmetry across physical package
> boundaries
> - with respect to the number of logical processors per package, number of cores per
> package.
> -
> - @param ApicId APIC ID of the target logical processor.
> - @param Location Returns the processor location information.
> -**/
> -VOID
> -SmmGetProcessorLocation (
> - IN UINT32 ApicId,
> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> - )
> -{
> - UINTN ThreadBits;
> - UINTN CoreBits;
> - UINT32 RegEax;
> - UINT32 RegEbx;
> - UINT32 RegEcx;
> - UINT32 RegEdx;
> - UINT32 MaxCpuIdIndex;
> - UINT32 SubIndex;
> - UINTN LevelType;
> - UINT32 MaxLogicProcessorsPerPackage;
> - UINT32 MaxCoresPerPackage;
> - BOOLEAN TopologyLeafSupported;
> -
> - ASSERT (Location != NULL);
> -
> - ThreadBits = 0;
> - CoreBits = 0;
> - TopologyLeafSupported = FALSE;
> -
> - //
> - // Check if the processor is capable of supporting more than one logical processor.
> - //
> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> - ASSERT ((RegEdx & BIT28) != 0);
> -
> - //
> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> - //
> -
> - //
> - // Get the max index of basic CPUID
> - //
> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> -
> - //
> - // If the extended topology enumeration leaf is available, it
> - // is the preferred mechanism for enumerating topology.
> - //
> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
> - //
> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
> - // supported on that processor.
> - //
> - if ((RegEbx & 0xffff) != 0) {
> - TopologyLeafSupported = TRUE;
> -
> - //
> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
> - // the SMT sub-field of x2APIC ID.
> - //
> - LevelType = (RegEcx >> 8) & 0xff;
> - ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> - if ((RegEbx & 0xffff) > 1 ) {
> - ThreadBits = RegEax & 0x1f;
> - } else {
> - //
> - // HT is not supported
> - //
> - ThreadBits = 0;
> - }
> -
> - //
> - // Software must not assume any "level type" encoding
> - // value to be related to any sub-leaf index, except sub-leaf 0.
> - //
> - SubIndex = 1;
> - do {
> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
> - LevelType = (RegEcx >> 8) & 0xff;
> - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> - break;
> - }
> - SubIndex++;
> - } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> - }
> - }
> -
> - if (!TopologyLeafSupported) {
> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> - } else {
> - //
> - // Must be a single-core processor.
> - //
> - MaxCoresPerPackage = 1;
> - }
> -
> - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> MaxCoresPerPackage - 1) + 1);
> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> - }
> -
> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> - Location->Package = (ApicId >> (ThreadBits+ CoreBits));
> -}
> -
> -/**
> Gets processor information on the requested processor at the instant this call is
> made.
>
> @param[in] This A pointer to the EFI_SMM_CPU_SERVICE_PROTOCOL
> instance.
> @@ -280,7 +161,7 @@ SmmAddProcessor (
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId == INVALID_APIC_ID) {
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> >ProcessorInfo[Index].Location);
> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> >ProcessorInfo[Index].Location);
>
> *ProcessorNumber = Index;
> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> --
> 1.9.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-27 23:15 ` Kinney, Michael D
@ 2016-10-28 0:44 ` Fan, Jeff
2016-10-28 1:00 ` Fan, Jeff
1 sibling, 0 replies; 13+ messages in thread
From: Fan, Jeff @ 2016-10-28 0:44 UTC (permalink / raw)
To: Kinney, Michael D, Leo Duran, edk2-devel@lists.01.org; +Cc: Gao, Liming
Leo and Mike,
GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
I suggest that we could add this API into UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
Thus, it could be consumed by modules across PEI/DXE/SMM modules.
Thanks!
Jeff
-----Original Message-----
From: Kinney, Michael D
Sent: Friday, October 28, 2016 7:16 AM
To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
Cc: Gao, Liming; Fan, Jeff
Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
Leo,
This looks like a good proposed change to the SmmFeaturesLib and PiSmmCpuDxeSmm module.
Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
There are 3 implementations of the SmmFeaturesLib in edk2/master.
This patch needs to update all 3, or some of the platforms in edk2/master will no longer build:
* OvmfPkg\Library\SmmCpuFeaturesLib
* QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
* UefiCpuPkg\Library\SmmCpuFeaturesLib
Thanks,
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leo Duran
> Sent: Thursday, October 27, 2016 3:30 PM
> To: edk2-devel@lists.01.org
> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
> library
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118 ++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +--------------------
> 3 files changed, 136 insertions(+), 120 deletions(-)
>
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> index 4478003..dd14ec5 100644
> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> IN UINTN Pages
> );
>
> +/**
> + Get Package ID/Core ID/Thread ID of a processor.
> +
> + APIC ID must be an initial APIC ID.
> +
> + The algorithm assumes the target system has symmetry across
> + physical package
> boundaries
> + with respect to the number of logical processors per package,
> + number of cores per
> package.
> +
> + @param ApicId APIC ID of the target logical processor.
> + @param Location Returns the processor location information.
> +**/
> +VOID
> +SmmCpuFeaturesGetProcessorLocation (
> + IN UINT32 ApicId,
> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 1754f2d..1e300f3 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> return NULL;
> }
>
> +/**
> + Get Package ID/Core ID/Thread ID of a processor.
> +
> + APIC ID must be an initial APIC ID.
> +
> + The algorithm below assumes the target system has symmetry across
> + physical package
> boundaries
> + with respect to the number of logical processors per package,
> + number of cores per
> package.
> +
> + @param ApicId APIC ID of the target logical processor.
> + @param Location Returns the processor location information.
> +**/
> +VOID
> +SmmCpuFeaturesGetProcessorLocation (
> + IN UINT32 ApicId,
> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> + )
> +{
> + UINTN ThreadBits;
> + UINTN CoreBits;
> + UINT32 RegEax;
> + UINT32 RegEbx;
> + UINT32 RegEcx;
> + UINT32 RegEdx;
> + UINT32 MaxCpuIdIndex;
> + UINT32 SubIndex;
> + UINTN LevelType;
> + UINT32 MaxLogicProcessorsPerPackage;
> + UINT32 MaxCoresPerPackage;
> + BOOLEAN TopologyLeafSupported;
> +
> + ASSERT (Location != NULL);
> +
> + ThreadBits = 0;
> + CoreBits = 0;
> + TopologyLeafSupported = FALSE;
> +
> + //
> + // Check if the processor is capable of supporting more than one logical processor.
> + //
> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx); ASSERT
> + ((RegEdx & BIT28) != 0);
> +
> + //
> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> + //
> +
> + //
> + // Get the max index of basic CPUID // AsmCpuid (CPUID_SIGNATURE,
> + &MaxCpuIdIndex, NULL, NULL, NULL);
> +
> + //
> + // If the extended topology enumeration leaf is available, it //
> + is the preferred mechanism for enumerating topology.
> + //
> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
> + //
> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
> + // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
> + // supported on that processor.
> + //
> + if ((RegEbx & 0xffff) != 0) {
> + TopologyLeafSupported = TRUE;
> +
> + //
> + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
> + // the SMT sub-field of x2APIC ID.
> + //
> + LevelType = (RegEcx >> 8) & 0xff;
> + ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> + if ((RegEbx & 0xffff) > 1 ) {
> + ThreadBits = RegEax & 0x1f;
> + } else {
> + //
> + // HT is not supported
> + //
> + ThreadBits = 0;
> + }
> +
> + //
> + // Software must not assume any "level type" encoding
> + // value to be related to any sub-leaf index, except sub-leaf 0.
> + //
> + SubIndex = 1;
> + do {
> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
> + LevelType = (RegEcx >> 8) & 0xff;
> + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> + break;
> + }
> + SubIndex++;
> + } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> + }
> + }
> +
> + if (!TopologyLeafSupported) {
> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> + } else {
> + //
> + // Must be a single-core processor.
> + //
> + MaxCoresPerPackage = 1;
> + }
> +
> + ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage
> + /
> MaxCoresPerPackage - 1) + 1);
> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> + }
> +
> + Location->Thread = ApicId & ~((-1) << ThreadBits);
> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index 40f2a17..2bfb1e8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL mSmmCpuService = {
> };
>
> /**
> - Get Package ID/Core ID/Thread ID of a processor.
> -
> - APIC ID must be an initial APIC ID.
> -
> - The algorithm below assumes the target system has symmetry across
> physical package boundaries
> - with respect to the number of logical processors per package,
> number of cores per package.
> -
> - @param ApicId APIC ID of the target logical processor.
> - @param Location Returns the processor location information.
> -**/
> -VOID
> -SmmGetProcessorLocation (
> - IN UINT32 ApicId,
> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> - )
> -{
> - UINTN ThreadBits;
> - UINTN CoreBits;
> - UINT32 RegEax;
> - UINT32 RegEbx;
> - UINT32 RegEcx;
> - UINT32 RegEdx;
> - UINT32 MaxCpuIdIndex;
> - UINT32 SubIndex;
> - UINTN LevelType;
> - UINT32 MaxLogicProcessorsPerPackage;
> - UINT32 MaxCoresPerPackage;
> - BOOLEAN TopologyLeafSupported;
> -
> - ASSERT (Location != NULL);
> -
> - ThreadBits = 0;
> - CoreBits = 0;
> - TopologyLeafSupported = FALSE;
> -
> - //
> - // Check if the processor is capable of supporting more than one logical processor.
> - //
> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> - ASSERT ((RegEdx & BIT28) != 0);
> -
> - //
> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> - //
> -
> - //
> - // Get the max index of basic CPUID
> - //
> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> -
> - //
> - // If the extended topology enumeration leaf is available, it
> - // is the preferred mechanism for enumerating topology.
> - //
> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
> - //
> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
> - // supported on that processor.
> - //
> - if ((RegEbx & 0xffff) != 0) {
> - TopologyLeafSupported = TRUE;
> -
> - //
> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
> - // the SMT sub-field of x2APIC ID.
> - //
> - LevelType = (RegEcx >> 8) & 0xff;
> - ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> - if ((RegEbx & 0xffff) > 1 ) {
> - ThreadBits = RegEax & 0x1f;
> - } else {
> - //
> - // HT is not supported
> - //
> - ThreadBits = 0;
> - }
> -
> - //
> - // Software must not assume any "level type" encoding
> - // value to be related to any sub-leaf index, except sub-leaf 0.
> - //
> - SubIndex = 1;
> - do {
> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
> - LevelType = (RegEcx >> 8) & 0xff;
> - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> - break;
> - }
> - SubIndex++;
> - } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> - }
> - }
> -
> - if (!TopologyLeafSupported) {
> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> - } else {
> - //
> - // Must be a single-core processor.
> - //
> - MaxCoresPerPackage = 1;
> - }
> -
> - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> MaxCoresPerPackage - 1) + 1);
> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> - }
> -
> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> -
> -/**
> Gets processor information on the requested processor at the
> instant this call is made.
>
> @param[in] This A pointer to the EFI_SMM_CPU_SERVICE_PROTOCOL
> instance.
> @@ -280,7 +161,7 @@ SmmAddProcessor (
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId == INVALID_APIC_ID) {
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> >ProcessorInfo[Index].Location);
> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> + &gSmmCpuPrivate-
> >ProcessorInfo[Index].Location);
>
> *ProcessorNumber = Index;
> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> --
> 1.9.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-27 23:15 ` Kinney, Michael D
2016-10-28 0:44 ` Fan, Jeff
@ 2016-10-28 1:00 ` Fan, Jeff
2016-10-28 3:19 ` Duran, Leo
1 sibling, 1 reply; 13+ messages in thread
From: Fan, Jeff @ 2016-10-28 1:00 UTC (permalink / raw)
To: Kinney, Michael D, Leo Duran, edk2-devel@lists.01.org; +Cc: Gao, Liming
Because the CPU location information are gotten from Initial APIC ID, it makes more sense to be added into Local APIC Lib.
The following is my proposal on its definition.
/**
Get CPU Package/Core/Thread location information.
@param[in] InitialApicId CPU APIC ID
@param[out] Package Pointer to Package ID
@param[out] Core Pointer to Core ID
@param[out] Thread Pointer to Thread ID
**/
VOID
ExtractProcessorLocation (
IN UINT32 InitialApicId,
OUT UINT32 *Package,
OUT UINT32 *Core,
OUT UINT32 *Thread
);
Leo, is it OK to meet your requirement?
Thanks!
Jeff
-----Original Message-----
From: Fan, Jeff
Sent: Friday, October 28, 2016 8:45 AM
To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
Cc: Gao, Liming
Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
Leo and Mike,
GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
I suggest that we could add this API into UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
Thus, it could be consumed by modules across PEI/DXE/SMM modules.
Thanks!
Jeff
-----Original Message-----
From: Kinney, Michael D
Sent: Friday, October 28, 2016 7:16 AM
To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
Cc: Gao, Liming; Fan, Jeff
Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
Leo,
This looks like a good proposed change to the SmmFeaturesLib and PiSmmCpuDxeSmm module.
Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
There are 3 implementations of the SmmFeaturesLib in edk2/master.
This patch needs to update all 3, or some of the platforms in edk2/master will no longer build:
* OvmfPkg\Library\SmmCpuFeaturesLib
* QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
* UefiCpuPkg\Library\SmmCpuFeaturesLib
Thanks,
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leo Duran
> Sent: Thursday, October 27, 2016 3:30 PM
> To: edk2-devel@lists.01.org
> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
> library
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118 ++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +--------------------
> 3 files changed, 136 insertions(+), 120 deletions(-)
>
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> index 4478003..dd14ec5 100644
> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> IN UINTN Pages
> );
>
> +/**
> + Get Package ID/Core ID/Thread ID of a processor.
> +
> + APIC ID must be an initial APIC ID.
> +
> + The algorithm assumes the target system has symmetry across
> + physical package
> boundaries
> + with respect to the number of logical processors per package,
> + number of cores per
> package.
> +
> + @param ApicId APIC ID of the target logical processor.
> + @param Location Returns the processor location information.
> +**/
> +VOID
> +SmmCpuFeaturesGetProcessorLocation (
> + IN UINT32 ApicId,
> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 1754f2d..1e300f3 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> return NULL;
> }
>
> +/**
> + Get Package ID/Core ID/Thread ID of a processor.
> +
> + APIC ID must be an initial APIC ID.
> +
> + The algorithm below assumes the target system has symmetry across
> + physical package
> boundaries
> + with respect to the number of logical processors per package,
> + number of cores per
> package.
> +
> + @param ApicId APIC ID of the target logical processor.
> + @param Location Returns the processor location information.
> +**/
> +VOID
> +SmmCpuFeaturesGetProcessorLocation (
> + IN UINT32 ApicId,
> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> + )
> +{
> + UINTN ThreadBits;
> + UINTN CoreBits;
> + UINT32 RegEax;
> + UINT32 RegEbx;
> + UINT32 RegEcx;
> + UINT32 RegEdx;
> + UINT32 MaxCpuIdIndex;
> + UINT32 SubIndex;
> + UINTN LevelType;
> + UINT32 MaxLogicProcessorsPerPackage;
> + UINT32 MaxCoresPerPackage;
> + BOOLEAN TopologyLeafSupported;
> +
> + ASSERT (Location != NULL);
> +
> + ThreadBits = 0;
> + CoreBits = 0;
> + TopologyLeafSupported = FALSE;
> +
> + //
> + // Check if the processor is capable of supporting more than one logical processor.
> + //
> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx); ASSERT
> + ((RegEdx & BIT28) != 0);
> +
> + //
> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> + //
> +
> + //
> + // Get the max index of basic CPUID // AsmCpuid (CPUID_SIGNATURE,
> + &MaxCpuIdIndex, NULL, NULL, NULL);
> +
> + //
> + // If the extended topology enumeration leaf is available, it //
> + is the preferred mechanism for enumerating topology.
> + //
> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
> + //
> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
> + // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
> + // supported on that processor.
> + //
> + if ((RegEbx & 0xffff) != 0) {
> + TopologyLeafSupported = TRUE;
> +
> + //
> + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
> + // the SMT sub-field of x2APIC ID.
> + //
> + LevelType = (RegEcx >> 8) & 0xff;
> + ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> + if ((RegEbx & 0xffff) > 1 ) {
> + ThreadBits = RegEax & 0x1f;
> + } else {
> + //
> + // HT is not supported
> + //
> + ThreadBits = 0;
> + }
> +
> + //
> + // Software must not assume any "level type" encoding
> + // value to be related to any sub-leaf index, except sub-leaf 0.
> + //
> + SubIndex = 1;
> + do {
> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
> + LevelType = (RegEcx >> 8) & 0xff;
> + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> + break;
> + }
> + SubIndex++;
> + } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> + }
> + }
> +
> + if (!TopologyLeafSupported) {
> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> + } else {
> + //
> + // Must be a single-core processor.
> + //
> + MaxCoresPerPackage = 1;
> + }
> +
> + ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage
> + /
> MaxCoresPerPackage - 1) + 1);
> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1); }
> +
> + Location->Thread = ApicId & ~((-1) << ThreadBits); Location->Core
> + = (ApicId >> ThreadBits) & ~((-1) << CoreBits); Location->Package =
> + (ApicId >> (ThreadBits+ CoreBits)); }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index 40f2a17..2bfb1e8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL mSmmCpuService = {
> };
>
> /**
> - Get Package ID/Core ID/Thread ID of a processor.
> -
> - APIC ID must be an initial APIC ID.
> -
> - The algorithm below assumes the target system has symmetry across
> physical package boundaries
> - with respect to the number of logical processors per package,
> number of cores per package.
> -
> - @param ApicId APIC ID of the target logical processor.
> - @param Location Returns the processor location information.
> -**/
> -VOID
> -SmmGetProcessorLocation (
> - IN UINT32 ApicId,
> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> - )
> -{
> - UINTN ThreadBits;
> - UINTN CoreBits;
> - UINT32 RegEax;
> - UINT32 RegEbx;
> - UINT32 RegEcx;
> - UINT32 RegEdx;
> - UINT32 MaxCpuIdIndex;
> - UINT32 SubIndex;
> - UINTN LevelType;
> - UINT32 MaxLogicProcessorsPerPackage;
> - UINT32 MaxCoresPerPackage;
> - BOOLEAN TopologyLeafSupported;
> -
> - ASSERT (Location != NULL);
> -
> - ThreadBits = 0;
> - CoreBits = 0;
> - TopologyLeafSupported = FALSE;
> -
> - //
> - // Check if the processor is capable of supporting more than one logical processor.
> - //
> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> - ASSERT ((RegEdx & BIT28) != 0);
> -
> - //
> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> - //
> -
> - //
> - // Get the max index of basic CPUID
> - //
> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> -
> - //
> - // If the extended topology enumeration leaf is available, it
> - // is the preferred mechanism for enumerating topology.
> - //
> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx, &RegEcx, NULL);
> - //
> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
> - // supported on that processor.
> - //
> - if ((RegEbx & 0xffff) != 0) {
> - TopologyLeafSupported = TRUE;
> -
> - //
> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
> - // the SMT sub-field of x2APIC ID.
> - //
> - LevelType = (RegEcx >> 8) & 0xff;
> - ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> - if ((RegEbx & 0xffff) > 1 ) {
> - ThreadBits = RegEax & 0x1f;
> - } else {
> - //
> - // HT is not supported
> - //
> - ThreadBits = 0;
> - }
> -
> - //
> - // Software must not assume any "level type" encoding
> - // value to be related to any sub-leaf index, except sub-leaf 0.
> - //
> - SubIndex = 1;
> - do {
> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax, NULL, &RegEcx, NULL);
> - LevelType = (RegEcx >> 8) & 0xff;
> - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> - break;
> - }
> - SubIndex++;
> - } while (LevelType != CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> - }
> - }
> -
> - if (!TopologyLeafSupported) {
> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> - } else {
> - //
> - // Must be a single-core processor.
> - //
> - MaxCoresPerPackage = 1;
> - }
> -
> - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> MaxCoresPerPackage - 1) + 1);
> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> - }
> -
> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> -
> -/**
> Gets processor information on the requested processor at the
> instant this call is made.
>
> @param[in] This A pointer to the EFI_SMM_CPU_SERVICE_PROTOCOL
> instance.
> @@ -280,7 +161,7 @@ SmmAddProcessor (
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId == INVALID_APIC_ID) {
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> >ProcessorInfo[Index].Location);
> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> + &gSmmCpuPrivate-
> >ProcessorInfo[Index].Location);
>
> *ProcessorNumber = Index;
> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> --
> 1.9.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 1:00 ` Fan, Jeff
@ 2016-10-28 3:19 ` Duran, Leo
2016-10-28 4:43 ` Fan, Jeff
0 siblings, 1 reply; 13+ messages in thread
From: Duran, Leo @ 2016-10-28 3:19 UTC (permalink / raw)
To: Fan, Jeff, Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Gao, Liming
Please see my reply below.
Thanks,
Leo
> -----Original Message-----
> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> Sent: Thursday, October 27, 2016 8:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> <leo.duran@amd.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Because the CPU location information are gotten from Initial APIC ID, it
> makes more sense to be added into Local APIC Lib.
>
> The following is my proposal on its definition.
> /**
> Get CPU Package/Core/Thread location information.
>
> @param[in] InitialApicId CPU APIC ID
> @param[out] Package Pointer to Package ID
> @param[out] Core Pointer to Core ID
> @param[out] Thread Pointer to Thread ID
> **/
> VOID
> ExtractProcessorLocation (
> IN UINT32 InitialApicId,
> OUT UINT32 *Package,
> OUT UINT32 *Core,
> OUT UINT32 *Thread
> );
>
> Leo, is it OK to meet your requirement?
>
> Thanks!
> Jeff
[Duran, Leo]
Sure thing.
The main point is to get the vendor-specific CPUID code out of the driver, and into a library.
>
> -----Original Message-----
> From: Fan, Jeff
> Sent: Friday, October 28, 2016 8:45 AM
> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> Cc: Gao, Liming
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Leo and Mike,
>
> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it is
> also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
>
> I suggest that we could add this API into UefiCpuPkg/Include/UefiCpuLib or
> UefiCpuPkg/Include/ LocalApicLib.h.
> Thus, it could be consumed by modules across PEI/DXE/SMM modules.
>
> Thanks!
> Jeff
>
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, October 28, 2016 7:16 AM
> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> Cc: Gao, Liming; Fan, Jeff
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Leo,
>
> This looks like a good proposed change to the SmmFeaturesLib and
> PiSmmCpuDxeSmm module.
>
> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
>
> There are 3 implementations of the SmmFeaturesLib in edk2/master.
> This patch needs to update all 3, or some of the platforms in edk2/master will
> no longer build:
>
> * OvmfPkg\Library\SmmCpuFeaturesLib
> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> * UefiCpuPkg\Library\SmmCpuFeaturesLib
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Leo Duran
> > Sent: Thursday, October 27, 2016 3:30 PM
> > To: edk2-devel@lists.01.org
> > Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> <liming.gao@intel.com>
> > Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> > 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
> > library
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > ---
> > UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> > .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> ++++++++++++++++++++
> > UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +------------------
> --
> > 3 files changed, 136 insertions(+), 120 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > index 4478003..dd14ec5 100644
> > --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > IN UINTN Pages
> > );
> >
> > +/**
> > + Get Package ID/Core ID/Thread ID of a processor.
> > +
> > + APIC ID must be an initial APIC ID.
> > +
> > + The algorithm assumes the target system has symmetry across
> > + physical package
> > boundaries
> > + with respect to the number of logical processors per package,
> > + number of cores per
> > package.
> > +
> > + @param ApicId APIC ID of the target logical processor.
> > + @param Location Returns the processor location information.
> > +**/
> > +VOID
> > +SmmCpuFeaturesGetProcessorLocation (
> > + IN UINT32 ApicId,
> > + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > + );
> > +
> > #endif
> > diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > index 1754f2d..1e300f3 100644
> > --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > return NULL;
> > }
> >
> > +/**
> > + Get Package ID/Core ID/Thread ID of a processor.
> > +
> > + APIC ID must be an initial APIC ID.
> > +
> > + The algorithm below assumes the target system has symmetry across
> > + physical package
> > boundaries
> > + with respect to the number of logical processors per package,
> > + number of cores per
> > package.
> > +
> > + @param ApicId APIC ID of the target logical processor.
> > + @param Location Returns the processor location information.
> > +**/
> > +VOID
> > +SmmCpuFeaturesGetProcessorLocation (
> > + IN UINT32 ApicId,
> > + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > + )
> > +{
> > + UINTN ThreadBits;
> > + UINTN CoreBits;
> > + UINT32 RegEax;
> > + UINT32 RegEbx;
> > + UINT32 RegEcx;
> > + UINT32 RegEdx;
> > + UINT32 MaxCpuIdIndex;
> > + UINT32 SubIndex;
> > + UINTN LevelType;
> > + UINT32 MaxLogicProcessorsPerPackage;
> > + UINT32 MaxCoresPerPackage;
> > + BOOLEAN TopologyLeafSupported;
> > +
> > + ASSERT (Location != NULL);
> > +
> > + ThreadBits = 0;
> > + CoreBits = 0;
> > + TopologyLeafSupported = FALSE;
> > +
> > + //
> > + // Check if the processor is capable of supporting more than one logical
> processor.
> > + //
> > + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> ASSERT
> > + ((RegEdx & BIT28) != 0);
> > +
> > + //
> > + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > + //
> > +
> > + //
> > + // Get the max index of basic CPUID // AsmCpuid (CPUID_SIGNATURE,
> > + &MaxCpuIdIndex, NULL, NULL, NULL);
> > +
> > + //
> > + // If the extended topology enumeration leaf is available, it //
> > + is the preferred mechanism for enumerating topology.
> > + //
> > + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> &RegEcx, NULL);
> > + //
> > + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input
> value for
> > + // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is
> not
> > + // supported on that processor.
> > + //
> > + if ((RegEbx & 0xffff) != 0) {
> > + TopologyLeafSupported = TRUE;
> > +
> > + //
> > + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters
> to extract
> > + // the SMT sub-field of x2APIC ID.
> > + //
> > + LevelType = (RegEcx >> 8) & 0xff;
> > + ASSERT (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > + if ((RegEbx & 0xffff) > 1 ) {
> > + ThreadBits = RegEax & 0x1f;
> > + } else {
> > + //
> > + // HT is not supported
> > + //
> > + ThreadBits = 0;
> > + }
> > +
> > + //
> > + // Software must not assume any "level type" encoding
> > + // value to be related to any sub-leaf index, except sub-leaf 0.
> > + //
> > + SubIndex = 1;
> > + do {
> > + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> NULL, &RegEcx, NULL);
> > + LevelType = (RegEcx >> 8) & 0xff;
> > + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > + CoreBits = (RegEax & 0x1f) - ThreadBits;
> > + break;
> > + }
> > + SubIndex++;
> > + } while (LevelType !=
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > + }
> > + }
> > +
> > + if (!TopologyLeafSupported) {
> > + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> NULL);
> > + MaxCoresPerPackage = (RegEax >> 26) + 1;
> > + } else {
> > + //
> > + // Must be a single-core processor.
> > + //
> > + MaxCoresPerPackage = 1;
> > + }
> > +
> > + ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage
> > + /
> > MaxCoresPerPackage - 1) + 1);
> > + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1); }
> > +
> > + Location->Thread = ApicId & ~((-1) << ThreadBits); Location->Core
> > + = (ApicId >> ThreadBits) & ~((-1) << CoreBits); Location->Package =
> > + (ApicId >> (ThreadBits+ CoreBits)); }
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > index 40f2a17..2bfb1e8 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> mSmmCpuService = {
> > };
> >
> > /**
> > - Get Package ID/Core ID/Thread ID of a processor.
> > -
> > - APIC ID must be an initial APIC ID.
> > -
> > - The algorithm below assumes the target system has symmetry across
> > physical package boundaries
> > - with respect to the number of logical processors per package,
> > number of cores per package.
> > -
> > - @param ApicId APIC ID of the target logical processor.
> > - @param Location Returns the processor location information.
> > -**/
> > -VOID
> > -SmmGetProcessorLocation (
> > - IN UINT32 ApicId,
> > - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > - )
> > -{
> > - UINTN ThreadBits;
> > - UINTN CoreBits;
> > - UINT32 RegEax;
> > - UINT32 RegEbx;
> > - UINT32 RegEcx;
> > - UINT32 RegEdx;
> > - UINT32 MaxCpuIdIndex;
> > - UINT32 SubIndex;
> > - UINTN LevelType;
> > - UINT32 MaxLogicProcessorsPerPackage;
> > - UINT32 MaxCoresPerPackage;
> > - BOOLEAN TopologyLeafSupported;
> > -
> > - ASSERT (Location != NULL);
> > -
> > - ThreadBits = 0;
> > - CoreBits = 0;
> > - TopologyLeafSupported = FALSE;
> > -
> > - //
> > - // Check if the processor is capable of supporting more than one logical
> processor.
> > - //
> > - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > - ASSERT ((RegEdx & BIT28) != 0);
> > -
> > - //
> > - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > - //
> > -
> > - //
> > - // Get the max index of basic CPUID
> > - //
> > - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > -
> > - //
> > - // If the extended topology enumeration leaf is available, it
> > - // is the preferred mechanism for enumerating topology.
> > - //
> > - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> &RegEcx, NULL);
> > - //
> > - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input
> value for
> > - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is
> not
> > - // supported on that processor.
> > - //
> > - if ((RegEbx & 0xffff) != 0) {
> > - TopologyLeafSupported = TRUE;
> > -
> > - //
> > - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters
> to extract
> > - // the SMT sub-field of x2APIC ID.
> > - //
> > - LevelType = (RegEcx >> 8) & 0xff;
> > - ASSERT (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > - if ((RegEbx & 0xffff) > 1 ) {
> > - ThreadBits = RegEax & 0x1f;
> > - } else {
> > - //
> > - // HT is not supported
> > - //
> > - ThreadBits = 0;
> > - }
> > -
> > - //
> > - // Software must not assume any "level type" encoding
> > - // value to be related to any sub-leaf index, except sub-leaf 0.
> > - //
> > - SubIndex = 1;
> > - do {
> > - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> NULL, &RegEcx, NULL);
> > - LevelType = (RegEcx >> 8) & 0xff;
> > - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > - CoreBits = (RegEax & 0x1f) - ThreadBits;
> > - break;
> > - }
> > - SubIndex++;
> > - } while (LevelType !=
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > - }
> > - }
> > -
> > - if (!TopologyLeafSupported) {
> > - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> > - MaxCoresPerPackage = (RegEax >> 26) + 1;
> > - } else {
> > - //
> > - // Must be a single-core processor.
> > - //
> > - MaxCoresPerPackage = 1;
> > - }
> > -
> > - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> > MaxCoresPerPackage - 1) + 1);
> > - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > - }
> > -
> > - Location->Thread = ApicId & ~((-1) << ThreadBits);
> > - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> > -
> > -/**
> > Gets processor information on the requested processor at the
> > instant this call is made.
> >
> > @param[in] This A pointer to the
> EFI_SMM_CPU_SERVICE_PROTOCOL
> > instance.
> > @@ -280,7 +161,7 @@ SmmAddProcessor (
> > gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> INVALID_APIC_ID) {
> > gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> > gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> > - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> > >ProcessorInfo[Index].Location);
> > + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> > + &gSmmCpuPrivate-
> > >ProcessorInfo[Index].Location);
> >
> > *ProcessorNumber = Index;
> > gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 3:19 ` Duran, Leo
@ 2016-10-28 4:43 ` Fan, Jeff
2016-10-28 10:28 ` Laszlo Ersek
2016-10-28 14:28 ` Duran, Leo
0 siblings, 2 replies; 13+ messages in thread
From: Fan, Jeff @ 2016-10-28 4:43 UTC (permalink / raw)
To: Duran, Leo, Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Gao, Liming
Leo,
Got it. Could you please create the patch to add GetProcessorLocation() in Local APIC Lib?
There are two library instances required to be updated
UefiCpuPkg\Library\BaseXApicLib
UefiCpuPkg\Library\BaseXApicX2ApicLib
Then you could remove the ExtractProcessorLocation() from PiSmmCpuDxeSmm.
Thanks!
Jeff
-----Original Message-----
From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Friday, October 28, 2016 11:20 AM
To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
Cc: Gao, Liming
Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
Please see my reply below.
Thanks,
Leo
> -----Original Message-----
> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> Sent: Thursday, October 27, 2016 8:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> <leo.duran@amd.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Because the CPU location information are gotten from Initial APIC ID,
> it makes more sense to be added into Local APIC Lib.
>
> The following is my proposal on its definition.
> /**
> Get CPU Package/Core/Thread location information.
>
> @param[in] InitialApicId CPU APIC ID
> @param[out] Package Pointer to Package ID
> @param[out] Core Pointer to Core ID
> @param[out] Thread Pointer to Thread ID
> **/
> VOID
> ExtractProcessorLocation (
> IN UINT32 InitialApicId,
> OUT UINT32 *Package,
> OUT UINT32 *Core,
> OUT UINT32 *Thread
> );
>
> Leo, is it OK to meet your requirement?
>
> Thanks!
> Jeff
[Duran, Leo]
Sure thing.
The main point is to get the vendor-specific CPUID code out of the driver, and into a library.
>
> -----Original Message-----
> From: Fan, Jeff
> Sent: Friday, October 28, 2016 8:45 AM
> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> Cc: Gao, Liming
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Leo and Mike,
>
> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it
> is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
>
> I suggest that we could add this API into
> UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
> Thus, it could be consumed by modules across PEI/DXE/SMM modules.
>
> Thanks!
> Jeff
>
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, October 28, 2016 7:16 AM
> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> Cc: Gao, Liming; Fan, Jeff
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Leo,
>
> This looks like a good proposed change to the SmmFeaturesLib and
> PiSmmCpuDxeSmm module.
>
> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
>
> There are 3 implementations of the SmmFeaturesLib in edk2/master.
> This patch needs to update all 3, or some of the platforms in
> edk2/master will no longer build:
>
> * OvmfPkg\Library\SmmCpuFeaturesLib
> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> * UefiCpuPkg\Library\SmmCpuFeaturesLib
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > Of Leo Duran
> > Sent: Thursday, October 27, 2016 3:30 PM
> > To: edk2-devel@lists.01.org
> > Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> <liming.gao@intel.com>
> > Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> > 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
> > library
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > ---
> > UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> > .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> ++++++++++++++++++++
> > UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +------------------
> --
> > 3 files changed, 136 insertions(+), 120 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > index 4478003..dd14ec5 100644
> > --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > IN UINTN Pages
> > );
> >
> > +/**
> > + Get Package ID/Core ID/Thread ID of a processor.
> > +
> > + APIC ID must be an initial APIC ID.
> > +
> > + The algorithm assumes the target system has symmetry across
> > + physical package
> > boundaries
> > + with respect to the number of logical processors per package,
> > + number of cores per
> > package.
> > +
> > + @param ApicId APIC ID of the target logical processor.
> > + @param Location Returns the processor location information.
> > +**/
> > +VOID
> > +SmmCpuFeaturesGetProcessorLocation (
> > + IN UINT32 ApicId,
> > + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > + );
> > +
> > #endif
> > diff --git
> > a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > index 1754f2d..1e300f3 100644
> > --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > return NULL;
> > }
> >
> > +/**
> > + Get Package ID/Core ID/Thread ID of a processor.
> > +
> > + APIC ID must be an initial APIC ID.
> > +
> > + The algorithm below assumes the target system has symmetry across
> > + physical package
> > boundaries
> > + with respect to the number of logical processors per package,
> > + number of cores per
> > package.
> > +
> > + @param ApicId APIC ID of the target logical processor.
> > + @param Location Returns the processor location information.
> > +**/
> > +VOID
> > +SmmCpuFeaturesGetProcessorLocation (
> > + IN UINT32 ApicId,
> > + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > + )
> > +{
> > + UINTN ThreadBits;
> > + UINTN CoreBits;
> > + UINT32 RegEax;
> > + UINT32 RegEbx;
> > + UINT32 RegEcx;
> > + UINT32 RegEdx;
> > + UINT32 MaxCpuIdIndex;
> > + UINT32 SubIndex;
> > + UINTN LevelType;
> > + UINT32 MaxLogicProcessorsPerPackage;
> > + UINT32 MaxCoresPerPackage;
> > + BOOLEAN TopologyLeafSupported;
> > +
> > + ASSERT (Location != NULL);
> > +
> > + ThreadBits = 0;
> > + CoreBits = 0;
> > + TopologyLeafSupported = FALSE;
> > +
> > + //
> > + // Check if the processor is capable of supporting more than one
> > + logical
> processor.
> > + //
> > + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> ASSERT
> > + ((RegEdx & BIT28) != 0);
> > +
> > + //
> > + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > + //
> > +
> > + //
> > + // Get the max index of basic CPUID // AsmCpuid
> > + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > +
> > + //
> > + // If the extended topology enumeration leaf is available, it //
> > + is the preferred mechanism for enumerating topology.
> > + //
> > + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> &RegEcx, NULL);
> > + //
> > + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> > + input
> value for
> > + // basic CPUID information is greater than 0BH, then CPUID.0BH
> > + leaf is
> not
> > + // supported on that processor.
> > + //
> > + if ((RegEbx & 0xffff) != 0) {
> > + TopologyLeafSupported = TRUE;
> > +
> > + //
> > + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> > + parameters
> to extract
> > + // the SMT sub-field of x2APIC ID.
> > + //
> > + LevelType = (RegEcx >> 8) & 0xff;
> > + ASSERT (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > + if ((RegEbx & 0xffff) > 1 ) {
> > + ThreadBits = RegEax & 0x1f;
> > + } else {
> > + //
> > + // HT is not supported
> > + //
> > + ThreadBits = 0;
> > + }
> > +
> > + //
> > + // Software must not assume any "level type" encoding
> > + // value to be related to any sub-leaf index, except sub-leaf 0.
> > + //
> > + SubIndex = 1;
> > + do {
> > + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> NULL, &RegEcx, NULL);
> > + LevelType = (RegEcx >> 8) & 0xff;
> > + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > + CoreBits = (RegEax & 0x1f) - ThreadBits;
> > + break;
> > + }
> > + SubIndex++;
> > + } while (LevelType !=
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > + }
> > + }
> > +
> > + if (!TopologyLeafSupported) {
> > + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> NULL);
> > + MaxCoresPerPackage = (RegEax >> 26) + 1;
> > + } else {
> > + //
> > + // Must be a single-core processor.
> > + //
> > + MaxCoresPerPackage = 1;
> > + }
> > +
> > + ThreadBits = (UINTN) (HighBitSet32
> > + (MaxLogicProcessorsPerPackage /
> > MaxCoresPerPackage - 1) + 1);
> > + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > + }
> > +
> > + Location->Thread = ApicId & ~((-1) << ThreadBits);
> > + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > index 40f2a17..2bfb1e8 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> mSmmCpuService = {
> > };
> >
> > /**
> > - Get Package ID/Core ID/Thread ID of a processor.
> > -
> > - APIC ID must be an initial APIC ID.
> > -
> > - The algorithm below assumes the target system has symmetry across
> > physical package boundaries
> > - with respect to the number of logical processors per package,
> > number of cores per package.
> > -
> > - @param ApicId APIC ID of the target logical processor.
> > - @param Location Returns the processor location information.
> > -**/
> > -VOID
> > -SmmGetProcessorLocation (
> > - IN UINT32 ApicId,
> > - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > - )
> > -{
> > - UINTN ThreadBits;
> > - UINTN CoreBits;
> > - UINT32 RegEax;
> > - UINT32 RegEbx;
> > - UINT32 RegEcx;
> > - UINT32 RegEdx;
> > - UINT32 MaxCpuIdIndex;
> > - UINT32 SubIndex;
> > - UINTN LevelType;
> > - UINT32 MaxLogicProcessorsPerPackage;
> > - UINT32 MaxCoresPerPackage;
> > - BOOLEAN TopologyLeafSupported;
> > -
> > - ASSERT (Location != NULL);
> > -
> > - ThreadBits = 0;
> > - CoreBits = 0;
> > - TopologyLeafSupported = FALSE;
> > -
> > - //
> > - // Check if the processor is capable of supporting more than one
> > logical
> processor.
> > - //
> > - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > - ASSERT ((RegEdx & BIT28) != 0);
> > -
> > - //
> > - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > - //
> > -
> > - //
> > - // Get the max index of basic CPUID
> > - //
> > - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > -
> > - //
> > - // If the extended topology enumeration leaf is available, it
> > - // is the preferred mechanism for enumerating topology.
> > - //
> > - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> &RegEcx, NULL);
> > - //
> > - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input
> value for
> > - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is
> not
> > - // supported on that processor.
> > - //
> > - if ((RegEbx & 0xffff) != 0) {
> > - TopologyLeafSupported = TRUE;
> > -
> > - //
> > - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters
> to extract
> > - // the SMT sub-field of x2APIC ID.
> > - //
> > - LevelType = (RegEcx >> 8) & 0xff;
> > - ASSERT (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > - if ((RegEbx & 0xffff) > 1 ) {
> > - ThreadBits = RegEax & 0x1f;
> > - } else {
> > - //
> > - // HT is not supported
> > - //
> > - ThreadBits = 0;
> > - }
> > -
> > - //
> > - // Software must not assume any "level type" encoding
> > - // value to be related to any sub-leaf index, except sub-leaf 0.
> > - //
> > - SubIndex = 1;
> > - do {
> > - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> NULL, &RegEcx, NULL);
> > - LevelType = (RegEcx >> 8) & 0xff;
> > - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > - CoreBits = (RegEax & 0x1f) - ThreadBits;
> > - break;
> > - }
> > - SubIndex++;
> > - } while (LevelType !=
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > - }
> > - }
> > -
> > - if (!TopologyLeafSupported) {
> > - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> > - MaxCoresPerPackage = (RegEax >> 26) + 1;
> > - } else {
> > - //
> > - // Must be a single-core processor.
> > - //
> > - MaxCoresPerPackage = 1;
> > - }
> > -
> > - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> > MaxCoresPerPackage - 1) + 1);
> > - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > - }
> > -
> > - Location->Thread = ApicId & ~((-1) << ThreadBits);
> > - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> > -
> > -/**
> > Gets processor information on the requested processor at the
> > instant this call is made.
> >
> > @param[in] This A pointer to the
> EFI_SMM_CPU_SERVICE_PROTOCOL
> > instance.
> > @@ -280,7 +161,7 @@ SmmAddProcessor (
> > gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> INVALID_APIC_ID) {
> > gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> > gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> > - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> > >ProcessorInfo[Index].Location);
> > + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> > + &gSmmCpuPrivate-
> > >ProcessorInfo[Index].Location);
> >
> > *ProcessorNumber = Index;
> > gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 4:43 ` Fan, Jeff
@ 2016-10-28 10:28 ` Laszlo Ersek
2016-10-28 15:29 ` Kinney, Michael D
2016-10-28 14:28 ` Duran, Leo
1 sibling, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2016-10-28 10:28 UTC (permalink / raw)
To: Fan, Jeff, Duran, Leo, Kinney, Michael D
Cc: edk2-devel@lists.01.org, Gao, Liming
On 10/28/16 06:43, Fan, Jeff wrote:
> Leo,
>
> Got it. Could you please create the patch to add GetProcessorLocation() in Local APIC Lib?
>
> There are two library instances required to be updated
> UefiCpuPkg\Library\BaseXApicLib
> UefiCpuPkg\Library\BaseXApicX2ApicLib
>
> Then you could remove the ExtractProcessorLocation() from PiSmmCpuDxeSmm.
First of all, thank you Mike for not forgetting about OvmfPkg :)
Second, to Leo: if the GetProcessorLocation() API (and its current
implementation) are moved to the LocalApicLib class (and the two
LocalApicLib instances named by Jeff), then at least OvmfPkg will need
no handling. OvmfPkg uses the BaseXApicX2ApicLib instance.
However, I think QuarkSocPkg might still require an update, because it
doesn't seem to resolve or depend on LocalApicLib at all, at the moment.
Thanks
Laszlo
>
> Thanks!
> Jeff
>
> -----Original Message-----
> From: Duran, Leo [mailto:leo.duran@amd.com]
> Sent: Friday, October 28, 2016 11:20 AM
> To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
> Cc: Gao, Liming
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
>
> Please see my reply below.
> Thanks,
> Leo
>
>> -----Original Message-----
>> From: Fan, Jeff [mailto:jeff.fan@intel.com]
>> Sent: Thursday, October 27, 2016 8:00 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
>> <leo.duran@amd.com>; edk2-devel@lists.01.org
>> Cc: Gao, Liming <liming.gao@intel.com>
>> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
>> SmmCpuFeaturesLib library.
>>
>> Because the CPU location information are gotten from Initial APIC ID,
>> it makes more sense to be added into Local APIC Lib.
>>
>> The following is my proposal on its definition.
>> /**
>> Get CPU Package/Core/Thread location information.
>>
>> @param[in] InitialApicId CPU APIC ID
>> @param[out] Package Pointer to Package ID
>> @param[out] Core Pointer to Core ID
>> @param[out] Thread Pointer to Thread ID
>> **/
>> VOID
>> ExtractProcessorLocation (
>> IN UINT32 InitialApicId,
>> OUT UINT32 *Package,
>> OUT UINT32 *Core,
>> OUT UINT32 *Thread
>> );
>>
>> Leo, is it OK to meet your requirement?
>>
>> Thanks!
>> Jeff
> [Duran, Leo]
> Sure thing.
> The main point is to get the vendor-specific CPUID code out of the driver, and into a library.
>>
>> -----Original Message-----
>> From: Fan, Jeff
>> Sent: Friday, October 28, 2016 8:45 AM
>> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
>> Cc: Gao, Liming
>> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
>> SmmCpuFeaturesLib library.
>>
>> Leo and Mike,
>>
>> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it
>> is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
>>
>> I suggest that we could add this API into
>> UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
>> Thus, it could be consumed by modules across PEI/DXE/SMM modules.
>>
>> Thanks!
>> Jeff
>>
>> -----Original Message-----
>> From: Kinney, Michael D
>> Sent: Friday, October 28, 2016 7:16 AM
>> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
>> Cc: Gao, Liming; Fan, Jeff
>> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
>> SmmCpuFeaturesLib library.
>>
>> Leo,
>>
>> This looks like a good proposed change to the SmmFeaturesLib and
>> PiSmmCpuDxeSmm module.
>>
>> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
>>
>> There are 3 implementations of the SmmFeaturesLib in edk2/master.
>> This patch needs to update all 3, or some of the platforms in
>> edk2/master will no longer build:
>>
>> * OvmfPkg\Library\SmmCpuFeaturesLib
>> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
>> * UefiCpuPkg\Library\SmmCpuFeaturesLib
>>
>> Thanks,
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>>> Of Leo Duran
>>> Sent: Thursday, October 27, 2016 3:30 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
>> <liming.gao@intel.com>
>>> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
>>> SmmCpuFeaturesLib library.
>>>
>>> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
>>> 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
>>> library
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Leo Duran <leo.duran@amd.com>
>>> ---
>>> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
>>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
>> ++++++++++++++++++++
>>> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +------------------
>> --
>>> 3 files changed, 136 insertions(+), 120 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
>>> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
>>> index 4478003..dd14ec5 100644
>>> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
>>> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
>>> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
>>> IN UINTN Pages
>>> );
>>>
>>> +/**
>>> + Get Package ID/Core ID/Thread ID of a processor.
>>> +
>>> + APIC ID must be an initial APIC ID.
>>> +
>>> + The algorithm assumes the target system has symmetry across
>>> + physical package
>>> boundaries
>>> + with respect to the number of logical processors per package,
>>> + number of cores per
>>> package.
>>> +
>>> + @param ApicId APIC ID of the target logical processor.
>>> + @param Location Returns the processor location information.
>>> +**/
>>> +VOID
>>> +SmmCpuFeaturesGetProcessorLocation (
>>> + IN UINT32 ApicId,
>>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
>>> + );
>>> +
>>> #endif
>>> diff --git
>>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> index 1754f2d..1e300f3 100644
>>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>>> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
>>> return NULL;
>>> }
>>>
>>> +/**
>>> + Get Package ID/Core ID/Thread ID of a processor.
>>> +
>>> + APIC ID must be an initial APIC ID.
>>> +
>>> + The algorithm below assumes the target system has symmetry across
>>> + physical package
>>> boundaries
>>> + with respect to the number of logical processors per package,
>>> + number of cores per
>>> package.
>>> +
>>> + @param ApicId APIC ID of the target logical processor.
>>> + @param Location Returns the processor location information.
>>> +**/
>>> +VOID
>>> +SmmCpuFeaturesGetProcessorLocation (
>>> + IN UINT32 ApicId,
>>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
>>> + )
>>> +{
>>> + UINTN ThreadBits;
>>> + UINTN CoreBits;
>>> + UINT32 RegEax;
>>> + UINT32 RegEbx;
>>> + UINT32 RegEcx;
>>> + UINT32 RegEdx;
>>> + UINT32 MaxCpuIdIndex;
>>> + UINT32 SubIndex;
>>> + UINTN LevelType;
>>> + UINT32 MaxLogicProcessorsPerPackage;
>>> + UINT32 MaxCoresPerPackage;
>>> + BOOLEAN TopologyLeafSupported;
>>> +
>>> + ASSERT (Location != NULL);
>>> +
>>> + ThreadBits = 0;
>>> + CoreBits = 0;
>>> + TopologyLeafSupported = FALSE;
>>> +
>>> + //
>>> + // Check if the processor is capable of supporting more than one
>>> + logical
>> processor.
>>> + //
>>> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
>> ASSERT
>>> + ((RegEdx & BIT28) != 0);
>>> +
>>> + //
>>> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
>>> + //
>>> +
>>> + //
>>> + // Get the max index of basic CPUID // AsmCpuid
>>> + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
>>> +
>>> + //
>>> + // If the extended topology enumeration leaf is available, it //
>>> + is the preferred mechanism for enumerating topology.
>>> + //
>>> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
>>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
>> &RegEcx, NULL);
>>> + //
>>> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
>>> + input
>> value for
>>> + // basic CPUID information is greater than 0BH, then CPUID.0BH
>>> + leaf is
>> not
>>> + // supported on that processor.
>>> + //
>>> + if ((RegEbx & 0xffff) != 0) {
>>> + TopologyLeafSupported = TRUE;
>>> +
>>> + //
>>> + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
>>> + parameters
>> to extract
>>> + // the SMT sub-field of x2APIC ID.
>>> + //
>>> + LevelType = (RegEcx >> 8) & 0xff;
>>> + ASSERT (LevelType ==
>> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
>>> + if ((RegEbx & 0xffff) > 1 ) {
>>> + ThreadBits = RegEax & 0x1f;
>>> + } else {
>>> + //
>>> + // HT is not supported
>>> + //
>>> + ThreadBits = 0;
>>> + }
>>> +
>>> + //
>>> + // Software must not assume any "level type" encoding
>>> + // value to be related to any sub-leaf index, except sub-leaf 0.
>>> + //
>>> + SubIndex = 1;
>>> + do {
>>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
>> NULL, &RegEcx, NULL);
>>> + LevelType = (RegEcx >> 8) & 0xff;
>>> + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
>>> + CoreBits = (RegEax & 0x1f) - ThreadBits;
>>> + break;
>>> + }
>>> + SubIndex++;
>>> + } while (LevelType !=
>> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
>>> + }
>>> + }
>>> +
>>> + if (!TopologyLeafSupported) {
>>> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
>>> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
>>> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
>>> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
>> NULL);
>>> + MaxCoresPerPackage = (RegEax >> 26) + 1;
>>> + } else {
>>> + //
>>> + // Must be a single-core processor.
>>> + //
>>> + MaxCoresPerPackage = 1;
>>> + }
>>> +
>>> + ThreadBits = (UINTN) (HighBitSet32
>>> + (MaxLogicProcessorsPerPackage /
>>> MaxCoresPerPackage - 1) + 1);
>>> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
>>> + }
>>> +
>>> + Location->Thread = ApicId & ~((-1) << ThreadBits);
>>> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
>>> + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>>> index 40f2a17..2bfb1e8 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
>>> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
>> mSmmCpuService = {
>>> };
>>>
>>> /**
>>> - Get Package ID/Core ID/Thread ID of a processor.
>>> -
>>> - APIC ID must be an initial APIC ID.
>>> -
>>> - The algorithm below assumes the target system has symmetry across
>>> physical package boundaries
>>> - with respect to the number of logical processors per package,
>>> number of cores per package.
>>> -
>>> - @param ApicId APIC ID of the target logical processor.
>>> - @param Location Returns the processor location information.
>>> -**/
>>> -VOID
>>> -SmmGetProcessorLocation (
>>> - IN UINT32 ApicId,
>>> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
>>> - )
>>> -{
>>> - UINTN ThreadBits;
>>> - UINTN CoreBits;
>>> - UINT32 RegEax;
>>> - UINT32 RegEbx;
>>> - UINT32 RegEcx;
>>> - UINT32 RegEdx;
>>> - UINT32 MaxCpuIdIndex;
>>> - UINT32 SubIndex;
>>> - UINTN LevelType;
>>> - UINT32 MaxLogicProcessorsPerPackage;
>>> - UINT32 MaxCoresPerPackage;
>>> - BOOLEAN TopologyLeafSupported;
>>> -
>>> - ASSERT (Location != NULL);
>>> -
>>> - ThreadBits = 0;
>>> - CoreBits = 0;
>>> - TopologyLeafSupported = FALSE;
>>> -
>>> - //
>>> - // Check if the processor is capable of supporting more than one
>>> logical
>> processor.
>>> - //
>>> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
>>> - ASSERT ((RegEdx & BIT28) != 0);
>>> -
>>> - //
>>> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
>>> - //
>>> -
>>> - //
>>> - // Get the max index of basic CPUID
>>> - //
>>> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
>>> -
>>> - //
>>> - // If the extended topology enumeration leaf is available, it
>>> - // is the preferred mechanism for enumerating topology.
>>> - //
>>> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
>>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
>> &RegEcx, NULL);
>>> - //
>>> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input
>> value for
>>> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is
>> not
>>> - // supported on that processor.
>>> - //
>>> - if ((RegEbx & 0xffff) != 0) {
>>> - TopologyLeafSupported = TRUE;
>>> -
>>> - //
>>> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters
>> to extract
>>> - // the SMT sub-field of x2APIC ID.
>>> - //
>>> - LevelType = (RegEcx >> 8) & 0xff;
>>> - ASSERT (LevelType ==
>> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
>>> - if ((RegEbx & 0xffff) > 1 ) {
>>> - ThreadBits = RegEax & 0x1f;
>>> - } else {
>>> - //
>>> - // HT is not supported
>>> - //
>>> - ThreadBits = 0;
>>> - }
>>> -
>>> - //
>>> - // Software must not assume any "level type" encoding
>>> - // value to be related to any sub-leaf index, except sub-leaf 0.
>>> - //
>>> - SubIndex = 1;
>>> - do {
>>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
>> NULL, &RegEcx, NULL);
>>> - LevelType = (RegEcx >> 8) & 0xff;
>>> - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
>>> - CoreBits = (RegEax & 0x1f) - ThreadBits;
>>> - break;
>>> - }
>>> - SubIndex++;
>>> - } while (LevelType !=
>> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
>>> - }
>>> - }
>>> -
>>> - if (!TopologyLeafSupported) {
>>> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
>>> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
>>> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
>>> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
>>> - MaxCoresPerPackage = (RegEax >> 26) + 1;
>>> - } else {
>>> - //
>>> - // Must be a single-core processor.
>>> - //
>>> - MaxCoresPerPackage = 1;
>>> - }
>>> -
>>> - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
>>> MaxCoresPerPackage - 1) + 1);
>>> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
>>> - }
>>> -
>>> - Location->Thread = ApicId & ~((-1) << ThreadBits);
>>> - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
>>> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
>>> -
>>> -/**
>>> Gets processor information on the requested processor at the
>>> instant this call is made.
>>>
>>> @param[in] This A pointer to the
>> EFI_SMM_CPU_SERVICE_PROTOCOL
>>> instance.
>>> @@ -280,7 +161,7 @@ SmmAddProcessor (
>>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
>> INVALID_APIC_ID) {
>>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
>>> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
>>> - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
>>>> ProcessorInfo[Index].Location);
>>> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
>>> + &gSmmCpuPrivate-
>>>> ProcessorInfo[Index].Location);
>>>
>>> *ProcessorNumber = Index;
>>> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 4:43 ` Fan, Jeff
2016-10-28 10:28 ` Laszlo Ersek
@ 2016-10-28 14:28 ` Duran, Leo
1 sibling, 0 replies; 13+ messages in thread
From: Duran, Leo @ 2016-10-28 14:28 UTC (permalink / raw)
To: 'Fan, Jeff', Kinney, Michael D, edk2-devel@lists.01.org
Cc: Gao, Liming
Jeff, et al,
Do we also remove GetProcessorLocation() from MpInitLib?
Leo
> -----Original Message-----
> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> Sent: Thursday, October 27, 2016 11:43 PM
> To: Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Leo,
>
> Got it. Could you please create the patch to add GetProcessorLocation() in
> Local APIC Lib?
>
> There are two library instances required to be updated
> UefiCpuPkg\Library\BaseXApicLib
> UefiCpuPkg\Library\BaseXApicX2ApicLib
>
> Then you could remove the ExtractProcessorLocation() from
> PiSmmCpuDxeSmm.
>
> Thanks!
> Jeff
>
> -----Original Message-----
> From: Duran, Leo [mailto:leo.duran@amd.com]
> Sent: Friday, October 28, 2016 11:20 AM
> To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
> Cc: Gao, Liming
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Please see my reply below.
> Thanks,
> Leo
>
> > -----Original Message-----
> > From: Fan, Jeff [mailto:jeff.fan@intel.com]
> > Sent: Thursday, October 27, 2016 8:00 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> > <leo.duran@amd.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > Because the CPU location information are gotten from Initial APIC ID,
> > it makes more sense to be added into Local APIC Lib.
> >
> > The following is my proposal on its definition.
> > /**
> > Get CPU Package/Core/Thread location information.
> >
> > @param[in] InitialApicId CPU APIC ID
> > @param[out] Package Pointer to Package ID
> > @param[out] Core Pointer to Core ID
> > @param[out] Thread Pointer to Thread ID
> > **/
> > VOID
> > ExtractProcessorLocation (
> > IN UINT32 InitialApicId,
> > OUT UINT32 *Package,
> > OUT UINT32 *Core,
> > OUT UINT32 *Thread
> > );
> >
> > Leo, is it OK to meet your requirement?
> >
> > Thanks!
> > Jeff
> [Duran, Leo]
> Sure thing.
> The main point is to get the vendor-specific CPUID code out of the driver,
> and into a library.
> >
> > -----Original Message-----
> > From: Fan, Jeff
> > Sent: Friday, October 28, 2016 8:45 AM
> > To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> > Cc: Gao, Liming
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > Leo and Mike,
> >
> > GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it
> > is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
> >
> > I suggest that we could add this API into
> > UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
> > Thus, it could be consumed by modules across PEI/DXE/SMM modules.
> >
> > Thanks!
> > Jeff
> >
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Friday, October 28, 2016 7:16 AM
> > To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> > Cc: Gao, Liming; Fan, Jeff
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > Leo,
> >
> > This looks like a good proposed change to the SmmFeaturesLib and
> > PiSmmCpuDxeSmm module.
> >
> > Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
> >
> > There are 3 implementations of the SmmFeaturesLib in edk2/master.
> > This patch needs to update all 3, or some of the platforms in
> > edk2/master will no longer build:
> >
> > * OvmfPkg\Library\SmmCpuFeaturesLib
> > * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> > * UefiCpuPkg\Library\SmmCpuFeaturesLib
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Leo Duran
> > > Sent: Thursday, October 27, 2016 3:30 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> > <liming.gao@intel.com>
> > > Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > > SmmCpuFeaturesLib library.
> > >
> > > 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> > > 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
> > > library
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > > ---
> > > UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> > > .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> > ++++++++++++++++++++
> > > UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +----------------
> --
> > --
> > > 3 files changed, 136 insertions(+), 120 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > index 4478003..dd14ec5 100644
> > > --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > > IN UINTN Pages
> > > );
> > >
> > > +/**
> > > + Get Package ID/Core ID/Thread ID of a processor.
> > > +
> > > + APIC ID must be an initial APIC ID.
> > > +
> > > + The algorithm assumes the target system has symmetry across
> > > + physical package
> > > boundaries
> > > + with respect to the number of logical processors per package,
> > > + number of cores per
> > > package.
> > > +
> > > + @param ApicId APIC ID of the target logical processor.
> > > + @param Location Returns the processor location information.
> > > +**/
> > > +VOID
> > > +SmmCpuFeaturesGetProcessorLocation (
> > > + IN UINT32 ApicId,
> > > + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > > + );
> > > +
> > > #endif
> > > diff --git
> > > a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > index 1754f2d..1e300f3 100644
> > > --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > > return NULL;
> > > }
> > >
> > > +/**
> > > + Get Package ID/Core ID/Thread ID of a processor.
> > > +
> > > + APIC ID must be an initial APIC ID.
> > > +
> > > + The algorithm below assumes the target system has symmetry across
> > > + physical package
> > > boundaries
> > > + with respect to the number of logical processors per package,
> > > + number of cores per
> > > package.
> > > +
> > > + @param ApicId APIC ID of the target logical processor.
> > > + @param Location Returns the processor location information.
> > > +**/
> > > +VOID
> > > +SmmCpuFeaturesGetProcessorLocation (
> > > + IN UINT32 ApicId,
> > > + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > > + )
> > > +{
> > > + UINTN ThreadBits;
> > > + UINTN CoreBits;
> > > + UINT32 RegEax;
> > > + UINT32 RegEbx;
> > > + UINT32 RegEcx;
> > > + UINT32 RegEdx;
> > > + UINT32 MaxCpuIdIndex;
> > > + UINT32 SubIndex;
> > > + UINTN LevelType;
> > > + UINT32 MaxLogicProcessorsPerPackage;
> > > + UINT32 MaxCoresPerPackage;
> > > + BOOLEAN TopologyLeafSupported;
> > > +
> > > + ASSERT (Location != NULL);
> > > +
> > > + ThreadBits = 0;
> > > + CoreBits = 0;
> > > + TopologyLeafSupported = FALSE;
> > > +
> > > + //
> > > + // Check if the processor is capable of supporting more than one
> > > + logical
> > processor.
> > > + //
> > > + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > ASSERT
> > > + ((RegEdx & BIT28) != 0);
> > > +
> > > + //
> > > + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > > + //
> > > +
> > > + //
> > > + // Get the max index of basic CPUID // AsmCpuid
> > > + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > > +
> > > + //
> > > + // If the extended topology enumeration leaf is available, it //
> > > + is the preferred mechanism for enumerating topology.
> > > + //
> > > + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > > + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> > &RegEcx, NULL);
> > > + //
> > > + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> > > + input
> > value for
> > > + // basic CPUID information is greater than 0BH, then CPUID.0BH
> > > + leaf is
> > not
> > > + // supported on that processor.
> > > + //
> > > + if ((RegEbx & 0xffff) != 0) {
> > > + TopologyLeafSupported = TRUE;
> > > +
> > > + //
> > > + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> > > + parameters
> > to extract
> > > + // the SMT sub-field of x2APIC ID.
> > > + //
> > > + LevelType = (RegEcx >> 8) & 0xff;
> > > + ASSERT (LevelType ==
> > CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > > + if ((RegEbx & 0xffff) > 1 ) {
> > > + ThreadBits = RegEax & 0x1f;
> > > + } else {
> > > + //
> > > + // HT is not supported
> > > + //
> > > + ThreadBits = 0;
> > > + }
> > > +
> > > + //
> > > + // Software must not assume any "level type" encoding
> > > + // value to be related to any sub-leaf index, except sub-leaf 0.
> > > + //
> > > + SubIndex = 1;
> > > + do {
> > > + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> > NULL, &RegEcx, NULL);
> > > + LevelType = (RegEcx >> 8) & 0xff;
> > > + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE)
> {
> > > + CoreBits = (RegEax & 0x1f) - ThreadBits;
> > > + break;
> > > + }
> > > + SubIndex++;
> > > + } while (LevelType !=
> > CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > > + }
> > > + }
> > > +
> > > + if (!TopologyLeafSupported) {
> > > + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > > + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > > + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > > + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> > NULL);
> > > + MaxCoresPerPackage = (RegEax >> 26) + 1;
> > > + } else {
> > > + //
> > > + // Must be a single-core processor.
> > > + //
> > > + MaxCoresPerPackage = 1;
> > > + }
> > > +
> > > + ThreadBits = (UINTN) (HighBitSet32
> > > + (MaxLogicProcessorsPerPackage /
> > > MaxCoresPerPackage - 1) + 1);
> > > + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > > + }
> > > +
> > > + Location->Thread = ApicId & ~((-1) << ThreadBits);
> > > + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > > + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > index 40f2a17..2bfb1e8 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> > mSmmCpuService = {
> > > };
> > >
> > > /**
> > > - Get Package ID/Core ID/Thread ID of a processor.
> > > -
> > > - APIC ID must be an initial APIC ID.
> > > -
> > > - The algorithm below assumes the target system has symmetry across
> > > physical package boundaries
> > > - with respect to the number of logical processors per package,
> > > number of cores per package.
> > > -
> > > - @param ApicId APIC ID of the target logical processor.
> > > - @param Location Returns the processor location information.
> > > -**/
> > > -VOID
> > > -SmmGetProcessorLocation (
> > > - IN UINT32 ApicId,
> > > - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > > - )
> > > -{
> > > - UINTN ThreadBits;
> > > - UINTN CoreBits;
> > > - UINT32 RegEax;
> > > - UINT32 RegEbx;
> > > - UINT32 RegEcx;
> > > - UINT32 RegEdx;
> > > - UINT32 MaxCpuIdIndex;
> > > - UINT32 SubIndex;
> > > - UINTN LevelType;
> > > - UINT32 MaxLogicProcessorsPerPackage;
> > > - UINT32 MaxCoresPerPackage;
> > > - BOOLEAN TopologyLeafSupported;
> > > -
> > > - ASSERT (Location != NULL);
> > > -
> > > - ThreadBits = 0;
> > > - CoreBits = 0;
> > > - TopologyLeafSupported = FALSE;
> > > -
> > > - //
> > > - // Check if the processor is capable of supporting more than one
> > > logical
> > processor.
> > > - //
> > > - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > > - ASSERT ((RegEdx & BIT28) != 0);
> > > -
> > > - //
> > > - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > > - //
> > > -
> > > - //
> > > - // Get the max index of basic CPUID
> > > - //
> > > - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > > -
> > > - //
> > > - // If the extended topology enumeration leaf is available, it
> > > - // is the preferred mechanism for enumerating topology.
> > > - //
> > > - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > > - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> > &RegEcx, NULL);
> > > - //
> > > - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input
> > value for
> > > - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is
> > not
> > > - // supported on that processor.
> > > - //
> > > - if ((RegEbx & 0xffff) != 0) {
> > > - TopologyLeafSupported = TRUE;
> > > -
> > > - //
> > > - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> parameters
> > to extract
> > > - // the SMT sub-field of x2APIC ID.
> > > - //
> > > - LevelType = (RegEcx >> 8) & 0xff;
> > > - ASSERT (LevelType ==
> > CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > > - if ((RegEbx & 0xffff) > 1 ) {
> > > - ThreadBits = RegEax & 0x1f;
> > > - } else {
> > > - //
> > > - // HT is not supported
> > > - //
> > > - ThreadBits = 0;
> > > - }
> > > -
> > > - //
> > > - // Software must not assume any "level type" encoding
> > > - // value to be related to any sub-leaf index, except sub-leaf 0.
> > > - //
> > > - SubIndex = 1;
> > > - do {
> > > - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> > NULL, &RegEcx, NULL);
> > > - LevelType = (RegEcx >> 8) & 0xff;
> > > - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE)
> {
> > > - CoreBits = (RegEax & 0x1f) - ThreadBits;
> > > - break;
> > > - }
> > > - SubIndex++;
> > > - } while (LevelType !=
> > CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > > - }
> > > - }
> > > -
> > > - if (!TopologyLeafSupported) {
> > > - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > > - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > > - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > > - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> NULL);
> > > - MaxCoresPerPackage = (RegEax >> 26) + 1;
> > > - } else {
> > > - //
> > > - // Must be a single-core processor.
> > > - //
> > > - MaxCoresPerPackage = 1;
> > > - }
> > > -
> > > - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> > > MaxCoresPerPackage - 1) + 1);
> > > - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > > - }
> > > -
> > > - Location->Thread = ApicId & ~((-1) << ThreadBits);
> > > - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > > - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> > > -
> > > -/**
> > > Gets processor information on the requested processor at the
> > > instant this call is made.
> > >
> > > @param[in] This A pointer to the
> > EFI_SMM_CPU_SERVICE_PROTOCOL
> > > instance.
> > > @@ -280,7 +161,7 @@ SmmAddProcessor (
> > > gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> > INVALID_APIC_ID) {
> > > gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> > > gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> > > - SmmGetProcessorLocation ((UINT32)ProcessorId,
> &gSmmCpuPrivate-
> > > >ProcessorInfo[Index].Location);
> > > + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> > > + &gSmmCpuPrivate-
> > > >ProcessorInfo[Index].Location);
> > >
> > > *ProcessorNumber = Index;
> > > gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 10:28 ` Laszlo Ersek
@ 2016-10-28 15:29 ` Kinney, Michael D
2016-10-28 15:30 ` Duran, Leo
0 siblings, 1 reply; 13+ messages in thread
From: Kinney, Michael D @ 2016-10-28 15:29 UTC (permalink / raw)
To: Laszlo Ersek, Fan, Jeff, Duran, Leo, Kinney, Michael D
Cc: edk2-devel@lists.01.org, Gao, Liming
Hi Laszlo,
Quark DSC files do map the LocalApicLib, so I think the proposal to add
GetProcessorLocation() to the LocalApicLib class and instances makes sense.
Thanks,
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 28, 2016 3:29 AM
> To: Fan, Jeff <jeff.fan@intel.com>; Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> On 10/28/16 06:43, Fan, Jeff wrote:
> > Leo,
> >
> > Got it. Could you please create the patch to add GetProcessorLocation() in Local APIC
> Lib?
> >
> > There are two library instances required to be updated
> > UefiCpuPkg\Library\BaseXApicLib
> > UefiCpuPkg\Library\BaseXApicX2ApicLib
> >
> > Then you could remove the ExtractProcessorLocation() from PiSmmCpuDxeSmm.
>
> First of all, thank you Mike for not forgetting about OvmfPkg :)
>
> Second, to Leo: if the GetProcessorLocation() API (and its current
> implementation) are moved to the LocalApicLib class (and the two
> LocalApicLib instances named by Jeff), then at least OvmfPkg will need
> no handling. OvmfPkg uses the BaseXApicX2ApicLib instance.
>
> However, I think QuarkSocPkg might still require an update, because it
> doesn't seem to resolve or depend on LocalApicLib at all, at the moment.
>
> Thanks
> Laszlo
>
> >
> > Thanks!
> > Jeff
> >
> > -----Original Message-----
> > From: Duran, Leo [mailto:leo.duran@amd.com]
> > Sent: Friday, October 28, 2016 11:20 AM
> > To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
> > Cc: Gao, Liming
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
> >
> > Please see my reply below.
> > Thanks,
> > Leo
> >
> >> -----Original Message-----
> >> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> >> Sent: Thursday, October 27, 2016 8:00 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> >> <leo.duran@amd.com>; edk2-devel@lists.01.org
> >> Cc: Gao, Liming <liming.gao@intel.com>
> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> >> SmmCpuFeaturesLib library.
> >>
> >> Because the CPU location information are gotten from Initial APIC ID,
> >> it makes more sense to be added into Local APIC Lib.
> >>
> >> The following is my proposal on its definition.
> >> /**
> >> Get CPU Package/Core/Thread location information.
> >>
> >> @param[in] InitialApicId CPU APIC ID
> >> @param[out] Package Pointer to Package ID
> >> @param[out] Core Pointer to Core ID
> >> @param[out] Thread Pointer to Thread ID
> >> **/
> >> VOID
> >> ExtractProcessorLocation (
> >> IN UINT32 InitialApicId,
> >> OUT UINT32 *Package,
> >> OUT UINT32 *Core,
> >> OUT UINT32 *Thread
> >> );
> >>
> >> Leo, is it OK to meet your requirement?
> >>
> >> Thanks!
> >> Jeff
> > [Duran, Leo]
> > Sure thing.
> > The main point is to get the vendor-specific CPUID code out of the driver, and into a
> library.
> >>
> >> -----Original Message-----
> >> From: Fan, Jeff
> >> Sent: Friday, October 28, 2016 8:45 AM
> >> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> >> Cc: Gao, Liming
> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> >> SmmCpuFeaturesLib library.
> >>
> >> Leo and Mike,
> >>
> >> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver, it
> >> is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
> >>
> >> I suggest that we could add this API into
> >> UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
> >> Thus, it could be consumed by modules across PEI/DXE/SMM modules.
> >>
> >> Thanks!
> >> Jeff
> >>
> >> -----Original Message-----
> >> From: Kinney, Michael D
> >> Sent: Friday, October 28, 2016 7:16 AM
> >> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> >> Cc: Gao, Liming; Fan, Jeff
> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> >> SmmCpuFeaturesLib library.
> >>
> >> Leo,
> >>
> >> This looks like a good proposed change to the SmmFeaturesLib and
> >> PiSmmCpuDxeSmm module.
> >>
> >> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
> >>
> >> There are 3 implementations of the SmmFeaturesLib in edk2/master.
> >> This patch needs to update all 3, or some of the platforms in
> >> edk2/master will no longer build:
> >>
> >> * OvmfPkg\Library\SmmCpuFeaturesLib
> >> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> >> * UefiCpuPkg\Library\SmmCpuFeaturesLib
> >>
> >> Thanks,
> >>
> >> Mike
> >>
> >>> -----Original Message-----
> >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >>> Of Leo Duran
> >>> Sent: Thursday, October 27, 2016 3:30 PM
> >>> To: edk2-devel@lists.01.org
> >>> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> >> <liming.gao@intel.com>
> >>> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> >>> SmmCpuFeaturesLib library.
> >>>
> >>> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm driver
> >>> 2) Add SmmCpuFeaturesGetProcessorLocation() to SmmCpuFeaturesLib
> >>> library
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Leo Duran <leo.duran@amd.com>
> >>> ---
> >>> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> >>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> >> ++++++++++++++++++++
> >>> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +------------------
> >> --
> >>> 3 files changed, 136 insertions(+), 120 deletions(-)
> >>>
> >>> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> >>> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> >>> index 4478003..dd14ec5 100644
> >>> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> >>> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> >>> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> >>> IN UINTN Pages
> >>> );
> >>>
> >>> +/**
> >>> + Get Package ID/Core ID/Thread ID of a processor.
> >>> +
> >>> + APIC ID must be an initial APIC ID.
> >>> +
> >>> + The algorithm assumes the target system has symmetry across
> >>> + physical package
> >>> boundaries
> >>> + with respect to the number of logical processors per package,
> >>> + number of cores per
> >>> package.
> >>> +
> >>> + @param ApicId APIC ID of the target logical processor.
> >>> + @param Location Returns the processor location information.
> >>> +**/
> >>> +VOID
> >>> +SmmCpuFeaturesGetProcessorLocation (
> >>> + IN UINT32 ApicId,
> >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> >>> + );
> >>> +
> >>> #endif
> >>> diff --git
> >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> >>> index 1754f2d..1e300f3 100644
> >>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> >>> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> >>> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory (
> >>> return NULL;
> >>> }
> >>>
> >>> +/**
> >>> + Get Package ID/Core ID/Thread ID of a processor.
> >>> +
> >>> + APIC ID must be an initial APIC ID.
> >>> +
> >>> + The algorithm below assumes the target system has symmetry across
> >>> + physical package
> >>> boundaries
> >>> + with respect to the number of logical processors per package,
> >>> + number of cores per
> >>> package.
> >>> +
> >>> + @param ApicId APIC ID of the target logical processor.
> >>> + @param Location Returns the processor location information.
> >>> +**/
> >>> +VOID
> >>> +SmmCpuFeaturesGetProcessorLocation (
> >>> + IN UINT32 ApicId,
> >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> >>> + )
> >>> +{
> >>> + UINTN ThreadBits;
> >>> + UINTN CoreBits;
> >>> + UINT32 RegEax;
> >>> + UINT32 RegEbx;
> >>> + UINT32 RegEcx;
> >>> + UINT32 RegEdx;
> >>> + UINT32 MaxCpuIdIndex;
> >>> + UINT32 SubIndex;
> >>> + UINTN LevelType;
> >>> + UINT32 MaxLogicProcessorsPerPackage;
> >>> + UINT32 MaxCoresPerPackage;
> >>> + BOOLEAN TopologyLeafSupported;
> >>> +
> >>> + ASSERT (Location != NULL);
> >>> +
> >>> + ThreadBits = 0;
> >>> + CoreBits = 0;
> >>> + TopologyLeafSupported = FALSE;
> >>> +
> >>> + //
> >>> + // Check if the processor is capable of supporting more than one
> >>> + logical
> >> processor.
> >>> + //
> >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> >> ASSERT
> >>> + ((RegEdx & BIT28) != 0);
> >>> +
> >>> + //
> >>> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> >>> + //
> >>> +
> >>> + //
> >>> + // Get the max index of basic CPUID // AsmCpuid
> >>> + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> >>> +
> >>> + //
> >>> + // If the extended topology enumeration leaf is available, it //
> >>> + is the preferred mechanism for enumerating topology.
> >>> + //
> >>> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> >> &RegEcx, NULL);
> >>> + //
> >>> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> >>> + input
> >> value for
> >>> + // basic CPUID information is greater than 0BH, then CPUID.0BH
> >>> + leaf is
> >> not
> >>> + // supported on that processor.
> >>> + //
> >>> + if ((RegEbx & 0xffff) != 0) {
> >>> + TopologyLeafSupported = TRUE;
> >>> +
> >>> + //
> >>> + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> >>> + parameters
> >> to extract
> >>> + // the SMT sub-field of x2APIC ID.
> >>> + //
> >>> + LevelType = (RegEcx >> 8) & 0xff;
> >>> + ASSERT (LevelType ==
> >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> >>> + if ((RegEbx & 0xffff) > 1 ) {
> >>> + ThreadBits = RegEax & 0x1f;
> >>> + } else {
> >>> + //
> >>> + // HT is not supported
> >>> + //
> >>> + ThreadBits = 0;
> >>> + }
> >>> +
> >>> + //
> >>> + // Software must not assume any "level type" encoding
> >>> + // value to be related to any sub-leaf index, except sub-leaf 0.
> >>> + //
> >>> + SubIndex = 1;
> >>> + do {
> >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> >> NULL, &RegEcx, NULL);
> >>> + LevelType = (RegEcx >> 8) & 0xff;
> >>> + if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> >>> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> >>> + break;
> >>> + }
> >>> + SubIndex++;
> >>> + } while (LevelType !=
> >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> >>> + }
> >>> + }
> >>> +
> >>> + if (!TopologyLeafSupported) {
> >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> >>> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> >>> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> >>> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> >> NULL);
> >>> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> >>> + } else {
> >>> + //
> >>> + // Must be a single-core processor.
> >>> + //
> >>> + MaxCoresPerPackage = 1;
> >>> + }
> >>> +
> >>> + ThreadBits = (UINTN) (HighBitSet32
> >>> + (MaxLogicProcessorsPerPackage /
> >>> MaxCoresPerPackage - 1) + 1);
> >>> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> >>> + }
> >>> +
> >>> + Location->Thread = ApicId & ~((-1) << ThreadBits);
> >>> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> >>> + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> >>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> >>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> >>> index 40f2a17..2bfb1e8 100644
> >>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> >>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> >>> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> >> mSmmCpuService = {
> >>> };
> >>>
> >>> /**
> >>> - Get Package ID/Core ID/Thread ID of a processor.
> >>> -
> >>> - APIC ID must be an initial APIC ID.
> >>> -
> >>> - The algorithm below assumes the target system has symmetry across
> >>> physical package boundaries
> >>> - with respect to the number of logical processors per package,
> >>> number of cores per package.
> >>> -
> >>> - @param ApicId APIC ID of the target logical processor.
> >>> - @param Location Returns the processor location information.
> >>> -**/
> >>> -VOID
> >>> -SmmGetProcessorLocation (
> >>> - IN UINT32 ApicId,
> >>> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> >>> - )
> >>> -{
> >>> - UINTN ThreadBits;
> >>> - UINTN CoreBits;
> >>> - UINT32 RegEax;
> >>> - UINT32 RegEbx;
> >>> - UINT32 RegEcx;
> >>> - UINT32 RegEdx;
> >>> - UINT32 MaxCpuIdIndex;
> >>> - UINT32 SubIndex;
> >>> - UINTN LevelType;
> >>> - UINT32 MaxLogicProcessorsPerPackage;
> >>> - UINT32 MaxCoresPerPackage;
> >>> - BOOLEAN TopologyLeafSupported;
> >>> -
> >>> - ASSERT (Location != NULL);
> >>> -
> >>> - ThreadBits = 0;
> >>> - CoreBits = 0;
> >>> - TopologyLeafSupported = FALSE;
> >>> -
> >>> - //
> >>> - // Check if the processor is capable of supporting more than one
> >>> logical
> >> processor.
> >>> - //
> >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> >>> - ASSERT ((RegEdx & BIT28) != 0);
> >>> -
> >>> - //
> >>> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> >>> - //
> >>> -
> >>> - //
> >>> - // Get the max index of basic CPUID
> >>> - //
> >>> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> >>> -
> >>> - //
> >>> - // If the extended topology enumeration leaf is available, it
> >>> - // is the preferred mechanism for enumerating topology.
> >>> - //
> >>> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> >> &RegEcx, NULL);
> >>> - //
> >>> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input
> >> value for
> >>> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is
> >> not
> >>> - // supported on that processor.
> >>> - //
> >>> - if ((RegEbx & 0xffff) != 0) {
> >>> - TopologyLeafSupported = TRUE;
> >>> -
> >>> - //
> >>> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters
> >> to extract
> >>> - // the SMT sub-field of x2APIC ID.
> >>> - //
> >>> - LevelType = (RegEcx >> 8) & 0xff;
> >>> - ASSERT (LevelType ==
> >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> >>> - if ((RegEbx & 0xffff) > 1 ) {
> >>> - ThreadBits = RegEax & 0x1f;
> >>> - } else {
> >>> - //
> >>> - // HT is not supported
> >>> - //
> >>> - ThreadBits = 0;
> >>> - }
> >>> -
> >>> - //
> >>> - // Software must not assume any "level type" encoding
> >>> - // value to be related to any sub-leaf index, except sub-leaf 0.
> >>> - //
> >>> - SubIndex = 1;
> >>> - do {
> >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> >> NULL, &RegEcx, NULL);
> >>> - LevelType = (RegEcx >> 8) & 0xff;
> >>> - if (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> >>> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> >>> - break;
> >>> - }
> >>> - SubIndex++;
> >>> - } while (LevelType !=
> >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> >>> - }
> >>> - }
> >>> -
> >>> - if (!TopologyLeafSupported) {
> >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> >>> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> >>> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> >>> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL, NULL);
> >>> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> >>> - } else {
> >>> - //
> >>> - // Must be a single-core processor.
> >>> - //
> >>> - MaxCoresPerPackage = 1;
> >>> - }
> >>> -
> >>> - ThreadBits = (UINTN) (HighBitSet32 (MaxLogicProcessorsPerPackage /
> >>> MaxCoresPerPackage - 1) + 1);
> >>> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> >>> - }
> >>> -
> >>> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> >>> - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> >>> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> >>> -
> >>> -/**
> >>> Gets processor information on the requested processor at the
> >>> instant this call is made.
> >>>
> >>> @param[in] This A pointer to the
> >> EFI_SMM_CPU_SERVICE_PROTOCOL
> >>> instance.
> >>> @@ -280,7 +161,7 @@ SmmAddProcessor (
> >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> >> INVALID_APIC_ID) {
> >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId = ProcessorId;
> >>> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> >>> - SmmGetProcessorLocation ((UINT32)ProcessorId, &gSmmCpuPrivate-
> >>>> ProcessorInfo[Index].Location);
> >>> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> >>> + &gSmmCpuPrivate-
> >>>> ProcessorInfo[Index].Location);
> >>>
> >>> *ProcessorNumber = Index;
> >>> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> >>> --
> >>> 1.9.1
> >>>
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 15:29 ` Kinney, Michael D
@ 2016-10-28 15:30 ` Duran, Leo
2016-10-28 15:32 ` Fan, Jeff
0 siblings, 1 reply; 13+ messages in thread
From: Duran, Leo @ 2016-10-28 15:30 UTC (permalink / raw)
To: 'Kinney, Michael D', Laszlo Ersek, Fan, Jeff
Cc: edk2-devel@lists.01.org, Gao, Liming
OK, the LocalApicLib patch is coming... please stay tuned.
Leo.
> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Friday, October 28, 2016 10:29 AM
> To: Laszlo Ersek <lersek@redhat.com>; Fan, Jeff <jeff.fan@intel.com>;
> Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Hi Laszlo,
>
> Quark DSC files do map the LocalApicLib, so I think the proposal to add
> GetProcessorLocation() to the LocalApicLib class and instances makes sense.
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Friday, October 28, 2016 3:29 AM
> > To: Fan, Jeff <jeff.fan@intel.com>; Duran, Leo <leo.duran@amd.com>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: Re: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > On 10/28/16 06:43, Fan, Jeff wrote:
> > > Leo,
> > >
> > > Got it. Could you please create the patch to add
> > > GetProcessorLocation() in Local APIC
> > Lib?
> > >
> > > There are two library instances required to be updated
> > > UefiCpuPkg\Library\BaseXApicLib
> > > UefiCpuPkg\Library\BaseXApicX2ApicLib
> > >
> > > Then you could remove the ExtractProcessorLocation() from
> PiSmmCpuDxeSmm.
> >
> > First of all, thank you Mike for not forgetting about OvmfPkg :)
> >
> > Second, to Leo: if the GetProcessorLocation() API (and its current
> > implementation) are moved to the LocalApicLib class (and the two
> > LocalApicLib instances named by Jeff), then at least OvmfPkg will need
> > no handling. OvmfPkg uses the BaseXApicX2ApicLib instance.
> >
> > However, I think QuarkSocPkg might still require an update, because it
> > doesn't seem to resolve or depend on LocalApicLib at all, at the moment.
> >
> > Thanks
> > Laszlo
> >
> > >
> > > Thanks!
> > > Jeff
> > >
> > > -----Original Message-----
> > > From: Duran, Leo [mailto:leo.duran@amd.com]
> > > Sent: Friday, October 28, 2016 11:20 AM
> > > To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
> > > Cc: Gao, Liming
> > > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > > to
> > SmmCpuFeaturesLib library.
> > >
> > > Please see my reply below.
> > > Thanks,
> > > Leo
> > >
> > >> -----Original Message-----
> > >> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> > >> Sent: Thursday, October 27, 2016 8:00 PM
> > >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> > >> <leo.duran@amd.com>; edk2-devel@lists.01.org
> > >> Cc: Gao, Liming <liming.gao@intel.com>
> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > >> to SmmCpuFeaturesLib library.
> > >>
> > >> Because the CPU location information are gotten from Initial APIC
> > >> ID, it makes more sense to be added into Local APIC Lib.
> > >>
> > >> The following is my proposal on its definition.
> > >> /**
> > >> Get CPU Package/Core/Thread location information.
> > >>
> > >> @param[in] InitialApicId CPU APIC ID
> > >> @param[out] Package Pointer to Package ID
> > >> @param[out] Core Pointer to Core ID
> > >> @param[out] Thread Pointer to Thread ID
> > >> **/
> > >> VOID
> > >> ExtractProcessorLocation (
> > >> IN UINT32 InitialApicId,
> > >> OUT UINT32 *Package,
> > >> OUT UINT32 *Core,
> > >> OUT UINT32 *Thread
> > >> );
> > >>
> > >> Leo, is it OK to meet your requirement?
> > >>
> > >> Thanks!
> > >> Jeff
> > > [Duran, Leo]
> > > Sure thing.
> > > The main point is to get the vendor-specific CPUID code out of the
> > > driver, and into a
> > library.
> > >>
> > >> -----Original Message-----
> > >> From: Fan, Jeff
> > >> Sent: Friday, October 28, 2016 8:45 AM
> > >> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> > >> Cc: Gao, Liming
> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > >> to SmmCpuFeaturesLib library.
> > >>
> > >> Leo and Mike,
> > >>
> > >> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm driver,
> > >> it is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
> > >>
> > >> I suggest that we could add this API into
> > >> UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
> > >> Thus, it could be consumed by modules across PEI/DXE/SMM modules.
> > >>
> > >> Thanks!
> > >> Jeff
> > >>
> > >> -----Original Message-----
> > >> From: Kinney, Michael D
> > >> Sent: Friday, October 28, 2016 7:16 AM
> > >> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> > >> Cc: Gao, Liming; Fan, Jeff
> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > >> to SmmCpuFeaturesLib library.
> > >>
> > >> Leo,
> > >>
> > >> This looks like a good proposed change to the SmmFeaturesLib and
> > >> PiSmmCpuDxeSmm module.
> > >>
> > >> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
> > >>
> > >> There are 3 implementations of the SmmFeaturesLib in edk2/master.
> > >> This patch needs to update all 3, or some of the platforms in
> > >> edk2/master will no longer build:
> > >>
> > >> * OvmfPkg\Library\SmmCpuFeaturesLib
> > >> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> > >> * UefiCpuPkg\Library\SmmCpuFeaturesLib
> > >>
> > >> Thanks,
> > >>
> > >> Mike
> > >>
> > >>> -----Original Message-----
> > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > >>> Behalf Of Leo Duran
> > >>> Sent: Thursday, October 27, 2016 3:30 PM
> > >>> To: edk2-devel@lists.01.org
> > >>> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> > >> <liming.gao@intel.com>
> > >>> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > >>> SmmCpuFeaturesLib library.
> > >>>
> > >>> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm
> driver
> > >>> 2) Add SmmCpuFeaturesGetProcessorLocation() to
> SmmCpuFeaturesLib
> > >>> library
> > >>>
> > >>> Contributed-under: TianoCore Contribution Agreement 1.0
> > >>> Signed-off-by: Leo Duran <leo.duran@amd.com>
> > >>> ---
> > >>> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> > >>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> > >> ++++++++++++++++++++
> > >>> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +-------------
> -----
> > >> --
> > >>> 3 files changed, 136 insertions(+), 120 deletions(-)
> > >>>
> > >>> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> index 4478003..dd14ec5 100644
> > >>> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > >>> IN UINTN Pages
> > >>> );
> > >>>
> > >>> +/**
> > >>> + Get Package ID/Core ID/Thread ID of a processor.
> > >>> +
> > >>> + APIC ID must be an initial APIC ID.
> > >>> +
> > >>> + The algorithm assumes the target system has symmetry across
> > >>> + physical package
> > >>> boundaries
> > >>> + with respect to the number of logical processors per package,
> > >>> + number of cores per
> > >>> package.
> > >>> +
> > >>> + @param ApicId APIC ID of the target logical processor.
> > >>> + @param Location Returns the processor location information.
> > >>> +**/
> > >>> +VOID
> > >>> +SmmCpuFeaturesGetProcessorLocation (
> > >>> + IN UINT32 ApicId,
> > >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > >>> + );
> > >>> +
> > >>> #endif
> > >>> diff --git
> > >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> index 1754f2d..1e300f3 100644
> > >>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory
> (
> > >>> return NULL;
> > >>> }
> > >>>
> > >>> +/**
> > >>> + Get Package ID/Core ID/Thread ID of a processor.
> > >>> +
> > >>> + APIC ID must be an initial APIC ID.
> > >>> +
> > >>> + The algorithm below assumes the target system has symmetry
> > >>> + across physical package
> > >>> boundaries
> > >>> + with respect to the number of logical processors per package,
> > >>> + number of cores per
> > >>> package.
> > >>> +
> > >>> + @param ApicId APIC ID of the target logical processor.
> > >>> + @param Location Returns the processor location information.
> > >>> +**/
> > >>> +VOID
> > >>> +SmmCpuFeaturesGetProcessorLocation (
> > >>> + IN UINT32 ApicId,
> > >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > >>> + )
> > >>> +{
> > >>> + UINTN ThreadBits;
> > >>> + UINTN CoreBits;
> > >>> + UINT32 RegEax;
> > >>> + UINT32 RegEbx;
> > >>> + UINT32 RegEcx;
> > >>> + UINT32 RegEdx;
> > >>> + UINT32 MaxCpuIdIndex;
> > >>> + UINT32 SubIndex;
> > >>> + UINTN LevelType;
> > >>> + UINT32 MaxLogicProcessorsPerPackage;
> > >>> + UINT32 MaxCoresPerPackage;
> > >>> + BOOLEAN TopologyLeafSupported;
> > >>> +
> > >>> + ASSERT (Location != NULL);
> > >>> +
> > >>> + ThreadBits = 0;
> > >>> + CoreBits = 0;
> > >>> + TopologyLeafSupported = FALSE;
> > >>> +
> > >>> + //
> > >>> + // Check if the processor is capable of supporting more than
> > >>> + one logical
> > >> processor.
> > >>> + //
> > >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > >> ASSERT
> > >>> + ((RegEdx & BIT28) != 0);
> > >>> +
> > >>> + //
> > >>> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > >>> + //
> > >>> +
> > >>> + //
> > >>> + // Get the max index of basic CPUID // AsmCpuid
> > >>> + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > >>> +
> > >>> + //
> > >>> + // If the extended topology enumeration leaf is available, it
> > >>> + // is the preferred mechanism for enumerating topology.
> > >>> + //
> > >>> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> > >> &RegEcx, NULL);
> > >>> + //
> > >>> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> > >>> + input
> > >> value for
> > >>> + // basic CPUID information is greater than 0BH, then
> > >>> + CPUID.0BH leaf is
> > >> not
> > >>> + // supported on that processor.
> > >>> + //
> > >>> + if ((RegEbx & 0xffff) != 0) {
> > >>> + TopologyLeafSupported = TRUE;
> > >>> +
> > >>> + //
> > >>> + // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> > >>> + parameters
> > >> to extract
> > >>> + // the SMT sub-field of x2APIC ID.
> > >>> + //
> > >>> + LevelType = (RegEcx >> 8) & 0xff;
> > >>> + ASSERT (LevelType ==
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > >>> + if ((RegEbx & 0xffff) > 1 ) {
> > >>> + ThreadBits = RegEax & 0x1f;
> > >>> + } else {
> > >>> + //
> > >>> + // HT is not supported
> > >>> + //
> > >>> + ThreadBits = 0;
> > >>> + }
> > >>> +
> > >>> + //
> > >>> + // Software must not assume any "level type" encoding
> > >>> + // value to be related to any sub-leaf index, except sub-leaf 0.
> > >>> + //
> > >>> + SubIndex = 1;
> > >>> + do {
> > >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex,
> &RegEax,
> > >> NULL, &RegEcx, NULL);
> > >>> + LevelType = (RegEcx >> 8) & 0xff;
> > >>> + if (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > >>> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> > >>> + break;
> > >>> + }
> > >>> + SubIndex++;
> > >>> + } while (LevelType !=
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + if (!TopologyLeafSupported) {
> > >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > >>> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > >>> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > >>> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> > >> NULL);
> > >>> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> > >>> + } else {
> > >>> + //
> > >>> + // Must be a single-core processor.
> > >>> + //
> > >>> + MaxCoresPerPackage = 1;
> > >>> + }
> > >>> +
> > >>> + ThreadBits = (UINTN) (HighBitSet32
> > >>> + (MaxLogicProcessorsPerPackage /
> > >>> MaxCoresPerPackage - 1) + 1);
> > >>> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) +
> > >>> + 1); }
> > >>> +
> > >>> + Location->Thread = ApicId & ~((-1) << ThreadBits);
> > >>> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > >>> + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> > >>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> index 40f2a17..2bfb1e8 100644
> > >>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> > >> mSmmCpuService = {
> > >>> };
> > >>>
> > >>> /**
> > >>> - Get Package ID/Core ID/Thread ID of a processor.
> > >>> -
> > >>> - APIC ID must be an initial APIC ID.
> > >>> -
> > >>> - The algorithm below assumes the target system has symmetry
> > >>> across physical package boundaries
> > >>> - with respect to the number of logical processors per package,
> > >>> number of cores per package.
> > >>> -
> > >>> - @param ApicId APIC ID of the target logical processor.
> > >>> - @param Location Returns the processor location information.
> > >>> -**/
> > >>> -VOID
> > >>> -SmmGetProcessorLocation (
> > >>> - IN UINT32 ApicId,
> > >>> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > >>> - )
> > >>> -{
> > >>> - UINTN ThreadBits;
> > >>> - UINTN CoreBits;
> > >>> - UINT32 RegEax;
> > >>> - UINT32 RegEbx;
> > >>> - UINT32 RegEcx;
> > >>> - UINT32 RegEdx;
> > >>> - UINT32 MaxCpuIdIndex;
> > >>> - UINT32 SubIndex;
> > >>> - UINTN LevelType;
> > >>> - UINT32 MaxLogicProcessorsPerPackage;
> > >>> - UINT32 MaxCoresPerPackage;
> > >>> - BOOLEAN TopologyLeafSupported;
> > >>> -
> > >>> - ASSERT (Location != NULL);
> > >>> -
> > >>> - ThreadBits = 0;
> > >>> - CoreBits = 0;
> > >>> - TopologyLeafSupported = FALSE;
> > >>> -
> > >>> - //
> > >>> - // Check if the processor is capable of supporting more than
> > >>> one logical
> > >> processor.
> > >>> - //
> > >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > >>> - ASSERT ((RegEdx & BIT28) != 0);
> > >>> -
> > >>> - //
> > >>> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > >>> - //
> > >>> -
> > >>> - //
> > >>> - // Get the max index of basic CPUID
> > >>> - //
> > >>> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL,
> NULL);
> > >>> -
> > >>> - //
> > >>> - // If the extended topology enumeration leaf is available, it
> > >>> - // is the preferred mechanism for enumerating topology.
> > >>> - //
> > >>> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> > >> &RegEcx, NULL);
> > >>> - //
> > >>> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> input
> > >> value for
> > >>> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf
> is
> > >> not
> > >>> - // supported on that processor.
> > >>> - //
> > >>> - if ((RegEbx & 0xffff) != 0) {
> > >>> - TopologyLeafSupported = TRUE;
> > >>> -
> > >>> - //
> > >>> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> parameters
> > >> to extract
> > >>> - // the SMT sub-field of x2APIC ID.
> > >>> - //
> > >>> - LevelType = (RegEcx >> 8) & 0xff;
> > >>> - ASSERT (LevelType ==
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > >>> - if ((RegEbx & 0xffff) > 1 ) {
> > >>> - ThreadBits = RegEax & 0x1f;
> > >>> - } else {
> > >>> - //
> > >>> - // HT is not supported
> > >>> - //
> > >>> - ThreadBits = 0;
> > >>> - }
> > >>> -
> > >>> - //
> > >>> - // Software must not assume any "level type" encoding
> > >>> - // value to be related to any sub-leaf index, except sub-leaf 0.
> > >>> - //
> > >>> - SubIndex = 1;
> > >>> - do {
> > >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> > >> NULL, &RegEcx, NULL);
> > >>> - LevelType = (RegEcx >> 8) & 0xff;
> > >>> - if (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > >>> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> > >>> - break;
> > >>> - }
> > >>> - SubIndex++;
> > >>> - } while (LevelType !=
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > >>> - }
> > >>> - }
> > >>> -
> > >>> - if (!TopologyLeafSupported) {
> > >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > >>> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > >>> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > >>> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> NULL);
> > >>> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> > >>> - } else {
> > >>> - //
> > >>> - // Must be a single-core processor.
> > >>> - //
> > >>> - MaxCoresPerPackage = 1;
> > >>> - }
> > >>> -
> > >>> - ThreadBits = (UINTN) (HighBitSet32
> (MaxLogicProcessorsPerPackage /
> > >>> MaxCoresPerPackage - 1) + 1);
> > >>> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > >>> - }
> > >>> -
> > >>> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> > >>> - Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > >>> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> > >>> -
> > >>> -/**
> > >>> Gets processor information on the requested processor at the
> > >>> instant this call is made.
> > >>>
> > >>> @param[in] This A pointer to the
> > >> EFI_SMM_CPU_SERVICE_PROTOCOL
> > >>> instance.
> > >>> @@ -280,7 +161,7 @@ SmmAddProcessor (
> > >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> > >> INVALID_APIC_ID) {
> > >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId =
> ProcessorId;
> > >>> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> > >>> - SmmGetProcessorLocation ((UINT32)ProcessorId,
> &gSmmCpuPrivate-
> > >>>> ProcessorInfo[Index].Location);
> > >>> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> > >>> + &gSmmCpuPrivate-
> > >>>> ProcessorInfo[Index].Location);
> > >>>
> > >>> *ProcessorNumber = Index;
> > >>> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> > >>> --
> > >>> 1.9.1
> > >>>
> > >>> _______________________________________________
> > >>> edk2-devel mailing list
> > >>> edk2-devel@lists.01.org
> > >>> https://lists.01.org/mailman/listinfo/edk2-devel
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 15:30 ` Duran, Leo
@ 2016-10-28 15:32 ` Fan, Jeff
2016-10-28 15:34 ` Duran, Leo
0 siblings, 1 reply; 13+ messages in thread
From: Fan, Jeff @ 2016-10-28 15:32 UTC (permalink / raw)
To: Duran, Leo, Kinney, Michael D, Laszlo Ersek
Cc: edk2-devel@lists.01.org, Gao, Liming
Leo,
Please try to use the copy of GetProcessorLocation() from MpInitLib implementation.
After GetProcessorLocation() added into Local APIC library, you could remove both of copies from MpInitLib and PiSmmCpuDxeSmm driver.
Thanks!
Jeff
-----Original Message-----
From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Friday, October 28, 2016 11:31 PM
To: Kinney, Michael D; Laszlo Ersek; Fan, Jeff
Cc: edk2-devel@lists.01.org; Gao, Liming
Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
OK, the LocalApicLib patch is coming... please stay tuned.
Leo.
> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Friday, October 28, 2016 10:29 AM
> To: Laszlo Ersek <lersek@redhat.com>; Fan, Jeff <jeff.fan@intel.com>;
> Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Hi Laszlo,
>
> Quark DSC files do map the LocalApicLib, so I think the proposal to
> add
> GetProcessorLocation() to the LocalApicLib class and instances makes sense.
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Friday, October 28, 2016 3:29 AM
> > To: Fan, Jeff <jeff.fan@intel.com>; Duran, Leo <leo.duran@amd.com>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: Re: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > to SmmCpuFeaturesLib library.
> >
> > On 10/28/16 06:43, Fan, Jeff wrote:
> > > Leo,
> > >
> > > Got it. Could you please create the patch to add
> > > GetProcessorLocation() in Local APIC
> > Lib?
> > >
> > > There are two library instances required to be updated
> > > UefiCpuPkg\Library\BaseXApicLib
> > > UefiCpuPkg\Library\BaseXApicX2ApicLib
> > >
> > > Then you could remove the ExtractProcessorLocation() from
> PiSmmCpuDxeSmm.
> >
> > First of all, thank you Mike for not forgetting about OvmfPkg :)
> >
> > Second, to Leo: if the GetProcessorLocation() API (and its current
> > implementation) are moved to the LocalApicLib class (and the two
> > LocalApicLib instances named by Jeff), then at least OvmfPkg will
> > need no handling. OvmfPkg uses the BaseXApicX2ApicLib instance.
> >
> > However, I think QuarkSocPkg might still require an update, because
> > it doesn't seem to resolve or depend on LocalApicLib at all, at the moment.
> >
> > Thanks
> > Laszlo
> >
> > >
> > > Thanks!
> > > Jeff
> > >
> > > -----Original Message-----
> > > From: Duran, Leo [mailto:leo.duran@amd.com]
> > > Sent: Friday, October 28, 2016 11:20 AM
> > > To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
> > > Cc: Gao, Liming
> > > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > > GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> > >
> > > Please see my reply below.
> > > Thanks,
> > > Leo
> > >
> > >> -----Original Message-----
> > >> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> > >> Sent: Thursday, October 27, 2016 8:00 PM
> > >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> > >> <leo.duran@amd.com>; edk2-devel@lists.01.org
> > >> Cc: Gao, Liming <liming.gao@intel.com>
> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > >> GetProcessorLocation() to SmmCpuFeaturesLib library.
> > >>
> > >> Because the CPU location information are gotten from Initial APIC
> > >> ID, it makes more sense to be added into Local APIC Lib.
> > >>
> > >> The following is my proposal on its definition.
> > >> /**
> > >> Get CPU Package/Core/Thread location information.
> > >>
> > >> @param[in] InitialApicId CPU APIC ID
> > >> @param[out] Package Pointer to Package ID
> > >> @param[out] Core Pointer to Core ID
> > >> @param[out] Thread Pointer to Thread ID
> > >> **/
> > >> VOID
> > >> ExtractProcessorLocation (
> > >> IN UINT32 InitialApicId,
> > >> OUT UINT32 *Package,
> > >> OUT UINT32 *Core,
> > >> OUT UINT32 *Thread
> > >> );
> > >>
> > >> Leo, is it OK to meet your requirement?
> > >>
> > >> Thanks!
> > >> Jeff
> > > [Duran, Leo]
> > > Sure thing.
> > > The main point is to get the vendor-specific CPUID code out of the
> > > driver, and into a
> > library.
> > >>
> > >> -----Original Message-----
> > >> From: Fan, Jeff
> > >> Sent: Friday, October 28, 2016 8:45 AM
> > >> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> > >> Cc: Gao, Liming
> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > >> GetProcessorLocation() to SmmCpuFeaturesLib library.
> > >>
> > >> Leo and Mike,
> > >>
> > >> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm
> > >> driver, it is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
> > >>
> > >> I suggest that we could add this API into
> > >> UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/ LocalApicLib.h.
> > >> Thus, it could be consumed by modules across PEI/DXE/SMM modules.
> > >>
> > >> Thanks!
> > >> Jeff
> > >>
> > >> -----Original Message-----
> > >> From: Kinney, Michael D
> > >> Sent: Friday, October 28, 2016 7:16 AM
> > >> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> > >> Cc: Gao, Liming; Fan, Jeff
> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > >> GetProcessorLocation() to SmmCpuFeaturesLib library.
> > >>
> > >> Leo,
> > >>
> > >> This looks like a good proposed change to the SmmFeaturesLib and
> > >> PiSmmCpuDxeSmm module.
> > >>
> > >> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
> > >>
> > >> There are 3 implementations of the SmmFeaturesLib in edk2/master.
> > >> This patch needs to update all 3, or some of the platforms in
> > >> edk2/master will no longer build:
> > >>
> > >> * OvmfPkg\Library\SmmCpuFeaturesLib
> > >> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> > >> * UefiCpuPkg\Library\SmmCpuFeaturesLib
> > >>
> > >> Thanks,
> > >>
> > >> Mike
> > >>
> > >>> -----Original Message-----
> > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > >>> Behalf Of Leo Duran
> > >>> Sent: Thursday, October 27, 2016 3:30 PM
> > >>> To: edk2-devel@lists.01.org
> > >>> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> > >> <liming.gao@intel.com>
> > >>> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > >>> to SmmCpuFeaturesLib library.
> > >>>
> > >>> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm
> driver
> > >>> 2) Add SmmCpuFeaturesGetProcessorLocation() to
> SmmCpuFeaturesLib
> > >>> library
> > >>>
> > >>> Contributed-under: TianoCore Contribution Agreement 1.0
> > >>> Signed-off-by: Leo Duran <leo.duran@amd.com>
> > >>> ---
> > >>> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> > >>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> > >> ++++++++++++++++++++
> > >>> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +-------------
> -----
> > >> --
> > >>> 3 files changed, 136 insertions(+), 120 deletions(-)
> > >>>
> > >>> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> index 4478003..dd14ec5 100644
> > >>> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > >>> @@ -398,4 +398,21 @@ SmmCpuFeaturesAllocatePageTableMemory (
> > >>> IN UINTN Pages
> > >>> );
> > >>>
> > >>> +/**
> > >>> + Get Package ID/Core ID/Thread ID of a processor.
> > >>> +
> > >>> + APIC ID must be an initial APIC ID.
> > >>> +
> > >>> + The algorithm assumes the target system has symmetry across
> > >>> + physical package
> > >>> boundaries
> > >>> + with respect to the number of logical processors per package,
> > >>> + number of cores per
> > >>> package.
> > >>> +
> > >>> + @param ApicId APIC ID of the target logical processor.
> > >>> + @param Location Returns the processor location information.
> > >>> +**/
> > >>> +VOID
> > >>> +SmmCpuFeaturesGetProcessorLocation (
> > >>> + IN UINT32 ApicId,
> > >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > >>> + );
> > >>> +
> > >>> #endif
> > >>> diff --git
> > >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> index 1754f2d..1e300f3 100644
> > >>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > >>> @@ -673,3 +673,121 @@ SmmCpuFeaturesAllocatePageTableMemory
> (
> > >>> return NULL;
> > >>> }
> > >>>
> > >>> +/**
> > >>> + Get Package ID/Core ID/Thread ID of a processor.
> > >>> +
> > >>> + APIC ID must be an initial APIC ID.
> > >>> +
> > >>> + The algorithm below assumes the target system has symmetry
> > >>> + across physical package
> > >>> boundaries
> > >>> + with respect to the number of logical processors per package,
> > >>> + number of cores per
> > >>> package.
> > >>> +
> > >>> + @param ApicId APIC ID of the target logical processor.
> > >>> + @param Location Returns the processor location information.
> > >>> +**/
> > >>> +VOID
> > >>> +SmmCpuFeaturesGetProcessorLocation (
> > >>> + IN UINT32 ApicId,
> > >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > >>> + )
> > >>> +{
> > >>> + UINTN ThreadBits;
> > >>> + UINTN CoreBits;
> > >>> + UINT32 RegEax;
> > >>> + UINT32 RegEbx;
> > >>> + UINT32 RegEcx;
> > >>> + UINT32 RegEdx;
> > >>> + UINT32 MaxCpuIdIndex;
> > >>> + UINT32 SubIndex;
> > >>> + UINTN LevelType;
> > >>> + UINT32 MaxLogicProcessorsPerPackage;
> > >>> + UINT32 MaxCoresPerPackage;
> > >>> + BOOLEAN TopologyLeafSupported;
> > >>> +
> > >>> + ASSERT (Location != NULL);
> > >>> +
> > >>> + ThreadBits = 0;
> > >>> + CoreBits = 0;
> > >>> + TopologyLeafSupported = FALSE;
> > >>> +
> > >>> + //
> > >>> + // Check if the processor is capable of supporting more than
> > >>> + one logical
> > >> processor.
> > >>> + //
> > >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > >> ASSERT
> > >>> + ((RegEdx & BIT28) != 0);
> > >>> +
> > >>> + //
> > >>> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > >>> + //
> > >>> +
> > >>> + //
> > >>> + // Get the max index of basic CPUID // AsmCpuid
> > >>> + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > >>> +
> > >>> + //
> > >>> + // If the extended topology enumeration leaf is available, it
> > >>> + // is the preferred mechanism for enumerating topology.
> > >>> + //
> > >>> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> > >> &RegEcx, NULL);
> > >>> + //
> > >>> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> > >>> + input
> > >> value for
> > >>> + // basic CPUID information is greater than 0BH, then
> > >>> + CPUID.0BH leaf is
> > >> not
> > >>> + // supported on that processor.
> > >>> + //
> > >>> + if ((RegEbx & 0xffff) != 0) {
> > >>> + TopologyLeafSupported = TRUE;
> > >>> +
> > >>> + //
> > >>> + // Sub-leaf index 0 (ECX= 0 as input) provides
> > >>> + enumeration parameters
> > >> to extract
> > >>> + // the SMT sub-field of x2APIC ID.
> > >>> + //
> > >>> + LevelType = (RegEcx >> 8) & 0xff;
> > >>> + ASSERT (LevelType ==
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > >>> + if ((RegEbx & 0xffff) > 1 ) {
> > >>> + ThreadBits = RegEax & 0x1f;
> > >>> + } else {
> > >>> + //
> > >>> + // HT is not supported
> > >>> + //
> > >>> + ThreadBits = 0;
> > >>> + }
> > >>> +
> > >>> + //
> > >>> + // Software must not assume any "level type" encoding
> > >>> + // value to be related to any sub-leaf index, except sub-leaf 0.
> > >>> + //
> > >>> + SubIndex = 1;
> > >>> + do {
> > >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex,
> &RegEax,
> > >> NULL, &RegEcx, NULL);
> > >>> + LevelType = (RegEcx >> 8) & 0xff;
> > >>> + if (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > >>> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> > >>> + break;
> > >>> + }
> > >>> + SubIndex++;
> > >>> + } while (LevelType !=
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + if (!TopologyLeafSupported) {
> > >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > >>> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > >>> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > >>> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> > >> NULL);
> > >>> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> > >>> + } else {
> > >>> + //
> > >>> + // Must be a single-core processor.
> > >>> + //
> > >>> + MaxCoresPerPackage = 1;
> > >>> + }
> > >>> +
> > >>> + ThreadBits = (UINTN) (HighBitSet32
> > >>> + (MaxLogicProcessorsPerPackage /
> > >>> MaxCoresPerPackage - 1) + 1);
> > >>> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) +
> > >>> + 1); }
> > >>> +
> > >>> + Location->Thread = ApicId & ~((-1) << ThreadBits);
> > >>> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > >>> + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> > >>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> index 40f2a17..2bfb1e8 100644
> > >>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > >>> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> > >> mSmmCpuService = {
> > >>> };
> > >>>
> > >>> /**
> > >>> - Get Package ID/Core ID/Thread ID of a processor.
> > >>> -
> > >>> - APIC ID must be an initial APIC ID.
> > >>> -
> > >>> - The algorithm below assumes the target system has symmetry
> > >>> across physical package boundaries
> > >>> - with respect to the number of logical processors per package,
> > >>> number of cores per package.
> > >>> -
> > >>> - @param ApicId APIC ID of the target logical processor.
> > >>> - @param Location Returns the processor location information.
> > >>> -**/
> > >>> -VOID
> > >>> -SmmGetProcessorLocation (
> > >>> - IN UINT32 ApicId,
> > >>> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > >>> - )
> > >>> -{
> > >>> - UINTN ThreadBits;
> > >>> - UINTN CoreBits;
> > >>> - UINT32 RegEax;
> > >>> - UINT32 RegEbx;
> > >>> - UINT32 RegEcx;
> > >>> - UINT32 RegEdx;
> > >>> - UINT32 MaxCpuIdIndex;
> > >>> - UINT32 SubIndex;
> > >>> - UINTN LevelType;
> > >>> - UINT32 MaxLogicProcessorsPerPackage;
> > >>> - UINT32 MaxCoresPerPackage;
> > >>> - BOOLEAN TopologyLeafSupported;
> > >>> -
> > >>> - ASSERT (Location != NULL);
> > >>> -
> > >>> - ThreadBits = 0;
> > >>> - CoreBits = 0;
> > >>> - TopologyLeafSupported = FALSE;
> > >>> -
> > >>> - //
> > >>> - // Check if the processor is capable of supporting more than
> > >>> one logical
> > >> processor.
> > >>> - //
> > >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > >>> - ASSERT ((RegEdx & BIT28) != 0);
> > >>> -
> > >>> - //
> > >>> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > >>> - //
> > >>> -
> > >>> - //
> > >>> - // Get the max index of basic CPUID
> > >>> - //
> > >>> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL,
> NULL);
> > >>> -
> > >>> - //
> > >>> - // If the extended topology enumeration leaf is available, it
> > >>> - // is the preferred mechanism for enumerating topology.
> > >>> - //
> > >>> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax, &RegEbx,
> > >> &RegEcx, NULL);
> > >>> - //
> > >>> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> input
> > >> value for
> > >>> - // basic CPUID information is greater than 0BH, then CPUID.0BH leaf
> is
> > >> not
> > >>> - // supported on that processor.
> > >>> - //
> > >>> - if ((RegEbx & 0xffff) != 0) {
> > >>> - TopologyLeafSupported = TRUE;
> > >>> -
> > >>> - //
> > >>> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> parameters
> > >> to extract
> > >>> - // the SMT sub-field of x2APIC ID.
> > >>> - //
> > >>> - LevelType = (RegEcx >> 8) & 0xff;
> > >>> - ASSERT (LevelType ==
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > >>> - if ((RegEbx & 0xffff) > 1 ) {
> > >>> - ThreadBits = RegEax & 0x1f;
> > >>> - } else {
> > >>> - //
> > >>> - // HT is not supported
> > >>> - //
> > >>> - ThreadBits = 0;
> > >>> - }
> > >>> -
> > >>> - //
> > >>> - // Software must not assume any "level type" encoding
> > >>> - // value to be related to any sub-leaf index, except sub-leaf 0.
> > >>> - //
> > >>> - SubIndex = 1;
> > >>> - do {
> > >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex, &RegEax,
> > >> NULL, &RegEcx, NULL);
> > >>> - LevelType = (RegEcx >> 8) & 0xff;
> > >>> - if (LevelType ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > >>> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> > >>> - break;
> > >>> - }
> > >>> - SubIndex++;
> > >>> - } while (LevelType !=
> > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > >>> - }
> > >>> - }
> > >>> -
> > >>> - if (!TopologyLeafSupported) {
> > >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > >>> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > >>> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > >>> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> NULL);
> > >>> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> > >>> - } else {
> > >>> - //
> > >>> - // Must be a single-core processor.
> > >>> - //
> > >>> - MaxCoresPerPackage = 1;
> > >>> - }
> > >>> -
> > >>> - ThreadBits = (UINTN) (HighBitSet32
> (MaxLogicProcessorsPerPackage /
> > >>> MaxCoresPerPackage - 1) + 1);
> > >>> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > >>> - }
> > >>> -
> > >>> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> > >>> - Location->Core = (ApicId >> ThreadBits) & ~((-1) <<
> > >>> CoreBits);
> > >>> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> > >>> -
> > >>> -/**
> > >>> Gets processor information on the requested processor at the
> > >>> instant this call is made.
> > >>>
> > >>> @param[in] This A pointer to the
> > >> EFI_SMM_CPU_SERVICE_PROTOCOL
> > >>> instance.
> > >>> @@ -280,7 +161,7 @@ SmmAddProcessor (
> > >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> > >> INVALID_APIC_ID) {
> > >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId =
> ProcessorId;
> > >>> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> > >>> - SmmGetProcessorLocation ((UINT32)ProcessorId,
> &gSmmCpuPrivate-
> > >>>> ProcessorInfo[Index].Location);
> > >>> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> > >>> + &gSmmCpuPrivate-
> > >>>> ProcessorInfo[Index].Location);
> > >>>
> > >>> *ProcessorNumber = Index;
> > >>> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> > >>> --
> > >>> 1.9.1
> > >>>
> > >>> _______________________________________________
> > >>> edk2-devel mailing list
> > >>> edk2-devel@lists.01.org
> > >>> https://lists.01.org/mailman/listinfo/edk2-devel
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.
2016-10-28 15:32 ` Fan, Jeff
@ 2016-10-28 15:34 ` Duran, Leo
0 siblings, 0 replies; 13+ messages in thread
From: Duran, Leo @ 2016-10-28 15:34 UTC (permalink / raw)
To: 'Fan, Jeff', Kinney, Michael D, Laszlo Ersek
Cc: edk2-devel@lists.01.org, Gao, Liming
Jeff,
That's exactly what I'm doing. Thanks.
Leo
> -----Original Message-----
> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> Sent: Friday, October 28, 2016 10:33 AM
> To: Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> Leo,
>
> Please try to use the copy of GetProcessorLocation() from MpInitLib
> implementation.
>
> After GetProcessorLocation() added into Local APIC library, you could
> remove both of copies from MpInitLib and PiSmmCpuDxeSmm driver.
>
> Thanks!
> Jeff
>
> -----Original Message-----
> From: Duran, Leo [mailto:leo.duran@amd.com]
> Sent: Friday, October 28, 2016 11:31 PM
> To: Kinney, Michael D; Laszlo Ersek; Fan, Jeff
> Cc: edk2-devel@lists.01.org; Gao, Liming
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
>
> OK, the LocalApicLib patch is coming... please stay tuned.
> Leo.
>
> > -----Original Message-----
> > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > Sent: Friday, October 28, 2016 10:29 AM
> > To: Laszlo Ersek <lersek@redhat.com>; Fan, Jeff <jeff.fan@intel.com>;
> > Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> > SmmCpuFeaturesLib library.
> >
> > Hi Laszlo,
> >
> > Quark DSC files do map the LocalApicLib, so I think the proposal to
> > add
> > GetProcessorLocation() to the LocalApicLib class and instances makes
> sense.
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > Sent: Friday, October 28, 2016 3:29 AM
> > > To: Fan, Jeff <jeff.fan@intel.com>; Duran, Leo <leo.duran@amd.com>;
> > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> > > <liming.gao@intel.com>
> > > Subject: Re: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > > to SmmCpuFeaturesLib library.
> > >
> > > On 10/28/16 06:43, Fan, Jeff wrote:
> > > > Leo,
> > > >
> > > > Got it. Could you please create the patch to add
> > > > GetProcessorLocation() in Local APIC
> > > Lib?
> > > >
> > > > There are two library instances required to be updated
> > > > UefiCpuPkg\Library\BaseXApicLib
> > > > UefiCpuPkg\Library\BaseXApicX2ApicLib
> > > >
> > > > Then you could remove the ExtractProcessorLocation() from
> > PiSmmCpuDxeSmm.
> > >
> > > First of all, thank you Mike for not forgetting about OvmfPkg :)
> > >
> > > Second, to Leo: if the GetProcessorLocation() API (and its current
> > > implementation) are moved to the LocalApicLib class (and the two
> > > LocalApicLib instances named by Jeff), then at least OvmfPkg will
> > > need no handling. OvmfPkg uses the BaseXApicX2ApicLib instance.
> > >
> > > However, I think QuarkSocPkg might still require an update, because
> > > it doesn't seem to resolve or depend on LocalApicLib at all, at the
> moment.
> > >
> > > Thanks
> > > Laszlo
> > >
> > > >
> > > > Thanks!
> > > > Jeff
> > > >
> > > > -----Original Message-----
> > > > From: Duran, Leo [mailto:leo.duran@amd.com]
> > > > Sent: Friday, October 28, 2016 11:20 AM
> > > > To: Fan, Jeff; Kinney, Michael D; edk2-devel@lists.01.org
> > > > Cc: Gao, Liming
> > > > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > > > GetProcessorLocation() to
> > > SmmCpuFeaturesLib library.
> > > >
> > > > Please see my reply below.
> > > > Thanks,
> > > > Leo
> > > >
> > > >> -----Original Message-----
> > > >> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> > > >> Sent: Thursday, October 27, 2016 8:00 PM
> > > >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Duran, Leo
> > > >> <leo.duran@amd.com>; edk2-devel@lists.01.org
> > > >> Cc: Gao, Liming <liming.gao@intel.com>
> > > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > > >> GetProcessorLocation() to SmmCpuFeaturesLib library.
> > > >>
> > > >> Because the CPU location information are gotten from Initial APIC
> > > >> ID, it makes more sense to be added into Local APIC Lib.
> > > >>
> > > >> The following is my proposal on its definition.
> > > >> /**
> > > >> Get CPU Package/Core/Thread location information.
> > > >>
> > > >> @param[in] InitialApicId CPU APIC ID
> > > >> @param[out] Package Pointer to Package ID
> > > >> @param[out] Core Pointer to Core ID
> > > >> @param[out] Thread Pointer to Thread ID
> > > >> **/
> > > >> VOID
> > > >> ExtractProcessorLocation (
> > > >> IN UINT32 InitialApicId,
> > > >> OUT UINT32 *Package,
> > > >> OUT UINT32 *Core,
> > > >> OUT UINT32 *Thread
> > > >> );
> > > >>
> > > >> Leo, is it OK to meet your requirement?
> > > >>
> > > >> Thanks!
> > > >> Jeff
> > > > [Duran, Leo]
> > > > Sure thing.
> > > > The main point is to get the vendor-specific CPUID code out of the
> > > > driver, and into a
> > > library.
> > > >>
> > > >> -----Original Message-----
> > > >> From: Fan, Jeff
> > > >> Sent: Friday, October 28, 2016 8:45 AM
> > > >> To: Kinney, Michael D; Leo Duran; edk2-devel@lists.01.org
> > > >> Cc: Gao, Liming
> > > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > > >> GetProcessorLocation() to SmmCpuFeaturesLib library.
> > > >>
> > > >> Leo and Mike,
> > > >>
> > > >> GetProcessorLocation() are not only used by PiSmmCpuDxeSmm
> > > >> driver, it is also duplicated in UefiCpuPkg\Library\MpInitLib\MpLib.c.
> > > >>
> > > >> I suggest that we could add this API into
> > > >> UefiCpuPkg/Include/UefiCpuLib or UefiCpuPkg/Include/
> LocalApicLib.h.
> > > >> Thus, it could be consumed by modules across PEI/DXE/SMM
> modules.
> > > >>
> > > >> Thanks!
> > > >> Jeff
> > > >>
> > > >> -----Original Message-----
> > > >> From: Kinney, Michael D
> > > >> Sent: Friday, October 28, 2016 7:16 AM
> > > >> To: Leo Duran; edk2-devel@lists.01.org; Kinney, Michael D
> > > >> Cc: Gao, Liming; Fan, Jeff
> > > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move
> > > >> GetProcessorLocation() to SmmCpuFeaturesLib library.
> > > >>
> > > >> Leo,
> > > >>
> > > >> This looks like a good proposed change to the SmmFeaturesLib and
> > > >> PiSmmCpuDxeSmm module.
> > > >>
> > > >> Adding UefiCpuPkg maintainer Jeff Fan to the Cc list.
> > > >>
> > > >> There are 3 implementations of the SmmFeaturesLib in edk2/master.
> > > >> This patch needs to update all 3, or some of the platforms in
> > > >> edk2/master will no longer build:
> > > >>
> > > >> * OvmfPkg\Library\SmmCpuFeaturesLib
> > > >> * QuarkSocPkg\QuarkNorthCluster\Library\SmmCpuFeaturesLib
> > > >> * UefiCpuPkg\Library\SmmCpuFeaturesLib
> > > >>
> > > >> Thanks,
> > > >>
> > > >> Mike
> > > >>
> > > >>> -----Original Message-----
> > > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > > >>> Behalf Of Leo Duran
> > > >>> Sent: Thursday, October 27, 2016 3:30 PM
> > > >>> To: edk2-devel@lists.01.org
> > > >>> Cc: Leo Duran <leo.duran@amd.com>; Gao, Liming
> > > >> <liming.gao@intel.com>
> > > >>> Subject: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation()
> > > >>> to SmmCpuFeaturesLib library.
> > > >>>
> > > >>> 1) Remove SmmGetProcessorLocation() from PiSmmCpuDxeSmm
> > driver
> > > >>> 2) Add SmmCpuFeaturesGetProcessorLocation() to
> > SmmCpuFeaturesLib
> > > >>> library
> > > >>>
> > > >>> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >>> Signed-off-by: Leo Duran <leo.duran@amd.com>
> > > >>> ---
> > > >>> UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 17 +++
> > > >>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 118
> > > >> ++++++++++++++++++++
> > > >>> UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 121 +----------
> ---
> > -----
> > > >> --
> > > >>> 3 files changed, 136 insertions(+), 120 deletions(-)
> > > >>>
> > > >>> diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > >>> b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > >>> index 4478003..dd14ec5 100644
> > > >>> --- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > >>> +++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
> > > >>> @@ -398,4 +398,21 @@
> SmmCpuFeaturesAllocatePageTableMemory (
> > > >>> IN UINTN Pages
> > > >>> );
> > > >>>
> > > >>> +/**
> > > >>> + Get Package ID/Core ID/Thread ID of a processor.
> > > >>> +
> > > >>> + APIC ID must be an initial APIC ID.
> > > >>> +
> > > >>> + The algorithm assumes the target system has symmetry across
> > > >>> + physical package
> > > >>> boundaries
> > > >>> + with respect to the number of logical processors per package,
> > > >>> + number of cores per
> > > >>> package.
> > > >>> +
> > > >>> + @param ApicId APIC ID of the target logical processor.
> > > >>> + @param Location Returns the processor location information.
> > > >>> +**/
> > > >>> +VOID
> > > >>> +SmmCpuFeaturesGetProcessorLocation (
> > > >>> + IN UINT32 ApicId,
> > > >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > > >>> + );
> > > >>> +
> > > >>> #endif
> > > >>> diff --git
> > > >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > >>> index 1754f2d..1e300f3 100644
> > > >>> ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > >>> +++
> > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> > > >>> @@ -673,3 +673,121 @@
> SmmCpuFeaturesAllocatePageTableMemory
> > (
> > > >>> return NULL;
> > > >>> }
> > > >>>
> > > >>> +/**
> > > >>> + Get Package ID/Core ID/Thread ID of a processor.
> > > >>> +
> > > >>> + APIC ID must be an initial APIC ID.
> > > >>> +
> > > >>> + The algorithm below assumes the target system has symmetry
> > > >>> + across physical package
> > > >>> boundaries
> > > >>> + with respect to the number of logical processors per package,
> > > >>> + number of cores per
> > > >>> package.
> > > >>> +
> > > >>> + @param ApicId APIC ID of the target logical processor.
> > > >>> + @param Location Returns the processor location information.
> > > >>> +**/
> > > >>> +VOID
> > > >>> +SmmCpuFeaturesGetProcessorLocation (
> > > >>> + IN UINT32 ApicId,
> > > >>> + OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > > >>> + )
> > > >>> +{
> > > >>> + UINTN ThreadBits;
> > > >>> + UINTN CoreBits;
> > > >>> + UINT32 RegEax;
> > > >>> + UINT32 RegEbx;
> > > >>> + UINT32 RegEcx;
> > > >>> + UINT32 RegEdx;
> > > >>> + UINT32 MaxCpuIdIndex;
> > > >>> + UINT32 SubIndex;
> > > >>> + UINTN LevelType;
> > > >>> + UINT32 MaxLogicProcessorsPerPackage;
> > > >>> + UINT32 MaxCoresPerPackage;
> > > >>> + BOOLEAN TopologyLeafSupported;
> > > >>> +
> > > >>> + ASSERT (Location != NULL);
> > > >>> +
> > > >>> + ThreadBits = 0;
> > > >>> + CoreBits = 0;
> > > >>> + TopologyLeafSupported = FALSE;
> > > >>> +
> > > >>> + //
> > > >>> + // Check if the processor is capable of supporting more than
> > > >>> + one logical
> > > >> processor.
> > > >>> + //
> > > >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > > >> ASSERT
> > > >>> + ((RegEdx & BIT28) != 0);
> > > >>> +
> > > >>> + //
> > > >>> + // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > > >>> + //
> > > >>> +
> > > >>> + //
> > > >>> + // Get the max index of basic CPUID // AsmCpuid
> > > >>> + (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
> > > >>> +
> > > >>> + //
> > > >>> + // If the extended topology enumeration leaf is available, it
> > > >>> + // is the preferred mechanism for enumerating topology.
> > > >>> + //
> > > >>> + if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > > >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax,
> &RegEbx,
> > > >> &RegEcx, NULL);
> > > >>> + //
> > > >>> + // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> > > >>> + input
> > > >> value for
> > > >>> + // basic CPUID information is greater than 0BH, then
> > > >>> + CPUID.0BH leaf is
> > > >> not
> > > >>> + // supported on that processor.
> > > >>> + //
> > > >>> + if ((RegEbx & 0xffff) != 0) {
> > > >>> + TopologyLeafSupported = TRUE;
> > > >>> +
> > > >>> + //
> > > >>> + // Sub-leaf index 0 (ECX= 0 as input) provides
> > > >>> + enumeration parameters
> > > >> to extract
> > > >>> + // the SMT sub-field of x2APIC ID.
> > > >>> + //
> > > >>> + LevelType = (RegEcx >> 8) & 0xff;
> > > >>> + ASSERT (LevelType ==
> > > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > > >>> + if ((RegEbx & 0xffff) > 1 ) {
> > > >>> + ThreadBits = RegEax & 0x1f;
> > > >>> + } else {
> > > >>> + //
> > > >>> + // HT is not supported
> > > >>> + //
> > > >>> + ThreadBits = 0;
> > > >>> + }
> > > >>> +
> > > >>> + //
> > > >>> + // Software must not assume any "level type" encoding
> > > >>> + // value to be related to any sub-leaf index, except sub-leaf 0.
> > > >>> + //
> > > >>> + SubIndex = 1;
> > > >>> + do {
> > > >>> + AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex,
> > &RegEax,
> > > >> NULL, &RegEcx, NULL);
> > > >>> + LevelType = (RegEcx >> 8) & 0xff;
> > > >>> + if (LevelType ==
> > CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > > >>> + CoreBits = (RegEax & 0x1f) - ThreadBits;
> > > >>> + break;
> > > >>> + }
> > > >>> + SubIndex++;
> > > >>> + } while (LevelType !=
> > > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > > >>> + }
> > > >>> + }
> > > >>> +
> > > >>> + if (!TopologyLeafSupported) {
> > > >>> + AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > > >>> + MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > > >>> + if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > > >>> + AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> > > >> NULL);
> > > >>> + MaxCoresPerPackage = (RegEax >> 26) + 1;
> > > >>> + } else {
> > > >>> + //
> > > >>> + // Must be a single-core processor.
> > > >>> + //
> > > >>> + MaxCoresPerPackage = 1;
> > > >>> + }
> > > >>> +
> > > >>> + ThreadBits = (UINTN) (HighBitSet32
> > > >>> + (MaxLogicProcessorsPerPackage /
> > > >>> MaxCoresPerPackage - 1) + 1);
> > > >>> + CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) +
> > > >>> + 1); }
> > > >>> +
> > > >>> + Location->Thread = ApicId & ~((-1) << ThreadBits);
> > > >>> + Location->Core = (ApicId >> ThreadBits) & ~((-1) << CoreBits);
> > > >>> + Location->Package = (ApicId >> (ThreadBits+ CoreBits)); }
> > > >>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > >>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > >>> index 40f2a17..2bfb1e8 100644
> > > >>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > >>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > > >>> @@ -27,125 +27,6 @@ EFI_SMM_CPU_SERVICE_PROTOCOL
> > > >> mSmmCpuService = {
> > > >>> };
> > > >>>
> > > >>> /**
> > > >>> - Get Package ID/Core ID/Thread ID of a processor.
> > > >>> -
> > > >>> - APIC ID must be an initial APIC ID.
> > > >>> -
> > > >>> - The algorithm below assumes the target system has symmetry
> > > >>> across physical package boundaries
> > > >>> - with respect to the number of logical processors per package,
> > > >>> number of cores per package.
> > > >>> -
> > > >>> - @param ApicId APIC ID of the target logical processor.
> > > >>> - @param Location Returns the processor location information.
> > > >>> -**/
> > > >>> -VOID
> > > >>> -SmmGetProcessorLocation (
> > > >>> - IN UINT32 ApicId,
> > > >>> - OUT EFI_CPU_PHYSICAL_LOCATION *Location
> > > >>> - )
> > > >>> -{
> > > >>> - UINTN ThreadBits;
> > > >>> - UINTN CoreBits;
> > > >>> - UINT32 RegEax;
> > > >>> - UINT32 RegEbx;
> > > >>> - UINT32 RegEcx;
> > > >>> - UINT32 RegEdx;
> > > >>> - UINT32 MaxCpuIdIndex;
> > > >>> - UINT32 SubIndex;
> > > >>> - UINTN LevelType;
> > > >>> - UINT32 MaxLogicProcessorsPerPackage;
> > > >>> - UINT32 MaxCoresPerPackage;
> > > >>> - BOOLEAN TopologyLeafSupported;
> > > >>> -
> > > >>> - ASSERT (Location != NULL);
> > > >>> -
> > > >>> - ThreadBits = 0;
> > > >>> - CoreBits = 0;
> > > >>> - TopologyLeafSupported = FALSE;
> > > >>> -
> > > >>> - //
> > > >>> - // Check if the processor is capable of supporting more than
> > > >>> one logical
> > > >> processor.
> > > >>> - //
> > > >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
> > > >>> - ASSERT ((RegEdx & BIT28) != 0);
> > > >>> -
> > > >>> - //
> > > >>> - // Assume three-level mapping of APIC ID: Package:Core:SMT.
> > > >>> - //
> > > >>> -
> > > >>> - //
> > > >>> - // Get the max index of basic CPUID
> > > >>> - //
> > > >>> - AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL,
> > NULL);
> > > >>> -
> > > >>> - //
> > > >>> - // If the extended topology enumeration leaf is available, it
> > > >>> - // is the preferred mechanism for enumerating topology.
> > > >>> - //
> > > >>> - if (MaxCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> > > >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 0, &RegEax,
> &RegEbx,
> > > >> &RegEcx, NULL);
> > > >>> - //
> > > >>> - // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum
> > input
> > > >> value for
> > > >>> - // basic CPUID information is greater than 0BH, then CPUID.0BH
> leaf
> > is
> > > >> not
> > > >>> - // supported on that processor.
> > > >>> - //
> > > >>> - if ((RegEbx & 0xffff) != 0) {
> > > >>> - TopologyLeafSupported = TRUE;
> > > >>> -
> > > >>> - //
> > > >>> - // Sub-leaf index 0 (ECX= 0 as input) provides enumeration
> > parameters
> > > >> to extract
> > > >>> - // the SMT sub-field of x2APIC ID.
> > > >>> - //
> > > >>> - LevelType = (RegEcx >> 8) & 0xff;
> > > >>> - ASSERT (LevelType ==
> > > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> > > >>> - if ((RegEbx & 0xffff) > 1 ) {
> > > >>> - ThreadBits = RegEax & 0x1f;
> > > >>> - } else {
> > > >>> - //
> > > >>> - // HT is not supported
> > > >>> - //
> > > >>> - ThreadBits = 0;
> > > >>> - }
> > > >>> -
> > > >>> - //
> > > >>> - // Software must not assume any "level type" encoding
> > > >>> - // value to be related to any sub-leaf index, except sub-leaf 0.
> > > >>> - //
> > > >>> - SubIndex = 1;
> > > >>> - do {
> > > >>> - AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, SubIndex,
> &RegEax,
> > > >> NULL, &RegEcx, NULL);
> > > >>> - LevelType = (RegEcx >> 8) & 0xff;
> > > >>> - if (LevelType ==
> > CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE) {
> > > >>> - CoreBits = (RegEax & 0x1f) - ThreadBits;
> > > >>> - break;
> > > >>> - }
> > > >>> - SubIndex++;
> > > >>> - } while (LevelType !=
> > > >> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID);
> > > >>> - }
> > > >>> - }
> > > >>> -
> > > >>> - if (!TopologyLeafSupported) {
> > > >>> - AsmCpuid (CPUID_VERSION_INFO, NULL, &RegEbx, NULL, NULL);
> > > >>> - MaxLogicProcessorsPerPackage = (RegEbx >> 16) & 0xff;
> > > >>> - if (MaxCpuIdIndex >= CPUID_CACHE_PARAMS) {
> > > >>> - AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &RegEax, NULL, NULL,
> > NULL);
> > > >>> - MaxCoresPerPackage = (RegEax >> 26) + 1;
> > > >>> - } else {
> > > >>> - //
> > > >>> - // Must be a single-core processor.
> > > >>> - //
> > > >>> - MaxCoresPerPackage = 1;
> > > >>> - }
> > > >>> -
> > > >>> - ThreadBits = (UINTN) (HighBitSet32
> > (MaxLogicProcessorsPerPackage /
> > > >>> MaxCoresPerPackage - 1) + 1);
> > > >>> - CoreBits = (UINTN) (HighBitSet32 (MaxCoresPerPackage - 1) + 1);
> > > >>> - }
> > > >>> -
> > > >>> - Location->Thread = ApicId & ~((-1) << ThreadBits);
> > > >>> - Location->Core = (ApicId >> ThreadBits) & ~((-1) <<
> > > >>> CoreBits);
> > > >>> - Location->Package = (ApicId >> (ThreadBits+ CoreBits)); -}
> > > >>> -
> > > >>> -/**
> > > >>> Gets processor information on the requested processor at the
> > > >>> instant this call is made.
> > > >>>
> > > >>> @param[in] This A pointer to the
> > > >> EFI_SMM_CPU_SERVICE_PROTOCOL
> > > >>> instance.
> > > >>> @@ -280,7 +161,7 @@ SmmAddProcessor (
> > > >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId ==
> > > >> INVALID_APIC_ID) {
> > > >>> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId =
> > ProcessorId;
> > > >>> gSmmCpuPrivate->ProcessorInfo[Index].StatusFlag = 0;
> > > >>> - SmmGetProcessorLocation ((UINT32)ProcessorId,
> > &gSmmCpuPrivate-
> > > >>>> ProcessorInfo[Index].Location);
> > > >>> + SmmCpuFeaturesGetProcessorLocation ((UINT32)ProcessorId,
> > > >>> + &gSmmCpuPrivate-
> > > >>>> ProcessorInfo[Index].Location);
> > > >>>
> > > >>> *ProcessorNumber = Index;
> > > >>> gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
> > > >>> --
> > > >>> 1.9.1
> > > >>>
> > > >>> _______________________________________________
> > > >>> edk2-devel mailing list
> > > >>> edk2-devel@lists.01.org
> > > >>> https://lists.01.org/mailman/listinfo/edk2-devel
> > > > _______________________________________________
> > > > edk2-devel mailing list
> > > > edk2-devel@lists.01.org
> > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > >
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-10-28 15:34 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-27 22:29 [PATCH] PiSmmCpuDxeSmm Leo Duran
2016-10-27 22:29 ` [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library Leo Duran
2016-10-27 23:15 ` Kinney, Michael D
2016-10-28 0:44 ` Fan, Jeff
2016-10-28 1:00 ` Fan, Jeff
2016-10-28 3:19 ` Duran, Leo
2016-10-28 4:43 ` Fan, Jeff
2016-10-28 10:28 ` Laszlo Ersek
2016-10-28 15:29 ` Kinney, Michael D
2016-10-28 15:30 ` Duran, Leo
2016-10-28 15:32 ` Fan, Jeff
2016-10-28 15:34 ` Duran, Leo
2016-10-28 14:28 ` Duran, Leo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox