From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [edk2-platforms] [PATCH 1/1] Platform/Sgi: Add support to disable isolated cpus To: Sami Mujawar ,devel@edk2.groups.io From: "Nishant Sharma" X-Originating-Location: Cambridge, England, GB (217.140.106.13) X-Originating-Platform: Windows Chrome 103 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Wed, 27 Jul 2022 06:53:50 -0700 References: <518c6649-c447-cb9d-7664-60d96059b43c@arm.com> In-Reply-To: <518c6649-c447-cb9d-7664-60d96059b43c@arm.com> Message-ID: <25832.1658930030427598465@groups.io> Content-Type: multipart/alternative; boundary="6W3hMkb2obr66dTu6Wb8" --6W3hMkb2obr66dTu6Wb8 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Sami, Please find my reply inline On Thu, Jul 21, 2022 at 12:47 PM, Sami Mujawar wrote: >=20 >=20 >=20 > Hi Nishant, >=20 >=20 >=20 > Please find my response inline marked [SAMI]. >=20 >=20 >=20 > Regards, >=20 >=20 >=20 > Sami Mujawar >=20 > On 17/06/2022 07:07 am, Nishant Sharma wrote: >=20 >> 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 >=20 > [SAMI] Can you explain the use-case/reason, please? [Nishant]: I will update in the next patchset. >=20 >=20 >> the CPUs that have to be isolated. This list is supplied via the >> NT_FW_CONFIG dtb. >>=20 >> Add support to search for isolated CPUs MPID list and, >> if present, >> update the MADT table to disable the corresponding CPUs. >>=20 >> Signed-off-by: Nishant Sharma ( >> nishant.sharma@arm.com ) >> --- >>=20 >> Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf | 1 - >>=20 >> Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf | 8 +- >>=20 >> Platform/ARM/SgiPkg/Include/SgiPlatform.h | 7 ++ >>=20 >> Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c | 131 >> +++++++++++++++++++- >>=20 >> Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c | 45 >> ++++++- >> 5 files changed, 186 insertions(+), 6 deletions(-) >>=20 >> 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 @@ >>=20 >> Dbg2.aslc >>=20 >> Fadt.aslc >>=20 >> Gtdt.aslc >>=20 >> - Iort.aslc >=20 > [SAMI] Why is IORT table being removed here? [Nishant]: I think some issue with patch generation. I will remove this cha= nge in the next patch. Thanks for pointing it out. >=20 >=20 >> Mcfg.aslc >>=20 >> RdN2Cfg1/Dsdt.asl >>=20 >> RdN2Cfg1/Madt.aslc >>=20 >> 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 @@ >> # >>=20 >> -# Copyright (c) 2018, ARM Limited. All rights reserved. >>=20 >> +#=20 >> Copyright (c) 2018-2022, ARM Limited. All rights reserved. >>=20 >> # >>=20 >> #=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >>=20 >> # >>=20 >> @@ -13,6 +13,7 @@ >> =20 >> ENTRY_POINT =3D SgiPlatformPeim >>=20 >> =20 >>=20 >> [Packages] >>=20 >> +=20 >> ArmPlatformPkg/ArmPlatformPkg.dec >>=20 >> EmbeddedPkg/EmbeddedPkg.dec >>=20 >> =20 >> MdePkg/MdePkg.dec >>=20 >> Platform/ARM/SgiPkg/SgiPlatform.dec >>=20 >> @@ -21,6 +22,11 >> @@ >> FdtLib >>=20 >> PeimEntryPoint >>=20 >> =20 >>=20 >> +[FixedPcd] >>=20 >> +=20 >> gArmSgiTokenSpaceGuid.PcdChipCount >>=20 >> +=20 >> gArmPlatformTokenSpaceGuid.PcdCoreCount >>=20 >> +=20 >> gArmPlatformTokenSpaceGuid.PcdClusterCount >>=20 >> + >>=20 >> [Sources] >>=20 >> =20 >> SgiPlatformPeim.c >>=20 >> =20 >>=20 >> 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) \ >>=20 >> (SGI_REMOTE_CHIP_MEM_OFFSET >> (ChipId) + FixedPcdGet64 (PcdDramBlock2Base)) >>=20 >> =20 >>=20 >> +// List of isolated >> CPUs MPID >>=20 >> +typedef struct { >>=20 >> + UINT64 Count; // Number >> of elements present in the list >>=20 >> + UINT64 Mpid[]; // List >> containing isolated CPU MPIDs >>=20 >> +} SGI_ISOLATED_CPU_LIST; >>=20 >> + >>=20 >> // ARM >> platform description data. >>=20 >> typedef struct { >>=20 >> UINTN PlatformId; >>=20 >> =20 >> UINTN ConfigId; >>=20 >> UINTN MultiChipMode; >>=20 >> + SGI_ISOLATED_CPU_LIST=20 >> IsolatedCpuList; >>=20 >> } SGI_PLATFORM_DESCRIPTOR; >>=20 >> =20 >>=20 >> // Arm SGI/RD Product >> IDs >>=20 >> 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 @@ >>=20 >> /** @file >>=20 >> * >>=20 >> -* Copyright (c) 2018, ARM Limited. All rights reserved. >>=20 >> +* Copyright (c) 2018 - 2022, ARM Limited. All rights reserved. >>=20 >> * >>=20 >> *=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >>=20 >> * >>=20 >> **/ >>=20 >> =20 >>=20 >> +#include >> >>=20 >> + >>=20 >> #include >>=20 >> #include >> >>=20 >> #include >>=20 >> +#include >> >>=20 >> #include >>=20 >> =20 >>=20 >> VOID >>=20 >> @@ -16,6 +19,127 @@ InitVirtioDevices ( >> VOID >>=20 >> ); >>=20 >> =20 >>=20 >> +/** >>=20 >> + Search >> for a MPID in a list >>=20 >> + >>=20 >> + Performs a linear search for a specified MPID >> on the given linear >>=20 >> + list of MPIDs. >>=20 >> + >>=20 >> + @param[in] MpidList=20 >> Pointer to list. >>=20 >> + @param[in] Count Number of the elements in the >> list. >>=20 >> + @param[in] Mpid Target MPID. >>=20 >> + >>=20 >> + @retval TRUE MPID is >> present. >>=20 >> + @retval FALSE MPID is not present. >>=20 >> +**/ >>=20 >> +STATIC >>=20 >> +BOOLEAN >>=20 >> +CheckIfMpidIsPresent ( >>=20 >> + IN UINT64 *MpidList, >>=20 >> + IN UINT64 Count, >>=20 >> + >> IN UINT64 Mpid >>=20 >> + ) >>=20 >> +{ >>=20 >> + UINT64 Idx; >>=20 >> + >>=20 >> + for (Idx =3D 0; Idx < >> Count; Idx++) { >>=20 >> + if (MpidList[Idx] =3D=3D Mpid) { >>=20 >> + return TRUE; >>=20 >> + >> } >>=20 >> + } >>=20 >> + >>=20 >> + return FALSE; >>=20 >> +} >>=20 >> + >>=20 >> +/** >>=20 >> + Disables isolated CPUs in >> the MADT table >>=20 >> + >>=20 >> + Parse the IsolatedCpuInfo from the Hob list and >> updates the MADT table to >=20 > [SAMI] Nit.=C2=A0 updates -> update [Nishant] Will update in next patch version. >=20 >=20 >> + disable cpu's which are not available on the platfrom. >>=20 >> + >>=20 >> +=20 >> @param[in] AcpiHeader Points to the Madt table. >>=20 >> + @param[in] HobData =20 >> Points to the unusable cpuinfo in hoblist. >>=20 >> +**/ >>=20 >> +STATIC >>=20 >> +VOID >>=20 >> +UpdateMadtTable ( >>=20 >> + IN EFI_ACPI_DESCRIPTION_HEADER *AcpiHeader, >>=20 >> + IN >> SGI_PLATFORM_DESCRIPTOR *HobData >>=20 >> + ) >>=20 >> +{ >>=20 >> + UINT8 >> *StructureListHead; >>=20 >> + UINT8 *StructureListTail; >>=20 >> +=20 >> EFI_ACPI_6_4_GIC_STRUCTURE *GicStructure; >>=20 >> + BOOLEAN MpidPresent; >>=20 >> + >>=20 >> +=20 >> if (HobData->IsolatedCpuList.Count =3D=3D 0) { >>=20 >> + return; >>=20 >> + } >>=20 >> + >>=20 >> +=20 >> StructureListHead =3D >>=20 >> + ((UINT8 *)AcpiHeader) + >>=20 >> + =20 >> sizeof(EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER); >>=20 >> +=20 >> StructureListTail =3D (UINT8 *)AcpiHeader + AcpiHeader->Length; >>=20 >> + >>=20 >> + // >> Locate ACPI GICC structure in the MADT table. >>=20 >> + while (StructureListHead >> < StructureListTail) { >>=20 >> + if (StructureListHead[0] =3D=3D EFI_ACPI_6_4_GIC) >> { >=20 >=20 >=20 > [SAMI] This is definitely not the way to parse an ACPI table. Please dont > do this. >=20 >=20 >=20 > Also, why are you not using DynamicTables framework? It is designed to > handle such cases. >=20 >=20 >=20 > [/SAMI] >=20 >=20 [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 p= lans to migrate to it. For the use of dynamic table [/Nishant] >=20 >=20 >> + GicStructure =3D (EFI_ACPI_6_4_GIC_STRUCTURE *)StructureListHead; >>=20 >> +=20 >> // Disable the CPU if its MPID is present in the list. >>=20 >> + MpidPresent >> =3D CheckIfMpidIsPresent( >>=20 >> + HobData->IsolatedCpuList.Mpid, >>=20 >> + =20 >> HobData->IsolatedCpuList.Count, >>=20 >> + GicStructure->MPIDR >>=20 >> + ); >> + if (MpidPresent =3D=3D TRUE) { >>=20 >> + DEBUG (( >>=20 >> + =20 >> DEBUG_INFO, >>=20 >> + "Disabling Core: %lu, MPID: 0x%llx in MADT\n", >>=20 >> +=20 >> GicStructure->AcpiProcessorUid, >>=20 >> + GicStructure->MPIDR >>=20 >> + =20 >> )); >>=20 >> + GicStructure->Flags =3D 0; >>=20 >> + } >>=20 >> + } >>=20 >> + >>=20 >> + // >> Second element in the structure component header is length >>=20 >> + =20 >> StructureListHead +=3D StructureListHead[1]; >>=20 >> + } >>=20 >> +} >>=20 >> + >>=20 >> +/** >>=20 >> + Callback >> to validate and/or update ACPI table. >>=20 >> + >>=20 >> + On finding a MADT table, >> disable the isolated CPUs in the MADT table. The >>=20 >> + list of isolated CPUs >> are obtained from the HOB data. >>=20 >> + >>=20 >> + @param[in] AcpiHeader Target ACPI >> table. >>=20 >> + >>=20 >> + @retval TURE Table validated/updated successfully. >>=20 >> +=20 >> @retval FALSE Error in Table validation/updation. >>=20 >> +**/ >>=20 >> +STATIC >>=20 >> +BOOLEAN >>=20 >> +CheckAndUpdateAcpiTable ( >>=20 >> + IN EFI_ACPI_DESCRIPTION_HEADER=20 >> *AcpiHeader >>=20 >> + ) >>=20 >> +{ >>=20 >> + VOID *SystemIdHob; >>=20 >> + SGI_PLATFORM_DESCRIPTOR >> *HobData; >>=20 >> + >>=20 >> + // This check updates the MADT table to disable isolated >> CPUs present on the >>=20 >> + // platform. >>=20 >> + if (AcpiHeader->Signature =3D=3D >> EFI_ACPI_1_0_APIC_SIGNATURE) { >=20 > [SAMI] Why EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE is not > used here? >=20 [Nishant] I will update in the next version. >=20 >=20 >> + SystemIdHob =3D GetFirstGuidHob (&gArmSgiPlatformIdDescriptorGuid); >>=20 >> +=20 >> if (SystemIdHob !=3D NULL) { >>=20 >> + HobData =3D (SGI_PLATFORM_DESCRIPTOR >> *)GET_GUID_HOB_DATA (SystemIdHob); >>=20 >> + UpdateMadtTable (AcpiHeader, >> HobData); >>=20 >> + } >>=20 >> + } >>=20 >> + >>=20 >> + return TRUE; >>=20 >> +} >>=20 >> + >>=20 >> EFI_STATUS >>=20 >> EFIAPI >>=20 >>=20 >> ArmSgiPkgEntryPoint ( >>=20 >> @@ -25,7 +149,10 @@ ArmSgiPkgEntryPoint ( >> { >>=20 >> =20 >> EFI_STATUS Status; >>=20 >> =20 >>=20 >> - Status =3D LocateAndInstallAcpiFromFv >> (&gArmSgiAcpiTablesGuid); >>=20 >> + Status =3D >> LocateAndInstallAcpiFromFvConditional ( >>=20 >> + =20 >> &gArmSgiAcpiTablesGuid, >>=20 >> + &CheckAndUpdateAcpiTable >>=20 >> + =20 >> ); >>=20 >> if (EFI_ERROR (Status)) { >>=20 >> DEBUG ((DEBUG_ERROR, "%a: Failed to >> install ACPI tables\n", __FUNCTION__)); >>=20 >> return Status; >>=20 >> 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 >>=20 >> * >>=20 >> -* Copyright (c) 2018, ARM Limited. All rights >> reserved. >>=20 >> +* Copyright (c) 2018-2022, ARM Limited. All rights reserved. >>=20 >> * >>=20 >> * SPDX-License-Identifier: BSD-2-Clause-Patent >>=20 >> * >>=20 >> @@ -38,6 +38,8 @@ >> GetSgiSystemId ( >> CONST VOID *NtFwCfgDtBlob; >>=20 >> =20 >> SGI_NT_FW_CONFIG_INFO_PPI *NtFwConfigInfoPpi; >>=20 >> EFI_STATUS =20 >> Status; >>=20 >> + UINT64 IsolatedCpuCount; >>=20 >> + UINT64 =20 >> CoreCount; >>=20 >> =20 >>=20 >> Status =3D PeiServicesLocatePpi >> (&gNtFwConfigDtInfoPpiGuid, 0, NULL, >>=20 >> =20 >> (VOID**)&NtFwConfigInfoPpi); >>=20 >> @@ -83,6 +85,32 @@ GetSgiSystemId ( >> =20 >> HobData->MultiChipMode =3D fdt32_to_cpu (*Property); >>=20 >> } >>=20 >> =20 >>=20 >> + Property =3D >> fdt_getprop (NtFwCfgDtBlob, Offset, "isolated-cpu-list", NULL); >>=20 >> + if >> (Property =3D=3D NULL) { >>=20 >> + DEBUG ((DEBUG_INFO, "%s property not found\n", >> "isolated-cpu-list")); >>=20 >> + HobData->IsolatedCpuList.Count =3D 0; >>=20 >> + } >> else { >>=20 >> + CopyMem (&IsolatedCpuCount, Property, sizeof >> (IsolatedCpuCount)); >>=20 >> + CoreCount =3D >>=20 >> + FixedPcdGet32 >> (PcdChipCount) * >>=20 >> + FixedPcdGet32 (PcdClusterCount) * >>=20 >> + =20 >> FixedPcdGet32 (PcdCoreCount); >>=20 >> + if (IsolatedCpuCount > CoreCount) { >>=20 >> + >> DEBUG (( >>=20 >> + DEBUG_ERROR, >>=20 >> + "IsolatedCpuCount(%u) is >> higher than CoreCount(%u)\n", >>=20 >> + IsolatedCpuCount, >>=20 >> + =20 >> CoreCount >>=20 >> + )); >>=20 >> + return EFI_SUCCESS; >=20 > [SAMI] Is the status code returned here correct? Should this be > EFI_INVALID_PARAMETER? Also the function name GetSgiSystemId() seems to n= o > 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 prov= ided is corrupted. I will update the function name in the next version. [/Nishant] >=20 >=20 >> + } >>=20 >> + CopyMem ( >>=20 >> + &HobData->IsolatedCpuList, >>=20 >> + =20 >> Property, >>=20 >> + sizeof(HobData->IsolatedCpuList) + (CoreCount * >> sizeof(UINT64)) >=20 > [SAMI] Coding convention is not followed here and at other places. Can yo= u > fix, please? [Nishant] Will update in the next patch version. >=20 >=20 >> + ); >>=20 >> + } >>=20 >> + >>=20 >> return EFI_SUCCESS; >>=20 >> } >>=20 >> =20 >>=20 >> @@ -104,11 +132,24 @@ >> SgiPlatformPeim ( >> { >>=20 >> SGI_PLATFORM_DESCRIPTOR *HobData; >>=20 >> =20 >> EFI_STATUS Status; >>=20 >> + UINT64 =20 >> CoreCount; >>=20 >> + UINTN HobSize; >>=20 >> =20 >>=20 >> + CoreCount =3D >>=20 >> + FixedPcdGet32 (PcdChipCount) * >>=20 >> + FixedPcdGet32 (PcdClusterCount) >> * >>=20 >> + FixedPcdGet32 (PcdCoreCount); >>=20 >> + >>=20 >> + // Additional size for >> SGI_ISOLATED_CPU_LIST. >>=20 >> + // Size =3D (MPID register size in bytes * >> CoreCount) + >>=20 >> + // sizeof(SGI_PLATFORM_DESCRIPTOR) >>=20 >> + HobSize =3D >>=20 >> + sizeof (SGI_PLATFORM_DESCRIPTOR) + >>=20 >> + (CoreCount * >> sizeof(UINT64)); >>=20 >> // Create platform descriptor HOB >>=20 >> HobData =3D >> (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob ( >>=20 >> =20 >> &gArmSgiPlatformIdDescriptorGuid, >>=20 >> - =20 >> sizeof (SGI_PLATFORM_DESCRIPTOR)); >>=20 >> + =20 >> HobSize); >>=20 >> =20 >>=20 >> // Get the system id from the platform specific >> nt_fw_config device tree >>=20 >> if (HobData =3D=3D NULL) { >>=20 >>=20 >=20 > 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 th= e > information in any medium. Thank you. --6W3hMkb2obr66dTu6Wb8 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable

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 use=
d 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 i=
s 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@arm.com>
---
 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/Platfo=
rm/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 cha= nge 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                    =3D SgiPlatformPeim

=20

 [Packages]

+  ArmPlatformPkg/ArmPlatformPkg.dec

   EmbeddedPkg/EmbeddedPkg.dec

   MdePkg/MdePkg.dec

   Platform/ARM/SgiPkg/SgiPlatform.dec

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

   PeimEntryPoint

=20

+[FixedPcd]

+  gArmSgiTokenSpaceGuid.PcdChipCount

+  gArmPlatformTokenSpaceGuid.PcdCoreCount

+  gArmPlatformTokenSpaceGuid.PcdClusterCount

+

 [Sources]

   SgiPlatformPeim.c

=20

diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPk=
g/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 (PcdDramBlo=
ck2Base))

=20

+// 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;

=20

 // Arm SGI/RD Product IDs

diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platfo=
rm/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

 *

 **/

=20

+#include <IndustryStandard/Acpi.h>

+

 #include <Library/AcpiLib.h>

 #include <Library/DebugLib.h>

 #include <Library/HobLib.h>

+#include <Library/UefiBootServicesTableLib.h>

 #include <SgiPlatform.h>

=20

 VOID

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

   );

=20

+/**

+  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 =3D 0; Idx < Count; Idx++) {

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

+      return TRUE;

+    }

+  }

+

+  return FALSE;

+}

+

+/**

+  Disables isolated CPUs in the MADT table

+

+  Parse the IsolatedCpuInfo from the Hob list and updates the MADT table t=
o
[SAMI] Nit.  updates -> update
[Nishant] Will update in next patch version.
+  disable cpu's which are not available on th=
e 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 =3D=3D 0) {

+    return;

+  }

+

+  StructureListHead =3D

+    ((UINT8 *)AcpiHeader) +

+    sizeof(EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER);

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

+

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

+  while (StructureListHead < StructureListTail) {

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

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

Also, why are you not using DynamicTables framework? It is designed to h= andle 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 sta= ndardised way of updating the ACPI table.

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

+      GicStructure =3D (EFI_ACPI_6_4_GIC_STRU=
CTURE *)StructureListHead;

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

+      MpidPresent =3D CheckIfMpidIsPresent(

+        HobData->IsolatedCpuList.Mpid,

+        HobData->IsolatedCpuList.Count,

+        GicStructure->MPIDR

+        );

+      if (MpidPresent =3D=3D TRUE) {

+        DEBUG ((

+          DEBUG_INFO,

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

+          GicStructure->AcpiProcessorUid,

+          GicStructure->MPIDR

+          ));

+        GicStructure->Flags =3D 0;

+      }

+    }

+

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

+    StructureListHead +=3D StructureListHead[1];

+  }

+}

+

+/**

+  Callback to validate and/or update ACPI table.

+

+  On finding a MADT table, disable the isolated CPUs in the MADT table. Th=
e

+  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 =3D=3D EFI_ACPI_1_0_APIC_SIGNATURE) {
[SAMI] Why EFI_ACPI_6_4_MULTIPLE_APIC_DESCR= IPTION_TABLE_SIGNATURE is not used here?
[Nishant] I will update in the next version.
+    SystemIdHob =3D GetFirstGuidHob (&gAr=
mSgiPlatformIdDescriptorGuid);

+    if (SystemIdHob !=3D NULL) {

+      HobData =3D (SGI_PLATFORM_DESCRIPTOR *)GET_GUID_HOB_DATA (SystemIdHo=
b);

+      UpdateMadtTable (AcpiHeader, HobData);

+    }

+  }

+

+  return TRUE;

+}

+

 EFI_STATUS

 EFIAPI

 ArmSgiPkgEntryPoint (

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

   EFI_STATUS              Status;

=20

-  Status =3D LocateAndInstallAcpiFromFv (&gArmSgiAcpiTablesGuid);

+  Status =3D 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;

=20

   Status =3D PeiServicesLocatePpi (&gNtFwConfigDtInfoPpiGuid, 0, NULL,

              (VOID**)&NtFwConfigInfoPpi);

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

   }

=20

+  Property =3D fdt_getprop (NtFwCfgDtBlob, Offset, "isolated-cpu-list", NU=
LL);

+  if (Property =3D=3D NULL) {

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

+    HobData->IsolatedCpuList.Count =3D 0;

+  } else {

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

+    CoreCount =3D

+      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 refl= ect 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 nam= e 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;

 }

=20

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

   SGI_PLATFORM_DESCRIPTOR       *HobData;

   EFI_STATUS                    Status;

+  UINT64                        CoreCount;

+  UINTN                         HobSize;

=20

+  CoreCount =3D

+    FixedPcdGet32 (PcdChipCount) *

+    FixedPcdGet32 (PcdClusterCount) *

+    FixedPcdGet32 (PcdCoreCount);

+

+  // Additional size for SGI_ISOLATED_CPU_LIST.

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

+  //        sizeof(SGI_PLATFORM_DESCRIPTOR)

+  HobSize =3D

+    sizeof (SGI_PLATFORM_DESCRIPTOR) +

+    (CoreCount * sizeof(UINT64));

   // Create platform descriptor HOB

   HobData =3D (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (

                                          &gArmSgiPlatformIdDescriptorG=
uid,

-                                         sizeof (SGI_PLATFORM_DESCRIPTOR))=
;

+                                         HobSize);

=20

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

   if (HobData =3D=3D NULL) {
IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease 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.
--6W3hMkb2obr66dTu6Wb8--