Re: [PATCH v2 1/2] CloudHv/arm: add PeiMemInfoLib

["Jianyong Wu" <jianyong.wu@arm.com>]

Thanks Sami, I will address all the comments in this patch in next version.

> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Thursday, September 1, 2022 11:43 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; devel@edk2.groups.io
> Cc: ardb+tianocore@kernel.org; Justin He <Justin.He@arm.com>; nd
> <nd@arm.com>
> Subject: Re: [PATCH v2 1/2] CloudHv/arm: add PeiMemInfoLib
> 
> Hi Jianyong,
> 
> Thank you for the updated patch.
> 
> I have one comment marked inline as [SAMI]. Other than that this patch
> looks good to me.
> 
> With that fixed.
> 
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> 
> Regards,
> 
> Sami Mujawar
> 
> On 19/08/2022 09:09 am, Jianyong Wu wrote:
> > Memory layout in CLoud Hypervisor for arm is changed and is different
> > with Qemu, thus we should build its own PeiMemInfoLib.
> > The main change in the memory layout is that normal ram may not
> > contiguous under 4G. The top 64M under 4G is reserved for 32bit device.
> >
> > What this patch does:
> > 1. get all of the memory node from DT; 2. Add all of the memory nodes
> > to Hob; 3. Init page table for each memory node;
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >   .../CloudHvVirtMemInfoLib.c                   | 230 ++++++++++++++++++
> >   .../CloudHvVirtMemInfoLib.h                   |  42 ++++
> >   .../CloudHvVirtMemInfoPeiLib.inf              |  46 ++++
> >   3 files changed, 318 insertions(+)
> >   create mode 100644
> ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> >   create mode 100644
> ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.h
> >   create mode 100644
> >
> ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoPeiLib.inf
> >
> > diff --git
> > a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > new file mode 100644
> > index 0000000000..d9c434754e
> > --- /dev/null
> > +++
> b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > @@ -0,0 +1,230 @@
> > +/** @file
> > +
> > +  Copyright (c) 2022, Arm Limited. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include <PiPei.h>
> > +
> > +#include <Base.h>
> > +#include <libfdt.h>
> > +#include <Library/ArmLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/MemoryAllocationLib.h> #include <Library/PcdLib.h>
> > +
> > +#include <Library/PrePiLib.h>
> > +
> > +#include "CloudHvVirtMemInfoLib.h"
> > +
> > +CLOUDHV_MEM_NODE_INFO
> CloudHvMemNode[CLOUDHV_MAX_MEM_NODE_NUM];
> > +
> > +RETURN_STATUS
> > +EFIAPI
> > +CloudHvVirtMemInfoPeiLibConstructor (
> > +  VOID
> > +  )
> > +{
> > +  VOID           *DeviceTreeBase;
> > +  EFI_RESOURCE_ATTRIBUTE_TYPE  ResourceAttributes;
> > +  INT32          Node, Prev;
> > +  UINT64         FirMemNodeBase, FirMemNodeSize;
> > +  UINT64         CurBase, MemBase;
> > +  UINT64         CurSize;
> > +  CONST CHAR8    *Type;
> > +  INT32          Len;
> > +  CONST UINT64   *RegProp;
> > +  RETURN_STATUS  PcdStatus;
> > +  UINT8          Index;
> > +
> > +  ZeroMem (CloudHvMemNode, sizeof(CloudHvMemNode));
> > +
> > +  FirMemNodeBase = 0;
> > +  FirMemNodeSize = 0;
> > +  Index = 0;
> > +  MemBase = FixedPcdGet64 (PcdSystemMemoryBase);
> ResourceAttributes
> > + = (
> > +                        EFI_RESOURCE_ATTRIBUTE_PRESENT |
> > +                        EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> > +                        EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> > +                        EFI_RESOURCE_ATTRIBUTE_TESTED
> > +                        );
> > +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64
> > + (PcdDeviceTreeInitialBaseAddress);
> > +  if (DeviceTreeBase == NULL) {
> > +    return EFI_NOT_FOUND;
> > +  }
> > +
> > +  //
> > +  // Make sure we have a valid device tree blob  //  if
> > + (fdt_check_header (DeviceTreeBase) != 0) {
> > +    return EFI_NOT_FOUND;
> > +  }
> > +
> > +  //
> > +  // Look for the lowest memory node
> > +  //
> > +  for (Prev = 0; ; Prev = Node) {
> > +    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> > +    if (Node < 0) {
> > +      break;
> > +    }
> > +
> > +    //
> > +    // Check for memory node
> > +    //
> > +    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> > +    if (Type && (AsciiStrnCmp (Type, "memory", Len) == 0)) {
> 
> [SAMI] Apologies I missed this in my earlier review. The above check should
> be
> 
>   + if ((Type !=0) && (AsciiStrnCmp (Type, "memory", Len) == 0)) {
> 
> as Type is not boolean.
> 
> [/SAMI]
> 
> > +      //
> > +      // Get the 'reg' property of this node. For now, we will assume
> > +      // two 8 byte quantities for base and size, respectively.
> > +      //
> > +      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> > +      if ((RegProp != 0) && (Len == (2 * sizeof (UINT64)))) {
> > +        CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> > +        CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
> > +
> > +        DEBUG ((
> > +          DEBUG_INFO,
> > +          "%a: System RAM @ 0x%lx - 0x%lx\n",
> > +          __FUNCTION__,
> > +          CurBase,
> > +          CurBase + CurSize - 1
> > +          ));
> > +
> > +        // We should build Hob seperately for the memory node except the
> first one
> > +        if (CurBase != MemBase) {
> > +          BuildResourceDescriptorHob (
> > +            EFI_RESOURCE_SYSTEM_MEMORY,
> > +            ResourceAttributes,
> > +            CurBase,
> > +            CurSize
> > +            );
> > +        } else {
> > +          FirMemNodeBase = CurBase;
> > +          FirMemNodeSize = CurSize;
> > +        }
> > +
> > +        CloudHvMemNode[Index].Base = CurBase;
> > +        CloudHvMemNode[Index].Size = CurSize;
> > +        Index++;
> > +
> > +        if (Index >= CLOUDHV_MAX_MEM_NODE_NUM) {
> > +          DEBUG ((
> > +            DEBUG_WARN,
> > +            "%a: memory node larger than %d will not be included into Memory
> System\n",
> > +            __FUNCTION__,
> > +            CLOUDHV_MAX_MEM_NODE_NUM
> > +            ));
> > +          break;
> > +        }
> > +      } else {
> > +        DEBUG ((
> > +          DEBUG_ERROR,
> > +          "%a: Failed to parse FDT memory node\n",
> > +          __FUNCTION__
> > +          ));
> > +      }
> > +    }
> > +  }
> > +
> > +  //
> > +  // Make sure the start of DRAM matches our expectation  //  if
> > + (FixedPcdGet64 (PcdSystemMemoryBase) != FirMemNodeBase) {
> > +    return EFI_NOT_FOUND;
> > +  }
> > +  PcdStatus = PcdSet64S (PcdSystemMemorySize, FirMemNodeSize);
> > + ASSERT_RETURN_ERROR (PcdStatus);  ASSERT (
> > +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> > +      (UINT64)PcdGet32 (PcdFdSize)) <= FirMemNodeBase) ||
> > +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (FirMemNodeBase +
> FirMemNodeSize))
> > +    );
> > +
> > +  return RETURN_SUCCESS;
> > +}
> > +
> > +/**
> > +  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;
> > +  UINT8 Index = 0, MemNodeIndex = 0;
> > +
> > +  ASSERT (VirtualMemoryMap != NULL);
> > +
> > +  VirtualMemoryTable = AllocatePool (
> > +                         sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> > +                         MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS
> > +                         );
> > +
> > +  if (VirtualMemoryTable == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n",
> __FUNCTION__));
> > +    return;
> > +  }
> > +    // System DRAM
> > +  while ((MemNodeIndex < CLOUDHV_MAX_MEM_NODE_NUM) &&
> (CloudHvMemNode[MemNodeIndex].Size != 0)) {
> > +    VirtualMemoryTable[Index].PhysicalBase =
> CloudHvMemNode[MemNodeIndex].Base;
> > +    VirtualMemoryTable[Index].VirtualBase  =
> CloudHvMemNode[MemNodeIndex].Base;
> > +    VirtualMemoryTable[Index].Length       =
> CloudHvMemNode[MemNodeIndex].Size;
> > +    VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> > +
> > +    DEBUG ((
> > +      DEBUG_INFO,
> > +      "%a: Dumping System DRAM Memory Node%d Map:\n"
> > +      "\tPhysicalBase: 0x%lX\n"
> > +      "\tVirtualBase: 0x%lX\n"
> > +      "\tLength: 0x%lX\n",
> > +      __FUNCTION__,
> > +      MemNodeIndex,
> > +      VirtualMemoryTable[Index].PhysicalBase,
> > +      VirtualMemoryTable[Index].VirtualBase,
> > +      VirtualMemoryTable[Index].Length
> > +      ));
> > +    Index++;
> > +    MemNodeIndex++;
> > +  }
> > +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> > + VirtualMemoryTable[Index].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> > + VirtualMemoryTable[Index].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> > +  VirtualMemoryTable[Index].Length       = MACH_VIRT_PERIPH_SIZE;
> > +  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  Index++;
> > +
> > +  // Map the FV region as normal executable memory
> > + VirtualMemoryTable[Index].PhysicalBase = PcdGet64
> > + (PcdFvBaseAddress);  VirtualMemoryTable[Index].VirtualBase  =
> VirtualMemoryTable[Index].PhysicalBase;
> > +  VirtualMemoryTable[Index].Length       = FixedPcdGet32 (PcdFvSize);
> > +  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> > +  Index++;
> > +
> > +  // Memory mapped for 32bit device (like TPM)
> > + VirtualMemoryTable[Index].PhysicalBase = TOP_32BIT_DEVICE_BASE;
> > + VirtualMemoryTable[Index].VirtualBase  = TOP_32BIT_DEVICE_BASE;
> > +  VirtualMemoryTable[Index].Length       = TOP_32BIT_DEVICE_SIZE;
> > +  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  Index++;
> > +
> > +  // End of Table
> > +  ZeroMem (&VirtualMemoryTable[Index], sizeof
> > + (ARM_MEMORY_REGION_DESCRIPTOR));
> > +
> > +  *VirtualMemoryMap = VirtualMemoryTable; }
> > diff --git
> > a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.h
> > b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.h
> > new file mode 100644
> > index 0000000000..e624373472
> > --- /dev/null
> > +++
> b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.h
> > @@ -0,0 +1,42 @@
> > +/** @file
> > +
> > +  Copyright (c) 2022, Arm Limited. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef _CLOUDHV_VIRT_MEM_INFO_LIB_H_ #define
> > +_CLOUDHV_VIRT_MEM_INFO_LIB_H_
> > +
> > +//
> > +// Cloud Hypervisor may have more than one memory nodes. Even there
> > +is no limit for that, // I think 10 is enough in general.
> > +//
> > +#define CLOUDHV_MAX_MEM_NODE_NUM 10
> > +
> > +// Record memory node info (base address and size) typedef struct {
> > +  UINT64 Base;
> > +  UINT64 Size;
> > +} CLOUDHV_MEM_NODE_INFO;
> > +
> > +// Number of Virtual Memory Map Descriptors #define
> > +MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  (4 +
> CLOUDHV_MAX_MEM_NODE_NUM)
> > +
> > +//
> > +// Core peripherals such as the UART, the GIC and the RTC are // all
> > +mapped in the 'miscellaneous device I/O' region, which we just map //
> > +in its entirety rather than device by device. Note that it does not
> > +// cover any of the NOR flash banks or PCI resource windows.
> > +//
> > +#define MACH_VIRT_PERIPH_BASE  0x00400000 #define
> > +MACH_VIRT_PERIPH_SIZE  0x0FC00000
> > +
> > +//
> > +// The top of the 64M memory region under 4GB reserved for device //
> > +#define TOP_32BIT_DEVICE_BASE  0xFC000000 #define
> > +TOP_32BIT_DEVICE_SIZE  0x04000000
> > +
> > +#endif // _CLOUDHV_VIRT_MEM_INFO_LIB_H_
> > diff --git
> >
> a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoPeiLib.i
> n
> > f
> >
> b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoPeiLib.i
> n
> > f
> > new file mode 100644
> > index 0000000000..30626776ae
> > --- /dev/null
> > +++
> b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoPeiLi
> > +++ b.inf
> > @@ -0,0 +1,46 @@
> > +/** @file
> > +
> > +  Copyright (c) 2022, Arm Limited. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x0001001B
> > +  BASE_NAME                      = CloudHvVirtMemInfoPeiLib
> > +  FILE_GUID                      = c7ada233-d35b-49c3-aa51-e2b5cd80c910
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> > +  CONSTRUCTOR                    = CloudHvVirtMemInfoPeiLibConstructor
> > +
> > +[Sources]
> > +  CloudHvVirtMemInfoLib.c
> > +  CloudHvVirtMemInfoLib.h
> > +
> > +[Packages]
> > +  ArmPkg/ArmPkg.dec
> > +  ArmVirtPkg/ArmVirtPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +
> > +[LibraryClasses]
> > +  ArmLib
> > +  BaseMemoryLib
> > +  DebugLib
> > +  FdtLib
> > +  MemoryAllocationLib
> > +  PcdLib
> > +
> > +[Pcd]
> > +  gArmTokenSpaceGuid.PcdFdBaseAddress
> > +  gArmTokenSpaceGuid.PcdFvBaseAddress
> > +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> > +  gArmTokenSpaceGuid.PcdSystemMemorySize
> > +
> > +[FixedPcd]
> > +  gArmTokenSpaceGuid.PcdFdSize
> > +  gArmTokenSpaceGuid.PcdFvSize
> > +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress