Re: [PATCH 12/15] ArmVirtPkg/ArmVirtQemu: add ArmVirtMemInfoLib implementation

[Laszlo Ersek <lersek@redhat.com>]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
> clean it up slightly (by removing the support for uncached DRAM mappings),
> and turn it into a new ArmVirtMemInfoLib implementation.

I've looked at this patch with "git show --find-copies-harder". It looks
OK, but the commit message could be improved:

(1) the support for uncached DRAM mappings is removed in the copy-origin
lib instance, in patch 09/15 ("ArmVirtPkg/ArmVirtPlatformLib: remove
support for uncached mappings"). I think this sentence should be dropped
from the commit message.

(2) There are other cleanups however:

- factor out TableSize and TopOfMemory

- replace EFI_D_* with DEBUG_*

- fetch PcdFdBaseAddress with PcdGet64(), not FixedPcdGet64(). (This is
matched by the [Pcd] / [FixedPcd] sections in the new INF file.)

Can you elaborate on the last item? I wonder if that change qualifies as
cleanup. (I'm fine if the change is justified by some other flexibility,
but it should be documented please.)

With the commit message updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 
> In a future patch, we will add this library to the ordinary ArmVirtQemu
> platform as well.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                             |   1 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  |  39 ++++++++
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S      |  24 +++++
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   | 101 ++++++++++++++++++++
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  53 ++++++++++
>  5 files changed, 218 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 7e5d584344b4..f50d30388cf2 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -49,6 +49,7 @@ [LibraryClasses.common]
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>  
>    ArmPlatformLib|ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ArmQemuRelocatablePlatformLib.inf
> +  ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
>  
>    TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>    NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> new file mode 100644
> index 000000000000..a1f6a194d59b
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> @@ -0,0 +1,39 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2016-2017, Linaro 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 <AsmMacroIoLibV8.h>
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mrs   x0, id_aa64mmfr0_el1
> +  adr   x1, .LPARanges
> +  and   x0, x0, #7
> +  ldrb  w1, [x1, x0]
> +  mov   x0, #1
> +  lsl   x0, x0, x1
> +  ret
> +
> +//
> +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> +// physical address space support on this CPU:
> +// 0 == 32 bits, 1 == 36 bits, etc etc
> +// 6 and 7 are reserved
> +//
> +.LPARanges:
> +  .byte 32, 36, 40, 42, 44, 48, -1, -1
> +
> +ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> new file mode 100644
> index 000000000000..9cd81529fb3d
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> @@ -0,0 +1,24 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro 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 <AsmMacroIoLib.h>
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mov   r0, #0x00000000
> +  mov   r1, #0x10000
> +  bx    lr
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..b08305fe4a4f
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -0,0 +1,101 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro 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 <Base.h>
> +#include <Library/ArmLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +// Number of Virtual Memory Map Descriptors
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
> +
> +EFI_PHYSICAL_ADDRESS
> +ArmGetPhysAddrTop (
> +  VOID
> +  );
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
> +                                    describing a Physical-to-Virtual Memory
> +                                    mapping. This array must be ended by a
> +                                    zero-filled entry
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> +  )
> +{
> +  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> +  UINT64                        TopOfMemory;
> +  UINTN                         TableSize;
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  TableSize = sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +              MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS;
> +  VirtualMemoryTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> +
> +  if (VirtualMemoryTable == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePages()\n", __FUNCTION__));
> +    return;
> +  }
> +
> +  // System DRAM
> +  VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
> +  VirtualMemoryTable[0].Length       = PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  DEBUG ((DEBUG_INFO, "%a: Dumping System DRAM Memory Map:\n"
> +      "\tPhysicalBase: 0x%lX\n"
> +      "\tVirtualBase: 0x%lX\n"
> +      "\tLength: 0x%lX\n",
> +      __FUNCTION__,
> +      VirtualMemoryTable[0].PhysicalBase,
> +      VirtualMemoryTable[0].VirtualBase,
> +      VirtualMemoryTable[0].Length));
> +
> +  // Peripheral space before DRAM
> +  VirtualMemoryTable[1].PhysicalBase = 0x0;
> +  VirtualMemoryTable[1].VirtualBase  = 0x0;
> +  VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
> +  VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Peripheral space after DRAM
> +  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> +                     ArmGetPhysAddrTop ());
> +  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = TopOfMemory -
> +                                       VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Remap the FD region as normal executable memory
> +  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> +  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
> +  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
> +  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // End of Table
> +  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> new file mode 100644
> index 000000000000..8f6ba006fdfd
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> @@ -0,0 +1,53 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro 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                      = QemuVirtMemInfoLib
> +  FILE_GUID                      = 9b30ca82-6746-4a82-a3e6-11ea79df3b46
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib
> +
> +[Sources]
> +  QemuVirtMemInfoLib.c
> +
> +[Sources.ARM]
> +  Arm/PhysAddrTop.S
> +
> +[Sources.AARCH64]
> +  AArch64/PhysAddrTop.S
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
>