On 12/04/15 16:26, Ard Biesheuvel wrote: > On 4 December 2015 at 12:53, Laszlo Ersek <[email protected]> wrote: >> Nice in general; I have three comments: >> >> On 12/04/15 11:13, Shannon Zhao wrote: >>> From: Shannon Zhao <[email protected]> >>> >>> Here we add the memory space for the high memory nodes except the lowest >>> one in FDT. So these spaces will show up in the UEFI memory map. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Shannon Zhao <[email protected]> >>> --- >>> ArmVirtPkg/ArmVirtQemu.dsc | 1 + >>> ArmVirtPkg/ArmVirtQemu.fdf | 1 + >>> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 105 >>> +++++++++++++++++++++++++++++++++++ >>> ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 51 +++++++++++++++++ >>> 4 files changed, 158 insertions(+) >>> create mode 100644 ArmVirtPkg/HighMemDxe/HighMemDxe.c >>> create mode 100644 ArmVirtPkg/HighMemDxe/HighMemDxe.inf >>> >>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >>> index b0d1d08..e6440ec 100644 >>> --- a/ArmVirtPkg/ArmVirtQemu.dsc >>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >>> @@ -302,6 +302,7 @@ >>> # Platform Driver >>> # >>> ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf >>> + ArmVirtPkg/HighMemDxe/HighMemDxe.inf >>> OvmfPkg/VirtioBlkDxe/VirtioBlk.inf >>> OvmfPkg/VirtioScsiDxe/VirtioScsi.inf >>> OvmfPkg/VirtioNetDxe/VirtioNet.inf >>> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf >>> index 738e3db..f5e6cbd 100644 >>> --- a/ArmVirtPkg/ArmVirtQemu.fdf >>> +++ b/ArmVirtPkg/ArmVirtQemu.fdf >>> @@ -108,6 +108,7 @@ READ_LOCK_STATUS = TRUE >>> INF MdeModulePkg/Core/Dxe/DxeMain.inf >>> INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf >>> INF ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf >>> + INF ArmVirtPkg/HighMemDxe/HighMemDxe.inf >>> >>> # >>> # PI DXE Drivers producing Architectural Protocols (EFI Services) >>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c >>> b/ArmVirtPkg/HighMemDxe/HighMemDxe.c >>> new file mode 100644 >>> index 0000000..08b010e >>> --- /dev/null >>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c >>> @@ -0,0 +1,105 @@ >>> +/** @file >>> +* High memory node enumeration DXE driver for ARM Virtual Machines >>> +* >>> +* Copyright (c) 2015, Linaro Ltd. All rights reserved. >>> +* >>> +* This program and the accompanying materials are >>> +* licensed and made available under the terms and conditions of the BSD >>> License >>> +* which accompanies this distribution. The full text of the license may >>> be found at >>> +* http://opensource.org/licenses/bsd-license.php >>> +* >>> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >>> IMPLIED. >>> +* >>> +**/ >>> + >>> +#include <Library/BaseLib.h> >>> +#include <Library/UefiDriverEntryPoint.h> >>> +#include <Library/DebugLib.h> >>> +#include <Library/PcdLib.h> >>> +#include <Library/HobLib.h> >>> +#include <libfdt.h> >>> +#include <Library/DxeServicesTableLib.h> >>> + >>> +EFI_STATUS >>> +EFIAPI >>> +InitializeHighMemDxe ( >>> + IN EFI_HANDLE ImageHandle, >>> + IN EFI_SYSTEM_TABLE *SystemTable >>> + ) >>> +{ >>> + VOID *Hob; >>> + VOID *DeviceTreeBase; >>> + INT32 Node, Prev; >>> + EFI_STATUS Status; >>> + CONST CHAR8 *Type; >>> + INT32 Len; >>> + CONST VOID *RegProp; >>> + UINT64 CurBase; >>> + UINT64 CurSize; >>> + >>> + Hob = GetFirstGuidHob(&gFdtHobGuid); >>> + if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) { >>> + return EFI_NOT_FOUND; >>> + } >>> + DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob); >>> + >>> + if (fdt_check_header (DeviceTreeBase) != 0) { >>> + DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__, >>> DeviceTreeBase)); >>> + return EFI_NOT_FOUND; >>> + } >>> + >>> + DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase)); >>> + >>> + // >>> + // Check for memory node and add the memory spaces expect the lowest one >>> + // >>> + for (Prev = 0;; Prev = Node) { >>> + Node = fdt_next_node (DeviceTreeBase, Prev, NULL); >>> + if (Node < 0) { >>> + break; >>> + } >>> + >>> + 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))) { >> >> (1) This is valid C, but our coding style requires "RegProp != NULL". >> >> I can fix this up when committing the patch. >> >>> + >>> + CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]); >>> + CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]); >>> + >>> + if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) { >>> + Status = gDS->AddMemorySpace ( >>> + EfiGcdMemoryTypeSystemMemory, >>> + CurBase, CurSize, >>> + EFI_MEMORY_WB | EFI_MEMORY_WC | >>> + EFI_MEMORY_WT | EFI_MEMORY_UC); >>> + >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((EFI_D_ERROR, "%a: Failed to add System RAM @ 0x%lx - >>> 0x%lx\n", >>> + __FUNCTION__, CurBase, CurBase + CurSize - 1)); >>> + continue; >>> + } >>> + >>> + Status = gDS->SetMemorySpaceAttributes ( >>> + CurBase, CurSize, >>> + EFI_MEMORY_WB); >>> + >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((EFI_D_ERROR, "%a: Failed to set System RAM @ 0x%lx - >>> 0x%lx attribute\n", >> >> (2) This line is way too long, my preference is no more than 79 >> characters. I can also fix this up when committing the patch. >> >> (3) When logging errors where a status code of type RETURN_STATUS or >> EFI_STATUS is available, we usually print it, with the edk2-specific %r >> conversion specifier. >> >> I can fix that up too. >> >> Reviewed-by: Laszlo Ersek <[email protected]> >> >> Ard, are you too okay with this patch? >> > > Yes, this looks great. Thanks for rewriting it. >
Committed to SVN as r19123 and r19124. Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
