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?
Thanks!
Laszlo
> + __FUNCTION__, CurBase, CurBase + CurSize - 1));
> + } else {
> + DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
> + __FUNCTION__, CurBase, CurBase + CurSize - 1));
> + }
> + }
> + }
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> new file mode 100644
> index 0000000..0eb37c7
> --- /dev/null
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> @@ -0,0 +1,51 @@
> +## @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.
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = HighMemDxe
> + FILE_GUID = 63EA1463-FBFA-428A-B97F-E222755852D7
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> +
> + ENTRY_POINT = InitializeHighMemDxe
> +
> +[Sources]
> + HighMemDxe.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + ArmPkg/ArmPkg.dec
> + ArmPlatformPkg/ArmPlatformPkg.dec
> + ArmVirtPkg/ArmVirtPkg.dec
> + EmbeddedPkg/EmbeddedPkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> + PcdLib
> + UefiDriverEntryPoint
> + FdtLib
> + HobLib
> + DxeServicesTableLib
> +
> +[Guids]
> + gFdtHobGuid
> +
> +[FixedPcd]
> + gArmTokenSpaceGuid.PcdSystemMemoryBase
> +
> +[Depex]
> + gEfiCpuArchProtocolGuid
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
