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) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_