From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 2FBEA81D4C for ; Fri, 28 Oct 2016 03:28:51 -0700 (PDT) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0DE82C054901; Fri, 28 Oct 2016 10:28:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-38.phx2.redhat.com [10.3.116.38]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9SASmYH019301; Fri, 28 Oct 2016 06:28:49 -0400 To: "Fan, Jeff" , "Duran, Leo" , "Kinney, Michael D" References: <1477607390-8225-1-git-send-email-leo.duran@amd.com> <1477607390-8225-2-git-send-email-leo.duran@amd.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2B909A@shsmsx102.ccr.corp.intel.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2B9645@shsmsx102.ccr.corp.intel.com> Cc: "edk2-devel@lists.01.org" , "Gao, Liming" From: Laszlo Ersek Message-ID: <0c52a13d-5487-4305-23e7-fc9ec8826a8a@redhat.com> Date: Fri, 28 Oct 2016 12:28:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4A2B9645@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 28 Oct 2016 10:28:51 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Oct 2016 10:28:51 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 ; Duran, Leo >> ; edk2-devel@lists.01.org >> Cc: Gao, Liming >> 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 ; Gao, Liming >> >>> 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 >>> --- >>> 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 >