From: "Sami Mujawar" <sami.mujawar@arm.com>
To: sahil <sahil@arm.com>, devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
Leif Lindholm <leif@nuviainc.com>, "nd@arm.com" <nd@arm.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH V3 1/1] Platform/ARM/N1sdp: Add support to parse NT_FW_CONFIG
Date: Tue, 12 Dec 2023 17:10:15 +0000 [thread overview]
Message-ID: <111f6ffd-eec2-45b6-86c2-1cc5b24bf3c3@arm.com> (raw)
In-Reply-To: <20230823110318.826467-1-sahil@arm.com>
[-- Attachment #1: Type: text/plain, Size: 27706 bytes --]
Hi Sahil,
Thank you for this patch.
I think this patch can be split into 3 or 4 patches. Can you look into
that, please?
I also have other feedback marked inline as [SAMI].
Regards,
Sami Mujawar
On 23/08/2023 12:03 pm, sahil wrote:
> NT_FW_CONFIG DTB contains platform information passed by
> Tf-A boot stage.
> This information is used for Virtual memory map generation
> during PEI phase and passed on to DXE phase as a HOB, where
> it is used in ConfigurationManagerDxe.
>
> This patch adds a PEI to parse NT_FW_CONFIG and pass it to
> other PEI modules(as PPI) and DXE modules(as HOB).
>
> Signed-off-by: sahil<sahil@arm.com>
> ---
>
> Notes:
> v3:
> - Fixed code review comments
> - Added a PEI to parse nt_fw_config
>
> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec | 8 +-
> Platform/ARM/N1Sdp/N1SdpPlatform.dsc | 5 +-
> Platform/ARM/N1Sdp/N1SdpPlatform.fdf | 3 +-
> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf | 6 +-
> Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf | 41 ++++++
> Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.inf | 8 +-
> Silicon/ARM/NeoverseN1Soc/Include/NeoverseN1Soc.h | 14 +--
> Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c | 24 ++--
> Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c | 133 ++++++++++++++++++++
> Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c | 12 +-
> Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLibMem.c | 33 +++--
> Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/AArch64/Helper.S | 4 +-
> 12 files changed, 255 insertions(+), 36 deletions(-)
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> index d59f25a5b915..7118da25dce0 100644
> --- a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> +++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> @@ -1,7 +1,7 @@
> ## @file
>
> # Describes the entire platform configuration.
>
> #
>
> -# Copyright (c) 2018 - 2021, ARM Limited. All rights reserved.<BR>
>
> +# Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -22,6 +22,8 @@
> Include # Root include for the package
>
>
>
> [Guids.common]
>
> + # ARM NeoverseN1Soc Platform Info descriptor
>
> + gArmNeoverseN1SocPlatformInfoDescriptorGuid = { 0x9fa16eb5, 0xce13, 0x4d37, { 0x96, 0xf0, 0x0a, 0xb5, 0xf1, 0xab, 0xff, 0x01 } }
>
> gArmNeoverseN1SocTokenSpaceGuid = { 0xab93eb78, 0x60d7, 0x4099, { 0xac, 0xeb, 0x6d, 0xb5, 0x02, 0x58, 0x7c, 0x24 } }
>
>
>
> [PcdsFixedAtBuild]
>
> @@ -83,3 +85,7 @@
> gArmNeoverseN1SocTokenSpaceGuid.PcdRemotePcieMmio32Translation|0x40000000000|UINT64|0x0000004F
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdRemotePcieMmio64Translation|0x40000000000|UINT64|0x00000050
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdRemotePcieSegmentNumber|2|UINT32|0x00000051
>
> +
>
> +[Ppis]
>
> + gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid = { 0x21D04AD4, 0x4D23, 0x426D, { 0x8D, 0x3E, 0x92, 0x23, 0x3E, 0xF4, 0xB9, 0x5E } }
>
> + gArmNeoverseN1SocParameterPpiGuid = { 0x4DDD5A72, 0x31AD, 0x4B20, { 0x8F, 0x5F, 0xB3, 0xE8, 0x24, 0x6F, 0x80, 0x2B } }
>
> diff --git a/Platform/ARM/N1Sdp/N1SdpPlatform.dsc b/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
> index d04b22d3ef51..691d1af3c25c 100644
> --- a/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
> +++ b/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
> @@ -4,7 +4,7 @@
> # This provides platform specific component descriptions and libraries that
>
> # conform to EFI/Framework standards.
>
> #
>
> -# Copyright (c) 2018 - 2021, ARM Limited. All rights reserved.<BR>
>
> +# Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -228,6 +228,9 @@
> # Platform driver
>
> Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.inf
>
>
>
> + # PEI Phase modules
>
> + Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf
>
> +
>
> # Human Interface Support
>
> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
>
>
>
> diff --git a/Platform/ARM/N1Sdp/N1SdpPlatform.fdf b/Platform/ARM/N1Sdp/N1SdpPlatform.fdf
> index e5e24ea50294..baac8a3ef727 100644
> --- a/Platform/ARM/N1Sdp/N1SdpPlatform.fdf
> +++ b/Platform/ARM/N1Sdp/N1SdpPlatform.fdf
> @@ -1,7 +1,7 @@
> ## @file
>
> # FDF file of N1Sdp
>
> #
>
> -# Copyright (c) 2018 - 2021, ARM Limited. All rights reserved.<BR>
>
> +# Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> ##
>
> @@ -195,6 +195,7 @@ READ_LOCK_STATUS = TRUE
> INF MdeModulePkg/Core/Pei/PeiMain.inf
>
> INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>
> INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>
> + INF Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf
>
>
>
> FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>
> SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
>
> diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
> index 4f8e7f13021a..242490caf860 100644
> --- a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
> +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
> @@ -1,7 +1,7 @@
> ## @file
>
> # Configuration Manager Dxe
>
> #
>
> -# Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
>
> +# Copyright (c) 2021 - 2023, ARM Limited. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -42,6 +42,7 @@
>
>
> [LibraryClasses]
>
> ArmPlatformLib
>
> + HobLib
>
> PrintLib
>
> UefiBootServicesTableLib
>
> UefiDriverEntryPoint
>
> @@ -170,5 +171,8 @@
> gArmNeoverseN1SocTokenSpaceGuid.PcdRemotePcieMmio64Translation
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdRemotePcieSegmentNumber
>
>
>
> +[Guids]
>
> + gArmNeoverseN1SocPlatformInfoDescriptorGuid
>
> +
>
> [Depex]
>
> TRUE
>
> diff --git a/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf
> new file mode 100644
> index 000000000000..2eec8c327205
> --- /dev/null
> +++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf
> @@ -0,0 +1,41 @@
> +## @file
>
> +# This PEI module parse the NtFwConfig for N1Sdp platform and produce
>
> +# the PPI and HOB.
>
> +#
>
> +# Copyright (c) 2023, ARM Limited. All rights reserved.<BR>
>
> +#
>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +#
>
> +##
>
> +
>
> +[Defines]
>
> + INF_VERSION = 0x0001001B
>
> + BASE_NAME = N1SdpNtFwConfigPei
>
> + FILE_GUID = CE76D56C-D3A5-4763-9138-DF09E1D1B614
>
> + MODULE_TYPE = PEIM
>
> + VERSION_STRING = 1.0
>
> + ENTRY_POINT = Load
>
> +
>
> +[Sources]
>
> + NtFwConfigPei.c
>
> +
>
> +[Packages]
>
> + EmbeddedPkg/EmbeddedPkg.dec
>
> + MdePkg/MdePkg.dec
>
> + Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>
> +
>
> +[LibraryClasses]
>
> + DebugLib
>
> + FdtLib
>
> + HobLib
>
> + PeimEntryPoint
>
> +
>
> +[Ppis]
>
> + gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid
>
> + gArmNeoverseN1SocParameterPpiGuid
>
> +
>
> +[Guids]
>
> + gArmNeoverseN1SocPlatformInfoDescriptorGuid
>
> +
>
> +[Depex]
>
> + gArmNeoverseN1SocParameterPpiGuid
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.inf b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.inf
> index 96e590cdd810..6e7e16b86838 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.inf
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.inf
> @@ -1,7 +1,7 @@
> ## @file
>
> # Platform Library for N1Sdp.
>
> #
>
> -# Copyright (c) 2018-2021, ARM Limited. All rights reserved.<BR>
>
> +# Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -58,8 +58,8 @@
>
>
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>
>
>
> -[Guids]
>
> - gEfiHobListGuid ## CONSUMES ## SystemTable
>
> -
>
> [Ppis]
>
> gArmMpCoreInfoPpiGuid
>
> + gArmNeoverseN1SocParameterPpiGuid
>
> + gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid
>
> +
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Include/NeoverseN1Soc.h b/Silicon/ARM/NeoverseN1Soc/Include/NeoverseN1Soc.h
> index 097160c7e2d1..2f83d582ccf3 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Include/NeoverseN1Soc.h
> +++ b/Silicon/ARM/NeoverseN1Soc/Include/NeoverseN1Soc.h
> @@ -1,6 +1,6 @@
> /** @file
>
> *
>
> -* Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
>
> +* Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.
>
> *
>
> * SPDX-License-Identifier: BSD-2-Clause-Patent
>
> *
>
> @@ -41,11 +41,6 @@
> #define NEOVERSEN1SOC_EXP_PERIPH_BASE0 0x1C000000
>
> #define NEOVERSEN1SOC_EXP_PERIPH_BASE0_SZ 0x1300000
>
>
>
> -// Base address to a structure of type NEOVERSEN1SOC_PLAT_INFO which is
>
> -// pre-populated by a earlier boot stage
>
> -#define NEOVERSEN1SOC_PLAT_INFO_STRUCT_BASE (NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + \
>
> - 0x00008000)
>
> -
>
> /*
>
> * Platform information structure stored in Non-secure SRAM. Platform
>
> * information are passed from the trusted firmware with the below structure
>
> @@ -55,12 +50,15 @@
> typedef struct {
>
> /*! 0 - Single Chip, 1 - Chip to Chip (C2C) */
>
> UINT8 MultichipMode;
>
> - /*! Slave count in C2C mode */
>
> - UINT8 SlaveCount;
>
> + /*! Secondary chip count in C2C mode */
>
> + UINT8 SecondaryChipCount;
>
> /*! Local DDR memory size in GigaBytes */
>
> UINT8 LocalDdrSize;
>
> /*! Remote DDR memory size in GigaBytes */
>
> UINT8 RemoteDdrSize;
>
> } NEOVERSEN1SOC_PLAT_INFO;
>
>
>
> +typedef struct {
>
> + CONST VOID *NtFwConfig;
>
> +} NEOVERSEN1SOC_EL3_FW_HANDOFF_PARAM_PPI;
>
> #endif
>
> diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
> index fa6408a7dd1e..e248826ec925 100644
> --- a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
> +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
> @@ -1,7 +1,7 @@
> /** @file
>
> Configuration Manager Dxe
>
>
>
> - Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
>
> + Copyright (c) 2021 - 2023, ARM Limited. All rights reserved.<BR>
>
>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -16,6 +16,7 @@
> #include <IndustryStandard/SerialPortConsoleRedirectionTable.h>
>
> #include <Library/ArmLib.h>
>
> #include <Library/DebugLib.h>
>
> +#include <Library/HobLib.h>
>
> #include <Library/IoLib.h>
>
> #include <Library/PcdLib.h>
>
> #include <Library/UefiBootServicesTableLib.h>
>
> @@ -28,6 +29,7 @@
> #include "Platform.h"
>
>
>
> extern struct EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE Hmat;
>
> +static NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
[SAMI] static should be STATIC and PlatInfo should be mPlatInfo (where m
is prefix for module level variables).
Also have you considered making 'NEOVERSEN1SOC_PLAT_INFO *PlatInfo;' a
field in EDKII_PLATFORM_REPOSITORY_INFO and intialising it in
InitialisePlatformRepository().
[/SAMI]
>
>
>
> /** The platform configuration repository information.
>
> */
>
> @@ -1242,13 +1244,21 @@ InitializePlatformRepository (
> IN EDKII_PLATFORM_REPOSITORY_INFO * CONST PlatRepoInfo
>
> )
>
> {
>
> - NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> UINT64 Dram2Size;
>
> UINT64 RemoteDdrSize;
>
> + VOID *PlatInfoHob;
>
> +
>
> + PlatInfoHob = GetFirstGuidHob (&gArmNeoverseN1SocPlatformInfoDescriptorGuid);
>
> +
>
> + if (PlatInfoHob == NULL) {
>
> + DEBUG ((DEBUG_ERROR, "Platform HOB is NULL\n"));
>
> + return EFI_NOT_FOUND;
>
> + }
>
> +
>
> + PlatInfo = (NEOVERSEN1SOC_PLAT_INFO *)GET_GUID_HOB_DATA (PlatInfoHob);
>
>
>
> RemoteDdrSize = 0;
>
>
>
> - PlatInfo = (NEOVERSEN1SOC_PLAT_INFO *)NEOVERSEN1SOC_PLAT_INFO_STRUCT_BASE;
>
> Dram2Size = ((PlatInfo->LocalDdrSize - 2) * SIZE_1GB);
>
>
>
> PlatRepoInfo->MemAffInfo[LOCAL_DDR_REGION2].Length = Dram2Size;
>
> @@ -1512,7 +1522,6 @@ GetGicCInfo (
> )
>
> {
>
> EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo;
>
> - NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> UINT32 TotalObjCount;
>
> UINT32 ObjIndex;
>
>
>
> @@ -1523,7 +1532,6 @@ GetGicCInfo (
> }
>
>
>
> PlatformRepo = This->PlatRepoInfo;
>
> - PlatInfo = (NEOVERSEN1SOC_PLAT_INFO *)NEOVERSEN1SOC_PLAT_INFO_STRUCT_BASE;
>
>
>
> if (PlatInfo->MultichipMode == 1) {
>
> TotalObjCount = PLAT_CPU_COUNT * 2;
>
> @@ -1623,7 +1631,6 @@ GetStandardNameSpaceObject (
> {
>
> EFI_STATUS Status;
>
> EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo;
>
> - NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> UINT32 AcpiTableCount;
>
>
>
> if ((This == NULL) || (CmObject == NULL)) {
>
> @@ -1634,7 +1641,7 @@ GetStandardNameSpaceObject (
>
>
> Status = EFI_NOT_FOUND;
>
> PlatformRepo = This->PlatRepoInfo;
>
> - PlatInfo = (NEOVERSEN1SOC_PLAT_INFO *)NEOVERSEN1SOC_PLAT_INFO_STRUCT_BASE;
>
> +
>
> AcpiTableCount = ARRAY_SIZE (PlatformRepo->CmAcpiTableList);
>
> if (PlatInfo->MultichipMode == 0)
>
> AcpiTableCount -= 1;
>
> @@ -1697,7 +1704,6 @@ GetArmNameSpaceObject (
> {
>
> EFI_STATUS Status;
>
> EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo;
>
> - NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> UINT32 GicRedistCount;
>
> UINT32 GicCpuCount;
>
> UINT32 ProcHierarchyInfoCount;
>
> @@ -1718,8 +1724,6 @@ GetArmNameSpaceObject (
> Status = EFI_NOT_FOUND;
>
> PlatformRepo = This->PlatRepoInfo;
>
>
>
> - // Probe for multi chip information
>
> - PlatInfo = (NEOVERSEN1SOC_PLAT_INFO *)NEOVERSEN1SOC_PLAT_INFO_STRUCT_BASE;
>
> if (PlatInfo->MultichipMode == 1) {
>
> GicRedistCount = 2;
>
> GicCpuCount = PLAT_CPU_COUNT * 2;
>
> diff --git a/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
> new file mode 100644
> index 000000000000..ff6a6a7933c3
> --- /dev/null
> +++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
> @@ -0,0 +1,133 @@
> +/** @file
>
> +
>
> + Copyright (c) 2023, ARM Limited. All rights reserved.<BR>
>
> + SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +**/
>
> +
>
> +#include <Library/DebugLib.h>
>
> +#include <Library/HobLib.h>
>
> +#include <Library/PeiServicesLib.h>
>
> +
>
> +#include <NeoverseN1Soc.h>
>
> +#include <libfdt.h>
>
> +
>
> +STATIC EFI_PEI_PPI_DESCRIPTOR gPpi;
[SAMI] The prefix should be 'm' not 'g'. 'g' is for global, which I do
not think is your intention here.
>
> +
>
> +/**
>
> + The entrypoint of the module, parse NtFwConfig and produce the PPI and HOB.
>
> +
>
> + @param[in] FileHandle Handle of the file being invoked.
>
> + @param[in] PeiServices Describes the list of possible PEI Services.
>
> +
>
> + @retval EFI_SUCCESS Either no NT_FW_CONFIG was given by EL3 firmware
>
> + OR the N1Sdp FDT HOB was successfully created.
>
> + @retval EFI_UNSUPPORTED FDT header sanity check failed.
>
> + @retval * Other errors are possible.
[SAMI] Please list the other possible values. At least the ones that are
returned by this function.
>
> +**/
>
> +
>
> +EFI_STATUS
>
> +EFIAPI
>
> +Load (
[SAMI] Is it possible to change the name of the function to something
like NtFwConfigPeEntryPoint() ?
>
> + IN EFI_PEI_FILE_HANDLE FileHandle,
>
> + IN CONST EFI_PEI_SERVICES **PeiServices
>
> + )
>
> +{
>
> + CONST NEOVERSEN1SOC_EL3_FW_HANDOFF_PARAM_PPI *ParamPpi;
>
> + CONST UINT32 *Property;
>
> + INT32 Offset;
>
> + NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> + INT32 Status;
>
> +
>
> + PlatInfo = BuildGuidHob (
>
> + &gArmNeoverseN1SocPlatformInfoDescriptorGuid,
>
> + sizeof (*PlatInfo)
>
> + );
>
> +
>
> + if (PlatInfo == NULL) {
>
> + DEBUG ((
>
> + DEBUG_ERROR,
>
> + "[%a]: failed to allocate platform info HOB\n",
>
> + gEfiCallerBaseName
>
> + ));
>
> + return EFI_INVALID_PARAMETER;
[SAMI] EFI_OUT_OF_RESOURCES ?
>
> + }
>
> +
>
> + Status = PeiServicesLocatePpi (
>
> + &gArmNeoverseN1SocParameterPpiGuid,
>
> + 0,
>
> + NULL,
>
> + (VOID **)&ParamPpi
>
> + );
>
> +
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((
>
> + DEBUG_ERROR,
>
> + "[%a]: failed to locate gArmNeoverseN1SocParameterPpiGuid - %r\n",
>
> + gEfiCallerBaseName,
>
> + Status
>
> + ));
>
> + return EFI_INVALID_PARAMETER;
[SAMI] Why not return the 'Status' that was returned?
>
> + }
>
> +
>
> + if (fdt_check_header (ParamPpi->NtFwConfig) != 0) {
>
> + DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed\n", ParamPpi->NtFwConfig));
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + Offset = fdt_subnode_offset (ParamPpi->NtFwConfig, 0, "platform-info");
>
> + if (Offset == -FDT_ERR_NOTFOUND) {
>
> + DEBUG ((DEBUG_ERROR, "Invalid DTB : platform-info node not found\n"));
>
> + return EFI_INVALID_PARAMETER;
[SAMI] EFI_NOT_FOUND ?
>
> + }
>
> +
>
> + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "local-ddr-size", NULL);
>
> + if (Property == NULL) {
>
> + DEBUG ((DEBUG_ERROR, "local-ddr-size property not found\n"));
>
> + return EFI_INVALID_PARAMETER;
[SAMI] EFI_NOT_FOUND ? Similar change at other places in this file.
>
> + }
>
> +
>
> + PlatInfo->LocalDdrSize = fdt32_to_cpu (*Property);
>
> +
>
> + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "remote-ddr-size", NULL);
>
> + if (Property == NULL) {
>
> + DEBUG ((DEBUG_ERROR, "remote-ddr-size property not found\n"));
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + PlatInfo->RemoteDdrSize = fdt32_to_cpu (*Property);
>
> +
>
> + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "secondary-chip-count", NULL);
>
> + if (Property == NULL) {
>
> + DEBUG ((DEBUG_ERROR, "secondary-chip-count property not found\n"));
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + PlatInfo->SecondaryChipCount = fdt32_to_cpu (*Property);
>
> +
>
> + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "multichip-mode", NULL);
>
> + if (Property == NULL) {
>
> + DEBUG ((DEBUG_ERROR, "multichip-mode property not found\n"));
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + PlatInfo->MultichipMode = fdt32_to_cpu (*Property);
>
> +
>
> + gPpi.Flags = EFI_PEI_PPI_DESCRIPTOR_PPI
>
> + | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
>
> + gPpi.Guid = &gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid;
>
> + gPpi.Ppi = PlatInfo;
>
> +
>
> + Status = PeiServicesInstallPpi (&gPpi);
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((
>
> + DEBUG_ERROR,
>
> + "[%a]: failed to install PEI service - %r\n",
>
> + gEfiCallerBaseName,
>
> + Status
>
> + ));
>
> + return EFI_INVALID_PARAMETER;
[SAMI] Drop the return statement in the line above and return 'Status'
in the line after the if block below.
>
> + }
>
> +
>
> + return EFI_SUCCESS;
>
> +}
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c
> index c0effd37f333..52ee6299b3fe 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c
> @@ -1,6 +1,6 @@
> /** @file
>
>
>
> - Copyright (c) 2018-2021, ARM Limited. All rights reserved.<BR>
>
> + Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
>
>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -8,8 +8,12 @@
>
>
> #include <Library/ArmPlatformLib.h>
>
> #include <Library/BaseLib.h>
>
> +#include <NeoverseN1Soc.h>
>
> #include <Ppi/ArmMpCoreInfo.h>
>
>
>
> +UINT64 gArgNtFwConfigDtPtr;
>
> +STATIC NEOVERSEN1SOC_EL3_FW_HANDOFF_PARAM_PPI mNeoverseN1SocParameterPpi;
>
> +
>
> STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>
> { 0x0, 0x0 }, // Cluster 0, Core 0
>
> { 0x0, 0x1 }, // Cluster 0, Core 1
>
> @@ -46,6 +50,7 @@ ArmPlatformInitialize (
> IN UINTN MpId
>
> )
>
> {
>
> + mNeoverseN1SocParameterPpi.NtFwConfig = (VOID *)gArgNtFwConfigDtPtr;
>
> return RETURN_SUCCESS;
>
> }
>
>
>
> @@ -80,6 +85,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
> EFI_PEI_PPI_DESCRIPTOR_PPI,
>
> &gArmMpCoreInfoPpiGuid,
>
> &mMpCoreInfoPpi
>
> + },
>
> + {
>
> + EFI_PEI_PPI_DESCRIPTOR_PPI,
>
> + &gArmNeoverseN1SocParameterPpiGuid,
>
> + &mNeoverseN1SocParameterPpi
>
> }
>
> };
>
>
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLibMem.c b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLibMem.c
> index 9e8a1efc557d..d6ef92677231 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLibMem.c
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLibMem.c
> @@ -1,6 +1,6 @@
> /** @file
>
>
>
> - Copyright (c) 2018 - 2021, ARM Limited. All rights reserved.<BR>
>
> + Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
>
>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -10,6 +10,7 @@
> #include <Library/DebugLib.h>
>
> #include <Library/HobLib.h>
>
> #include <Library/MemoryAllocationLib.h>
>
> +#include <Library/PeiServicesLib.h>
>
> #include <NeoverseN1Soc.h>
>
>
>
> // The total number of descriptors, including the final "end-of-table" descriptor.
>
> @@ -30,15 +31,31 @@ ArmPlatformGetVirtualMemoryMap (
> IN ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
>
> )
>
> {
>
> - UINTN Index;
>
> - ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
>
> - EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
>
> - NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> - UINT64 DramBlock2Size;
>
> - UINT64 RemoteDdrSize;
>
> + UINTN Index;
>
> + ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
>
> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
>
> + CONST NEOVERSEN1SOC_PLAT_INFO *PlatInfo;
>
> + UINT64 DramBlock2Size;
>
> + UINT64 RemoteDdrSize;
>
> + EFI_STATUS Status;
>
>
>
> Index = 0;
>
> - PlatInfo = (NEOVERSEN1SOC_PLAT_INFO *)NEOVERSEN1SOC_PLAT_INFO_STRUCT_BASE;
>
> +
>
> + Status = PeiServicesLocatePpi (
>
> + &gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid,
>
> + 0,
>
> + NULL,
>
> + (VOID **)&PlatInfo
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((
>
> + DEBUG_ERROR,
>
> + "[%a]: failed to locate gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid - %r\n",
>
> + gEfiCallerBaseName,
>
> + Status
>
> + ));
[SAMI] I think the code should not continue otherwise dereferencing the
PlatInfo pointer will result in a crash.
It may also be useful if you set *VirtualMemoryMap = NULL.
I acknowledge the problem would not be fixed entirely, as the calling
function does not handle this. But at least this function would be doing
the right thing.
Ideally we need ArmPlatformGetVirtualMemoryMap() to return an error code
and the calling function(s) should handle the errors appropriately.
However, that is a bigger change and not part of this patch series.
[/SAMI]
>
> + }
>
> +
>
> DramBlock2Size = ((UINT64)(PlatInfo->LocalDdrSize -
>
> NEOVERSEN1SOC_DRAM_BLOCK1_SIZE / SIZE_1GB) *
>
> (UINT64)SIZE_1GB);
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/AArch64/Helper.S b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/AArch64/Helper.S
> index 8d2069dea837..cbbe7aae9858 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/AArch64/Helper.S
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/AArch64/Helper.S
> @@ -1,6 +1,6 @@
> /** @file
>
> *
>
> -* Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
>
> +* Copyright (c) 2019 - 2023, ARM Limited. All rights reserved.
>
> *
>
> * SPDX-License-Identifier: BSD-2-Clause-Patent
>
> *
>
> @@ -25,6 +25,8 @@ GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> // the UEFI firmware through the CPU registers.
>
> //
>
> ASM_PFX(ArmPlatformPeiBootAction):
>
> + adr x10, gArgNtFwConfigDtPtr
>
> + str x0, [x10]
>
> ret
>
>
>
> //
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112452): https://edk2.groups.io/g/devel/message/112452
Mute This Topic: https://groups.io/mt/100912169/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 28145 bytes --]
prev parent reply other threads:[~2023-12-12 17:10 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-23 11:03 [edk2-devel] [edk2-platforms][PATCH V3 1/1] Platform/ARM/N1sdp: Add support to parse NT_FW_CONFIG sahil
2023-10-11 5:25 ` chandni cherukuri
2023-12-07 10:52 ` Thomas Abraham
2023-12-12 17:10 ` Sami Mujawar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=111f6ffd-eec2-45b6-86c2-1cc5b24bf3c3@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox