Re: [PATCH] UefiCpuPkg: Move GetProcessorLocation() to SmmCpuFeaturesLib library.

["Duran, Leo" <leo.duran@amd.com>]

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 <michael.d.kinney@intel.com>; Duran, Leo
> <leo.duran@amd.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@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-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 <leo.duran@amd.com>; Gao, Liming
> <liming.gao@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.duran@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