From: "Gao, Liming" <liming.gao@intel.com>
To: Leo Duran <leo.duran@amd.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Justen, Jordan L" <jordan.l.justen@intel.com>
Subject: Re: [PATCH] UefiCpuPkg: ApicLib
Date: Mon, 4 Sep 2017 05:09:02 +0000 [thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D78822F@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <1501616113-21992-2-git-send-email-leo.duran@amd.com>
Leo:
Sorry for the late response. This patch is good to me. Reviewed-by: Liming Gao <liming.gao@intel.com>
Thanks
Liming
>-----Original Message-----
>From: Leo Duran [mailto:leo.duran@amd.com]
>Sent: Wednesday, August 02, 2017 3:35 AM
>To: edk2-devel@lists.01.org
>Cc: Leo Duran <leo.duran@amd.com>; Justen, Jordan L
><jordan.l.justen@intel.com>; Fan, Jeff <jeff.fan@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: [PATCH] UefiCpuPkg: ApicLib
>
>GetProcessorLocationByApicId ()
>- Use max possible thread count to decode InitialApicId on AMD processor.
>- Clean-up on C Coding standards.
>
>Cc: Jordan Justen <jordan.l.justen@intel.com>
>Cc: Jeff Fan <jeff.fan@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Leo Duran <leo.duran@amd.com>
>---
> UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 64 ++++++----------------
> .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 64 ++++++----------------
> 2 files changed, 32 insertions(+), 96 deletions(-)
>
>diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>index 2091e5e..b0b7e32 100644
>--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>@@ -48,7 +48,7 @@ StandardSignatureIsAuthenticAMD (
> UINT32 RegEcx;
> UINT32 RegEdx;
>
>- AsmCpuid(CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
>+ AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
>@@ -1000,7 +1000,6 @@ GetProcessorLocationByApicId (
> CPUID_EXTENDED_TOPOLOGY_ECX ExtendedTopologyEcx;
> CPUID_AMD_EXTENDED_CPU_SIG_ECX AmdExtendedCpuSigEcx;
> CPUID_AMD_PROCESSOR_TOPOLOGY_EBX AmdProcessorTopologyEbx;
>- CPUID_AMD_PROCESSOR_TOPOLOGY_ECX AmdProcessorTopologyEcx;
> CPUID_AMD_VIR_PHY_ADDRESS_SIZE_ECX AmdVirPhyAddressSizeEcx;
> UINT32 MaxStandardCpuIdIndex;
> UINT32 MaxExtendedCpuIdIndex;
>@@ -1008,18 +1007,13 @@ GetProcessorLocationByApicId (
> UINTN LevelType;
> UINT32 MaxLogicProcessorsPerPackage;
> UINT32 MaxCoresPerPackage;
>- UINT32 MaxThreadPerPackageMask;
>- UINT32 ActualThreadPerPackageMask;
>- UINT32 MaxCoresPerNode;
>- UINT32 CorePerNodeMask;
>- UINT32 ApicIdShift;
> UINTN ThreadBits;
> UINTN CoreBits;
>
> //
> // Check if the processor is capable of supporting more than one logical
>processor.
> //
>- AsmCpuid(CPUID_VERSION_INFO, NULL, NULL, NULL,
>&VersionInfoEdx.Uint32);
>+ AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL,
>&VersionInfoEdx.Uint32);
> if (VersionInfoEdx.Bits.HTT == 0) {
> if (Thread != NULL) {
> *Thread = 0;
>@@ -1042,8 +1036,8 @@ GetProcessorLocationByApicId (
> //
> // Get max index of CPUID
> //
>- AsmCpuid(CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL,
>NULL);
>- AsmCpuid(CPUID_EXTENDED_FUNCTION, &MaxExtendedCpuIdIndex, NULL,
>NULL, NULL);
>+ AsmCpuid (CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL,
>NULL);
>+ AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedCpuIdIndex,
>NULL, NULL, NULL);
>
> //
> // If the extended topology enumeration leaf is available, it
>@@ -1072,7 +1066,7 @@ GetProcessorLocationByApicId (
> // the SMT sub-field of x2APIC ID.
> //
> LevelType = ExtendedTopologyEcx.Bits.LevelType;
>- ASSERT(LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
>+ ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> ThreadBits = ExtendedTopologyEax.Bits.ApicIdShift;
>
> //
>@@ -1081,7 +1075,7 @@ GetProcessorLocationByApicId (
> //
> SubIndex = 1;
> do {
>- AsmCpuidEx(
>+ AsmCpuidEx (
> CPUID_EXTENDED_TOPOLOGY,
> SubIndex,
> &ExtendedTopologyEax.Uint32,
>@@ -1103,7 +1097,7 @@ GetProcessorLocationByApicId (
> //
> // Get logical processor count
> //
>- AsmCpuid(CPUID_VERSION_INFO, NULL, &VersionInfoEbx.Uint32, NULL,
>NULL);
>+ AsmCpuid (CPUID_VERSION_INFO, NULL, &VersionInfoEbx.Uint32, NULL,
>NULL);
> MaxLogicProcessorsPerPackage =
>VersionInfoEbx.Bits.MaximumAddressableIdsForLogicalProcessors;
>
> //
>@@ -1116,45 +1110,19 @@ GetProcessorLocationByApicId (
> //
> if (StandardSignatureIsAuthenticAMD()) {
> if (MaxExtendedCpuIdIndex >= CPUID_AMD_PROCESSOR_TOPOLOGY) {
>- AsmCpuid(CPUID_EXTENDED_CPU_SIG, NULL, NULL,
>&AmdExtendedCpuSigEcx.Uint32, NULL);
>+ AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL,
>&AmdExtendedCpuSigEcx.Uint32, NULL);
> if (AmdExtendedCpuSigEcx.Bits.TopologyExtensions != 0) {
>- AsmCpuid(CPUID_AMD_PROCESSOR_TOPOLOGY, NULL,
>&AmdProcessorTopologyEbx.Uint32,
>- &AmdProcessorTopologyEcx.Uint32, NULL);
>+ //
>+ // Account for max possible thread count to decode ApicId
>+ //
>+ AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
>&AmdVirPhyAddressSizeEcx.Uint32, NULL);
>+ MaxLogicProcessorsPerPackage = 1 <<
>AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize;
>+
> //
> // Get cores per processor package
> //
>+ AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, NULL,
>&AmdProcessorTopologyEbx.Uint32, NULL, NULL);
> MaxCoresPerPackage = MaxLogicProcessorsPerPackage /
>(AmdProcessorTopologyEbx.Bits.ThreadsPerCore + 1);
>-
>- //
>- // Account for actual thread count (e.g., SMT disabled)
>- //
>- AsmCpuid(CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
>&AmdVirPhyAddressSizeEcx.Uint32, NULL);
>- MaxThreadPerPackageMask = 1 <<
>AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize;
>- ActualThreadPerPackageMask = 1;
>- while (ActualThreadPerPackageMask < MaxLogicProcessorsPerPackage)
>{
>- ActualThreadPerPackageMask <<= 1;
>- }
>-
>- //
>- // Adjust APIC Id to report concatenation of Package|Core|Thread.
>- //
>- if (ActualThreadPerPackageMask < MaxThreadPerPackageMask) {
>- MaxCoresPerNode = MaxCoresPerPackage /
>(AmdProcessorTopologyEcx.Bits.NodesPerProcessor + 1);
>-
>- CorePerNodeMask = 1;
>- while (CorePerNodeMask < MaxCoresPerNode) {
>- CorePerNodeMask <<= 1;
>- }
>- CorePerNodeMask -= 1;
>-
>- ApicIdShift = 0;
>- do {
>- ApicIdShift += 1;
>- ActualThreadPerPackageMask <<= 1;
>- } while (ActualThreadPerPackageMask < MaxThreadPerPackageMask);
>-
>- InitialApicId = ((InitialApicId & ~CorePerNodeMask) >> ApicIdShift) |
>(InitialApicId & CorePerNodeMask);
>- }
> }
> }
> }
>@@ -1163,7 +1131,7 @@ GetProcessorLocationByApicId (
> // Extract core count based on CACHE information
> //
> if (MaxStandardCpuIdIndex >= CPUID_CACHE_PARAMS) {
>- AsmCpuidEx(CPUID_CACHE_PARAMS, 0, &CacheParamsEax.Uint32,
>NULL, NULL, NULL);
>+ AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &CacheParamsEax.Uint32,
>NULL, NULL, NULL);
> if (CacheParamsEax.Uint32 != 0) {
> MaxCoresPerPackage =
>CacheParamsEax.Bits.MaximumAddressableIdsForLogicalProcessors + 1;
> }
>diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>index d5d4efa..1f4dcf7 100644
>--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>@@ -49,7 +49,7 @@ StandardSignatureIsAuthenticAMD (
> UINT32 RegEcx;
> UINT32 RegEdx;
>
>- AsmCpuid(CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
>+ AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
>@@ -1095,7 +1095,6 @@ GetProcessorLocationByApicId (
> CPUID_EXTENDED_TOPOLOGY_ECX ExtendedTopologyEcx;
> CPUID_AMD_EXTENDED_CPU_SIG_ECX AmdExtendedCpuSigEcx;
> CPUID_AMD_PROCESSOR_TOPOLOGY_EBX AmdProcessorTopologyEbx;
>- CPUID_AMD_PROCESSOR_TOPOLOGY_ECX AmdProcessorTopologyEcx;
> CPUID_AMD_VIR_PHY_ADDRESS_SIZE_ECX AmdVirPhyAddressSizeEcx;
> UINT32 MaxStandardCpuIdIndex;
> UINT32 MaxExtendedCpuIdIndex;
>@@ -1103,18 +1102,13 @@ GetProcessorLocationByApicId (
> UINTN LevelType;
> UINT32 MaxLogicProcessorsPerPackage;
> UINT32 MaxCoresPerPackage;
>- UINT32 MaxThreadPerPackageMask;
>- UINT32 ActualThreadPerPackageMask;
>- UINT32 MaxCoresPerNode;
>- UINT32 CorePerNodeMask;
>- UINT32 ApicIdShift;
> UINTN ThreadBits;
> UINTN CoreBits;
>
> //
> // Check if the processor is capable of supporting more than one logical
>processor.
> //
>- AsmCpuid(CPUID_VERSION_INFO, NULL, NULL, NULL,
>&VersionInfoEdx.Uint32);
>+ AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL,
>&VersionInfoEdx.Uint32);
> if (VersionInfoEdx.Bits.HTT == 0) {
> if (Thread != NULL) {
> *Thread = 0;
>@@ -1137,8 +1131,8 @@ GetProcessorLocationByApicId (
> //
> // Get max index of CPUID
> //
>- AsmCpuid(CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL,
>NULL);
>- AsmCpuid(CPUID_EXTENDED_FUNCTION, &MaxExtendedCpuIdIndex, NULL,
>NULL, NULL);
>+ AsmCpuid (CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL,
>NULL);
>+ AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedCpuIdIndex,
>NULL, NULL, NULL);
>
> //
> // If the extended topology enumeration leaf is available, it
>@@ -1167,7 +1161,7 @@ GetProcessorLocationByApicId (
> // the SMT sub-field of x2APIC ID.
> //
> LevelType = ExtendedTopologyEcx.Bits.LevelType;
>- ASSERT(LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
>+ ASSERT (LevelType == CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT);
> ThreadBits = ExtendedTopologyEax.Bits.ApicIdShift;
>
> //
>@@ -1176,7 +1170,7 @@ GetProcessorLocationByApicId (
> //
> SubIndex = 1;
> do {
>- AsmCpuidEx(
>+ AsmCpuidEx (
> CPUID_EXTENDED_TOPOLOGY,
> SubIndex,
> &ExtendedTopologyEax.Uint32,
>@@ -1198,7 +1192,7 @@ GetProcessorLocationByApicId (
> //
> // Get logical processor count
> //
>- AsmCpuid(CPUID_VERSION_INFO, NULL, &VersionInfoEbx.Uint32, NULL,
>NULL);
>+ AsmCpuid (CPUID_VERSION_INFO, NULL, &VersionInfoEbx.Uint32, NULL,
>NULL);
> MaxLogicProcessorsPerPackage =
>VersionInfoEbx.Bits.MaximumAddressableIdsForLogicalProcessors;
>
> //
>@@ -1211,45 +1205,19 @@ GetProcessorLocationByApicId (
> //
> if (StandardSignatureIsAuthenticAMD()) {
> if (MaxExtendedCpuIdIndex >= CPUID_AMD_PROCESSOR_TOPOLOGY) {
>- AsmCpuid(CPUID_EXTENDED_CPU_SIG, NULL, NULL,
>&AmdExtendedCpuSigEcx.Uint32, NULL);
>+ AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL,
>&AmdExtendedCpuSigEcx.Uint32, NULL);
> if (AmdExtendedCpuSigEcx.Bits.TopologyExtensions != 0) {
>- AsmCpuid(CPUID_AMD_PROCESSOR_TOPOLOGY, NULL,
>&AmdProcessorTopologyEbx.Uint32,
>- &AmdProcessorTopologyEcx.Uint32, NULL);
>+ //
>+ // Account for max possible thread count to decode ApicId
>+ //
>+ AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
>&AmdVirPhyAddressSizeEcx.Uint32, NULL);
>+ MaxLogicProcessorsPerPackage = 1 <<
>AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize;
>+
> //
> // Get cores per processor package
> //
>+ AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, NULL,
>&AmdProcessorTopologyEbx.Uint32, NULL, NULL);
> MaxCoresPerPackage = MaxLogicProcessorsPerPackage /
>(AmdProcessorTopologyEbx.Bits.ThreadsPerCore + 1);
>-
>- //
>- // Account for actual thread count (e.g., SMT disabled)
>- //
>- AsmCpuid(CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
>&AmdVirPhyAddressSizeEcx.Uint32, NULL);
>- MaxThreadPerPackageMask = 1 <<
>AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize;
>- ActualThreadPerPackageMask = 1;
>- while (ActualThreadPerPackageMask < MaxLogicProcessorsPerPackage)
>{
>- ActualThreadPerPackageMask <<= 1;
>- }
>-
>- //
>- // Adjust APIC Id to report concatenation of Package|Core|Thread.
>- //
>- if (ActualThreadPerPackageMask < MaxThreadPerPackageMask) {
>- MaxCoresPerNode = MaxCoresPerPackage /
>(AmdProcessorTopologyEcx.Bits.NodesPerProcessor + 1);
>-
>- CorePerNodeMask = 1;
>- while (CorePerNodeMask < MaxCoresPerNode) {
>- CorePerNodeMask <<= 1;
>- }
>- CorePerNodeMask -= 1;
>-
>- ApicIdShift = 0;
>- do {
>- ApicIdShift += 1;
>- ActualThreadPerPackageMask <<= 1;
>- } while (ActualThreadPerPackageMask < MaxThreadPerPackageMask);
>-
>- InitialApicId = ((InitialApicId & ~CorePerNodeMask) >> ApicIdShift) |
>(InitialApicId & CorePerNodeMask);
>- }
> }
> }
> }
>@@ -1258,7 +1226,7 @@ GetProcessorLocationByApicId (
> // Extract core count based on CACHE information
> //
> if (MaxStandardCpuIdIndex >= CPUID_CACHE_PARAMS) {
>- AsmCpuidEx(CPUID_CACHE_PARAMS, 0, &CacheParamsEax.Uint32,
>NULL, NULL, NULL);
>+ AsmCpuidEx (CPUID_CACHE_PARAMS, 0, &CacheParamsEax.Uint32,
>NULL, NULL, NULL);
> if (CacheParamsEax.Uint32 != 0) {
> MaxCoresPerPackage =
>CacheParamsEax.Bits.MaximumAddressableIdsForLogicalProcessors + 1;
> }
>--
>2.7.4
next prev parent reply other threads:[~2017-09-04 5:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-01 19:35 [PATCH] UefiCpuPkg: ApicLib Leo Duran
2017-08-01 19:35 ` Leo Duran
2017-09-04 5:09 ` Gao, Liming [this message]
-- strict thread matches above, loose matches on Subject: below --
2017-07-01 0:41 Leo Duran
2017-07-01 0:41 ` Leo Duran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A89E2EF3DFEDB4C8BFDE51014F606A14D78822F@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox