Re: [PATCH v2 edk2-platforms 01/20] Platform/Broadcom/RPi3: Add Reset and Memory Init libraries

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Mon, 10 Dec 2018 at 13:39, Pete Batard <pete@akeo.ie> wrote:
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c   | 183 ++++++++++++++++++++
>  Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |  51 ++++++
>  Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c                   | 104 +++++++++++
>  Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf                 |  46 +++++
>  4 files changed, 384 insertions(+)
>
> diff --git a/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> new file mode 100644
> index 000000000000..81d810b5d428
> --- /dev/null
> +++ b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> @@ -0,0 +1,183 @@
> +/** @file
> + *
> + *  Copyright (c) 2017-2018, Andrey Warkentin <andrey.warkentin@gmail.com>
> + *  Copyright (c) 2011-2015, 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 <PiPei.h>
> +
> +#include <Library/ArmMmuLib.h>
> +#include <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +
> +extern UINT64 mSystemMemoryEnd;
> +
> +VOID
> +BuildMemoryTypeInformationHob (
> +  VOID
> +  );
> +
> +STATIC
> +VOID
> +InitMmu (
> +  IN ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable
> +  )
> +{
> +
> +  VOID                          *TranslationTableBase;
> +  UINTN                         TranslationTableSize;

You can drop these

> +  RETURN_STATUS                 Status;
> +
> +  //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in
> +  //      DRAM (even at the top of DRAM as it is the first permanent memory allocation)
> +  Status = ArmConfigureMmu (MemoryTable, &TranslationTableBase, &TranslationTableSize);

... given that they are only passed here, and are actually OPTIONAL

> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Error: Failed to enable MMU\n"));
> +  }
> +}
> +
> +STATIC
> +VOID
> +AddAndRTSData(ARM_MEMORY_REGION_DESCRIPTOR *Desc)

What does AddAnd mean? Can we improve the naming in the file?
Also, please use a space before (


> +{
> +  BuildResourceDescriptorHob (
> +                              EFI_RESOURCE_SYSTEM_MEMORY,
> +                              EFI_RESOURCE_ATTRIBUTE_PRESENT |
> +                              EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_TESTED,
> +                              Desc->PhysicalBase,
> +                              Desc->Length
> +                              );
> +
> +  BuildMemoryAllocationHob (
> +                            Desc->PhysicalBase,
> +                            Desc->Length,
> +                            EfiRuntimeServicesData
> +                            );
> +}
> +
> +STATIC
> +VOID
> +AddAndReserved(ARM_MEMORY_REGION_DESCRIPTOR *Desc)
> +{
> +  BuildResourceDescriptorHob (
> +                              EFI_RESOURCE_SYSTEM_MEMORY,
> +                              EFI_RESOURCE_ATTRIBUTE_PRESENT |
> +                              EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_TESTED,
> +                              Desc->PhysicalBase,
> +                              Desc->Length
> +                              );
> +
> +  BuildMemoryAllocationHob (
> +                            Desc->PhysicalBase,
> +                            Desc->Length,
> +                            EfiReservedMemoryType
> +                            );
> +}
> +
> +STATIC
> +VOID
> +AddAndMmio(ARM_MEMORY_REGION_DESCRIPTOR *Desc)
> +{
> +  BuildResourceDescriptorHob (
> +                              EFI_RESOURCE_SYSTEM_MEMORY,
> +                              (EFI_RESOURCE_ATTRIBUTE_PRESENT    |
> +                               EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +                               EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
> +                               EFI_RESOURCE_ATTRIBUTE_TESTED),
> +                              Desc->PhysicalBase,
> +                              Desc->Length
> +                              );
> +
> +  BuildMemoryAllocationHob (
> +                            Desc->PhysicalBase,
> +                            Desc->Length,
> +                            EfiMemoryMappedIO
> +                            );
> +}
> +
> +/*++
> +
> +Routine Description:
> +
> +
> +
> +Arguments:
> +
> +  FileHandle  - Handle of the file being invoked.
> +  PeiServices - Describes the list of possible PEI Services.
> +
> +Returns:
> +
> +  Status -  EFI_SUCCESS if the boot mode could be set
> +
> +--*/
> +EFI_STATUS
> +EFIAPI
> +MemoryPeim (
> +  IN EFI_PHYSICAL_ADDRESS               UefiMemoryBase,
> +  IN UINT64                             UefiMemorySize
> +  )
> +{
> +  ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> +
> +  // Get Virtual Memory Map from the Platform Library
> +  ArmPlatformGetVirtualMemoryMap (&MemoryTable);
> +
> +  // Ensure PcdSystemMemorySize has been set
> +  ASSERT (PcdGet64 (PcdSystemMemorySize) != 0);
> +

This function refers to array entries by index, which is a bit nasty.
It would be better just to take the contents of
ArmPlatformGetVirtualMemoryMap() and move them into this file.

> +  // FD without variable store
> +  AddAndReserved(&MemoryTable[0]);
> +
> +  // Variable store.
> +  AddAndRTSData(&MemoryTable[1]);
> +
> +  // Trusted Firmware region
> +  AddAndReserved(&MemoryTable[2]);
> +
> +  // Usable memory.
> +  BuildResourceDescriptorHob (
> +                              EFI_RESOURCE_SYSTEM_MEMORY,
> +                              EFI_RESOURCE_ATTRIBUTE_PRESENT |
> +                              EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> +                              EFI_RESOURCE_ATTRIBUTE_TESTED,
> +                              MemoryTable[3].PhysicalBase,
> +                              MemoryTable[3].Length
> +                              );
> +
> +  AddAndReserved(&MemoryTable[4]);
> +  AddAndMmio(&MemoryTable[5]);
> +

Drop the last one. MMIO is not system memory, so it does not belong in
the memory map (unless it requires a runtime mapping, but that is up
to the DXE runtime driver)

> +  // Build Memory Allocation Hob
> +  InitMmu (MemoryTable);
> +
> +  if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) {
> +    // Optional feature that helps prevent EFI memory map fragmentation.
> +    BuildMemoryTypeInformationHob ();
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> new file mode 100644
> index 000000000000..9f5204a210de
> --- /dev/null
> +++ b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> @@ -0,0 +1,51 @@
> +#/** @file
> +#
> +#  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
> +#  Copyright (c) 2011-2014, ARM Ltd. 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                    = 0x00010005
> +  BASE_NAME                      = MemoryInitPeiLib
> +  FILE_GUID                      = 4bbc9c10-a100-43fb-8311-332ba497d1b4
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = MemoryInitPeiLib|SEC PEIM
> +
> +[Sources]
> +  MemoryInitPeiLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  HobLib
> +  ArmMmuLib
> +  ArmPlatformLib
> +
> +[Guids]
> +  gEfiMemoryTypeInformationGuid
> +
> +[FeaturePcd]
> +  gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[Depex]
> +  TRUE
> diff --git a/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c
> new file mode 100644
> index 000000000000..1a3944b71d03
> --- /dev/null
> +++ b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c
> @@ -0,0 +1,104 @@
> +/** @file
> + *
> + *  Support ResetSystem Runtime call using PSCI calls.
> + *  Signals the gRaspberryPiEventResetGuid event group on reset.
> + *
> + *  Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> + *  Copyright (c) 2014, Linaro Ltd. All rights reserved.
> + *  Copyright (c) 2013-2015, ARM Ltd. All rights reserved.
> + *  Copyright (c) 2008-2009, Apple Inc. 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 <PiDxe.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/EfiResetSystemLib.h>
> +#include <Library/ArmSmcLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeLib.h>
> +
> +#include <IndustryStandard/ArmStdSmc.h>
> +
> +/**
> +  Resets the entire platform.
> +
> +  @param  ResetType             The type of reset to perform.
> +  @param  ResetStatus           The status code for the reset.
> +  @param  DataSize              The size, in bytes, of WatchdogData.
> +  @param  ResetData             For a ResetType of EfiResetCold, EfiResetWarm, or
> +                                EfiResetShutdown the data buffer starts with a Null-terminated
> +                                Unicode string, optionally followed by additional binary data.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibResetSystem (
> +  IN EFI_RESET_TYPE   ResetType,
> +  IN EFI_STATUS       ResetStatus,
> +  IN UINTN            DataSize,
> +  IN CHAR16           *ResetData OPTIONAL
> +  )
> +{
> +  ARM_SMC_ARGS ArmSmcArgs;
> +
> +  if (!EfiAtRuntime ()) {
> +    /*
> +     * Only if still in UEFI.
> +     */
> +    EfiEventGroupSignal(&gRaspberryPiEventResetGuid);
> +  }

Please drop this module entirely, and use the notification
functionality provided by MdeModulePkg/Universal/ResetSystemRuntimeDxe
(and use the generic PSCI library)

> +
> +  switch (ResetType) {
> +  case EfiResetPlatformSpecific:
> +    // Map the platform specific reset as reboot
> +  case EfiResetWarm:
> +    // Map a warm reset into a cold reset
> +  case EfiResetCold:
> +    // Send a PSCI 0.2 SYSTEM_RESET command
> +    ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
> +    break;
> +  case EfiResetShutdown:
> +    // Send a PSCI 0.2 SYSTEM_OFF command
> +    ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> +    break;
> +  default:
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  ArmCallSmc (&ArmSmcArgs);
> +
> +  // We should never be here
> +  DEBUG ((DEBUG_ERROR, "%a: PSCI Reset failed\n", __FUNCTION__));
> +  CpuDeadLoop ();
> +  return EFI_UNSUPPORTED;
> +}
> +
> +/**
> +  Initialize any infrastructure required for LibResetSystem () to function.
> +
> +  @param  ImageHandle   The firmware allocated handle for the EFI image.
> +  @param  SystemTable   A pointer to the EFI System Table.
> +
> +  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibInitializeResetSystem (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> diff --git a/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf
> new file mode 100644
> index 000000000000..1c32a4b08162
> --- /dev/null
> +++ b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf
> @@ -0,0 +1,46 @@
> +#/** @file
> +#
> +#  Reset System lib using PSCI hypervisor or secure monitor calls.
> +#  Signals the gRaspberryPiEventResetGuid event group on reset.
> +#
> +#  Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> +#  Copyright (c) 2014, Linaro Ltd. All rights reserved.
> +#  Copyright (c) 2014, ARM Ltd. All rights reserved.
> +#  Copyright (c) 2008, Apple Inc. 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                    = 0x00010005
> +  BASE_NAME                      = ResetLib
> +  FILE_GUID                      = B9F59B69-A105-41C7-8F5A-2C60DD7FD7AB
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = EfiResetSystemLib
> +
> +[Sources]
> +  ResetLib.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  MdePkg/MdePkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  Platform/Broadcom/Bcm283x/RaspberryPiPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  BaseLib
> +  ArmSmcLib
> +  UefiLib
> +  UefiRuntimeLib
> +
> +[Guids]
> +  gRaspberryPiEventResetGuid
> --
> 2.17.0.windows.1
>