Thanks Sami, I will address all the comments in this patch in next version.
> -----Original Message-----
> From: Sami Mujawar <sami.muja...@arm.com>
> Sent: Thursday, September 1, 2022 11:43 PM
> To: Jianyong Wu <jianyong...@arm.com>; devel@edk2.groups.io
> Cc: ardb+tianoc...@kernel.org; Justin He <justin...@arm.com>; nd
> <n...@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.muja...@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...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93079): https://edk2.groups.io/g/devel/message/93079
Mute This Topic: https://groups.io/mt/93120531/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
