Re: [edk2-devel] [PATCH v1 07/11] ArmVirtPkg: kvmtool platform memory map

["Laszlo Ersek" <lersek@redhat.com>]

On 05/14/20 10:40, Sami Mujawar wrote:
> Kvmtool is a virtual machine manager that enables
> hosting KVM guests. Kvmtool allows to vary the
> hardware configuration of the emulated platform
> it provides to the guest partition. It provides
> the current hardware configuration to the firmware
> by handing off a device tree containing the hardware
> information.
> 
> This library parses the kvmtool provided device
> tree and populates the system memory map for the
> kvmtool emulated platform.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.c   | 114 ++++++++++++++++++++
>  ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.inf |  42 ++++++++
>  2 files changed, 156 insertions(+)
> 
> diff --git a/ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.c b/ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..0d353733478b2d097d160246007022990a9cbacb
> --- /dev/null
> +++ b/ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.c
> @@ -0,0 +1,114 @@
> +/** @file
> +
> +  Copyright (c) 2018, ARM Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/ArmLib.h>
> +#include <Library/BaseLib.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
> +
> +#define LOG_VM_MAP(txt)                       \

(1) Please use "Txt" at least.

There are some other mild coding style discrepancies in this patch, but
I don't want to obsess about those.

With (1) fixed:

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

Thanks
Laszlo

> +  DEBUG ((                                    \
> +    DEBUG_ERROR,                              \
> +    "%a: " #txt "\n"                          \
> +    "\tPhysicalBase: 0x%lX\n"                 \
> +    "\tVirtualBase: 0x%lX\n"                  \
> +    "\tLength: 0x%lX\n",                      \
> +    __FUNCTION__,                             \
> +    VirtualMemoryTable[Idx].PhysicalBase,     \
> +    VirtualMemoryTable[Idx].VirtualBase,      \
> +    VirtualMemoryTable[Idx].Length            \
> +    ));
> +
> +/**
> +  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. The allocated memory
> +                                    will not be freed.
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> +  )
> +{
> +  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> +  UINTN                          Idx = 0;
> +  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace;
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  TopOfAddressSpace = LShiftU64 (1ULL, ArmGetPhysicalAddressBits ());
> +
> +  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)
> +                        AllocatePages (
> +                          EFI_SIZE_TO_PAGES (
> +                            sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +                            MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS
> +                            )
> +                          );
> +
> +  if (VirtualMemoryTable == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Error: Failed to Allocate Pages\n",
> +      __FUNCTION__
> +      ));
> +    return;
> +  }
> +
> +  // System DRAM
> +  VirtualMemoryTable[Idx].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[Idx].VirtualBase  = VirtualMemoryTable[Idx].PhysicalBase;
> +  VirtualMemoryTable[Idx].Length       = PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[Idx].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +  LOG_VM_MAP ("System DRAM Memory Map");
> +
> +  // Peripheral space before DRAM
> +  VirtualMemoryTable[++Idx].PhysicalBase = 0x0;
> +  VirtualMemoryTable[Idx].VirtualBase    = 0x0;
> +  VirtualMemoryTable[Idx].Length         = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[Idx].Attributes     = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_VM_MAP ("Peripheral space before DRAM");
> +
> +  // Peripheral space after DRAM
> +  VirtualMemoryTable[++Idx].PhysicalBase = PcdGet64 (PcdSystemMemoryBase) +
> +                                           PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[Idx].VirtualBase    = VirtualMemoryTable[Idx].PhysicalBase;
> +  VirtualMemoryTable[Idx].Length         = TopOfAddressSpace -
> +                                           VirtualMemoryTable[Idx].PhysicalBase;
> +  VirtualMemoryTable[Idx].Attributes     = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_VM_MAP ("Peripheral space after DRAM");
> +
> +  // Map the FV region as normal executable memory
> +  VirtualMemoryTable[++Idx].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> +  VirtualMemoryTable[Idx].VirtualBase  = VirtualMemoryTable[Idx].PhysicalBase;
> +  VirtualMemoryTable[Idx].Length       = FixedPcdGet32 (PcdFvSize);
> +  VirtualMemoryTable[Idx].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +  LOG_VM_MAP ("FV region");
> +
> +  // End of Table
> +  VirtualMemoryTable[++Idx].PhysicalBase  = 0;
> +  VirtualMemoryTable[Idx].VirtualBase     = 0;
> +  VirtualMemoryTable[Idx].Length          = 0;
> +  VirtualMemoryTable[Idx].Attributes      = (ARM_MEMORY_REGION_ATTRIBUTES)0;
> +
> +  ASSERT((Idx + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.inf b/ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.inf
> new file mode 100644
> index 0000000000000000000000000000000000000000..dbf4ceabe3ae0db5e743e1d9a575542dca32ed0a
> --- /dev/null
> +++ b/ArmVirtPkg/Library/KvmtoolVirtMemInfoLib/KvmtoolVirtMemInfoLib.inf
> @@ -0,0 +1,42 @@
> +#/* @file
> +#
> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001B
> +  BASE_NAME                      = KvmtoolVirtMemInfoLib
> +  FILE_GUID                      = B752E953-394F-462C-811C-F8BE35C8C071
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib
> +
> +[Sources]
> +  KvmtoolVirtMemInfoLib.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseLib
> +  BaseMemoryLib
> +  DebugLib
> +  MemoryAllocationLib
> +  PcdLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFvSize
> +
>