[PATCH 0/2] ARM Dynamic Configuration

[PATCH 0/2] ARM Dynamic Configuration

[Chandni Cherukuri]

On SGI platforms, the trusted firmware executes prior to the SEC
phase. It supplies to the SEC phase a pointer to a HW CONFIG fdt
in the x1 which contains platform specific information such as part
number and config number of the SGI platform. 

The HW CONFIG FDT would look as,
/dts-v1/;
/ {
        /* compatible string */
        compatible = "arm,sgi-isys3";

	/* platform ID node */
	system-id {
	     platform-id = <0x03000783>;
	}
};

In the very first step of the assembly code which executes in SEC phase
the fdt pointer is stored in a global variable from the register. A PPI 
is created during the SEC phase which stores the global variable.

During PEI phase, a Platform ID PEIM is installed which accessess the PPI
and retrieves the platform information using FDT helper functions and a
PlatformId HOB is created and populated with the information.
 
During DXE phase the drivers can access the Platform ID HOB to get the
platform information and perform platform specific functions based on the
platform.

Chandni Cherukuri (2):
  Platform/ARM/Sgi: Install a Platform ID HOB
  Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

 .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 25 +++++-
 .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 +
 Platform/ARM/SgiPkg/Include/SgiPlatform.h          | 18 +++++
 .../SgiPkg/Library/PlatformLib/AArch64/Helper.S    |  7 ++
 .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   | 10 +++
 .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |  3 +
 .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 92 ++++++++++++++++++++++
 .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf        | 40 ++++++++++
 Platform/ARM/SgiPkg/SgiPlatform.dec                |  5 ++
 Platform/ARM/SgiPkg/SgiPlatform.dsc                |  1 +
 Platform/ARM/SgiPkg/SgiPlatform.fdf                |  1 +
 11 files changed, 203 insertions(+), 1 deletion(-)
 create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
 create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf

-- 
2.7.4

[PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

[Chandni Cherukuri]

On SGI platforms, the trusted firmware executes prior to
the SEC phase. It supplies to the SEC phase a pointer to
a fdt, that contains platform specific information such as
part number and config number of the SGI platform. The
platform code that executes during the SEC phase creates a
PPI that allows access to other PEIMs to obtain a copy of the
fdt pointer.

Further, during the PEI phase, a Platform ID PEIM installs a
Platform ID HOB. The Platform ID HOB can be used by DXE
drivers to identify the platform it is executing on.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
---
 .../SgiPkg/Library/PlatformLib/AArch64/Helper.S    |  15 ++-
 .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
 .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
 .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 +++++++++++++++++++++
 .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf        |  40 ++++++++
 Platform/ARM/SgiPkg/SgiPlatform.dec                |   5 +
 6 files changed, 177 insertions(+), 4 deletions(-)
 create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
 create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf

diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
index dab6c77..80b93ea 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
@@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
 GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
 GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
 GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
+GCC_ASM_EXPORT(HwConfigDtBlob)
+
+HwConfigDtBlob: .long 0
 
 //
 // First platform specific function to be called in the PEI phase
 //
-// This function is actually the first function called by the PrePi
-// or PrePeiCore modules. It allows to retrieve arguments passed to
-// the UEFI firmware through the CPU registers.
-//
+// The trusted firmware passed the hw config DT blob in x1 register.
+// Keep a copy of it in a global variable.
+//VOID
+//ArmPlatformPeiBootAction (
+//  VOID
+//  );
 ASM_PFX(ArmPlatformPeiBootAction):
+  ldr  x10, =HwConfigDtBlob
+  str  x1, [x10]
   ret
 
 //UINTN
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
index ea3201a..54860e0 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
@@ -15,6 +15,10 @@
 #include <Library/ArmPlatformLib.h>
 #include <Library/BaseLib.h>
 #include <Ppi/ArmMpCoreInfo.h>
+#include <SgiPlatform.h>
+
+extern UINT64 HwConfigDtBlob;
+STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
 
 STATIC ARM_CORE_INFO mCoreInfoTable[] = {
   {
@@ -36,6 +40,7 @@ ArmPlatformInitialize (
   IN  UINTN                     MpId
   )
 {
+  mHwConfigDtInfoPpi.HwConfigDtAddr = &HwConfigDtBlob;
   return RETURN_SUCCESS;
 }
 
@@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
     EFI_PEI_PPI_DESCRIPTOR_PPI,
     &gArmMpCoreInfoPpiGuid,
     &mMpCoreInfoPpi
+  },
+  {
+    EFI_PEI_PPI_DESCRIPTOR_PPI,
+    &gHwConfigDtInfoPpiGuid,
+    &mHwConfigDtInfoPpi
   }
 };
 
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
index 42e14d5..5d4bf72 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
@@ -61,7 +61,10 @@
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
 
 [Guids]
+  gArmSgiPlatformIdDescriptorGuid
   gEfiHobListGuid          ## CONSUMES  ## SystemTable
+  gFdtTableGuid
 
 [Ppis]
   gArmMpCoreInfoPpiGuid
+  gHwConfigDtInfoPpiGuid
diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
new file mode 100644
index 0000000..f6446e6
--- /dev/null
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
@@ -0,0 +1,108 @@
+/** @file
+*
+*  Copyright (c) 2018, ARM Limited. All rights reserved.
+*
+*  This program and the accompanying materials are licensed and made available
+*  under the terms and conditions of the BSD License which accompanies this
+*  distribution.  The full text of the license may be found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/PeimEntryPoint.h>
+#include <Library/PeiServicesLib.h>
+#include <libfdt.h>
+#include <SgiPlatform.h>
+
+/**
+
+  This function returns the Platform ID of the platform
+
+  This functions locates the HwConfig PPI and gets the base address of HW CONFIG
+  DT from which the platform ID is obtained using FDT helper functions
+
+  @return returns the platform ID on success else returns 0 on error
+
+**/
+
+STATIC
+UINT32
+GetSgiPlatformId (
+ VOID
+)
+{
+  CONST UINT32                  *Property;
+  INT32                         Offset;
+  CONST VOID                    *HwCfgDtBlob;
+  SGI_HW_CONFIG_INFO_PPI        *HwConfigInfoPpi;
+  EFI_STATUS                    Status;
+
+  Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL, (VOID**)&HwConfigInfoPpi);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "PeiServicesLocatePpi failed with error %r\n", Status));
+    return 0;
+  }
+
+  HwCfgDtBlob = (VOID *)(*(HwConfigInfoPpi->HwConfigDtAddr));
+  if (fdt_check_header (HwCfgDtBlob) != 0 ) {
+    DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed as HW_CONFIG\n", HwCfgDtBlob));
+    return 0;
+  }
+
+  Offset = fdt_subnode_offset (HwCfgDtBlob, 0, "system-id");
+  if (Offset == -FDT_ERR_NOTFOUND) {
+    DEBUG ((DEBUG_ERROR, "Invalid DTB : system-id node not found\n"));
+    return 0;
+  }
+
+  Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
+  if (Property == NULL) {
+    DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
+    return 0;
+  }
+
+  return fdt32_to_cpu (*Property);
+}
+
+/**
+
+ This function creates a Platform ID HOB and assigns PlatformId as the
+ HobData
+
+ @return asserts on error and returns EFI_INVALID_PARAMETER. On success
+ returns EFI_SUCCESS
+
+**/
+EFI_STATUS
+EFIAPI
+SgiPlatformIdPeim (
+ IN       EFI_PEI_FILE_HANDLE  FileHandle,
+ IN CONST EFI_PEI_SERVICES     **PeiServices
+)
+{
+  SGI_PLATFORM_DESCRIPTOR       *HobData;
+
+  //Create platform descriptor HOB
+  HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (&gArmSgiPlatformIdDescriptorGuid,
+            sizeof (SGI_PLATFORM_DESCRIPTOR));
+
+  //Get the platform id from the platform specific hw_config device tree
+  if (HobData != NULL) {
+    HobData->PlatformId = GetSgiPlatformId ();
+    if (HobData->PlatformId == 0) {
+      ASSERT (FALSE);
+      return EFI_INVALID_PARAMETER;
+    }
+  } else {
+    DEBUG ((DEBUG_ERROR, "Platform HoB is NULL\n"));
+    ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  return EFI_SUCCESS;
+}
diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
new file mode 100644
index 0000000..502d6ac
--- /dev/null
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
@@ -0,0 +1,40 @@
+#
+#  Copyright (c) 2018, ARM Limited. All rights reserved.
+#
+#  This program and the accompanying materials are licensed and made available
+#  under the terms and conditions of the BSD License which accompanies this
+#  distribution.  The full text of the license may be found at
+#  http://opensource.org/licenses/bsd-license.php
+#
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = SgiPlatformIdPeiLib
+  FILE_GUID                      = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = SgiPlatformIdPeim
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
+  Platform/ARM/SgiPkg/SgiPlatform.dec
+
+[LibraryClasses]
+  FdtLib
+  PeimEntryPoint
+
+[Sources]
+  SgiPlatformPeiLib.c
+
+[Guids]
+  gArmSgiPlatformIdDescriptorGuid
+
+[Ppis]
+  gHwConfigDtInfoPpiGuid
+
+[Depex]
+  TRUE
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
index d995937..ea9f6c5 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -29,9 +29,14 @@
   Include                        # Root include for the package
 
 [Guids.common]
+  # ARM Sgi Platform ID descriptor
+  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
   gArmSgiTokenSpaceGuid      = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
   gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
 
 [PcdsFeatureFlag.common]
   # Set this PCD to TRUE to enable virtio support.
   gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
+
+[Ppis]
+  gHwConfigDtInfoPpiGuid     = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
-- 
2.7.4

Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

[Ard Biesheuvel]

Hello Chandni,

On 15 June 2018 at 07:55, Chandni Cherukuri <chandni.cherukuri@arm.com> wrote:
> On SGI platforms, the trusted firmware executes prior to
> the SEC phase. It supplies to the SEC phase a pointer to
> a fdt, that contains platform specific information such as
> part number and config number of the SGI platform. The
> platform code that executes during the SEC phase creates a
> PPI that allows access to other PEIMs to obtain a copy of the
> fdt pointer.
>
> Further, during the PEI phase, a Platform ID PEIM installs a
> Platform ID HOB. The Platform ID HOB can be used by DXE
> drivers to identify the platform it is executing on.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> ---
>  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S    |  15 ++-
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
>  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 +++++++++++++++++++++
>  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf        |  40 ++++++++
>  Platform/ARM/SgiPkg/SgiPlatform.dec                |   5 +
>  6 files changed, 177 insertions(+), 4 deletions(-)
>  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>

Please use '--stat=250 --stat-graph-width=20' when using git
format-patch so that we get to see the entire path names in the
diffstat

> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> index dab6c77..80b93ea 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> +GCC_ASM_EXPORT(HwConfigDtBlob)
> +
> +HwConfigDtBlob: .long 0
>

Please move this definition to PlatformLib.c

>  //
>  // First platform specific function to be called in the PEI phase
>  //
> -// This function is actually the first function called by the PrePi
> -// or PrePeiCore modules. It allows to retrieve arguments passed to
> -// the UEFI firmware through the CPU registers.
> -//
> +// The trusted firmware passed the hw config DT blob in x1 register.
> +// Keep a copy of it in a global variable.
> +//VOID
> +//ArmPlatformPeiBootAction (
> +//  VOID
> +//  );
>  ASM_PFX(ArmPlatformPeiBootAction):
> +  ldr  x10, =HwConfigDtBlob

Please use adr to take the address of HwConfigDtBlob

> +  str  x1, [x10]
>    ret
>
>  //UINTN
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> index ea3201a..54860e0 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> @@ -15,6 +15,10 @@
>  #include <Library/ArmPlatformLib.h>
>  #include <Library/BaseLib.h>
>  #include <Ppi/ArmMpCoreInfo.h>
> +#include <SgiPlatform.h>
> +
> +extern UINT64 HwConfigDtBlob;
> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>

Where is this PPI declared? A PPI is like a protocol, it has its own
declaration in a header file under Include/Ppi in the package

>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>    {
> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>    IN  UINTN                     MpId
>    )
>  {
> +  mHwConfigDtInfoPpi.HwConfigDtAddr = &HwConfigDtBlob;
>    return RETURN_SUCCESS;
>  }
>
> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>      EFI_PEI_PPI_DESCRIPTOR_PPI,
>      &gArmMpCoreInfoPpiGuid,
>      &mMpCoreInfoPpi
> +  },
> +  {
> +    EFI_PEI_PPI_DESCRIPTOR_PPI,
> +    &gHwConfigDtInfoPpiGuid,
> +    &mHwConfigDtInfoPpi
>    }
>  };
>
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> index 42e14d5..5d4bf72 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> @@ -61,7 +61,10 @@
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>    gEfiHobListGuid          ## CONSUMES  ## SystemTable
> +  gFdtTableGuid
>
>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> +  gHwConfigDtInfoPpiGuid
> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> new file mode 100644
> index 0000000..f6446e6
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> @@ -0,0 +1,108 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PeimEntryPoint.h>
> +#include <Library/PeiServicesLib.h>
> +#include <libfdt.h>
> +#include <SgiPlatform.h>
> +
> +/**
> +
> +  This function returns the Platform ID of the platform
> +
> +  This functions locates the HwConfig PPI and gets the base address of HW CONFIG
> +  DT from which the platform ID is obtained using FDT helper functions
> +
> +  @return returns the platform ID on success else returns 0 on error
> +
> +**/
> +
> +STATIC
> +UINT32
> +GetSgiPlatformId (
> + VOID
> +)
> +{
> +  CONST UINT32                  *Property;
> +  INT32                         Offset;
> +  CONST VOID                    *HwCfgDtBlob;
> +  SGI_HW_CONFIG_INFO_PPI        *HwConfigInfoPpi;
> +  EFI_STATUS                    Status;
> +
> +  Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL, (VOID**)&HwConfigInfoPpi);

Please check all your patches for overly long lines. 80 columns is that maximum

> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "PeiServicesLocatePpi failed with error %r\n", Status));
> +    return 0;
> +  }
> +
> +  HwCfgDtBlob = (VOID *)(*(HwConfigInfoPpi->HwConfigDtAddr));
> +  if (fdt_check_header (HwCfgDtBlob) != 0 ) {
> +    DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed as HW_CONFIG\n", HwCfgDtBlob));
> +    return 0;
> +  }
> +
> +  Offset = fdt_subnode_offset (HwCfgDtBlob, 0, "system-id");
> +  if (Offset == -FDT_ERR_NOTFOUND) {
> +    DEBUG ((DEBUG_ERROR, "Invalid DTB : system-id node not found\n"));
> +    return 0;
> +  }
> +
> +  Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
> +  if (Property == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
> +    return 0;
> +  }
> +
> +  return fdt32_to_cpu (*Property);
> +}
> +
> +/**
> +
> + This function creates a Platform ID HOB and assigns PlatformId as the
> + HobData
> +
> + @return asserts on error and returns EFI_INVALID_PARAMETER. On success
> + returns EFI_SUCCESS
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SgiPlatformIdPeim (
> + IN       EFI_PEI_FILE_HANDLE  FileHandle,
> + IN CONST EFI_PEI_SERVICES     **PeiServices
> +)
> +{
> +  SGI_PLATFORM_DESCRIPTOR       *HobData;
> +
> +  //Create platform descriptor HOB
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (&gArmSgiPlatformIdDescriptorGuid,
> +            sizeof (SGI_PLATFORM_DESCRIPTOR));
> +
> +  //Get the platform id from the platform specific hw_config device tree
> +  if (HobData != NULL) {

Please invert the sense of this conditional, and return
EFI_OUT_OF_RESOURCES if HobData is NULL

> +    HobData->PlatformId = GetSgiPlatformId ();
> +    if (HobData->PlatformId == 0) {
> +      ASSERT (FALSE);
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  } else {

After you have inverted the if() condition, you can drop the else as well

> +    DEBUG ((DEBUG_ERROR, "Platform HoB is NULL\n"));
> +    ASSERT (FALSE);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> new file mode 100644
> index 0000000..502d6ac
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> @@ -0,0 +1,40 @@
> +#
> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials are licensed and made available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = SgiPlatformIdPeiLib
> +  FILE_GUID                      = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = SgiPlatformIdPeim
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/ARM/SgiPkg/SgiPlatform.dec
> +
> +[LibraryClasses]
> +  FdtLib
> +  PeimEntryPoint
> +
> +[Sources]
> +  SgiPlatformPeiLib.c
> +
> +[Guids]
> +  gArmSgiPlatformIdDescriptorGuid
> +
> +[Ppis]
> +  gHwConfigDtInfoPpiGuid
> +
> +[Depex]
> +  TRUE
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
> index d995937..ea9f6c5 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> @@ -29,9 +29,14 @@
>    Include                        # Root include for the package
>
>  [Guids.common]
> +  # ARM Sgi Platform ID descriptor
> +  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
>    gArmSgiTokenSpaceGuid      = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
>    gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
>
>  [PcdsFeatureFlag.common]
>    # Set this PCD to TRUE to enable virtio support.
>    gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
> +
> +[Ppis]
> +  gHwConfigDtInfoPpiGuid     = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> --
> 2.7.4
>

Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

[Ard Biesheuvel]

On 15 June 2018 at 08:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Hello Chandni,
>
> On 15 June 2018 at 07:55, Chandni Cherukuri <chandni.cherukuri@arm.com> wrote:
>> On SGI platforms, the trusted firmware executes prior to
>> the SEC phase. It supplies to the SEC phase a pointer to
>> a fdt, that contains platform specific information such as
>> part number and config number of the SGI platform. The
>> platform code that executes during the SEC phase creates a
>> PPI that allows access to other PEIMs to obtain a copy of the
>> fdt pointer.
>>
>> Further, during the PEI phase, a Platform ID PEIM installs a
>> Platform ID HOB. The Platform ID HOB can be used by DXE
>> drivers to identify the platform it is executing on.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
>> ---
>>  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S    |  15 ++-
>>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
>>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
>>  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 +++++++++++++++++++++
>>  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf        |  40 ++++++++
>>  Platform/ARM/SgiPkg/SgiPlatform.dec                |   5 +
>>  6 files changed, 177 insertions(+), 4 deletions(-)
>>  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>>  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>>
>
> Please use '--stat=250 --stat-graph-width=20' when using git
> format-patch so that we get to see the entire path names in the
> diffstat
>
>> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
>> index dab6c77..80b93ea 100644
>> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
>> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
>> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
>> +GCC_ASM_EXPORT(HwConfigDtBlob)
>> +
>> +HwConfigDtBlob: .long 0
>>
>
> Please move this definition to PlatformLib.c
>
>>  //
>>  // First platform specific function to be called in the PEI phase
>>  //
>> -// This function is actually the first function called by the PrePi
>> -// or PrePeiCore modules. It allows to retrieve arguments passed to
>> -// the UEFI firmware through the CPU registers.
>> -//
>> +// The trusted firmware passed the hw config DT blob in x1 register.
>> +// Keep a copy of it in a global variable.
>> +//VOID
>> +//ArmPlatformPeiBootAction (
>> +//  VOID
>> +//  );
>>  ASM_PFX(ArmPlatformPeiBootAction):
>> +  ldr  x10, =HwConfigDtBlob
>
> Please use adr to take the address of HwConfigDtBlob
>
>> +  str  x1, [x10]
>>    ret
>>
>>  //UINTN
>> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
>> index ea3201a..54860e0 100644
>> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
>> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
>> @@ -15,6 +15,10 @@
>>  #include <Library/ArmPlatformLib.h>
>>  #include <Library/BaseLib.h>
>>  #include <Ppi/ArmMpCoreInfo.h>
>> +#include <SgiPlatform.h>
>> +
>> +extern UINT64 HwConfigDtBlob;
>> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>>
>
> Where is this PPI declared? A PPI is like a protocol, it has its own
> declaration in a header file under Include/Ppi in the package
>
>>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>>    {
>> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>>    IN  UINTN                     MpId
>>    )
>>  {
>> +  mHwConfigDtInfoPpi.HwConfigDtAddr = &HwConfigDtBlob;
>>    return RETURN_SUCCESS;
>>  }
>>
>> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>>      EFI_PEI_PPI_DESCRIPTOR_PPI,
>>      &gArmMpCoreInfoPpiGuid,
>>      &mMpCoreInfoPpi
>> +  },
>> +  {
>> +    EFI_PEI_PPI_DESCRIPTOR_PPI,
>> +    &gHwConfigDtInfoPpiGuid,
>> +    &mHwConfigDtInfoPpi
>>    }
>>  };
>>
>> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
>> index 42e14d5..5d4bf72 100644
>> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
>> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
>> @@ -61,7 +61,10 @@
>>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>
>>  [Guids]
>> +  gArmSgiPlatformIdDescriptorGuid
>>    gEfiHobListGuid          ## CONSUMES  ## SystemTable
>> +  gFdtTableGuid
>>
>>  [Ppis]
>>    gArmMpCoreInfoPpiGuid
>> +  gHwConfigDtInfoPpiGuid
>> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>> new file mode 100644
>> index 0000000..f6446e6
>> --- /dev/null
>> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>> @@ -0,0 +1,108 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
>> +*
>> +*  This program and the accompanying materials are licensed and made available
>> +*  under the terms and conditions of the BSD License which accompanies this
>> +*  distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#include <Library/DebugLib.h>
>> +#include <Library/HobLib.h>
>> +#include <Library/PeimEntryPoint.h>
>> +#include <Library/PeiServicesLib.h>
>> +#include <libfdt.h>
>> +#include <SgiPlatform.h>
>> +
>> +/**
>> +
>> +  This function returns the Platform ID of the platform
>> +
>> +  This functions locates the HwConfig PPI and gets the base address of HW CONFIG
>> +  DT from which the platform ID is obtained using FDT helper functions
>> +
>> +  @return returns the platform ID on success else returns 0 on error
>> +
>> +**/
>> +
>> +STATIC
>> +UINT32
>> +GetSgiPlatformId (
>> + VOID
>> +)
>> +{
>> +  CONST UINT32                  *Property;
>> +  INT32                         Offset;
>> +  CONST VOID                    *HwCfgDtBlob;
>> +  SGI_HW_CONFIG_INFO_PPI        *HwConfigInfoPpi;
>> +  EFI_STATUS                    Status;
>> +
>> +  Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL, (VOID**)&HwConfigInfoPpi);
>
> Please check all your patches for overly long lines. 80 columns is that maximum
>
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "PeiServicesLocatePpi failed with error %r\n", Status));
>> +    return 0;
>> +  }
>> +
>> +  HwCfgDtBlob = (VOID *)(*(HwConfigInfoPpi->HwConfigDtAddr));
>> +  if (fdt_check_header (HwCfgDtBlob) != 0 ) {
>> +    DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed as HW_CONFIG\n", HwCfgDtBlob));
>> +    return 0;
>> +  }
>> +
>> +  Offset = fdt_subnode_offset (HwCfgDtBlob, 0, "system-id");
>> +  if (Offset == -FDT_ERR_NOTFOUND) {
>> +    DEBUG ((DEBUG_ERROR, "Invalid DTB : system-id node not found\n"));
>> +    return 0;
>> +  }
>> +
>> +  Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
>> +  if (Property == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
>> +    return 0;
>> +  }
>> +
>> +  return fdt32_to_cpu (*Property);
>> +}
>> +
>> +/**
>> +
>> + This function creates a Platform ID HOB and assigns PlatformId as the
>> + HobData
>> +
>> + @return asserts on error and returns EFI_INVALID_PARAMETER. On success
>> + returns EFI_SUCCESS
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +SgiPlatformIdPeim (
>> + IN       EFI_PEI_FILE_HANDLE  FileHandle,
>> + IN CONST EFI_PEI_SERVICES     **PeiServices
>> +)
>> +{
>> +  SGI_PLATFORM_DESCRIPTOR       *HobData;
>> +
>> +  //Create platform descriptor HOB
>> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (&gArmSgiPlatformIdDescriptorGuid,
>> +            sizeof (SGI_PLATFORM_DESCRIPTOR));
>> +
>> +  //Get the platform id from the platform specific hw_config device tree
>> +  if (HobData != NULL) {
>
> Please invert the sense of this conditional, and return
> EFI_OUT_OF_RESOURCES if HobData is NULL
>
>> +    HobData->PlatformId = GetSgiPlatformId ();
>> +    if (HobData->PlatformId == 0) {
>> +      ASSERT (FALSE);
>> +      return EFI_INVALID_PARAMETER;
>> +    }
>> +  } else {
>
> After you have inverted the if() condition, you can drop the else as well
>
>> +    DEBUG ((DEBUG_ERROR, "Platform HoB is NULL\n"));
>> +    ASSERT (FALSE);
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>> new file mode 100644
>> index 0000000..502d6ac
>> --- /dev/null
>> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>> @@ -0,0 +1,40 @@
>> +#
>> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
>> +#
>> +#  This program and the accompanying materials are licensed and made available
>> +#  under the terms and conditions of the BSD License which accompanies this
>> +#  distribution.  The full text of the license may be found at
>> +#  http://opensource.org/licenses/bsd-license.php
>> +#
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +#
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x0001001A
>> +  BASE_NAME                      = SgiPlatformIdPeiLib

I forgot: this is a module not a library. So it should be called
SgiPlatformIdPei or SgiPlatformIdPeim not ...Lib

>> +  FILE_GUID                      = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
>> +  MODULE_TYPE                    = PEIM
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = SgiPlatformIdPeim
>> +
>> +[Packages]
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  MdePkg/MdePkg.dec
>> +  Platform/ARM/SgiPkg/SgiPlatform.dec
>> +
>> +[LibraryClasses]
>> +  FdtLib
>> +  PeimEntryPoint
>> +
>> +[Sources]
>> +  SgiPlatformPeiLib.c
>> +
>> +[Guids]
>> +  gArmSgiPlatformIdDescriptorGuid
>> +
>> +[Ppis]
>> +  gHwConfigDtInfoPpiGuid
>> +
>> +[Depex]
>> +  TRUE
>> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
>> index d995937..ea9f6c5 100644
>> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
>> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
>> @@ -29,9 +29,14 @@
>>    Include                        # Root include for the package
>>
>>  [Guids.common]
>> +  # ARM Sgi Platform ID descriptor
>> +  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
>>    gArmSgiTokenSpaceGuid      = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
>>    gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
>>
>>  [PcdsFeatureFlag.common]
>>    # Set this PCD to TRUE to enable virtio support.
>>    gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
>> +
>> +[Ppis]
>> +  gHwConfigDtInfoPpiGuid     = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
>> --
>> 2.7.4
>>

Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

[Leif Lindholm]

On Fri, Jun 15, 2018 at 08:19:35AM +0200, Ard Biesheuvel wrote:
> On 15 June 2018 at 08:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> >> new file mode 100644
> >> index 0000000..502d6ac
> >> --- /dev/null
> >> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> >> @@ -0,0 +1,40 @@
> >> +#
> >> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> >> +#
> >> +#  This program and the accompanying materials are licensed and made available
> >> +#  under the terms and conditions of the BSD License which accompanies this
> >> +#  distribution.  The full text of the license may be found at
> >> +#  http://opensource.org/licenses/bsd-license.php
> >> +#
> >> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +#
> >> +
> >> +[Defines]
> >> +  INF_VERSION                    = 0x0001001A
> >> +  BASE_NAME                      = SgiPlatformIdPeiLib
> 
> I forgot: this is a module not a library. So it should be called
> SgiPlatformIdPei or SgiPlatformIdPeim not ...Lib

A grep through the edk2 tree suggests that:
- the directory is usually called *Pei
- .inf/.uni usually *Pei.*
- entry point and source files *Peim*

/
    Leif

> >> +  FILE_GUID                      = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
> >> +  MODULE_TYPE                    = PEIM
> >> +  VERSION_STRING                 = 1.0
> >> +  ENTRY_POINT                    = SgiPlatformIdPeim
> >> +
> >> +[Packages]
> >> +  EmbeddedPkg/EmbeddedPkg.dec
> >> +  MdePkg/MdePkg.dec
> >> +  Platform/ARM/SgiPkg/SgiPlatform.dec
> >> +
> >> +[LibraryClasses]
> >> +  FdtLib
> >> +  PeimEntryPoint
> >> +
> >> +[Sources]
> >> +  SgiPlatformPeiLib.c
> >> +
> >> +[Guids]
> >> +  gArmSgiPlatformIdDescriptorGuid
> >> +
> >> +[Ppis]
> >> +  gHwConfigDtInfoPpiGuid
> >> +
> >> +[Depex]
> >> +  TRUE
> >> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
> >> index d995937..ea9f6c5 100644
> >> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> >> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> >> @@ -29,9 +29,14 @@
> >>    Include                        # Root include for the package
> >>
> >>  [Guids.common]
> >> +  # ARM Sgi Platform ID descriptor
> >> +  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
> >>    gArmSgiTokenSpaceGuid      = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
> >>    gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
> >>
> >>  [PcdsFeatureFlag.common]
> >>    # Set this PCD to TRUE to enable virtio support.
> >>    gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
> >> +
> >> +[Ppis]
> >> +  gHwConfigDtInfoPpiGuid     = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> >> --
> >> 2.7.4
> >>

Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

[Leif Lindholm]

Just some minor formatting points in addition to the comment on Ard's
reply..

On Fri, Jun 15, 2018 at 11:25:31AM +0530, Chandni Cherukuri wrote:
> On SGI platforms, the trusted firmware executes prior to
> the SEC phase. It supplies to the SEC phase a pointer to
> a fdt, that contains platform specific information such as
> part number and config number of the SGI platform. The
> platform code that executes during the SEC phase creates a
> PPI that allows access to other PEIMs to obtain a copy of the
> fdt pointer.
> 
> Further, during the PEI phase, a Platform ID PEIM installs a
> Platform ID HOB. The Platform ID HOB can be used by DXE
> drivers to identify the platform it is executing on.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> ---
>  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S    |  15 ++-
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
>  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 +++++++++++++++++++++
>  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf        |  40 ++++++++
>  Platform/ARM/SgiPkg/SgiPlatform.dec                |   5 +
>  6 files changed, 177 insertions(+), 4 deletions(-)
>  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> 
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> index dab6c77..80b93ea 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> +GCC_ASM_EXPORT(HwConfigDtBlob)
> +
> +HwConfigDtBlob: .long 0
>  
>  //
>  // First platform specific function to be called in the PEI phase
>  //
> -// This function is actually the first function called by the PrePi
> -// or PrePeiCore modules. It allows to retrieve arguments passed to
> -// the UEFI firmware through the CPU registers.
> -//
> +// The trusted firmware passed the hw config DT blob in x1 register.
> +// Keep a copy of it in a global variable.
> +//VOID
> +//ArmPlatformPeiBootAction (
> +//  VOID
> +//  );
>  ASM_PFX(ArmPlatformPeiBootAction):
> +  ldr  x10, =HwConfigDtBlob
> +  str  x1, [x10]
>    ret
>  
>  //UINTN
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> index ea3201a..54860e0 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> @@ -15,6 +15,10 @@
>  #include <Library/ArmPlatformLib.h>
>  #include <Library/BaseLib.h>
>  #include <Ppi/ArmMpCoreInfo.h>
> +#include <SgiPlatform.h>
> +
> +extern UINT64 HwConfigDtBlob;
> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>  
>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>    {
> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>    IN  UINTN                     MpId
>    )
>  {
> +  mHwConfigDtInfoPpi.HwConfigDtAddr = &HwConfigDtBlob;
>    return RETURN_SUCCESS;
>  }
>  
> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>      EFI_PEI_PPI_DESCRIPTOR_PPI,
>      &gArmMpCoreInfoPpiGuid,
>      &mMpCoreInfoPpi
> +  },
> +  {
> +    EFI_PEI_PPI_DESCRIPTOR_PPI,
> +    &gHwConfigDtInfoPpiGuid,
> +    &mHwConfigDtInfoPpi
>    }
>  };
>  
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> index 42e14d5..5d4bf72 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> @@ -61,7 +61,10 @@
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>  
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>    gEfiHobListGuid          ## CONSUMES  ## SystemTable
> +  gFdtTableGuid
>  
>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> +  gHwConfigDtInfoPpiGuid
> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> new file mode 100644
> index 0000000..f6446e6
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> @@ -0,0 +1,108 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PeimEntryPoint.h>
> +#include <Library/PeiServicesLib.h>
> +#include <libfdt.h>
> +#include <SgiPlatform.h>
> +
> +/**
> +
> +  This function returns the Platform ID of the platform
> +
> +  This functions locates the HwConfig PPI and gets the base address of HW CONFIG
> +  DT from which the platform ID is obtained using FDT helper functions
> +
> +  @return returns the platform ID on success else returns 0 on error
> +
> +**/
> +
> +STATIC
> +UINT32
> +GetSgiPlatformId (
> + VOID
> +)
> +{
> +  CONST UINT32                  *Property;
> +  INT32                         Offset;
> +  CONST VOID                    *HwCfgDtBlob;
> +  SGI_HW_CONFIG_INFO_PPI        *HwConfigInfoPpi;
> +  EFI_STATUS                    Status;
> +
> +  Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL, (VOID**)&HwConfigInfoPpi);

93 characters. Please run BaseTools/Scripts/PatchCheck.py.

> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "PeiServicesLocatePpi failed with error %r\n", Status));
> +    return 0;
> +  }
> +
> +  HwCfgDtBlob = (VOID *)(*(HwConfigInfoPpi->HwConfigDtAddr));
> +  if (fdt_check_header (HwCfgDtBlob) != 0 ) {

Trailing space after 0.

> +    DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed as HW_CONFIG\n", HwCfgDtBlob));
> +    return 0;
> +  }
> +
> +  Offset = fdt_subnode_offset (HwCfgDtBlob, 0, "system-id");
> +  if (Offset == -FDT_ERR_NOTFOUND) {
> +    DEBUG ((DEBUG_ERROR, "Invalid DTB : system-id node not found\n"));
> +    return 0;
> +  }
> +
> +  Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
> +  if (Property == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
> +    return 0;
> +  }
> +
> +  return fdt32_to_cpu (*Property);
> +}
> +
> +/**
> +
> + This function creates a Platform ID HOB and assigns PlatformId as the
> + HobData
> +
> + @return asserts on error and returns EFI_INVALID_PARAMETER. On success
> + returns EFI_SUCCESS
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SgiPlatformIdPeim (
> + IN       EFI_PEI_FILE_HANDLE  FileHandle,
> + IN CONST EFI_PEI_SERVICES     **PeiServices
> +)
> +{
> +  SGI_PLATFORM_DESCRIPTOR       *HobData;
> +
> +  //Create platform descriptor HOB
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (&gArmSgiPlatformIdDescriptorGuid,
> +            sizeof (SGI_PLATFORM_DESCRIPTOR));
> +
> +  //Get the platform id from the platform specific hw_config device tree
> +  if (HobData != NULL) {
> +    HobData->PlatformId = GetSgiPlatformId ();
> +    if (HobData->PlatformId == 0) {
> +      ASSERT (FALSE);
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "Platform HoB is NULL\n"));

HOB

/
    Leif

> +    ASSERT (FALSE);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> new file mode 100644
> index 0000000..502d6ac
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> @@ -0,0 +1,40 @@
> +#
> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials are licensed and made available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = SgiPlatformIdPeiLib
> +  FILE_GUID                      = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = SgiPlatformIdPeim
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/ARM/SgiPkg/SgiPlatform.dec
> +
> +[LibraryClasses]
> +  FdtLib
> +  PeimEntryPoint
> +
> +[Sources]
> +  SgiPlatformPeiLib.c
> +
> +[Guids]
> +  gArmSgiPlatformIdDescriptorGuid
> +
> +[Ppis]
> +  gHwConfigDtInfoPpiGuid
> +
> +[Depex]
> +  TRUE
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
> index d995937..ea9f6c5 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> @@ -29,9 +29,14 @@
>    Include                        # Root include for the package
>  
>  [Guids.common]
> +  # ARM Sgi Platform ID descriptor
> +  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
>    gArmSgiTokenSpaceGuid      = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
>    gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
>  
>  [PcdsFeatureFlag.common]
>    # Set this PCD to TRUE to enable virtio support.
>    gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
> +
> +[Ppis]
> +  gHwConfigDtInfoPpiGuid     = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

[chandni cherukuri]

On Fri, Jun 15, 2018 at 3:48 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> Just some minor formatting points in addition to the comment on Ard's
> reply..
>
> On Fri, Jun 15, 2018 at 11:25:31AM +0530, Chandni Cherukuri wrote:
> > On SGI platforms, the trusted firmware executes prior to
> > the SEC phase. It supplies to the SEC phase a pointer to
> > a fdt, that contains platform specific information such as
> > part number and config number of the SGI platform. The
> > platform code that executes during the SEC phase creates a
> > PPI that allows access to other PEIMs to obtain a copy of the
> > fdt pointer.
> >
> > Further, during the PEI phase, a Platform ID PEIM installs a
> > Platform ID HOB. The Platform ID HOB can be used by DXE
> > drivers to identify the platform it is executing on.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> > ---
> >  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S    |  15 ++-
> >  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
> >  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
> >  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 +++++++++++++++++++++
> >  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf        |  40 ++++++++
> >  Platform/ARM/SgiPkg/SgiPlatform.dec                |   5 +
> >  6 files changed, 177 insertions(+), 4 deletions(-)
> >  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> >  create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> >
> > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> > index dab6c77..80b93ea 100644
> > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> > @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
> >  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
> >  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
> >  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> > +GCC_ASM_EXPORT(HwConfigDtBlob)
> > +
> > +HwConfigDtBlob: .long 0
> >
> >  //
> >  // First platform specific function to be called in the PEI phase
> >  //
> > -// This function is actually the first function called by the PrePi
> > -// or PrePeiCore modules. It allows to retrieve arguments passed to
> > -// the UEFI firmware through the CPU registers.
> > -//
> > +// The trusted firmware passed the hw config DT blob in x1 register.
> > +// Keep a copy of it in a global variable.
> > +//VOID
> > +//ArmPlatformPeiBootAction (
> > +//  VOID
> > +//  );
> >  ASM_PFX(ArmPlatformPeiBootAction):
> > +  ldr  x10, =HwConfigDtBlob
> > +  str  x1, [x10]
> >    ret
> >
> >  //UINTN
> > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> > index ea3201a..54860e0 100644
> > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> > @@ -15,6 +15,10 @@
> >  #include <Library/ArmPlatformLib.h>
> >  #include <Library/BaseLib.h>
> >  #include <Ppi/ArmMpCoreInfo.h>
> > +#include <SgiPlatform.h>
> > +
> > +extern UINT64 HwConfigDtBlob;
> > +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
> >
> >  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
> >    {
> > @@ -36,6 +40,7 @@ ArmPlatformInitialize (
> >    IN  UINTN                     MpId
> >    )
> >  {
> > +  mHwConfigDtInfoPpi.HwConfigDtAddr = &HwConfigDtBlob;
> >    return RETURN_SUCCESS;
> >  }
> >
> > @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
> >      EFI_PEI_PPI_DESCRIPTOR_PPI,
> >      &gArmMpCoreInfoPpiGuid,
> >      &mMpCoreInfoPpi
> > +  },
> > +  {
> > +    EFI_PEI_PPI_DESCRIPTOR_PPI,
> > +    &gHwConfigDtInfoPpiGuid,
> > +    &mHwConfigDtInfoPpi
> >    }
> >  };
> >
> > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> > index 42e14d5..5d4bf72 100644
> > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> > @@ -61,7 +61,10 @@
> >    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> >
> >  [Guids]
> > +  gArmSgiPlatformIdDescriptorGuid
> >    gEfiHobListGuid          ## CONSUMES  ## SystemTable
> > +  gFdtTableGuid
> >
> >  [Ppis]
> >    gArmMpCoreInfoPpiGuid
> > +  gHwConfigDtInfoPpiGuid
> > diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> > new file mode 100644
> > index 0000000..f6446e6
> > --- /dev/null
> > +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> > @@ -0,0 +1,108 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> > +*
> > +*  This program and the accompanying materials are licensed and made available
> > +*  under the terms and conditions of the BSD License which accompanies this
> > +*  distribution.  The full text of the license may be found at
> > +*  http://opensource.org/licenses/bsd-license.php
> > +*
> > +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +*
> > +**/
> > +
> > +#include <Library/DebugLib.h>
> > +#include <Library/HobLib.h>
> > +#include <Library/PeimEntryPoint.h>
> > +#include <Library/PeiServicesLib.h>
> > +#include <libfdt.h>
> > +#include <SgiPlatform.h>
> > +
> > +/**
> > +
> > +  This function returns the Platform ID of the platform
> > +
> > +  This functions locates the HwConfig PPI and gets the base address of HW CONFIG
> > +  DT from which the platform ID is obtained using FDT helper functions
> > +
> > +  @return returns the platform ID on success else returns 0 on error
> > +
> > +**/
> > +
> > +STATIC
> > +UINT32
> > +GetSgiPlatformId (
> > + VOID
> > +)
> > +{
> > +  CONST UINT32                  *Property;
> > +  INT32                         Offset;
> > +  CONST VOID                    *HwCfgDtBlob;
> > +  SGI_HW_CONFIG_INFO_PPI        *HwConfigInfoPpi;
> > +  EFI_STATUS                    Status;
> > +
> > +  Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL, (VOID**)&HwConfigInfoPpi);
>
> 93 characters. Please run BaseTools/Scripts/PatchCheck.py.
>
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "PeiServicesLocatePpi failed with error %r\n", Status));
> > +    return 0;
> > +  }
> > +
> > +  HwCfgDtBlob = (VOID *)(*(HwConfigInfoPpi->HwConfigDtAddr));
> > +  if (fdt_check_header (HwCfgDtBlob) != 0 ) {
>
> Trailing space after 0.
>
> > +    DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed as HW_CONFIG\n", HwCfgDtBlob));
> > +    return 0;
> > +  }
> > +
> > +  Offset = fdt_subnode_offset (HwCfgDtBlob, 0, "system-id");
> > +  if (Offset == -FDT_ERR_NOTFOUND) {
> > +    DEBUG ((DEBUG_ERROR, "Invalid DTB : system-id node not found\n"));
> > +    return 0;
> > +  }
> > +
> > +  Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
> > +  if (Property == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
> > +    return 0;
> > +  }
> > +
> > +  return fdt32_to_cpu (*Property);
> > +}
> > +
> > +/**
> > +
> > + This function creates a Platform ID HOB and assigns PlatformId as the
> > + HobData
> > +
> > + @return asserts on error and returns EFI_INVALID_PARAMETER. On success
> > + returns EFI_SUCCESS
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +SgiPlatformIdPeim (
> > + IN       EFI_PEI_FILE_HANDLE  FileHandle,
> > + IN CONST EFI_PEI_SERVICES     **PeiServices
> > +)
> > +{
> > +  SGI_PLATFORM_DESCRIPTOR       *HobData;
> > +
> > +  //Create platform descriptor HOB
> > +  HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (&gArmSgiPlatformIdDescriptorGuid,
> > +            sizeof (SGI_PLATFORM_DESCRIPTOR));
> > +
> > +  //Get the platform id from the platform specific hw_config device tree
> > +  if (HobData != NULL) {
> > +    HobData->PlatformId = GetSgiPlatformId ();
> > +    if (HobData->PlatformId == 0) {
> > +      ASSERT (FALSE);
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +  } else {
> > +    DEBUG ((DEBUG_ERROR, "Platform HoB is NULL\n"));
>
> HOB
>
> /
>     Leif


Hi Ard, Leif,

Thank you for your comments. All the changes as per your comments have
been made and updated patches have been posted for your review.

Thanks,
Chandni.




>
> > +    ASSERT (FALSE);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> > new file mode 100644
> > index 0000000..502d6ac
> > --- /dev/null
> > +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> > @@ -0,0 +1,40 @@
> > +#
> > +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> > +#
> > +#  This program and the accompanying materials are licensed and made available
> > +#  under the terms and conditions of the BSD License which accompanies this
> > +#  distribution.  The full text of the license may be found at
> > +#  http://opensource.org/licenses/bsd-license.php
> > +#
> > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +#
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x0001001A
> > +  BASE_NAME                      = SgiPlatformIdPeiLib
> > +  FILE_GUID                      = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
> > +  MODULE_TYPE                    = PEIM
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = SgiPlatformIdPeim
> > +
> > +[Packages]
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Platform/ARM/SgiPkg/SgiPlatform.dec
> > +
> > +[LibraryClasses]
> > +  FdtLib
> > +  PeimEntryPoint
> > +
> > +[Sources]
> > +  SgiPlatformPeiLib.c
> > +
> > +[Guids]
> > +  gArmSgiPlatformIdDescriptorGuid
> > +
> > +[Ppis]
> > +  gHwConfigDtInfoPpiGuid
> > +
> > +[Depex]
> > +  TRUE
> > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
> > index d995937..ea9f6c5 100644
> > --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> > @@ -29,9 +29,14 @@
> >    Include                        # Root include for the package
> >
> >  [Guids.common]
> > +  # ARM Sgi Platform ID descriptor
> > +  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
> >    gArmSgiTokenSpaceGuid      = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
> >    gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
> >
> >  [PcdsFeatureFlag.common]
> >    # Set this PCD to TRUE to enable virtio support.
> >    gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
> > +
> > +[Ppis]
> > +  gHwConfigDtInfoPpiGuid     = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> > --
> > 2.7.4
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

[Chandni Cherukuri]

Use the platform ID and config ID values from the platform ID
HOB to choose the right set of ACPI tables to be installed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
---
 .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 ++++++++++++++++++----
 .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
 Platform/ARM/SgiPkg/Include/SgiPlatform.h          | 24 ++++++++++++++--
 Platform/ARM/SgiPkg/SgiPlatform.dsc                |  1 +
 Platform/ARM/SgiPkg/SgiPlatform.fdf                |  1 +
 5 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
index edaae5b..d0d5808 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
@@ -14,6 +14,8 @@
 
 #include <Library/AcpiLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <SgiPlatform.h>
 
 EFI_STATUS
 InitVirtioBlockIo (
@@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
   )
 {
   EFI_STATUS              Status;
+  VOID                    *PlatformIdHob;
+  SGI_PLATFORM_DESCRIPTOR *HobData;
+  UINT32                  ConfigId;
+  UINT32                  PartNum;
 
-  Status = LocateAndInstallAcpiFromFv (&gSgi575AcpiTablesiFileGuid);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
-    return Status;
+  PlatformIdHob = GetFirstGuidHob (&gArmSgiPlatformIdDescriptorGuid);
+  if (PlatformIdHob == NULL) {
+    DEBUG ((DEBUG_ERROR, "Platform ID Hob is NULL\n"));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
+
+  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
+  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
+
+  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
+    Status = LocateAndInstallAcpiFromFv (&gSgi575AcpiTablesiFileGuid);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
+      return Status;
+    }
+  } else {
+    DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
+    return EFI_UNSUPPORTED;
   }
 
-  Status = EFI_REQUEST_UNLOAD_IMAGE;
   if (FeaturePcdGet (PcdVirtioSupported)) {
     Status = InitVirtioBlockIo (ImageHandle);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
         __FUNCTION__));
     }
+  } else {
+    Status = EFI_REQUEST_UNLOAD_IMAGE;
   }
 
   return Status;
diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
index 51ad22f..b6b8209 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
@@ -30,10 +30,12 @@
 
 [LibraryClasses]
   AcpiLib
+  HobLib
   UefiDriverEntryPoint
   VirtioMmioDeviceLib
 
 [Guids]
+  gArmSgiPlatformIdDescriptorGuid
   gSgi575AcpiTablesiFileGuid
 
 [FeaturePcd]
diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
index 00ca7e9..756954c 100644
--- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
+++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
@@ -12,8 +12,8 @@
 *
 **/
 
-#ifndef __SGI_PLATFORM_H__
-#define __SGI_PLATFORM_H__
+#ifndef SGI_PLATFORM_H_
+#define SGI_PLATFORM_H_
 
 /***********************************************************************************
 // Platform Memory Map
@@ -68,4 +68,22 @@
 #define SGI_SYSPH_SYS_REG_FLASH                   0x4C
 #define SGI_SYSPH_SYS_REG_FLASH_RWEN              0x1
 
-#endif // __SGI_PLATFORM_H__
+// SGI575_VERSION values
+#define SGI575_CONF_NUM                           0x3
+#define SGI575_PART_NUM                           0x783
+
+#define SGI_CONFIG_MASK                           0x0F
+#define SGI_CONFIG_SHIFT                          0x1C
+#define SGI_PART_NUM_MASK                         0xFFF
+
+//HwConfig DT structure^M
+typedef struct {
+  UINT64                  *HwConfigDtAddr;
+} SGI_HW_CONFIG_INFO_PPI;
+
+// ARM platform description data.
+typedef struct {
+  UINTN  PlatformId;
+} SGI_PLATFORM_DESCRIPTOR;
+
+#endif // SGI_PLATFORM_H_
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc b/Platform/ARM/SgiPkg/SgiPlatform.dsc
index a56175e..f571897 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
@@ -199,6 +199,7 @@
     <LibraryClasses>
       NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
   }
+  Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
 
   #
   # DXE
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf b/Platform/ARM/SgiPkg/SgiPlatform.fdf
index 17cdf48..05203a8 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
+++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
@@ -220,6 +220,7 @@ READ_LOCK_STATUS   = TRUE
   INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+  INF Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
 
   FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
     SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
-- 
2.7.4

Re: [PATCH 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

[Ard Biesheuvel]

Hello Chandni,

On 15 June 2018 at 07:55, Chandni Cherukuri <chandni.cherukuri@arm.com> wrote:
> Use the platform ID and config ID values from the platform ID
> HOB to choose the right set of ACPI tables to be installed.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> ---
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 ++++++++++++++++++----
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
>  Platform/ARM/SgiPkg/Include/SgiPlatform.h          | 24 ++++++++++++++--
>  Platform/ARM/SgiPkg/SgiPlatform.dsc                |  1 +
>  Platform/ARM/SgiPkg/SgiPlatform.fdf                |  1 +
>  5 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> index edaae5b..d0d5808 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -14,6 +14,8 @@
>
>  #include <Library/AcpiLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <SgiPlatform.h>
>
>  EFI_STATUS
>  InitVirtioBlockIo (
> @@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
>    )
>  {
>    EFI_STATUS              Status;
> +  VOID                    *PlatformIdHob;
> +  SGI_PLATFORM_DESCRIPTOR *HobData;
> +  UINT32                  ConfigId;
> +  UINT32                  PartNum;
>
> -  Status = LocateAndInstallAcpiFromFv (&gSgi575AcpiTablesiFileGuid);
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
> -    return Status;
> +  PlatformIdHob = GetFirstGuidHob (&gArmSgiPlatformIdDescriptorGuid);
> +  if (PlatformIdHob == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Platform ID Hob is NULL\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
> +
> +  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
> +  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
> +
> +  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
> +    Status = LocateAndInstallAcpiFromFv (&gSgi575AcpiTablesiFileGuid);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
> +      return Status;
> +    }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
> +    return EFI_UNSUPPORTED;
>    }
>
> -  Status = EFI_REQUEST_UNLOAD_IMAGE;
>    if (FeaturePcdGet (PcdVirtioSupported)) {
>      Status = InitVirtioBlockIo (ImageHandle);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
>          __FUNCTION__));
>      }
> +  } else {
> +    Status = EFI_REQUEST_UNLOAD_IMAGE;
>    }
>
>    return Status;
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index 51ad22f..b6b8209 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -30,10 +30,12 @@
>
>  [LibraryClasses]
>    AcpiLib
> +  HobLib
>    UefiDriverEntryPoint
>    VirtioMmioDeviceLib
>
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>    gSgi575AcpiTablesiFileGuid
>
>  [FeaturePcd]
> diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> index 00ca7e9..756954c 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> @@ -12,8 +12,8 @@
>  *
>  **/
>
> -#ifndef __SGI_PLATFORM_H__
> -#define __SGI_PLATFORM_H__
> +#ifndef SGI_PLATFORM_H_
> +#define SGI_PLATFORM_H_
>
>  /***********************************************************************************
>  // Platform Memory Map
> @@ -68,4 +68,22 @@
>  #define SGI_SYSPH_SYS_REG_FLASH                   0x4C
>  #define SGI_SYSPH_SYS_REG_FLASH_RWEN              0x1
>
> -#endif // __SGI_PLATFORM_H__
> +// SGI575_VERSION values
> +#define SGI575_CONF_NUM                           0x3
> +#define SGI575_PART_NUM                           0x783
> +
> +#define SGI_CONFIG_MASK                           0x0F
> +#define SGI_CONFIG_SHIFT                          0x1C
> +#define SGI_PART_NUM_MASK                         0xFFF
> +
> +//HwConfig DT structure^M

^M ?

> +typedef struct {
> +  UINT64                  *HwConfigDtAddr;
> +} SGI_HW_CONFIG_INFO_PPI;
> +

Please declare this PPI properly

> +// ARM platform description data.
> +typedef struct {
> +  UINTN  PlatformId;
> +} SGI_PLATFORM_DESCRIPTOR;
> +
> +#endif // SGI_PLATFORM_H_
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index a56175e..f571897 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -199,6 +199,7 @@
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>    }
> +  Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>
>    #
>    # DXE
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> index 17cdf48..05203a8 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> @@ -220,6 +220,7 @@ READ_LOCK_STATUS   = TRUE
>    INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
>    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> +  INF Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>
>    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>      SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
> --
> 2.7.4
>

Re: [PATCH 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

[Leif Lindholm]

On Fri, Jun 15, 2018 at 11:25:32AM +0530, Chandni Cherukuri wrote:
> Use the platform ID and config ID values from the platform ID
> HOB to choose the right set of ACPI tables to be installed.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> ---
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 ++++++++++++++++++----
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
>  Platform/ARM/SgiPkg/Include/SgiPlatform.h          | 24 ++++++++++++++--
>  Platform/ARM/SgiPkg/SgiPlatform.dsc                |  1 +
>  Platform/ARM/SgiPkg/SgiPlatform.fdf                |  1 +
>  5 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> index edaae5b..d0d5808 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -14,6 +14,8 @@
>  
>  #include <Library/AcpiLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <SgiPlatform.h>
>  
>  EFI_STATUS
>  InitVirtioBlockIo (
> @@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
>    )
>  {
>    EFI_STATUS              Status;
> +  VOID                    *PlatformIdHob;
> +  SGI_PLATFORM_DESCRIPTOR *HobData;
> +  UINT32                  ConfigId;
> +  UINT32                  PartNum;
>  
> -  Status = LocateAndInstallAcpiFromFv (&gSgi575AcpiTablesiFileGuid);
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
> -    return Status;
> +  PlatformIdHob = GetFirstGuidHob (&gArmSgiPlatformIdDescriptorGuid);
> +  if (PlatformIdHob == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Platform ID Hob is NULL\n"));

HOB

> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
> +
> +  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
> +  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
> +
> +  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
> +    Status = LocateAndInstallAcpiFromFv (&gSgi575AcpiTablesiFileGuid);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
> +      return Status;
> +    }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
> +    return EFI_UNSUPPORTED;
>    }
>  
> -  Status = EFI_REQUEST_UNLOAD_IMAGE;
>    if (FeaturePcdGet (PcdVirtioSupported)) {
>      Status = InitVirtioBlockIo (ImageHandle);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
>          __FUNCTION__));
>      }
> +  } else {
> +    Status = EFI_REQUEST_UNLOAD_IMAGE;
>    }
>  
>    return Status;
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index 51ad22f..b6b8209 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -30,10 +30,12 @@
>  
>  [LibraryClasses]
>    AcpiLib
> +  HobLib
>    UefiDriverEntryPoint
>    VirtioMmioDeviceLib
>  
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>    gSgi575AcpiTablesiFileGuid
>  
>  [FeaturePcd]
> diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> index 00ca7e9..756954c 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> @@ -12,8 +12,8 @@
>  *
>  **/
>  
> -#ifndef __SGI_PLATFORM_H__
> -#define __SGI_PLATFORM_H__
> +#ifndef SGI_PLATFORM_H_
> +#define SGI_PLATFORM_H_

Non-functional change. Please submit as separate change or not at all.

/
    Leif

>  
>  /***********************************************************************************
>  // Platform Memory Map
> @@ -68,4 +68,22 @@
>  #define SGI_SYSPH_SYS_REG_FLASH                   0x4C
>  #define SGI_SYSPH_SYS_REG_FLASH_RWEN              0x1
>  
> -#endif // __SGI_PLATFORM_H__
> +// SGI575_VERSION values
> +#define SGI575_CONF_NUM                           0x3
> +#define SGI575_PART_NUM                           0x783
> +
> +#define SGI_CONFIG_MASK                           0x0F
> +#define SGI_CONFIG_SHIFT                          0x1C
> +#define SGI_PART_NUM_MASK                         0xFFF
> +
> +//HwConfig DT structure^M
> +typedef struct {
> +  UINT64                  *HwConfigDtAddr;
> +} SGI_HW_CONFIG_INFO_PPI;
> +
> +// ARM platform description data.
> +typedef struct {
> +  UINTN  PlatformId;
> +} SGI_PLATFORM_DESCRIPTOR;
> +
> +#endif // SGI_PLATFORM_H_
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index a56175e..f571897 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -199,6 +199,7 @@
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>    }
> +  Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>  
>    #
>    # DXE
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> index 17cdf48..05203a8 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> @@ -220,6 +220,7 @@ READ_LOCK_STATUS   = TRUE
>    INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
>    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> +  INF Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>  
>    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>      SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
> -- 
> 2.7.4
>