Re: [edk2-platforms] [PATCH 1/1] Platform/Sgi: Add support to disable isolated cpus


Nishant Sharma
 

Hi Sami,
Please find my reply inline

On Thu, Jul 21, 2022 at 12:47 PM, Sami Mujawar wrote:

Hi Nishant,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 17/06/2022 07:07 am, Nishant Sharma wrote:
Isolated CPUs are those that are not to be used on the platform for
various reasons. The isolated CPU list is an array of MPID values of
[SAMI] Can you explain the use-case/reason, please?
[Nishant]: I will update in the next patchset.
the CPUs that have to be isolated. This list is supplied via the
NT_FW_CONFIG dtb.

Add support to search for isolated CPUs MPID list and, if present,
update the MADT table to disable the corresponding CPUs.

Signed-off-by: Nishant Sharma <nishant.sharma@...>
---
 Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf         |   1 -
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |   8 +-
 Platform/ARM/SgiPkg/Include/SgiPlatform.h                     |   7 ++
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c         | 131 +++++++++++++++++++-
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  |  45 ++++++-
 5 files changed, 186 insertions(+), 6 deletions(-)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
index 4b36c3e5ceb2..e13c2f08ce6e 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
@@ -18,7 +18,6 @@
   Dbg2.aslc

   Fadt.aslc

   Gtdt.aslc

-  Iort.aslc
[SAMI] Why is IORT table being removed here?
[Nishant]: I think some issue with patch generation. I will remove this change in the next patch. Thanks for pointing it out.
   Mcfg.aslc

   RdN2Cfg1/Dsdt.asl

   RdN2Cfg1/Madt.aslc

diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
index 407160c07563..fbf061ad3bdb 100644
--- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
@@ -1,5 +1,5 @@
 #

-#  Copyright (c) 2018, ARM Limited. All rights reserved.

+#  Copyright (c) 2018-2022, ARM Limited. All rights reserved.

 #

 #  SPDX-License-Identifier: BSD-2-Clause-Patent

 #

@@ -13,6 +13,7 @@
   ENTRY_POINT                    = SgiPlatformPeim

 

 [Packages]

+  ArmPlatformPkg/ArmPlatformPkg.dec

   EmbeddedPkg/EmbeddedPkg.dec

   MdePkg/MdePkg.dec

   Platform/ARM/SgiPkg/SgiPlatform.dec

@@ -21,6 +22,11 @@
   FdtLib

   PeimEntryPoint

 

+[FixedPcd]

+  gArmSgiTokenSpaceGuid.PcdChipCount

+  gArmPlatformTokenSpaceGuid.PcdCoreCount

+  gArmPlatformTokenSpaceGuid.PcdClusterCount

+

 [Sources]

   SgiPlatformPeim.c

 

diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
index dddb58832d73..311286ce5337 100644
--- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
+++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
@@ -65,11 +65,18 @@
 #define DRAM_BLOCK2_BASE_REMOTE(ChipId) \

           (SGI_REMOTE_CHIP_MEM_OFFSET (ChipId) + FixedPcdGet64 (PcdDramBlock2Base))

 

+// List of isolated CPUs MPID

+typedef struct {

+  UINT64  Count;                // Number of elements present in the list

+  UINT64  Mpid[];               // List containing isolated CPU MPIDs

+} SGI_ISOLATED_CPU_LIST;

+

 // ARM platform description data.

 typedef struct {

   UINTN  PlatformId;

   UINTN  ConfigId;

   UINTN  MultiChipMode;

+  SGI_ISOLATED_CPU_LIST  IsolatedCpuList;

 } SGI_PLATFORM_DESCRIPTOR;

 

 // Arm SGI/RD Product IDs

diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
index 2f72e7152ff3..80190120ff32 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
@@ -1,14 +1,17 @@
 /** @file

 *

-*  Copyright (c) 2018, ARM Limited. All rights reserved.

+*  Copyright (c) 2018 - 2022, ARM Limited. All rights reserved.

 *

 *  SPDX-License-Identifier: BSD-2-Clause-Patent

 *

 **/

 

+#include <IndustryStandard/Acpi.h>

+

 #include <Library/AcpiLib.h>

 #include <Library/DebugLib.h>

 #include <Library/HobLib.h>

+#include <Library/UefiBootServicesTableLib.h>

 #include <SgiPlatform.h>

 

 VOID

@@ -16,6 +19,127 @@ InitVirtioDevices (
   VOID

   );

 

+/**

+  Search for a MPID in a list

+

+  Performs a linear search for a specified MPID on the given linear

+  list of MPIDs.

+

+  @param[in]  MpidList  Pointer to list.

+  @param[in]  Count     Number of the elements in the list.

+  @param[in]  Mpid      Target MPID.

+

+  @retval TRUE   MPID is present.

+  @retval FALSE  MPID is not present.

+**/

+STATIC

+BOOLEAN

+CheckIfMpidIsPresent (

+  IN UINT64  *MpidList,

+  IN UINT64  Count,

+  IN UINT64  Mpid

+  )

+{

+  UINT64 Idx;

+

+  for (Idx = 0; Idx < Count; Idx++) {

+    if (MpidList[Idx] == Mpid) {

+      return TRUE;

+    }

+  }

+

+  return FALSE;

+}

+

+/**

+  Disables isolated CPUs in the MADT table

+

+  Parse the IsolatedCpuInfo from the Hob list and updates the MADT table to
[SAMI] Nit.  updates -> update
[Nishant] Will update in next patch version.
+  disable cpu's which are not available on the platfrom.

+

+  @param[in] AcpiHeader  Points to the Madt table.

+  @param[in] HobData     Points to the unusable cpuinfo in hoblist.

+**/

+STATIC

+VOID

+UpdateMadtTable (

+  IN EFI_ACPI_DESCRIPTION_HEADER  *AcpiHeader,

+  IN SGI_PLATFORM_DESCRIPTOR      *HobData

+  )

+{

+  UINT8 *StructureListHead;

+  UINT8 *StructureListTail;

+  EFI_ACPI_6_4_GIC_STRUCTURE *GicStructure;

+  BOOLEAN MpidPresent;

+

+  if (HobData->IsolatedCpuList.Count == 0) {

+    return;

+  }

+

+  StructureListHead =

+    ((UINT8 *)AcpiHeader) +

+    sizeof(EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER);

+  StructureListTail = (UINT8 *)AcpiHeader + AcpiHeader->Length;

+

+  // Locate ACPI GICC structure in the MADT table.

+  while (StructureListHead < StructureListTail) {

+    if (StructureListHead[0] == EFI_ACPI_6_4_GIC) {

[SAMI] This is definitely not the way to parse an ACPI table. Please dont do this.

Also, why are you not using DynamicTables framework? It is designed to handle such cases.

[/SAMI]

[Nishant]
Could you please add more details on what is wrong with this approach?
Please point me to the documentation or code that has a standardised way of updating the ACPI table.

Regarding the use of the Dynamic Table Framework, there are no short-term plans to migrate to it.
For the use of dynamic table 
[/Nishant]

+      GicStructure = (EFI_ACPI_6_4_GIC_STRUCTURE *)StructureListHead;

+      // Disable the CPU if its MPID is present in the list.

+      MpidPresent = CheckIfMpidIsPresent(

+        HobData->IsolatedCpuList.Mpid,

+        HobData->IsolatedCpuList.Count,

+        GicStructure->MPIDR

+        );

+      if (MpidPresent == TRUE) {

+        DEBUG ((

+          DEBUG_INFO,

+          "Disabling Core: %lu, MPID: 0x%llx in MADT\n",

+          GicStructure->AcpiProcessorUid,

+          GicStructure->MPIDR

+          ));

+        GicStructure->Flags = 0;

+      }

+    }

+

+    // Second element in the structure component header is length

+    StructureListHead += StructureListHead[1];

+  }

+}

+

+/**

+  Callback to validate and/or update ACPI table.

+

+  On finding a MADT table, disable the isolated CPUs in the MADT table. The

+  list of isolated CPUs are obtained from the HOB data.

+

+  @param[in] AcpiHeader  Target ACPI table.

+

+  @retval  TURE   Table validated/updated successfully.

+  @retval  FALSE  Error in Table validation/updation.

+**/

+STATIC

+BOOLEAN

+CheckAndUpdateAcpiTable (

+  IN EFI_ACPI_DESCRIPTION_HEADER  *AcpiHeader

+  )

+{

+  VOID *SystemIdHob;

+  SGI_PLATFORM_DESCRIPTOR *HobData;

+

+  // This check updates the MADT table to disable isolated CPUs present on the

+  // platform.

+  if (AcpiHeader->Signature == EFI_ACPI_1_0_APIC_SIGNATURE) {
[SAMI] Why EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE is not used here?
[Nishant] I will update in the next version.
+    SystemIdHob = GetFirstGuidHob (&gArmSgiPlatformIdDescriptorGuid);

+    if (SystemIdHob != NULL) {

+      HobData = (SGI_PLATFORM_DESCRIPTOR *)GET_GUID_HOB_DATA (SystemIdHob);

+      UpdateMadtTable (AcpiHeader, HobData);

+    }

+  }

+

+  return TRUE;

+}

+

 EFI_STATUS

 EFIAPI

 ArmSgiPkgEntryPoint (

@@ -25,7 +149,10 @@ ArmSgiPkgEntryPoint (
 {

   EFI_STATUS              Status;

 

-  Status = LocateAndInstallAcpiFromFv (&gArmSgiAcpiTablesGuid);

+  Status = LocateAndInstallAcpiFromFvConditional (

+             &gArmSgiAcpiTablesGuid,

+             &CheckAndUpdateAcpiTable

+             );

   if (EFI_ERROR (Status)) {

     DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));

     return Status;

diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
index 7df52cc4fd7c..f778dc8ac7c1 100644
--- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
@@ -1,6 +1,6 @@
 /** @file

 *

-*  Copyright (c) 2018, ARM Limited. All rights reserved.

+*  Copyright (c) 2018-2022, ARM Limited. All rights reserved.

 *

 *  SPDX-License-Identifier: BSD-2-Clause-Patent

 *

@@ -38,6 +38,8 @@ GetSgiSystemId (
   CONST VOID                    *NtFwCfgDtBlob;

   SGI_NT_FW_CONFIG_INFO_PPI     *NtFwConfigInfoPpi;

   EFI_STATUS                    Status;

+  UINT64                        IsolatedCpuCount;

+  UINT64                        CoreCount;

 

   Status = PeiServicesLocatePpi (&gNtFwConfigDtInfoPpiGuid, 0, NULL,

              (VOID**)&NtFwConfigInfoPpi);

@@ -83,6 +85,32 @@ GetSgiSystemId (
     HobData->MultiChipMode = fdt32_to_cpu (*Property);

   }

 

+  Property = fdt_getprop (NtFwCfgDtBlob, Offset, "isolated-cpu-list", NULL);

+  if (Property == NULL) {

+    DEBUG ((DEBUG_INFO, "%s property not found\n", "isolated-cpu-list"));

+    HobData->IsolatedCpuList.Count = 0;

+  } else {

+    CopyMem (&IsolatedCpuCount, Property, sizeof (IsolatedCpuCount));

+    CoreCount =

+      FixedPcdGet32 (PcdChipCount) *

+      FixedPcdGet32 (PcdClusterCount) *

+      FixedPcdGet32 (PcdCoreCount);

+    if (IsolatedCpuCount > CoreCount) {

+      DEBUG ((

+            DEBUG_ERROR,

+            "IsolatedCpuCount(%u) is higher than CoreCount(%u)\n",

+            IsolatedCpuCount,

+            CoreCount

+            ));

+      return EFI_SUCCESS;
[SAMI] Is the status code returned here correct? Should this be EFI_INVALID_PARAMETER? Also the function name GetSgiSystemId() seems to no longer reflect what the function does. Hace you considered renaming it.
[Nishant]
This is done intentionally, we want to keep booting even if the config provided is corrupted.

I will update the function name in the next version.
[/Nishant]
+    }

+    CopyMem (

+      &HobData->IsolatedCpuList,

+      Property,

+      sizeof(HobData->IsolatedCpuList) + (CoreCount * sizeof(UINT64))
[SAMI] Coding convention is not followed here and at other places. Can you fix, please?
[Nishant] Will update in the next patch version.
+      );

+  }

+

   return EFI_SUCCESS;

 }

 

@@ -104,11 +132,24 @@ SgiPlatformPeim (
 {

   SGI_PLATFORM_DESCRIPTOR       *HobData;

   EFI_STATUS                    Status;

+  UINT64                        CoreCount;

+  UINTN                         HobSize;

 

+  CoreCount =

+    FixedPcdGet32 (PcdChipCount) *

+    FixedPcdGet32 (PcdClusterCount) *

+    FixedPcdGet32 (PcdCoreCount);

+

+  // Additional size for SGI_ISOLATED_CPU_LIST.

+  // Size = (MPID register size in bytes * CoreCount) +

+  //        sizeof(SGI_PLATFORM_DESCRIPTOR)

+  HobSize =

+    sizeof (SGI_PLATFORM_DESCRIPTOR) +

+    (CoreCount * sizeof(UINT64));

   // Create platform descriptor HOB

   HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (

                                          &gArmSgiPlatformIdDescriptorGuid,

-                                         sizeof (SGI_PLATFORM_DESCRIPTOR));

+                                         HobSize);

 

   // Get the system id from the platform specific nt_fw_config device tree

   if (HobData == NULL) {

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Join devel@edk2.groups.io to automatically receive all group messages.