Re: [edk2-devel] [edk2-platforms: PATCH] BIOS needs to present cores in order of relative performance in MADT

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

Hi Jack,

I like these updates to capture the 8-bit core type values.

From the SDM and the MdePkg/Include/Registers/Intel/Cpuid.h file
I see the following defines:

///
/// @{ Define value for CPUID_NATIVE_MODEL_ID_AND_CORE_TYPE_EAX.CoreType
///
#define   CPUID_CORE_TYPE_INTEL_ATOM  0x20
#define   CPUID_CORE_TYPE_INTEL_CORE  0x40
///
/// @}
///

It looks like the default sorting policy to add CPUs with a
CPU Type of CPUID_CORE_TYPE_INTEL_CORE first, and then add
all the other in any order?  I see that only CORE and ATOM
are defined at this time.  Does this imply that this logic
will need to be updated if additional core types are added?

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of JackX Lin
> Sent: Tuesday, November 15, 2022 10:30 PM
> To: devel@edk2.groups.io
> Cc: Lin, JackX <jackx.lin@intel.com>; Lin, JackX <jackx.lin@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone,
> Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Dong, Eric <eric.dong@intel.com>; Kuo, Donald <donald.kuo@intel.com>; Kumar, Chandana C <chandana.c.kumar@intel.com>
> Subject: [edk2-devel] [edk2-platforms: PATCH] BIOS needs to present cores in order of relative performance in MADT
> 
> BIOS should keep MADT ordering by big core first then small core
> 
> Signed-off-by: JackX Lin <JackX.Lin@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Donald Kuo <Donald.Kuo@intel.com>
> Cc: Chandana C Kumar <chandana.c.kumar@intel.com>
> Cc: JackX Lin <JackX.Lin@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 94
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 87 insertions(+), 7 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index 6e57b638e0..bafe359668 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -18,6 +18,7 @@ typedef struct {
>    UINT32   Flags;
>    UINT32   SocketNum;
>    UINT32   Thread;
> +  UINT8    CpuCoreType;
>  } EFI_CPU_ID_ORDER_MAP;
> 
>  //
> @@ -131,6 +132,49 @@ AppendCpuMapTableEntry (
> 
>  }
> 
> +/**
> +  Function will go through all processors to identify Core or Atom
> +  by checking Core Type and update in IsBigCore.
> +
> +  @param[in] CpuApicIdOrderTable         Point to a buffer which will be filled in Core type information.
> +**/
> +VOID
> +STATIC
> +EFIAPI
> +CollectCpuCoreType (
> +  IN EFI_CPU_ID_ORDER_MAP        *CpuApicIdOrderTable
> +  )
> +{
> +  CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_EDX     Edx;
> +  UINT32                                          Eax;
> +  UINTN                                           ApNumber;
> +  EFI_STATUS                                      Status;
> +  UINT8                                           CoreType;
> +
> +  Status = mMpService->WhoAmI (
> +                         mMpService,
> +                         &ApNumber
> +                         );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  ///
> +  /// Check Hetero feature is supported
> +  /// with CPUID.(EAX=7,ECX=0):EDX[15]=1
> +  ///
> +  AsmCpuidEx (CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, 0, NULL, NULL, NULL, &Edx.Uint32);
> +  if (Edx.Bits.Hybrid == 1) {
> +    //
> +    // Check which is the running core by reading CPUID.(EAX=1AH, ECX=00H):EAX
> +    //
> +    AsmCpuid (CPUID_HYBRID_INFORMATION, &Eax, NULL, NULL, NULL);
> +    CoreType = (UINT8) ((Eax & 0xFF000000) >> 24);
> +  } else {
> +    CoreType = CPUID_CORE_TYPE_INTEL_CORE;
> +  }
> +
> +  CpuApicIdOrderTable[ApNumber].CpuCoreType = CoreType;
> +}
> +
>  /**
>    Collect all processors information and create a Cpu Apic Id table.
> 
> @@ -138,7 +182,7 @@ AppendCpuMapTableEntry (
>  **/
>  EFI_STATUS
>  CreateCpuLocalApicInTable (
> -  IN EFI_CPU_ID_ORDER_MAP *CpuApicIdOrderTable
> +  IN EFI_CPU_ID_ORDER_MAP                   *CpuApicIdOrderTable
>    )
>  {
>    EFI_STATUS                                Status;
> @@ -146,9 +190,24 @@ CreateCpuLocalApicInTable (
>    UINT32                                    Index;
>    UINT32                                    CurrProcessor;
>    EFI_CPU_ID_ORDER_MAP                      *CpuIdMapPtr;
> +  EFI_CPU_ID_ORDER_MAP                      *TempCpuApicIdOrderTable;
>    UINT32                                    Socket;
> 
> -  Status     = EFI_SUCCESS;
> +  TempCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof (EFI_CPU_ID_ORDER_MAP));
> +  if (TempCpuApicIdOrderTable == NULL) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  CollectCpuCoreType (TempCpuApicIdOrderTable);
> +  mMpService->StartupAllAPs (
> +                mMpService,                               // This
> +                (EFI_AP_PROCEDURE) CollectCpuCoreType,    // Procedure
> +                TRUE,                                     // SingleThread
> +                NULL,                                     // WaitEvent
> +                0,                                        // TimeoutInMicrosecsond
> +                TempCpuApicIdOrderTable,                  // ProcedureArgument
> +                NULL                                      // FailedCpuList
> +                );
> 
>    for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++, Index++) {
>      Status = mMpService->GetProcessorInfo (
> @@ -157,9 +216,9 @@ CreateCpuLocalApicInTable (
>                             &ProcessorInfoBuffer
>                             );
> 
> -    CpuIdMapPtr = (EFI_CPU_ID_ORDER_MAP *) &CpuApicIdOrderTable[Index];
> +    CpuIdMapPtr = (EFI_CPU_ID_ORDER_MAP *) &TempCpuApicIdOrderTable[Index];
>      if ((ProcessorInfoBuffer.StatusFlag & PROCESSOR_ENABLED_BIT) != 0) {
> -      CpuIdMapPtr->ApicId  = (UINT32)ProcessorInfoBuffer.ProcessorId;
> +      CpuIdMapPtr->ApicId  = (UINT32) ProcessorInfoBuffer.ProcessorId;
>        CpuIdMapPtr->Thread  = ProcessorInfoBuffer.Location.Thread;
>        CpuIdMapPtr->Flags   = ((ProcessorInfoBuffer.StatusFlag & PROCESSOR_ENABLED_BIT) != 0);
>        CpuIdMapPtr->SocketNum = ProcessorInfoBuffer.Location.Package;
> @@ -184,22 +243,43 @@ CreateCpuLocalApicInTable (
>    //
>    DEBUG ((DEBUG_INFO, "BspApicId - 0x%x\n", GetApicId ()));
> 
> -
>    //
>    // Fill in AcpiProcessorUid.
>    //
>    for (Socket = 0; Socket < FixedPcdGet32 (PcdMaxCpuSocketCount); Socket++) {
>      for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
> -      if (CpuApicIdOrderTable[CurrProcessor].Flags && (CpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
> -        CpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid = (CpuApicIdOrderTable[CurrProcessor].SocketNum <<
> mNumOfBitShift) + Index;
> +      if (TempCpuApicIdOrderTable[CurrProcessor].Flags && (TempCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
> +        TempCpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid = (TempCpuApicIdOrderTable[CurrProcessor].SocketNum <<
> mNumOfBitShift) + Index;
>          Index++;
>        }
>      }
>    }
> 
> +  //
> +  // Re-ordering Cpu cores information to CpuApicIdOrderTable
> +  // by big core first, then small core.
> +  //
> +  for (Index = 0, CurrProcessor = 0; Index < mNumberOfCpus; Index++) {
> +    if (TempCpuApicIdOrderTable[Index].CpuCoreType == CPUID_CORE_TYPE_INTEL_CORE) {
> +      CopyMem (&CpuApicIdOrderTable[CurrProcessor], &TempCpuApicIdOrderTable[Index], sizeof (EFI_CPU_ID_ORDER_MAP));
> +      CurrProcessor++;
> +    }
> +  }
> +
> +  for (Index = 0; Index < mNumberOfCpus; Index++) {
> +    if (TempCpuApicIdOrderTable[Index].CpuCoreType != CPUID_CORE_TYPE_INTEL_CORE) {
> +      CopyMem (&CpuApicIdOrderTable[CurrProcessor], &TempCpuApicIdOrderTable[Index], sizeof (EFI_CPU_ID_ORDER_MAP));
> +      CurrProcessor++;
> +    }
> +  }
> +
>    DEBUG ((DEBUG_INFO, "::ACPI::  APIC ID Order Table Init.   mNumOfBitShift = %x\n", mNumOfBitShift));
>    DebugDisplayReOrderTable (CpuApicIdOrderTable);
> 
> +  if (TempCpuApicIdOrderTable != NULL) {
> +    FreePool (TempCpuApicIdOrderTable);
> +  }
> +
>    return Status;
>  }
> 
> --
> 2.32.0.windows.2
> 
> 
> 
> 
>