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

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

Jeff, et al,
Do we also remove GetProcessorLocation()  from MpInitLib?
Leo

> -----Original Message-----
> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> Sent: Thursday, October 27, 2016 11:43 PM
> To: Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
> 
> 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.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 <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