From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 44BC921E3EA7D for ; Sun, 3 Sep 2017 22:06:20 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Sep 2017 22:09:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,473,1498546800"; d="scan'208";a="147800761" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga006.fm.intel.com with ESMTP; 03 Sep 2017 22:09:07 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 3 Sep 2017 22:09:06 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.39]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.98]) with mapi id 14.03.0319.002; Mon, 4 Sep 2017 13:09:04 +0800 From: "Gao, Liming" To: Leo Duran , "edk2-devel@lists.01.org" CC: "Justen, Jordan L" Thread-Topic: [PATCH] UefiCpuPkg: ApicLib Thread-Index: AQHTCv1cTT3cUuZR70y26Dx1P6BxVaKkYYog Date: Mon, 4 Sep 2017 05:09:02 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D78822F@shsmsx102.ccr.corp.intel.com> References: <1501616113-21992-1-git-send-email-leo.duran@amd.com> <1501616113-21992-2-git-send-email-leo.duran@amd.com> In-Reply-To: <1501616113-21992-2-git-send-email-leo.duran@amd.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] UefiCpuPkg: ApicLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Sep 2017 05:06:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Leo: Sorry for the late response. This patch is good to me. Reviewed-by: Limin= g Gao 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 ; Justen, Jordan L >; Fan, Jeff ; Gao, Liming > >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 >Cc: Jeff Fan >Cc: Liming Gao >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Leo Duran >--- > 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 =3D=3D CPUID_SIGNATURE_AUTHENTIC_AMD_EBX && > RegEcx =3D=3D CPUID_SIGNATURE_AUTHENTIC_AMD_ECX && > RegEdx =3D=3D 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 logica= l >processor. > // >- AsmCpuid(CPUID_VERSION_INFO, NULL, NULL, NULL, >&VersionInfoEdx.Uint32); >+ AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, >&VersionInfoEdx.Uint32); > if (VersionInfoEdx.Bits.HTT =3D=3D 0) { > if (Thread !=3D NULL) { > *Thread =3D 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 =3D ExtendedTopologyEcx.Bits.LevelType; >- ASSERT(LevelType =3D=3D CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT); >+ ASSERT (LevelType =3D=3D CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT); > ThreadBits =3D ExtendedTopologyEax.Bits.ApicIdShift; > > // >@@ -1081,7 +1075,7 @@ GetProcessorLocationByApicId ( > // > SubIndex =3D 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 =3D >VersionInfoEbx.Bits.MaximumAddressableIdsForLogicalProcessors; > > // >@@ -1116,45 +1110,19 @@ GetProcessorLocationByApicId ( > // > if (StandardSignatureIsAuthenticAMD()) { > if (MaxExtendedCpuIdIndex >=3D 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 !=3D 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 =3D 1 << >AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize; >+ > // > // Get cores per processor package > // >+ AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, NULL, >&AmdProcessorTopologyEbx.Uint32, NULL, NULL); > MaxCoresPerPackage =3D 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 =3D 1 << >AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize; >- ActualThreadPerPackageMask =3D 1; >- while (ActualThreadPerPackageMask < MaxLogicProcessorsPerPackag= e) >{ >- ActualThreadPerPackageMask <<=3D 1; >- } >- >- // >- // Adjust APIC Id to report concatenation of Package|Core|Threa= d. >- // >- if (ActualThreadPerPackageMask < MaxThreadPerPackageMask) { >- MaxCoresPerNode =3D MaxCoresPerPackage / >(AmdProcessorTopologyEcx.Bits.NodesPerProcessor + 1); >- >- CorePerNodeMask =3D 1; >- while (CorePerNodeMask < MaxCoresPerNode) { >- CorePerNodeMask <<=3D 1; >- } >- CorePerNodeMask -=3D 1; >- >- ApicIdShift =3D 0; >- do { >- ApicIdShift +=3D 1; >- ActualThreadPerPackageMask <<=3D 1; >- } while (ActualThreadPerPackageMask < MaxThreadPerPackageMask= ); >- >- InitialApicId =3D ((InitialApicId & ~CorePerNodeMask) >> Apic= IdShift) | >(InitialApicId & CorePerNodeMask); >- } > } > } > } >@@ -1163,7 +1131,7 @@ GetProcessorLocationByApicId ( > // Extract core count based on CACHE information > // > if (MaxStandardCpuIdIndex >=3D 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 !=3D 0) { > MaxCoresPerPackage =3D >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 =3D=3D CPUID_SIGNATURE_AUTHENTIC_AMD_EBX && > RegEcx =3D=3D CPUID_SIGNATURE_AUTHENTIC_AMD_ECX && > RegEdx =3D=3D 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 logica= l >processor. > // >- AsmCpuid(CPUID_VERSION_INFO, NULL, NULL, NULL, >&VersionInfoEdx.Uint32); >+ AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, >&VersionInfoEdx.Uint32); > if (VersionInfoEdx.Bits.HTT =3D=3D 0) { > if (Thread !=3D NULL) { > *Thread =3D 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 =3D ExtendedTopologyEcx.Bits.LevelType; >- ASSERT(LevelType =3D=3D CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT); >+ ASSERT (LevelType =3D=3D CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT); > ThreadBits =3D ExtendedTopologyEax.Bits.ApicIdShift; > > // >@@ -1176,7 +1170,7 @@ GetProcessorLocationByApicId ( > // > SubIndex =3D 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 =3D >VersionInfoEbx.Bits.MaximumAddressableIdsForLogicalProcessors; > > // >@@ -1211,45 +1205,19 @@ GetProcessorLocationByApicId ( > // > if (StandardSignatureIsAuthenticAMD()) { > if (MaxExtendedCpuIdIndex >=3D 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 !=3D 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 =3D 1 << >AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize; >+ > // > // Get cores per processor package > // >+ AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, NULL, >&AmdProcessorTopologyEbx.Uint32, NULL, NULL); > MaxCoresPerPackage =3D 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 =3D 1 << >AmdVirPhyAddressSizeEcx.Bits.ApicIdCoreIdSize; >- ActualThreadPerPackageMask =3D 1; >- while (ActualThreadPerPackageMask < MaxLogicProcessorsPerPackag= e) >{ >- ActualThreadPerPackageMask <<=3D 1; >- } >- >- // >- // Adjust APIC Id to report concatenation of Package|Core|Threa= d. >- // >- if (ActualThreadPerPackageMask < MaxThreadPerPackageMask) { >- MaxCoresPerNode =3D MaxCoresPerPackage / >(AmdProcessorTopologyEcx.Bits.NodesPerProcessor + 1); >- >- CorePerNodeMask =3D 1; >- while (CorePerNodeMask < MaxCoresPerNode) { >- CorePerNodeMask <<=3D 1; >- } >- CorePerNodeMask -=3D 1; >- >- ApicIdShift =3D 0; >- do { >- ApicIdShift +=3D 1; >- ActualThreadPerPackageMask <<=3D 1; >- } while (ActualThreadPerPackageMask < MaxThreadPerPackageMask= ); >- >- InitialApicId =3D ((InitialApicId & ~CorePerNodeMask) >> Apic= IdShift) | >(InitialApicId & CorePerNodeMask); >- } > } > } > } >@@ -1258,7 +1226,7 @@ GetProcessorLocationByApicId ( > // Extract core count based on CACHE information > // > if (MaxStandardCpuIdIndex >=3D 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 !=3D 0) { > MaxCoresPerPackage =3D >CacheParamsEax.Bits.MaximumAddressableIdsForLogicalProcessors + 1; > } >-- >2.7.4