Re: [edk2-devel][edk2-platforms][PATCH V2 1/1] MinPlatformPkg: AcpiTables: Add additional thread mapping in MADT

["Oram, Isaac W" <isaac.w.oram@intel.com>]

Generally, convention for "Pkg-Module" is "Pkg/Module: " not "Pkg: Module: ".  Kind of a nit, but using : as separator will make parsing tools harder to write.

Convention for comments is "no newline between comment block and code block" is that the comment applies to that block.  A blank line indicates the comment applies to multiple code blocks.

Please fix the comments to match the new code.  Prior comments made sense; first, middle, last.  Second, third, and fourth threads "in the middle" makes less sense.  I think explaining why we are grouping into the order we are is perhaps important.  There is nothing explaining why this ordering and why third and fourth are being added when they appear to have been previously ignored.  That info may belong in the commit message if not appropriate for the comments.

Also note that your author name format settings need to be modified such that the author field in the log is: Ankit Sinha <ankit.sinha@intel.com>

Regards,
Isaac

-----Original Message-----
From: Sinha, Ankit <ankit.sinha@intel.com> 
Sent: Wednesday, July 27, 2022 12:13 PM
To: devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel][edk2-platforms][PATCH V2 1/1] MinPlatformPkg: AcpiTables: Add additional thread mapping in MADT

Adding second, third and fourth thread mapping to MADT

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Ankit Sinha <ankit.sinha@intel.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index c7e87cbd7d9d..524f9914b0b1 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -261,7 +261,7 @@ SortCpuLocalApicInTable (
   }
 
   //
-  // 3. Sort and map the second threads to the middle of the CpuApicIdOrderTable
+  // 3. Sort and map the second, third and fourth threads to the middle 
+ of the CpuApicIdOrderTable
   //
   for (Index = 0; Index < mNumberOfCpus; Index++) {
     if ((TempCpuApicIdOrderTable[Index].Thread) == 1) { //second thread @@ -270,6 +270,20 @@ SortCpuLocalApicInTable (
     }
   }
 
+  for (Index = 0; Index < mNumberOfCpus; Index++) {
+    if ((TempCpuApicIdOrderTable[Index].Thread) == 2) { // third thread
+      CopyMem (&mCpuApicIdOrderTable[CurrProcessor], &TempCpuApicIdOrderTable[Index], sizeof (EFI_CPU_ID_ORDER_MAP));
+      CurrProcessor++;
+    }
+  }
+
+  for (Index = 0; Index < mNumberOfCpus; Index++) {
+    if ((TempCpuApicIdOrderTable[Index].Thread) == 3) { // fourth 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
   //
--
2.27.0.windows.1