* [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 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
* 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
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