From: "Duran, Leo" <leo.duran@amd.com>
To: "'Kinney, Michael D'" <michael.d.kinney@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Justen, Jordan L" <jordan.l.justen@intel.com>,
"Fan, Jeff" <jeff.fan@intel.com>,
"Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH v3] UefiCpuPkg: ApicLib
Date: Thu, 6 Jul 2017 18:08:54 +0000 [thread overview]
Message-ID: <DM5PR12MB1243652AEE62DFEDDE0BDE62F9D50@DM5PR12MB1243.namprd12.prod.outlook.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5A7D5507C@ORSMSX113.amr.corp.intel.com>
Hi Mike,
> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Thursday, July 06, 2017 11:36 AM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Fan, Jeff
> <jeff.fan@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH v3] UefiCpuPkg: ApicLib
>
> Hi Leo,
>
> Some of the change here do not follow the coding standard. For function
> calls with many arguments, either put the function call on a single line, or
> break it up with each arg on its own line.
>
[Duran, Leo]
Yes, I do see a malformed AsmCpuId() function call... Somehow that was missed in the original submission that's already upstream.
Anyway, good catch!... I'll fix it.
Thanks.
> This was clarified in the latest version of the EDK II C Coding Standard.
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=425
> https://github.com/tianocore-docs/edk2-
> CCodingStandardsSpecification/commit/3f1100beda60843361a9761b43ba7b1
> 8adf0d265
>
> Mike
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leo Duran
> Sent: Thursday, July 6, 2017 7:48 AM
> To: edk2-devel@lists.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Leo Duran
> <leo.duran@amd.com>; Fan, Jeff <jeff.fan@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH v3] UefiCpuPkg: ApicLib
>
> GetProcessorLocationByApicId ()
> - Adjust InitialApicId to properly concatenate Package on AMD processor.
> - Clean-ups 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>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
> UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 42 +++++++++++++----
> -----
> .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 40 ++++++++++++--------
> -
> 2 files changed, 49 insertions(+), 33 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index 2091e5e..ce6d9d7 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);
> @@ -338,7 +338,7 @@ GetInitialApicId (
> AsmCpuid (CPUID_SIGNATURE, &MaxCpuIdIndex, NULL, NULL, NULL);
>
> //
> - // If CPUID Leaf B is supported,
> + // If CPUID Leaf B is supported,
> // And CPUID.0BH:EBX[15:0] reports a non-zero value,
> // Then the initial 32-bit APIC ID = CPUID.0BH:EDX
> // Else the initial 8-bit APIC ID = CPUID.1:EBX[31:24] @@ -1013,13 +1013,14
> @@ GetProcessorLocationByApicId (
> UINT32 MaxCoresPerNode;
> UINT32 CorePerNodeMask;
> UINT32 ApicIdShift;
> + UINT32 ApicIdMask;
> 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 +1043,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 @@ -1051,7
> +1052,7 @@ GetProcessorLocationByApicId (
> //
> TopologyLeafSupported = FALSE;
> if (MaxStandardCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> - AsmCpuidEx(
> + AsmCpuidEx (
> CPUID_EXTENDED_TOPOLOGY,
> 0,
> &ExtendedTopologyEax.Uint32,
> @@ -1072,7 +1073,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 +1082,7 @@ GetProcessorLocationByApicId (
> //
> SubIndex = 1;
> do {
> - AsmCpuidEx(
> + AsmCpuidEx (
> CPUID_EXTENDED_TOPOLOGY,
> SubIndex,
> &ExtendedTopologyEax.Uint32,
> @@ -1103,7 +1104,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;
>
> //
> @@ -1114,11 +1115,11 @@ GetProcessorLocationByApicId (
> //
> // Check for topology extensions on AMD processor
> //
> - if (StandardSignatureIsAuthenticAMD()) {
> + 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,
> + AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, NULL,
> + &AmdProcessorTopologyEbx.Uint32,
> &AmdProcessorTopologyEcx.Uint32, NULL);
> //
> // Get cores per processor package @@ -1128,7 +1129,7 @@
> GetProcessorLocationByApicId (
> //
> // Account for actual thread count (e.g., SMT disabled)
> //
> - AsmCpuid(CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
> &AmdVirPhyAddressSizeEcx.Uint32, NULL);
> + AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
> + &AmdVirPhyAddressSizeEcx.Uint32, NULL);
> MaxThreadPerPackageMask = 1 <<
> AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize;
> ActualThreadPerPackageMask = 1;
> while (ActualThreadPerPackageMask <
> MaxLogicProcessorsPerPackage) { @@ -1136,7 +1137,7 @@
> GetProcessorLocationByApicId (
> }
>
> //
> - // Adjust APIC Id to report concatenation of Package|Core|Thread.
> + // Adjust APIC Id to report concatenation of Core|Thread.
> //
> if (ActualThreadPerPackageMask < MaxThreadPerPackageMask) {
> MaxCoresPerNode = MaxCoresPerPackage /
> (AmdProcessorTopologyEcx.Bits.NodesPerProcessor + 1); @@ -1148,13
> +1149,20 @@ GetProcessorLocationByApicId (
> CorePerNodeMask -= 1;
>
> ApicIdShift = 0;
> + ApicIdMask = ActualThreadPerPackageMask;
> do {
> ApicIdShift += 1;
> - ActualThreadPerPackageMask <<= 1;
> - } while (ActualThreadPerPackageMask <
> MaxThreadPerPackageMask);
> + ApicIdMask <<= 1;
> + } while (ApicIdMask < MaxThreadPerPackageMask);
>
> InitialApicId = ((InitialApicId & ~CorePerNodeMask) >> ApicIdShift) |
> (InitialApicId & CorePerNodeMask);
> }
> + //
> + // Adjust APIC Id to report concatenation of Package|Core|Thread.
> + //
> + if ((InitialApicId & ~(MaxThreadPerPackageMask - 1)) != 0) {
> + InitialApicId = (InitialApicId & (ActualThreadPerPackageMask - 1)) |
> ActualThreadPerPackageMask;
> + }
> }
> }
> }
> @@ -1163,7 +1171,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..54ea492 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);
> @@ -1108,13 +1108,14 @@ GetProcessorLocationByApicId (
> UINT32 MaxCoresPerNode;
> UINT32 CorePerNodeMask;
> UINT32 ApicIdShift;
> + UINT32 ApicIdMask;
> 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 +1138,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 @@ -1146,7
> +1147,7 @@ GetProcessorLocationByApicId (
> //
> TopologyLeafSupported = FALSE;
> if (MaxStandardCpuIdIndex >= CPUID_EXTENDED_TOPOLOGY) {
> - AsmCpuidEx(
> + AsmCpuidEx (
> CPUID_EXTENDED_TOPOLOGY,
> 0,
> &ExtendedTopologyEax.Uint32,
> @@ -1167,7 +1168,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 +1177,7 @@ GetProcessorLocationByApicId (
> //
> SubIndex = 1;
> do {
> - AsmCpuidEx(
> + AsmCpuidEx (
> CPUID_EXTENDED_TOPOLOGY,
> SubIndex,
> &ExtendedTopologyEax.Uint32,
> @@ -1198,7 +1199,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;
>
> //
> @@ -1209,11 +1210,11 @@ GetProcessorLocationByApicId (
> //
> // Check for topology extensions on AMD processor
> //
> - if (StandardSignatureIsAuthenticAMD()) {
> + 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,
> + AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, NULL,
> + &AmdProcessorTopologyEbx.Uint32,
> &AmdProcessorTopologyEcx.Uint32, NULL);
> //
> // Get cores per processor package @@ -1223,7 +1224,7 @@
> GetProcessorLocationByApicId (
> //
> // Account for actual thread count (e.g., SMT disabled)
> //
> - AsmCpuid(CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
> &AmdVirPhyAddressSizeEcx.Uint32, NULL);
> + AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, NULL, NULL,
> + &AmdVirPhyAddressSizeEcx.Uint32, NULL);
> MaxThreadPerPackageMask = 1 <<
> AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize;
> ActualThreadPerPackageMask = 1;
> while (ActualThreadPerPackageMask <
> MaxLogicProcessorsPerPackage) { @@ -1231,7 +1232,7 @@
> GetProcessorLocationByApicId (
> }
>
> //
> - // Adjust APIC Id to report concatenation of Package|Core|Thread.
> + // Adjust APIC Id to report concatenation of Core|Thread.
> //
> if (ActualThreadPerPackageMask < MaxThreadPerPackageMask) {
> MaxCoresPerNode = MaxCoresPerPackage /
> (AmdProcessorTopologyEcx.Bits.NodesPerProcessor + 1); @@ -1243,13
> +1244,20 @@ GetProcessorLocationByApicId (
> CorePerNodeMask -= 1;
>
> ApicIdShift = 0;
> + ApicIdMask = ActualThreadPerPackageMask;
> do {
> ApicIdShift += 1;
> - ActualThreadPerPackageMask <<= 1;
> - } while (ActualThreadPerPackageMask <
> MaxThreadPerPackageMask);
> + ApicIdMask <<= 1;
> + } while (ApicIdMask < MaxThreadPerPackageMask);
>
> InitialApicId = ((InitialApicId & ~CorePerNodeMask) >> ApicIdShift) |
> (InitialApicId & CorePerNodeMask);
> }
> + //
> + // Adjust APIC Id to report concatenation of Package|Core|Thread.
> + //
> + if ((InitialApicId & ~(MaxThreadPerPackageMask - 1)) != 0) {
> + InitialApicId = (InitialApicId & (ActualThreadPerPackageMask - 1)) |
> ActualThreadPerPackageMask;
> + }
> }
> }
> }
> @@ -1258,7 +1266,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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
prev parent reply other threads:[~2017-07-06 18:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-06 14:47 [PATCH v3] UefiCpuPkg: ApicLib Leo Duran
2017-07-06 14:47 ` Leo Duran
2017-07-06 16:35 ` Kinney, Michael D
2017-07-06 18:08 ` Duran, Leo [this message]
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=DM5PR12MB1243652AEE62DFEDDE0BDE62F9D50@DM5PR12MB1243.namprd12.prod.outlook.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