On 4 December 2015 at 12:53, Laszlo Ersek <ler...@redhat.com> wrote:
> Nice in general; I have three comments:
>
> On 12/04/15 11:13, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.z...@linaro.org>
>>
>> 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 <shannon.z...@linaro.org>
>> ---
>> 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 <ler...@redhat.com>
>
> Ard, are you too okay with this patch?
>
Yes, this looks great. Thanks for rewriting it.
--
Ard.
>> + __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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
