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

["JackX Lin" <JackX.Lin@intel.com>]

Hi Kenny,

Yes, So far we just have 2 CPU core types, which are big core (0x40) and small core (0x20)
But I am not sure if there will be  another type in the future.

In order to avoid missed CPU cores, I add all the other cores in the other order.

Do you prefer separate them by using CORE and ATOM than if/else ?

Best regards
Jack

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Thursday, November 17, 2022 11:45 AM
To: devel@edk2.groups.io; Lin, JackX <jackx.lin@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: 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: RE: [edk2-devel] [edk2-platforms: PATCH] BIOS needs to present cores in order of relative performance in MADT

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
> 
> 
> 
> 
>