Hi Mike,
> -----Original Message----- > From: Kinney, Michael D [mailto:[email protected]] > Sent: Thursday, July 06, 2017 11:36 AM > To: Duran, Leo <[email protected]>; [email protected]; Kinney, > Michael D <[email protected]> > Cc: Justen, Jordan L <[email protected]>; Fan, Jeff > <[email protected]>; Gao, Liming <[email protected]> > 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:[email protected]] On Behalf Of > Leo Duran > Sent: Thursday, July 6, 2017 7:48 AM > To: [email protected] > Cc: Justen, Jordan L <[email protected]>; Leo Duran > <[email protected]>; Fan, Jeff <[email protected]>; Gao, Liming > <[email protected]> > 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 <[email protected]> > Cc: Jeff Fan <[email protected]> > Cc: Liming Gao <[email protected]> > Cc: Brijesh Singh <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Leo Duran <[email protected]> > --- > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
