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.sha...@arm.com> (
>> nishant.sha...@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/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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91894): https://edk2.groups.io/g/devel/message/91894
Mute This Topic: https://groups.io/mt/91816363/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
