Re: [edk2-platforms: PATCH] Modify processor _UID ordering by CPU default fused in MADT

["Ni, Ray" <ray.ni@intel.com>]

Good to know. Thanks😊

> -----Original Message-----
> From: Lin, JackX <jackx.lin@intel.com>
> Sent: Wednesday, August 10, 2022 11:51 AM
> To: Ni, Ray <ray.ni@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; devel@edk2.groups.io; Kuo, Donald
> <donald.kuo@intel.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>; Palakshareddy, Lavanya C
> <lavanya.c.palakshareddy@intel.com>; Palakshareddy, Lavanya C
> <lavanya.c.palakshareddy@intel.com>
> Subject: RE: [edk2-platforms: PATCH] Modify processor _UID ordering by
> CPU default fused in MADT
> 
> Hi Ray,
> 
> I know this patch, and the thread 2 and 3 are added by my request for a
> reason on that time.
> I will re-sent the code patch.
> Thank you.
> 
> Jack
> 
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, August 10, 2022 11:48 AM
> To: Lin, JackX <jackx.lin@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; devel@edk2.groups.io; Kuo, Donald
> <donald.kuo@intel.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>; Palakshareddy, Lavanya C
> <lavanya.c.palakshareddy@intel.com>; Palakshareddy, Lavanya C
> <lavanya.c.palakshareddy@intel.com>
> Subject: RE: [edk2-platforms: PATCH] Modify processor _UID ordering by
> CPU default fused in MADT
> 
> Jack,
> Your patch cannot be merged to trunk because Ankit just did some change in
> the same C file, in below commit.
> * MinPlatformPkg/AcpiTables: Add additional thread mapping in MADT
> 
> Ankit,
> It seems your patch is to add support for thread #2 and #3. Jack's patch is to
> remove the additional sorting that put secondary threads after first threads.
> Do you see an issue that we remove the thread sorting logic?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Lin, JackX <jackx.lin@intel.com>
> > Sent: Monday, August 8, 2022 4:21 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>; Dong,
> > Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Kuo, Donald <donald.kuo@intel.com>;
> > Kumar, Chandana C <chandana.c.kumar@intel.com>; Palakshareddy;
> > Palakshareddy, Lavanya C <lavanya.c.palakshareddy@intel.com>
> > Subject: [edk2-platforms: PATCH] Modify processor _UID ordering by CPU
> > default fused in MADT
> >
> > BIOS should not reordering cpu processor_uid
> >
> > Signed-off-by: JackX Lin <JackX.Lin@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Dong Eric <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> > Cc: Donald Kuo <Donald.Kuo@intel.com>
> > Cc: Chandana C Kumar <chandana.c.kumar@intel.com>
> > Cc: Palakshareddy, Lavanya C <lavanya.c.palakshareddy@intel.com>
> > Cc: JackX Lin <JackX.Lin@intel.com>
> > ---
> >  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 174
> > +++++++++++++++++++++++++++++++++++++++-----------------------------
> --
> > ----------------------------------------------------------------------
> > ------------------------
> > ----------
> >  1 file changed, 39 insertions(+), 135 deletions(-)
> >
> > diff --git
> > a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > index c7e87cbd7d..176e422e81 100644
> > --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > @@ -57,38 +57,9 @@ BOOLEAN                     mForceX2ApicId;
> >  BOOLEAN                     mX2ApicEnabled;
> >
> >  EFI_MP_SERVICES_PROTOCOL    *mMpService;
> > -BOOLEAN                     mCpuOrderSorted;
> > -EFI_CPU_ID_ORDER_MAP        *mCpuApicIdOrderTable = NULL;
> >  UINTN                       mNumberOfCpus = 0;
> >  UINTN                       mNumberOfEnabledCPUs = 0;
> >
> > -
> > -/**
> > -  The function is called by PerformQuickSort to compare int values.
> > -
> > -  @param[in] Left            The pointer to first buffer.
> > -  @param[in] Right           The pointer to second buffer.
> > -
> > -  @return -1                 Buffer1 is less than Buffer2.
> > -  @return  1                 Buffer1 is greater than Buffer2.
> > -
> > -**/
> > -INTN
> > -EFIAPI
> > -ApicIdCompareFunction (
> > -  IN CONST VOID                         *Left,
> > -  IN CONST VOID                         *Right
> > -  )
> > -{
> > -  UINT32  LeftApicId;
> > -  UINT32  RightApicId;
> > -
> > -  LeftApicId = ((EFI_CPU_ID_ORDER_MAP *) Left)->ApicId;
> > -  RightApicId = ((EFI_CPU_ID_ORDER_MAP *) Right)->ApicId;
> > -
> > -  return (LeftApicId > RightApicId)? 1 : (-1); -}
> > -
> >  /**
> >    Print Cpu Apic ID Table
> >
> > @@ -116,7 +87,8 @@ DebugDisplayReOrderTable (  EFI_STATUS
> > AppendCpuMapTableEntry (
> >      IN VOID   *ApicPtr,
> > -    IN UINT32 LocalApicCounter
> > +    IN UINT32 LocalApicCounter,
> > +    IN EFI_CPU_ID_ORDER_MAP *CpuApicIdOrderTable
> >    )
> >  {
> >    EFI_STATUS    Status;
> > @@ -131,9 +103,9 @@ AppendCpuMapTableEntry (
> >
> >    if(Type == EFI_ACPI_6_3_PROCESSOR_LOCAL_APIC) {
> >      if(!mX2ApicEnabled) {
> > -      LocalApicPtr->Flags            =
> > (UINT8)mCpuApicIdOrderTable[LocalApicCounter].Flags;
> > -      LocalApicPtr->ApicId           =
> > (UINT8)mCpuApicIdOrderTable[LocalApicCounter].ApicId;
> > -      LocalApicPtr->AcpiProcessorUid =
> > (UINT8)mCpuApicIdOrderTable[LocalApicCounter].AcpiProcessorUid;
> > +      LocalApicPtr->Flags            =
> > (UINT8)CpuApicIdOrderTable[LocalApicCounter].Flags;
> > +      LocalApicPtr->ApicId           =
> > (UINT8)CpuApicIdOrderTable[LocalApicCounter].ApicId;
> > +      LocalApicPtr->AcpiProcessorUid =
> > (UINT8)CpuApicIdOrderTable[LocalApicCounter].AcpiProcessorUid;
> >      } else {
> >        LocalApicPtr->Flags            = 0;
> >        LocalApicPtr->ApicId           = 0xFF;
> > @@ -142,9 +114,9 @@ AppendCpuMapTableEntry (
> >      }
> >    } else if(Type == EFI_ACPI_6_3_PROCESSOR_LOCAL_X2APIC) {
> >      if(mX2ApicEnabled) {
> > -      LocalX2ApicPtr->Flags            =
> > (UINT8)mCpuApicIdOrderTable[LocalApicCounter].Flags;
> > -      LocalX2ApicPtr->X2ApicId         =
> > mCpuApicIdOrderTable[LocalApicCounter].ApicId;
> > -      LocalX2ApicPtr->AcpiProcessorUid =
> > mCpuApicIdOrderTable[LocalApicCounter].AcpiProcessorUid;
> > +      LocalX2ApicPtr->Flags            =
> > (UINT8)CpuApicIdOrderTable[LocalApicCounter].Flags;
> > +      LocalX2ApicPtr->X2ApicId         =
> > CpuApicIdOrderTable[LocalApicCounter].ApicId;
> > +      LocalX2ApicPtr->AcpiProcessorUid =
> > CpuApicIdOrderTable[LocalApicCounter].AcpiProcessorUid;
> >      } else {
> >        LocalX2ApicPtr->Flags            = 0;
> >        LocalX2ApicPtr->X2ApicId         = (UINT32)-1;
> > @@ -159,32 +131,25 @@ AppendCpuMapTableEntry (
> >
> >  }
> >
> > +/**
> > +  Collect all processors information and create a Cpu Apic Id table.
> > +
> > +  @param[in]  CpuApicIdOrderTable       Buffer to store information of Cpu.
> > +**/
> >  EFI_STATUS
> > -SortCpuLocalApicInTable (
> > -  VOID
> > +CreateCpuLocalApicInTable (
> > +  IN EFI_CPU_ID_ORDER_MAP *CpuApicIdOrderTable
> >    )
> >  {
> >    EFI_STATUS                                Status;
> >    EFI_PROCESSOR_INFORMATION                 ProcessorInfoBuffer;
> >    UINT32                                    Index;
> >    UINT32                                    CurrProcessor;
> > -  UINT32                                    BspApicId;
> > -  EFI_CPU_ID_ORDER_MAP                      TempVal;
> >    EFI_CPU_ID_ORDER_MAP                      *CpuIdMapPtr;
> > -  UINT32                                    CoreThreadMask;
> > -  EFI_CPU_ID_ORDER_MAP                      *TempCpuApicIdOrderTable;
> >    UINT32                                    Socket;
> >
> > -  Index      = 0;
> >    Status     = EFI_SUCCESS;
> >
> > -  if (mCpuOrderSorted) {
> > -    return Status;
> > -  }
> > -
> > -  TempCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus *
> sizeof
> > (EFI_CPU_ID_ORDER_MAP));
> > -  CoreThreadMask = (UINT32) ((1 << mNumOfBitShift) - 1);
> > -
> >    for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus;
> > CurrProcessor++, Index++) {
> >      Status = mMpService->GetProcessorInfo (
> >                             mMpService, @@ -192,7 +157,7 @@
> > SortCpuLocalApicInTable (
> >                             &ProcessorInfoBuffer
> >                             );
> >
> > -    CpuIdMapPtr = (EFI_CPU_ID_ORDER_MAP *)
> > &TempCpuApicIdOrderTable[Index];
> > +    CpuIdMapPtr = (EFI_CPU_ID_ORDER_MAP *)
> > &CpuApicIdOrderTable[Index];
> >      if ((ProcessorInfoBuffer.StatusFlag & PROCESSOR_ENABLED_BIT) != 0) {
> >        CpuIdMapPtr->ApicId  = (UINT32)ProcessorInfoBuffer.ProcessorId;
> >        CpuIdMapPtr->Thread  = ProcessorInfoBuffer.Location.Thread;
> > @@ -214,89 +179,26 @@ SortCpuLocalApicInTable (
> >      } //end if PROC ENABLE
> >    } //end for CurrentProcessor
> >
> > -  //keep for debug purpose
> > -  DEBUG ((DEBUG_INFO, "::ACPI::  APIC ID Order Table Init.
> > CoreThreadMask = %x,  mNumOfBitShift = %x\n", CoreThreadMask,
> > mNumOfBitShift));
> > -  DebugDisplayReOrderTable (TempCpuApicIdOrderTable);
> > -
> >    //
> >    // Get Bsp Apic Id
> >    //
> > -  BspApicId = GetApicId ();
> > -  DEBUG ((DEBUG_INFO, "BspApicId - 0x%x\n", BspApicId));
> > -
> > -  //
> > -  //check to see if 1st entry is BSP, if not swap it
> > -  //
> > -  if (TempCpuApicIdOrderTable[0].ApicId != BspApicId) {
> > -    for (Index = 0; Index < mNumberOfCpus; Index++) {
> > -      if ((TempCpuApicIdOrderTable[Index].Flags == 1) &&
> > (TempCpuApicIdOrderTable[Index].ApicId == BspApicId)) {
> > -        CopyMem (&TempVal, &TempCpuApicIdOrderTable[Index], sizeof
> > (EFI_CPU_ID_ORDER_MAP));
> > -        CopyMem (&TempCpuApicIdOrderTable[Index],
> > &TempCpuApicIdOrderTable[0], sizeof (EFI_CPU_ID_ORDER_MAP));
> > -        CopyMem (&TempCpuApicIdOrderTable[0], &TempVal, sizeof
> > (EFI_CPU_ID_ORDER_MAP));
> > -        break;
> > -      }
> > -    }
> > -
> > -    if (mNumberOfCpus <= Index) {
> > -      DEBUG ((DEBUG_ERROR, "Asserting the SortCpuLocalApicInTable Index
> > Bufferflow\n"));
> > -      return EFI_INVALID_PARAMETER;
> > -    }
> > -  }
> > -
> > -  //
> > -  // 1. Sort TempCpuApicIdOrderTable,
> > -  //    sort it by using ApicId from minimum to maximum (Socket0 to
> SocketN),
> > and the BSP must in the fist location of the table.
> > -  //    So, start sorting the table from the second element and total
> elements
> > are mNumberOfCpus-1.
> > -  //
> > -  PerformQuickSort ((TempCpuApicIdOrderTable + 1), (mNumberOfCpus -
> > 1), sizeof (EFI_CPU_ID_ORDER_MAP), (SORT_COMPARE)
> > ApicIdCompareFunction);
> > -
> > -  //
> > -  // 2. Sort and map the primary threads to the front of the
> > CpuApicIdOrderTable
> > -  //
> > -  for (CurrProcessor = 0, Index = 0; Index < mNumberOfCpus; Index++) {
> > -    if ((TempCpuApicIdOrderTable[Index].Thread) == 0) { // primary thread
> > -      CopyMem (&mCpuApicIdOrderTable[CurrProcessor],
> > &TempCpuApicIdOrderTable[Index], sizeof (EFI_CPU_ID_ORDER_MAP));
> > -      CurrProcessor++;
> > -    }
> > -  }
> > +  DEBUG ((DEBUG_INFO, "BspApicId - 0x%x\n", GetApicId ()));
> >
> > -  //
> > -  // 3. Sort and map the second threads to the middle of the
> > CpuApicIdOrderTable
> > -  //
> > -  for (Index = 0; Index < mNumberOfCpus; Index++) {
> > -    if ((TempCpuApicIdOrderTable[Index].Thread) == 1) { //second thread
> > -      CopyMem (&mCpuApicIdOrderTable[CurrProcessor],
> > &TempCpuApicIdOrderTable[Index], sizeof (EFI_CPU_ID_ORDER_MAP));
> > -      CurrProcessor++;
> > -    }
> > -  }
> >
> >    //
> > -  // 4. Sort and map the not enabled threads to the bottom of the
> > CpuApicIdOrderTable
> > -  //
> > -  for (Index = 0; Index < mNumberOfCpus; Index++) {
> > -    if (TempCpuApicIdOrderTable[Index].Flags == 0) { // not enabled
> > -      CopyMem (&mCpuApicIdOrderTable[CurrProcessor],
> > &TempCpuApicIdOrderTable[Index], sizeof (EFI_CPU_ID_ORDER_MAP));
> > -      CurrProcessor++;
> > -    }
> > -  }
> > -
> > -  //
> > -  // 5. Re-assign AcpiProcessorId for AcpiProcessorUid uses purpose.
> > +  // Fill in AcpiProcessorUid.
> >    //
> >    for (Socket = 0; Socket < FixedPcdGet32 (PcdMaxCpuSocketCount);
> > Socket++) {
> >      for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus;
> > CurrProcessor++) {
> > -      if (mCpuApicIdOrderTable[CurrProcessor].Flags &&
> > (mCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
> > -        mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid =
> > (mCpuApicIdOrderTable[CurrProcessor].SocketNum << mNumOfBitShift) +
> > Index;
> > +      if (CpuApicIdOrderTable[CurrProcessor].Flags &&
> > (CpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
> > +        CpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid =
> > (CpuApicIdOrderTable[CurrProcessor].SocketNum << mNumOfBitShift) +
> > Index;
> >          Index++;
> >        }
> >      }
> >    }
> >
> > -  //keep for debug purpose
> > -  DEBUG ((DEBUG_INFO, "APIC ID Order Table ReOrdered\n"));
> > -  DebugDisplayReOrderTable (mCpuApicIdOrderTable);
> > -
> > -  mCpuOrderSorted = TRUE;
> > +  DEBUG ((DEBUG_INFO, "::ACPI::  APIC ID Order Table Init.
> > mNumOfBitShift = %x\n", mNumOfBitShift));
> > +  DebugDisplayReOrderTable (CpuApicIdOrderTable);
> >
> >    return Status;
> >  }
> > @@ -750,6 +652,7 @@ InstallMadtFromScratch (
> >    EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE               LocalApciNmiStruct;
> >    EFI_ACPI_6_3_PROCESSOR_LOCAL_X2APIC_STRUCTURE
> > ProcLocalX2ApicStruct;
> >    EFI_ACPI_6_3_LOCAL_X2APIC_NMI_STRUCTURE
> > LocalX2ApicNmiStruct;
> > +  EFI_CPU_ID_ORDER_MAP                                *CpuApicIdOrderTable;
> >    STRUCTURE_HEADER                                    **MadtStructs;
> >    UINTN                                               MaxMadtStructCount;
> >    UINTN                                               MadtStructsIndex;
> > @@ -760,18 +663,19 @@ InstallMadtFromScratch (
> >
> >    MadtStructs = NULL;
> >    NewMadtTable = NULL;
> > +  CpuApicIdOrderTable = NULL;
> >    MaxMadtStructCount = 0;
> >
> > -  mCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof
> > (EFI_CPU_ID_ORDER_MAP));
> > -  if (mCpuApicIdOrderTable == NULL) {
> > -    DEBUG ((DEBUG_ERROR, "Could not allocate mCpuApicIdOrderTable
> > structure pointer array\n"));
> > +  CpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof
> > (EFI_CPU_ID_ORDER_MAP));
> > +  if (CpuApicIdOrderTable == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "Could not allocate CpuApicIdOrderTable
> > structure pointer array\n"));
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> >
> >    // Call for Local APIC ID Reorder
> > -  Status = SortCpuLocalApicInTable ();
> > +  Status = CreateCpuLocalApicInTable (CpuApicIdOrderTable);
> >    if (EFI_ERROR (Status)) {
> > -    DEBUG ((DEBUG_ERROR, "SortCpuLocalApicInTable failed: %r\n",
> Status));
> > +    DEBUG ((DEBUG_ERROR, "CreateCpuLocalApicInTable failed: %r\n",
> > Status));
> >      goto Done;
> >    }
> >
> > @@ -824,10 +728,10 @@ InstallMadtFromScratch (
> >      // APIC ID as a UINT8, use a processor local APIC structure. Otherwise,
> >      // use a processor local x2APIC structure.
> >      //
> > -    if (!mX2ApicEnabled && mCpuApicIdOrderTable[Index].ApicId <
> > MAX_UINT8) {
> > -      ProcLocalApicStruct.Flags            = (UINT8)
> > mCpuApicIdOrderTable[Index].Flags;
> > -      ProcLocalApicStruct.ApicId           = (UINT8)
> > mCpuApicIdOrderTable[Index].ApicId;
> > -      ProcLocalApicStruct.AcpiProcessorUid = (UINT8)
> > mCpuApicIdOrderTable[Index].AcpiProcessorUid;
> > +    if (!mX2ApicEnabled && CpuApicIdOrderTable[Index].ApicId <
> > MAX_UINT8) {
> > +      ProcLocalApicStruct.Flags            = (UINT8)
> > CpuApicIdOrderTable[Index].Flags;
> > +      ProcLocalApicStruct.ApicId           = (UINT8)
> > CpuApicIdOrderTable[Index].ApicId;
> > +      ProcLocalApicStruct.AcpiProcessorUid = (UINT8)
> > CpuApicIdOrderTable[Index].AcpiProcessorUid;
> >
> >        ASSERT (MadtStructsIndex < MaxMadtStructCount);
> >        Status = CopyStructure (
> > @@ -835,10 +739,10 @@ InstallMadtFromScratch (
> >                   (STRUCTURE_HEADER *) &ProcLocalApicStruct,
> >                   &MadtStructs[MadtStructsIndex++]
> >                   );
> > -    } else if (mCpuApicIdOrderTable[Index].ApicId != 0xFFFFFFFF) {
> > -      ProcLocalX2ApicStruct.Flags            = (UINT8)
> > mCpuApicIdOrderTable[Index].Flags;
> > -      ProcLocalX2ApicStruct.X2ApicId         =
> > mCpuApicIdOrderTable[Index].ApicId;
> > -      ProcLocalX2ApicStruct.AcpiProcessorUid =
> > mCpuApicIdOrderTable[Index].AcpiProcessorUid;
> > +    } else if (CpuApicIdOrderTable[Index].ApicId != 0xFFFFFFFF) {
> > +      ProcLocalX2ApicStruct.Flags            = (UINT8)
> > CpuApicIdOrderTable[Index].Flags;
> > +      ProcLocalX2ApicStruct.X2ApicId         =
> > CpuApicIdOrderTable[Index].ApicId;
> > +      ProcLocalX2ApicStruct.AcpiProcessorUid =
> > CpuApicIdOrderTable[Index].AcpiProcessorUid;
> >
> >        ASSERT (MadtStructsIndex < MaxMadtStructCount);
> >        Status = CopyStructure (
> > @@ -1033,8 +937,8 @@ Done:
> >      FreePool (NewMadtTable);
> >    }
> >
> > -  if (mCpuApicIdOrderTable != NULL) {
> > -    FreePool (mCpuApicIdOrderTable);
> > +  if (CpuApicIdOrderTable != NULL) {
> > +    FreePool (CpuApicIdOrderTable);
> >    }
> >
> >    return Status;
> > --
> > 2.32.0.windows.2