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

["Fan, Jeff" <jeff.fan@intel.com>]

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