Re: [edk2-platforms: PATCH V8] Platform/Intel: Correct CPU APIC IDs

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

Hi Ray,

Thanks for recommendations.

For item5, after referencing the log from server platform.
I think it is fine to keep original coding.

For item1 to item4, all of them are optimized.

I will re-sent the code patch.

Thank you.
Jack

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Thursday, August 19, 2021 9:23 PM
To: Lin, JackX <jackx.lin@intel.com>; devel@edk2.groups.io
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>; Kuo, Donald <donald.kuo@intel.com>; Kumar, Chandana C <chandana.c.kumar@intel.com>
Subject: RE: [edk2-platforms: PATCH V8] Platform/Intel: Correct CPU APIC IDs

> +      CpuIdMapPtr->AcpiProcessorId = 
> + (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) +
> (UINT32)ProcessorInfoBuffer.ProcessorId;

1. I don't understand this assignment. Since you will re-assign this later, why not remove this assignment?


> +
> +  //make sure 1st entry is BSP
> +  if (mX2ApicEnabled) {
> +    BspApicId = (UINT32)AsmReadMsr64 (0x802);  } else {
> +    BspApicId = (*(volatile UINT32 *)(UINTN)0xFEE00020) >> 24;  }  
> + DEBUG ((DEBUG_INFO, "BspApicId - 0x%x\n", BspApicId));

2. you can use GetApicId() API from LocalApicLib library in UefiCpuPkg.


> -    if(mCpuApicIdOrderTable[0].ApicId != BspApicId) {
> -      //check to see if 1st entry is BSP, if not swap it
> -      Index = ApicId2SwProcApicId(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));
> +    TempCpuApicIdOrderTable[0].Flags = 1;

3. Why is " TempCpuApicIdOrderTable[0].Flags = 1;" needed? 

> +  for (CurrProcessor = 1; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
> +    for (Index = CurrProcessor+1; Index < mNumberOfCpus; Index++) {
> +      if (TempCpuApicIdOrderTable[CurrProcessor].AcpiProcessorId > TempCpuApicIdOrderTable[Index].AcpiProcessorId) {
> +        CopyMem (&TempVal, &TempCpuApicIdOrderTable[CurrProcessor], sizeof (EFI_CPU_ID_ORDER_MAP));
> +        CopyMem (&TempCpuApicIdOrderTable[CurrProcessor], 
> + &TempCpuApicIdOrderTable[Index], sizeof
> (EFI_CPU_ID_ORDER_MAP));
> +        CopyMem (&TempCpuApicIdOrderTable[Index], &TempVal, sizeof 
> + (EFI_CPU_ID_ORDER_MAP));
>        }
> +    }
> +  }

4. Can you use PerformQuickSort() API from SortLib in MdeModulePkg?



> +
> +  //
> +  // 5. Re-assigen AcpiProcessorId for AcpiProcessorUId uses purpose.
> +  //
> +  for (Socket = 0; Socket < MAX_SOCKET; Socket++) {
> +    for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
> +      if (mCpuApicIdOrderTable[CurrProcessor].Flags && (mCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
> +        mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorId = 
> + (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) +
> Index;
> +        Index++;

5. I think you need a AcpiProcessorId[MAX_SOCKET] array to remember the "AcpiProcessorId" for each socket/package.
For example:
AcpiProcessorId[MAX_SOCKET];
ZeroMem (AcpiProcessorId);
for (Socket = 0; ...) {
  for (CurrProcessor = 0; ...) {
    if (...Flags == 1 && ...SocketNum == Socket) {
      mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorId = (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) + AcpiProcessorId[Socket];
      AcpiProcessorId[Socket]++;