[edk2-devel] [edk2-platforms:PATCH] Modify processor _UID ordering by CPU default fused in MADT
JackX Lin
JackX.Lin at intel.com
Mon Aug 8 08:19:47 UTC 2022
Hi Ray,
Sure, I will complete all these below.
Thank you.
Jack
-----Original Message-----
From: Ni, Ray <ray.ni at intel.com>
Sent: Friday, August 5, 2022 2:43 PM
To: Lin, JackX <jackx.lin at intel.com>; devel at edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu at intel.com>; Dong, Eric <eric.dong at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>; Kuo, Donald <donald.kuo at intel.com>; Kumar, Chandana C <chandana.c.kumar at intel.com>; Palakshareddy, Lavanya C <lavanya.c.palakshareddy at intel.com>; Palakshareddy, Lavanya C <lavanya.c.palakshareddy at 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.lin at intel.com>
> Sent: Thursday, July 28, 2022 3:25 PM
> To: devel at edk2.groups.io
> Cc: Lin, JackX <jackx.lin at intel.com>; Lin, JackX
> <jackx.lin at intel.com>; Chiu, Chasel <chasel.chiu at intel.com>; Dong,
> Eric <eric.dong at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Ni,
> Ray <ray.ni at intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty at intel.com>; Kuo, Donald <donald.kuo at intel.com>;
> Kumar, Chandana C <chandana.c.kumar at intel.com>; Palakshareddy;
> Palakshareddy, Lavanya C <lavanya.c.palakshareddy at 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 at intel.com>
> Cc: Chasel Chiu <chasel.chiu at intel.com>
> Cc: Dong Eric <eric.dong at intel.com>
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty at intel.com>
> Cc: Donald Kuo <Donald.Kuo at intel.com>
> Cc: Chandana C Kumar <chandana.c.kumar at intel.com>
> Cc: Palakshareddy, Lavanya C <lavanya.c.palakshareddy at intel.com>
> Cc: JackX Lin <JackX.Lin at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list