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

[Pete Batard <pete@akeo.ie>]

On 2018.12.12 20:43, Ard Biesheuvel wrote:
> 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

Good point. We'll drop these and pass VOID instead.

>> +  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?

I'm not sure myself, so I agree this is confusing and needs to be changed.

> Can we improve the naming in the file?

We'll rename all the AddAnd... function calls to something that is more 
explicit wrt the goal of these functions.

> Also, please use a space before (

Okay.

>> +{
>> +  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.

Fair enough. We'll move ArmPlatformGetVirtualMemoryMap() 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)

Makes sense. We'll do as suggested.

>> +  // 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)

Okay. I'll look into it.

>> +
>> +  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
>>