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.
Thanks!
Jeff
-----Original Message-----
From: Duran, Leo [mailto:leo.du...@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....@intel.com]
> Sent: Thursday, October 27, 2016 8:00 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Duran, Leo
> <leo.du...@amd.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming....@intel.com>
> 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-boun...@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 <leo.du...@amd.com>; Gao, Liming
> <liming....@intel.com>
> > 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 <leo.du...@amd.com>
> > ---
> > 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
