Hi Ray,
Sure, I will complete all these below.
Thank you.
Jack
-----Original Message-----
From: Ni, Ray <ray...@intel.com>
Sent: Friday, August 5, 2022 2:43 PM
To: Lin, JackX <jackx....@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.c...@intel.com>; Dong, Eric <eric.d...@intel.com>;
Yao, Jiewen <jiewen....@intel.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@intel.com>; Kuo, Donald <donald....@intel.com>; Kumar,
Chandana C <chandana.c.ku...@intel.com>; Palakshareddy, Lavanya C
<lavanya.c.palakshare...@intel.com>; Palakshareddy, Lavanya C
<lavanya.c.palakshare...@intel.com>
Subject: RE: [edk2-platforms:PATCH] Modify processor _UID ordering by CPU
default fused in MADT
Jack,
The patch removes all the sorting logic. It simplifies the logic a lot. Thanks
for that!
ACPI spec " 5.2.12.1 MADT Processor Local APIC / SAPIC Structure Entry Order"
talked about why the BSP needs to be in the first entry of MADT and why first
logical processor of each core needs to be before second logical processor.
With the reasons, I totally agree that we don't need to sort the MADT any more
after the hyper-threading and many-core support have been enabled for quite a
long time in OS.
Some minor comments:
1. Can you check if "CoreThreadMask" can be removed?
2. Can you please rename the SortCpuLocalApicInTable? Maybe just use
"CreateCpuLocalApicInTable"? There is no sorting any more😊
3. Can you please check if " mCpuOrderSorted" is needed? It's needed when the
function is called multiple times.
4. If it's needed, can you please rename it to "mCpuLocalApicInTableCreated"?
5. If it's not needed, can you please think about if " mCpuApicIdOrderTable"
can be changed to a local variable?
Thanks,
Ray
> -----Original Message-----
> From: Lin, JackX <jackx....@intel.com>
> Sent: Thursday, July 28, 2022 3:25 PM
> To: devel@edk2.groups.io
> Cc: Lin, JackX <jackx....@intel.com>; Lin, JackX
> <jackx....@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; Dong,
> Eric <eric.d...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Ni,
> Ray <ray...@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaga...@intel.com>; Kuo, Donald <donald....@intel.com>;
> Kumar, Chandana C <chandana.c.ku...@intel.com>; Palakshareddy;
> Palakshareddy, Lavanya C <lavanya.c.palakshare...@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....@intel.com>
> Cc: Chasel Chiu <chasel.c...@intel.com>
> Cc: Dong Eric <eric.d...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com>
> Cc: Donald Kuo <donald....@intel.com>
> Cc: Chandana C Kumar <chandana.c.ku...@intel.com>
> Cc: Palakshareddy, Lavanya C <lavanya.c.palakshare...@intel.com>
> Cc: JackX Lin <jackx....@intel.com>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 99
> ++++---
> ----------------------------------------------------------------------
> ----------------------
> 1 file changed, 4 insertions(+), 95 deletions(-)
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index c7e87cbd7d..d0e8891918 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -62,33 +62,6 @@ 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
>
> @@ -168,21 +141,16 @@ SortCpuLocalApicInTable (
> 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++) {
> @@ -192,7 +160,7 @@ SortCpuLocalApicInTable (
> &ProcessorInfoBuffer
> );
>
> - CpuIdMapPtr = (EFI_CPU_ID_ORDER_MAP *)
> &TempCpuApicIdOrderTable[Index];
> + CpuIdMapPtr = (EFI_CPU_ID_ORDER_MAP *)
> &mCpuApicIdOrderTable[Index];
> if ((ProcessorInfoBuffer.StatusFlag & PROCESSOR_ENABLED_BIT) != 0) {
> CpuIdMapPtr->ApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
> CpuIdMapPtr->Thread = ProcessorInfoBuffer.Location.Thread;
> @@ -214,74 +182,16 @@ 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);
> + DEBUG ((DEBUG_INFO, "BspApicId - 0x%x\n", GetApicId ()));
>
> - //
> - // 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++;
> - }
> - }
>
> //
> - // 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++) {
> @@ -292,8 +202,7 @@ SortCpuLocalApicInTable (
> }
> }
>
> - //keep for debug purpose
> - DEBUG ((DEBUG_INFO, "APIC ID Order Table ReOrdered\n"));
> + DEBUG ((DEBUG_INFO, "::ACPI:: APIC ID Order Table Init.
> CoreThreadMask = %x, mNumOfBitShift = %x\n", CoreThreadMask,
> mNumOfBitShift));
> DebugDisplayReOrderTable (mCpuApicIdOrderTable);
>
> mCpuOrderSorted = TRUE;
> --
> 2.32.0.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92192): https://edk2.groups.io/g/devel/message/92192
Mute This Topic: https://groups.io/mt/92666022/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
