<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="EN-GB" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="mso-fareast-language:EN-US">Hi Jianyong,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal">You need to check if you're subscribed to the <i>EDK II development mailing list</i>. Otherwise, your patch email will get rejected. You can subscribe here: <a href="https://edk2.groups.io/g/devel" target="_blank">https://edk2.groups.io/g/devel</a>.<o:p></o:p></p>
<p class="MsoNormal">Make sure that you reply to the email with subscription confirmation sent from <a href="mailto:noreply@groups.io" target="_blank">noreply@groups.io</a>. Unless you do it, you won't become a member of the mailing list.
<o:p></o:p></p>
<p class="MsoNormal">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.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Regards,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Sami Mujawar<span style="mso-fareast-language:EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:12.0pt;color:black">From:
</span></b><span style="font-size:12.0pt;color:black">Laszlo Ersek <lersek@redhat.com><br>
<b>Date: </b>Thursday, 22 April 2021 at 14:57<br>
<b>To: </b>Jianyong Wu <Jianyong.Wu@arm.com>, edk2-devel-groups-io <devel@edk2.groups.io><br>
<b>Cc: </b>Justin He <Justin.He@arm.com>, Ard Biesheuvel <ardb+tianocore@kernel.org>, Leif Lindholm <leif@nuviainc.com>, Sami Mujawar <Sami.Mujawar@arm.com><br>
<b>Subject: </b>Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">Hi Jianyong,<br>
<br>
On 04/22/21 10:24, Jianyong Wu wrote:<br>
> Cloud Hypervisor is kvm based VMM implemented in rust.<br>
> <br>
> This library populates the system memory map for the<br>
> Cloud Hypervisor virtual platform.<br>
> <br>
> Cc: Laszlo Ersek <lersek@redhat.com><br>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org><br>
> Cc: Leif Lindholm <leif@nuviainc.com><br>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com><br>
> ---<br>
> ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf | 47 +++++++++<br>
> ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c | 94 ++++++++++++++++++<br>
> ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++<br>
> 3 files changed, 241 insertions(+)<br>
<br>
let's sort out the meta-problems first:<br>
<br>
(1) you need a Feature Request BZ for this;<br>
<<a href="https://bugzilla.tianocore.org/">https://bugzilla.tianocore.org/</a>>. The commit messages should reference<br>
the specific bugzilla ticket URL.<br>
<br>
(2) "Clh" is a catastrophically bad abbreviation. The whole point of<br>
your work is to add Cloud Hypervisor support, so why trash the most<br>
relevant information in the file names with an inane abbreviation?<br>
<br>
(Not to mention that the name "Cloud Hypervisor" itself is as<br>
nondescript as possible. :/)<br>
<br>
(3) I have not received a cover letter (0/4). Not sure if you sent one.<br>
<br>
(4) I don't see the messages in my edk2-devel folder, or in the mailing<br>
list archives, or in the messages held for moderation at the groups.io<br>
WebUI.<br>
<br>
(5) "Cloud Hypervisor" is not something that I can justifiably spend<br>
much time on. I'm willing to review this series at the level at which<br>
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing<br>
on style and potential regressions. However, that's not enough for the<br>
long term: someone from ARM (or elsewhere) will have to step up for<br>
permanent reviewership. Please add a patch for extending<br>
"Maintainers.txt" appropriately. Example subsystems:<br>
<br>
- ArmVirtPkg: modules used on Xen<br>
- ArmVirtPkg: Kvmtool emulated platform support<br>
- OvmfPkg: bhyve-related modules<br>
- OvmfPkg: Xen-related modules<br>
<br>
Please keep the subsystem titles alphabetically sorted in the file.<br>
<br>
Please resend.<br>
<br>
(I'm posting these comments at once because they are understandable to<br>
the community even in the absence of your patches on the list.)<br>
<br>
Thanks<br>
Laszlo<br>
<br>
> <br>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf<br>
> new file mode 100644<br>
> index 000000000000..04cb1f2a581a<br>
> --- /dev/null<br>
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf<br>
> @@ -0,0 +1,47 @@<br>
> +#/* @file<br>
> +#<br>
> +# Copyright (c) 2011-2015, ARM Limited. All rights reserved.<br>
> +# Copyright (c) 2014-2017, Linaro Limited. All rights reserved.<br>
> +#<br>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent<br>
> +#<br>
> +#*/<br>
> +<br>
> +[Defines]<br>
> + INF_VERSION = 0x0001001A<br>
> + BASE_NAME = ClhVirtMemInfoPeiLib<br>
> + FILE_GUID = 3E29D940-0591-EE6A-CAD4-223A9CF55E75<br>
> + MODULE_TYPE = BASE<br>
> + VERSION_STRING = 1.0<br>
> + LIBRARY_CLASS = ArmVirtMemInfoLib|PEIM<br>
> + CONSTRUCTOR = ClhVirtMemInfoPeiLibConstructor<br>
> +<br>
> +[Sources]<br>
> + ClhVirtMemInfoLib.c<br>
> + ClhVirtMemInfoPeiLibConstructor.c<br>
> +<br>
> +[Packages]<br>
> + ArmPkg/ArmPkg.dec<br>
> + ArmVirtPkg/ArmVirtPkg.dec<br>
> + EmbeddedPkg/EmbeddedPkg.dec<br>
> + MdeModulePkg/MdeModulePkg.dec<br>
> + MdePkg/MdePkg.dec<br>
> +<br>
> +[LibraryClasses]<br>
> + ArmLib<br>
> + BaseMemoryLib<br>
> + DebugLib<br>
> + FdtLib<br>
> + PcdLib<br>
> + MemoryAllocationLib<br>
> +<br>
> +[Pcd]<br>
> + gArmTokenSpaceGuid.PcdFdBaseAddress<br>
> + gArmTokenSpaceGuid.PcdFvBaseAddress<br>
> + gArmTokenSpaceGuid.PcdSystemMemoryBase<br>
> + gArmTokenSpaceGuid.PcdSystemMemorySize<br>
> +<br>
> +[FixedPcd]<br>
> + gArmTokenSpaceGuid.PcdFdSize<br>
> + gArmTokenSpaceGuid.PcdFvSize<br>
> + gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress<br>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c<br>
> new file mode 100644<br>
> index 000000000000..829d7d7aa259<br>
> --- /dev/null<br>
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c<br>
> @@ -0,0 +1,94 @@<br>
> +/** @file<br>
> +<br>
> + Copyright (c) 2014-2017, Linaro Limited. All rights reserved.<br>
> +<br>
> + SPDX-License-Identifier: BSD-2-Clause-Patent<br>
> +<br>
> +**/<br>
> +<br>
> +#include <Base.h><br>
> +#include <Library/ArmLib.h><br>
> +#include <Library/BaseMemoryLib.h><br>
> +#include <Library/DebugLib.h><br>
> +#include <Library/MemoryAllocationLib.h><br>
> +<br>
> +// Number of Virtual Memory Map Descriptors<br>
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5<br>
> +<br>
> +//<br>
> +// mach-virt's core peripherals such as the UART, the GIC and the RTC are<br>
> +// all mapped in the 'miscellaneous device I/O' region, which we just map<br>
> +// in its entirety rather than device by device. Note that it does not<br>
> +// cover any of the NOR flash banks or PCI resource windows.<br>
> +//<br>
> +#define MACH_VIRT_PERIPH_BASE 0x08000000<br>
> +#define MACH_VIRT_PERIPH_SIZE SIZE_128MB<br>
> +<br>
> +//<br>
> +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI<br>
> +//<br>
> +#define CLH_UEFI_MEM_BASE 0x0<br>
> +#define CLH_UEFI_MEM_SIZE 0x08000000<br>
> +<br>
> +/**<br>
> + Return the Virtual Memory Map of your platform<br>
> +<br>
> + This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU<br>
> + on your platform.<br>
> +<br>
> + @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR<br>
> + describing a Physical-to-Virtual Memory<br>
> + mapping. This array must be ended by a<br>
> + zero-filled entry. The allocated memory<br>
> + will not be freed.<br>
> +<br>
> +**/<br>
> +VOID<br>
> +ArmVirtGetMemoryMap (<br>
> + OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap<br>
> + )<br>
> +{<br>
> + ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;<br>
> +<br>
> + ASSERT (VirtualMemoryMap != NULL);<br>
> +<br>
> + VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *<br>
> + MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);<br>
> +<br>
> + if (VirtualMemoryTable == NULL) {<br>
> + DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", __FUNCTION__));<br>
> + return;<br>
> + }<br>
> +<br>
> + // System DRAM<br>
> + VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);<br>
> + VirtualMemoryTable[0].VirtualBase = VirtualMemoryTable[0].PhysicalBase;<br>
> + VirtualMemoryTable[0].Length = PcdGet64 (PcdSystemMemorySize);<br>
> + VirtualMemoryTable[0].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;<br>
> +<br>
> + DEBUG ((DEBUG_INFO, "%a: Dumping System DRAM Memory Map:\n"<br>
> + "\tPhysicalBase: 0x%lX\n"<br>
> + "\tVirtualBase: 0x%lX\n"<br>
> + "\tLength: 0x%lX\n",<br>
> + __FUNCTION__,<br>
> + VirtualMemoryTable[0].PhysicalBase,<br>
> + VirtualMemoryTable[0].VirtualBase,<br>
> + VirtualMemoryTable[0].Length));<br>
> +<br>
> + // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)<br>
> + VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;<br>
> + VirtualMemoryTable[1].VirtualBase = MACH_VIRT_PERIPH_BASE;<br>
> + VirtualMemoryTable[1].Length = MACH_VIRT_PERIPH_SIZE;<br>
> + VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;<br>
> +<br>
> + // Map the FV region as normal executable memory<br>
> + VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);<br>
> + VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;<br>
> + VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize);<br>
> + VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;<br>
> +<br>
> + // End of Table<br>
> + ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));<br>
> +<br>
> + *VirtualMemoryMap = VirtualMemoryTable;<br>
> +}<br>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c<br>
> new file mode 100644<br>
> index 000000000000..5f89b70df990<br>
> --- /dev/null<br>
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c<br>
> @@ -0,0 +1,100 @@<br>
> +/** @file<br>
> +<br>
> + Copyright (c) 2014-2017, Linaro Limited. All rights reserved.<br>
> +<br>
> + SPDX-License-Identifier: BSD-2-Clause-Patent<br>
> +<br>
> +**/<br>
> +<br>
> +#include <Base.h><br>
> +#include <Library/DebugLib.h><br>
> +#include <Library/PcdLib.h><br>
> +#include <libfdt.h><br>
> +<br>
> +RETURN_STATUS<br>
> +EFIAPI<br>
> +ClhVirtMemInfoPeiLibConstructor (<br>
> + VOID<br>
> + )<br>
> +{<br>
> + VOID *DeviceTreeBase;<br>
> + INT32 Node, Prev;<br>
> + UINT64 NewBase, CurBase;<br>
> + UINT64 NewSize, CurSize;<br>
> + CONST CHAR8 *Type;<br>
> + INT32 Len;<br>
> + CONST UINT64 *RegProp;<br>
> + RETURN_STATUS PcdStatus;<br>
> +<br>
> + NewBase = 0;<br>
> + NewSize = 0;<br>
> +<br>
> + DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);<br>
> + ASSERT (DeviceTreeBase != NULL);<br>
> +<br>
> + //<br>
> + // Make sure we have a valid device tree blob<br>
> + //<br>
> + ASSERT (fdt_check_header (DeviceTreeBase) == 0);<br>
> +<br>
> + //<br>
> + // Look for the lowest memory node<br>
> + //<br>
> + for (Prev = 0;; Prev = Node) {<br>
> + Node = fdt_next_node (DeviceTreeBase, Prev, NULL);<br>
> + if (Node < 0) {<br>
> + break;<br>
> + }<br>
> +<br>
> + //<br>
> + // Check for memory node<br>
> + //<br>
> + Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);<br>
> + if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {<br>
> + //<br>
> + // Get the 'reg' property of this node. For now, we will assume<br>
> + // two 8 byte quantities for base and size, respectively.<br>
> + //<br>
> + RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);<br>
> + if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {<br>
> +<br>
> + CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));<br>
> + CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));<br>
> +<br>
> + DEBUG ((DEBUG_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",<br>
> + __FUNCTION__, CurBase, CurBase + CurSize - 1));<br>
> +<br>
> + if (NewBase > CurBase || NewBase == 0) {<br>
> + NewBase = CurBase;<br>
> + NewSize = CurSize;<br>
> + }<br>
> + } else {<br>
> + DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",<br>
> + __FUNCTION__));<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + //<br>
> + // Make sure the start of DRAM matches our expectation<br>
> + //<br>
> + ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);<br>
> + PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);<br>
> + ASSERT_RETURN_ERROR (PcdStatus);<br>
> +<br>
> + //<br>
> + // We need to make sure that the machine we are running on has at least<br>
> + // 128 MB of memory configured, and is currently executing this binary from<br>
> + // NOR flash. This prevents a device tree image in DRAM from getting<br>
> + // clobbered when our caller installs permanent PEI RAM, before we have a<br>
> + // chance of marking its location as reserved or copy it to a freshly<br>
> + // allocated block in the permanent PEI RAM in the platform PEIM.<br>
> + //<br>
> + ASSERT (NewSize >= SIZE_128MB);<br>
> + ASSERT (<br>
> + (((UINT64)PcdGet64 (PcdFdBaseAddress) +<br>
> + (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||<br>
> + ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));<br>
> +<br>
> + return RETURN_SUCCESS;<br>
> +}<br>
> <o:p></o:p></p>
</div>
</div>
</body>
</html>
<div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/74367">View/Reply Online (#74367)</a> | | <a target="_blank" href="https://groups.io/mt/82285671/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a><br> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div>