Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Laszlo Ersek]

Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
> Cloud Hypervisor is kvm based VMM implemented in rust.
> 
> This library populates the system memory map for the
> Cloud Hypervisor virtual platform.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf          |  47 +++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c               |  94 ++++++++++++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
>  3 files changed, 241 insertions(+)

let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo

> 
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> new file mode 100644
> index 000000000000..04cb1f2a581a
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> @@ -0,0 +1,47 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = ClhVirtMemInfoPeiLib
> +  FILE_GUID                      = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +  CONSTRUCTOR                    = ClhVirtMemInfoPeiLibConstructor
> +
> +[Sources]
> +  ClhVirtMemInfoLib.c
> +  ClhVirtMemInfoPeiLibConstructor.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmTokenSpaceGuid.PcdFvSize
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..829d7d7aa259
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#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
> +
> +//
> +// mach-virt's 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       0x08000000
> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> +
> +//
> +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
> +//
> +#define CLH_UEFI_MEM_BASE       0x0
> +#define CLH_UEFI_MEM_SIZE       0x08000000
> +
> +/**
> +  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;
> +
> +  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
> +  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));
> +
> +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> +  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
> +  VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Map the FV region as normal executable memory
> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // End of Table
> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> new file mode 100644
> index 000000000000..5f89b70df990
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> @@ -0,0 +1,100 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <libfdt.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +ClhVirtMemInfoPeiLibConstructor (
> +  VOID
> +  )
> +{
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
> +
> +  NewBase = 0;
> +  NewSize = 0;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  //
> +  // Make sure we have a valid device tree blob
> +  //
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  //
> +  // 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) {
> +      //
> +      // 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));
> +
> +        if (NewBase > CurBase || NewBase == 0) {
> +          NewBase = CurBase;
> +          NewSize = CurSize;
> +        }
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> +          __FUNCTION__));
> +      }
> +    }
> +  }
> +
> +  //
> +  // Make sure the start of DRAM matches our expectation
> +  //
> +  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // We need to make sure that the machine we are running on has at least
> +  // 128 MB of memory configured, and is currently executing this binary from
> +  // NOR flash. This prevents a device tree image in DRAM from getting
> +  // clobbered when our caller installs permanent PEI RAM, before we have a
> +  // chance of marking its location as reserved or copy it to a freshly
> +  // allocated block in the permanent PEI RAM in the platform PEIM.
> +  //
> +  ASSERT (NewSize >= SIZE_128MB);
> +  ASSERT (
> +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> +
> +  return RETURN_SUCCESS;
> +}
> 

Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 12519 bytes --]

Hi Jianyong,

You need to check if you're subscribed to the EDK II development mailing list. Otherwise, your patch email will get rejected. You can subscribe here: https://edk2.groups.io/g/devel.
Make sure that you reply to the email with subscription confirmation sent from noreply@groups.io<mailto:noreply@groups.io>. Unless you do it, you won't become a member of the mailing list.
I would also recommend that you wait for a day after confirming, as I believe the edk2.groups.io admin will need to approve your membership.

Regards,

Sami Mujawar

From: Laszlo Ersek <lersek@redhat.com>
Date: Thursday, 22 April 2021 at 14:57
To: Jianyong Wu <Jianyong.Wu@arm.com>, edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Justin He <Justin.He@arm.com>, Ard Biesheuvel <ardb+tianocore@kernel.org>, Leif Lindholm <leif@nuviainc.com>, Sami Mujawar <Sami.Mujawar@arm.com>
Subject: Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor
Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
> Cloud Hypervisor is kvm based VMM implemented in rust.
>
> This library populates the system memory map for the
> Cloud Hypervisor virtual platform.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf          |  47 +++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c               |  94 ++++++++++++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
>  3 files changed, 241 insertions(+)

let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo

>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> new file mode 100644
> index 000000000000..04cb1f2a581a
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> @@ -0,0 +1,47 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = ClhVirtMemInfoPeiLib
> +  FILE_GUID                      = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +  CONSTRUCTOR                    = ClhVirtMemInfoPeiLibConstructor
> +
> +[Sources]
> +  ClhVirtMemInfoLib.c
> +  ClhVirtMemInfoPeiLibConstructor.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmTokenSpaceGuid.PcdFvSize
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..829d7d7aa259
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#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
> +
> +//
> +// mach-virt's 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       0x08000000
> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> +
> +//
> +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
> +//
> +#define CLH_UEFI_MEM_BASE       0x0
> +#define CLH_UEFI_MEM_SIZE       0x08000000
> +
> +/**
> +  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;
> +
> +  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
> +  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));
> +
> +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> +  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
> +  VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Map the FV region as normal executable memory
> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // End of Table
> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> new file mode 100644
> index 000000000000..5f89b70df990
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> @@ -0,0 +1,100 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <libfdt.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +ClhVirtMemInfoPeiLibConstructor (
> +  VOID
> +  )
> +{
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
> +
> +  NewBase = 0;
> +  NewSize = 0;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  //
> +  // Make sure we have a valid device tree blob
> +  //
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  //
> +  // 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) {
> +      //
> +      // 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));
> +
> +        if (NewBase > CurBase || NewBase == 0) {
> +          NewBase = CurBase;
> +          NewSize = CurSize;
> +        }
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> +          __FUNCTION__));
> +      }
> +    }
> +  }
> +
> +  //
> +  // Make sure the start of DRAM matches our expectation
> +  //
> +  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // We need to make sure that the machine we are running on has at least
> +  // 128 MB of memory configured, and is currently executing this binary from
> +  // NOR flash. This prevents a device tree image in DRAM from getting
> +  // clobbered when our caller installs permanent PEI RAM, before we have a
> +  // chance of marking its location as reserved or copy it to a freshly
> +  // allocated block in the permanent PEI RAM in the platform PEIM.
> +  //
> +  ASSERT (NewSize >= SIZE_128MB);
> +  ASSERT (
> +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> +
> +  return RETURN_SUCCESS;
> +}
>

[-- Attachment #2: Type: text/html, Size: 20948 bytes --]

Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Laszlo Ersek]

Hi Jianyong,

On 04/22/21 15:56, Laszlo Ersek wrote:

> (2) "Clh" is a catastrophically bad abbreviation. The whole point of
> your work is to add Cloud Hypervisor support, so why trash the most
> relevant information in the file names with an inane abbreviation?
> 
> (Not to mention that the name "Cloud Hypervisor" itself is as
> nondescript as possible. :/)

In an attempt to approach this constructively, I've given it more
thought. Does "CloudHv" sound acceptable to the community? I've seen
"hv" stand for "hypervisor" frequently.


I have another high-level note. I could delay it until after you post
v2, but I figure I could save you some time by sharing my observation
with you right now.

I think that the ACPI platform stuff, in patch#2, does not belong in
OvmfPkg/AcpiPlatformDxe. What's more, I don't think it belongs in
OvmfPkg, even.

The CloudHvAcpiPlatformDxe and CloudHvPlatformHasAcpiDtDxe drivers
should exist as stand-alone, self-contained drivers; they should be as
minimal as possible. This is already a given for
"CloudHvPlatformHasAcpiDtDxe", but it should also be possible for
"CloudHvAcpiPlatformDxe". OvmfPkg/AcpiPlatformDxe is a complex driver,
and the overlap between what OvmfPkg/AcpiPlatformDxe currently does, and
what CloudHvAcpiPlatformDxe actually *needs*, is virtually nil.

And so, the series shouldn't touch OvmfPkg at all.

Ultimately I suggest following the Xen pattern that can be seen under
ArmVirtPkg already. In detail, the following files and directories
should contain the new platform:

  ArmVirtPkg/ArmVirtCloudHv.dsc
  ArmVirtPkg/ArmVirtCloudHv.fdf
  ArmVirtPkg/CloudHvAcpiPlatformDxe/
  ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/
  ArmVirtPkg/Library/CloudHvVirtMemInfoLib/

(And I don't really see the point of an FDF include file.)

Thanks!
Laszlo

Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[jianyong.wu]

[-- Attachment #1: Type: text/plain, Size: 13448 bytes --]

Thanks Sami, I have registered the edk2 mail list and now I can receive the mails from it. Also, I thinks I am the member of edk2 group now.

From: Sami Mujawar <Sami.Mujawar@arm.com>
Sent: Thursday, April 22, 2021 10:13 PM
To: Laszlo Ersek <lersek@redhat.com>; Jianyong Wu <Jianyong.Wu@arm.com>; edk2-devel-groups-io <devel@edk2.groups.io>; edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Justin He <Justin.He@arm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>; nd <nd@arm.com>
Subject: Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Hi Jianyong,

You need to check if you're subscribed to the EDK II development mailing list. Otherwise, your patch email will get rejected. You can subscribe here: https://edk2.groups.io/g/devel.
Make sure that you reply to the email with subscription confirmation sent from noreply@groups.io<mailto:noreply@groups.io>. Unless you do it, you won't become a member of the mailing list.
I would also recommend that you wait for a day after confirming, as I believe the edk2.groups.io admin will need to approve your membership.

Regards,

Sami Mujawar

From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Date: Thursday, 22 April 2021 at 14:57
To: Jianyong Wu <Jianyong.Wu@arm.com<mailto:Jianyong.Wu@arm.com>>, edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Justin He <Justin.He@arm.com<mailto:Justin.He@arm.com>>, Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>, Leif Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>, Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>
Subject: Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor
Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
> Cloud Hypervisor is kvm based VMM implemented in rust.
>
> This library populates the system memory map for the
> Cloud Hypervisor virtual platform.
>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> Cc: Leif Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com<mailto:jianyong.wu@arm.com>>
> ---
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf          |  47 +++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c               |  94 ++++++++++++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
>  3 files changed, 241 insertions(+)

let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo

>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> new file mode 100644
> index 000000000000..04cb1f2a581a
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> @@ -0,0 +1,47 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = ClhVirtMemInfoPeiLib
> +  FILE_GUID                      = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +  CONSTRUCTOR                    = ClhVirtMemInfoPeiLibConstructor
> +
> +[Sources]
> +  ClhVirtMemInfoLib.c
> +  ClhVirtMemInfoPeiLibConstructor.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmTokenSpaceGuid.PcdFvSize
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..829d7d7aa259
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#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
> +
> +//
> +// mach-virt's 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       0x08000000
> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> +
> +//
> +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
> +//
> +#define CLH_UEFI_MEM_BASE       0x0
> +#define CLH_UEFI_MEM_SIZE       0x08000000
> +
> +/**
> +  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;
> +
> +  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
> +  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));
> +
> +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> +  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
> +  VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Map the FV region as normal executable memory
> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // End of Table
> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> new file mode 100644
> index 000000000000..5f89b70df990
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> @@ -0,0 +1,100 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <libfdt.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +ClhVirtMemInfoPeiLibConstructor (
> +  VOID
> +  )
> +{
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
> +
> +  NewBase = 0;
> +  NewSize = 0;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  //
> +  // Make sure we have a valid device tree blob
> +  //
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  //
> +  // 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) {
> +      //
> +      // 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));
> +
> +        if (NewBase > CurBase || NewBase == 0) {
> +          NewBase = CurBase;
> +          NewSize = CurSize;
> +        }
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> +          __FUNCTION__));
> +      }
> +    }
> +  }
> +
> +  //
> +  // Make sure the start of DRAM matches our expectation
> +  //
> +  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // We need to make sure that the machine we are running on has at least
> +  // 128 MB of memory configured, and is currently executing this binary from
> +  // NOR flash. This prevents a device tree image in DRAM from getting
> +  // clobbered when our caller installs permanent PEI RAM, before we have a
> +  // chance of marking its location as reserved or copy it to a freshly
> +  // allocated block in the permanent PEI RAM in the platform PEIM.
> +  //
> +  ASSERT (NewSize >= SIZE_128MB);
> +  ASSERT (
> +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> +
> +  return RETURN_SUCCESS;
> +}
>

[-- Attachment #2: Type: text/html, Size: 23272 bytes --]

Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Jianyong Wu]

Hi Laszlo,

Thanks for your time of commenting on my "horrible" patch set. It's very helpful. I will refactor the patch set base on your comments and resend it later more carefully.

Thanks
Jianyong


> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, April 22, 2021 9:57 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; edk2-devel-groups-io
> <devel@edk2.groups.io>
> Cc: Justin He <Justin.He@arm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>; Sami
> Mujawar <Sami.Mujawar@arm.com>
> Subject: Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for
> Cloud Hypervisor
>
> Hi Jianyong,
>
> On 04/22/21 10:24, Jianyong Wu wrote:
> > Cloud Hypervisor is kvm based VMM implemented in rust.
> >
> > This library populates the system memory map for the Cloud Hypervisor
> > virtual platform.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf          |
> 47 +++++++++
> >  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c               |  94
> ++++++++++++++++++
> >
> >
> ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> > | 100 ++++++++++++++++++++
> >  3 files changed, 241 insertions(+)
>
> let's sort out the meta-problems first:
>
> (1) you need a Feature Request BZ for this; <https://bugzilla.tianocore.org/>.
> The commit messages should reference the specific bugzilla ticket URL.
>
> (2) "Clh" is a catastrophically bad abbreviation. The whole point of your work
> is to add Cloud Hypervisor support, so why trash the most relevant
> information in the file names with an inane abbreviation?
>
> (Not to mention that the name "Cloud Hypervisor" itself is as nondescript as
> possible. :/)
>
> (3) I have not received a cover letter (0/4). Not sure if you sent one.
>
> (4) I don't see the messages in my edk2-devel folder, or in the mailing list
> archives, or in the messages held for moderation at the groups.io WebUI.
>
> (5) "Cloud Hypervisor" is not something that I can justifiably spend much time
> on. I'm willing to review this series at the level at which I've reviewed (for
> example) XenPVH or Bhyve in the past, mainly focusing on style and
> potential regressions. However, that's not enough for the long term:
> someone from ARM (or elsewhere) will have to step up for permanent
> reviewership. Please add a patch for extending "Maintainers.txt"
> appropriately. Example subsystems:
>
> - ArmVirtPkg: modules used on Xen
> - ArmVirtPkg: Kvmtool emulated platform support
> - OvmfPkg: bhyve-related modules
> - OvmfPkg: Xen-related modules
>
> Please keep the subsystem titles alphabetically sorted in the file.
>
> Please resend.
>
> (I'm posting these comments at once because they are understandable to
> the community even in the absence of your patches on the list.)
>
> Thanks
> Laszlo
>
> >
> > diff --git
> > a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> > b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> > new file mode 100644
> > index 000000000000..04cb1f2a581a
> > --- /dev/null
> > +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> > @@ -0,0 +1,47 @@
> > +#/* @file
> > +#
> > +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> > +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent # #*/
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x0001001A
> > +  BASE_NAME                      = ClhVirtMemInfoPeiLib
> > +  FILE_GUID                      = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> > +  CONSTRUCTOR                    = ClhVirtMemInfoPeiLibConstructor
> > +
> > +[Sources]
> > +  ClhVirtMemInfoLib.c
> > +  ClhVirtMemInfoPeiLibConstructor.c
> > +
> > +[Packages]
> > +  ArmPkg/ArmPkg.dec
> > +  ArmVirtPkg/ArmVirtPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +
> > +[LibraryClasses]
> > +  ArmLib
> > +  BaseMemoryLib
> > +  DebugLib
> > +  FdtLib
> > +  PcdLib
> > +  MemoryAllocationLib
> > +
> > +[Pcd]
> > +  gArmTokenSpaceGuid.PcdFdBaseAddress
> > +  gArmTokenSpaceGuid.PcdFvBaseAddress
> > +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> > +  gArmTokenSpaceGuid.PcdSystemMemorySize
> > +
> > +[FixedPcd]
> > +  gArmTokenSpaceGuid.PcdFdSize
> > +  gArmTokenSpaceGuid.PcdFvSize
> > +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> > diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> > b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> > new file mode 100644
> > index 000000000000..829d7d7aa259
> > --- /dev/null
> > +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> > @@ -0,0 +1,94 @@
> > +/** @file
> > +
> > +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#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
> > +
> > +//
> > +// mach-virt's 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       0x08000000
> > +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> > +
> > +//
> > +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory
> > +for UEFI //
> > +#define CLH_UEFI_MEM_BASE       0x0
> > +#define CLH_UEFI_MEM_SIZE       0x08000000
> > +
> > +/**
> > +  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;
> > +
> > +  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
> > +  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));
> > +
> > +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> > + VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> > + VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> > +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
> > +  VirtualMemoryTable[1].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +
> > +  // Map the FV region as normal executable memory
> > + VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> > + VirtualMemoryTable[2].VirtualBase  =
> VirtualMemoryTable[2].PhysicalBase;
> > +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
> > +  VirtualMemoryTable[2].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> > +
> > +  // End of Table
> > +  ZeroMem (&VirtualMemoryTable[3], sizeof
> > + (ARM_MEMORY_REGION_DESCRIPTOR));
> > +
> > +  *VirtualMemoryMap = VirtualMemoryTable; }
> > diff --git
> >
> a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor
> > .c
> >
> b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor
> > .c
> > new file mode 100644
> > index 000000000000..5f89b70df990
> > --- /dev/null
> > +++
> b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstru
> > +++ ctor.c
> > @@ -0,0 +1,100 @@
> > +/** @file
> > +
> > +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include <Base.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <libfdt.h>
> > +
> > +RETURN_STATUS
> > +EFIAPI
> > +ClhVirtMemInfoPeiLibConstructor (
> > +  VOID
> > +  )
> > +{
> > +  VOID          *DeviceTreeBase;
> > +  INT32         Node, Prev;
> > +  UINT64        NewBase, CurBase;
> > +  UINT64        NewSize, CurSize;
> > +  CONST CHAR8   *Type;
> > +  INT32         Len;
> > +  CONST UINT64  *RegProp;
> > +  RETURN_STATUS PcdStatus;
> > +
> > +  NewBase = 0;
> > +  NewSize = 0;
> > +
> > +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64
> > + (PcdDeviceTreeInitialBaseAddress);
> > +  ASSERT (DeviceTreeBase != NULL);
> > +
> > +  //
> > +  // Make sure we have a valid device tree blob  //  ASSERT
> > + (fdt_check_header (DeviceTreeBase) == 0);
> > +
> > +  //
> > +  // 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) {
> > +      //
> > +      // 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));
> > +
> > +        if (NewBase > CurBase || NewBase == 0) {
> > +          NewBase = CurBase;
> > +          NewSize = CurSize;
> > +        }
> > +      } else {
> > +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> > +          __FUNCTION__));
> > +      }
> > +    }
> > +  }
> > +
> > +  //
> > +  // Make sure the start of DRAM matches our expectation  //  ASSERT
> > + (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);  PcdStatus =
> > + PcdSet64S (PcdSystemMemorySize, NewSize);  ASSERT_RETURN_ERROR
> > + (PcdStatus);
> > +
> > +  //
> > +  // We need to make sure that the machine we are running on has at
> > + least  // 128 MB of memory configured, and is currently executing
> > + this binary from  // NOR flash. This prevents a device tree image in
> > + DRAM from getting  // clobbered when our caller installs permanent
> > + PEI RAM, before we have a  // chance of marking its location as
> > + reserved or copy it to a freshly  // allocated block in the permanent PEI
> RAM in the platform PEIM.
> > +  //
> > +  ASSERT (NewSize >= SIZE_128MB);
> > +  ASSERT (
> > +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> > +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> > +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> > +
> > +  return RETURN_SUCCESS;
> > +}
> >

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Jianyong Wu]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, April 23, 2021 8:00 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; edk2-devel-groups-io
> <devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Justin He <Justin.He@arm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory
> initialization for Cloud Hypervisor
>
> Hi Jianyong,
>
> On 04/22/21 15:56, Laszlo Ersek wrote:
>
> > (2) "Clh" is a catastrophically bad abbreviation. The whole point of
> > your work is to add Cloud Hypervisor support, so why trash the most
> > relevant information in the file names with an inane abbreviation?
> >
> > (Not to mention that the name "Cloud Hypervisor" itself is as
> > nondescript as possible. :/)
>
> In an attempt to approach this constructively, I've given it more thought.
> Does "CloudHv" sound acceptable to the community? I've seen "hv" stand
> for "hypervisor" frequently.
>
>
Yeah, CloudHv is better, as the original name is too long. I will take it as the abbreviation of Cloud Hypervisor.

> I have another high-level note. I could delay it until after you post v2, but I
> figure I could save you some time by sharing my observation with you right
> now.
>
> I think that the ACPI platform stuff, in patch#2, does not belong in
> OvmfPkg/AcpiPlatformDxe. What's more, I don't think it belongs in OvmfPkg,
> even.
>
> The CloudHvAcpiPlatformDxe and CloudHvPlatformHasAcpiDtDxe drivers
> should exist as stand-alone, self-contained drivers; they should be as minimal
> as possible. This is already a given for "CloudHvPlatformHasAcpiDtDxe", but it
> should also be possible for "CloudHvAcpiPlatformDxe".
> OvmfPkg/AcpiPlatformDxe is a complex driver, and the overlap between
> what OvmfPkg/AcpiPlatformDxe currently does, and what
> CloudHvAcpiPlatformDxe actually *needs*, is virtually nil.
>
> And so, the series shouldn't touch OvmfPkg at all.
>
> Ultimately I suggest following the Xen pattern that can be seen under
> ArmVirtPkg already. In detail, the following files and directories should
> contain the new platform:
>
>   ArmVirtPkg/ArmVirtCloudHv.dsc
>   ArmVirtPkg/ArmVirtCloudHv.fdf
>   ArmVirtPkg/CloudHvAcpiPlatformDxe/
>   ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/
>   ArmVirtPkg/Library/CloudHvVirtMemInfoLib/
>
Ok , it seems more coherent. I will reorganize the files according to Acpi.

> (And I don't really see the point of an FDF include file.)

Yeah, I can include them into the fdf file directly.

Thanks
Jianyong

>
> Thanks!
> Laszlo

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Hi Laszlo,

On Thu, Apr 22, 2021 at 06:56 AM, Laszlo Ersek wrote:

> 
> 5) "Cloud Hypervisor" is not something that I can justifiably spend
> much time on. I'm willing to review this series at the level at which
> I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
> on style and potential regressions. However, that's not enough for the
> long term: someone from ARM (or elsewhere) will have to step up for
> permanent reviewership. Please add a patch for extending
> "Maintainers.txt" appropriately. Example subsystems:

I can help to review the 'Cloud Hypervisor' patches and will send out a patch to update the reviewership once the initial series is merged.

Hi Jainyong,

I could not find the remaining patches from your v1 series. Can you forward them to me, please?

I can review and provide feedback so that they are addressed in your v2 series.

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 2221 bytes --]

Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

[Laszlo Ersek]

On 05/04/21 10:31, Sami Mujawar wrote:
> Hi Laszlo,
> 
> On Thu, Apr 22, 2021 at 06:56 AM, Laszlo Ersek wrote:
> 
>>
>> 5) "Cloud Hypervisor" is not something that I can justifiably spend
>> much time on. I'm willing to review this series at the level at which
>> I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
>> on style and potential regressions. However, that's not enough for the
>> long term: someone from ARM (or elsewhere) will have to step up for
>> permanent reviewership. Please add a patch for extending
>> "Maintainers.txt" appropriately. Example subsystems:
> 
> I can help to review the 'Cloud Hypervisor' patches and will send out a patch to update the reviewership once the initial series is merged.

OK, thanks!

> 
> Hi Jainyong,
> 
> I could not find the remaining patches from your v1 series. Can you forward them to me, please?

Indeed, the v1 patches have not reached the list.

Thanks,
Laszlo

> 
> I can review and provide feedback so that they are addressed in your v2 series.
> 
> Regards,
> 
> Sami Mujawar
>