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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Hi Laszlo,

Quark DSC files do map the LocalApicLib, so I think the proposal to add
GetProcessorLocation() to the LocalApicLib class and instances makes sense.

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 28, 2016 3:29 AM
> To: Fan, Jeff <jeff.fan@intel.com>; Duran, Leo <leo.duran@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg: Move GetProcessorLocation() to
> SmmCpuFeaturesLib library.
> 
> 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 <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
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >