From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C16041A1EFE for ; Thu, 15 Sep 2016 07:15:49 -0700 (PDT) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 44DFCC05006A; Thu, 15 Sep 2016 14:15:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-94.phx2.redhat.com [10.3.116.94]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8FEFlZo013913; Thu, 15 Sep 2016 10:15:48 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org References: <1473946233-10547-1-git-send-email-ard.biesheuvel@linaro.org> <1473946233-10547-5-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <6c55da74-6982-d31e-c487-0fb1d6ccda8f@redhat.com> Date: Thu, 15 Sep 2016 16:15:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1473946233-10547-5-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 15 Sep 2016 14:15:49 +0000 (UTC) Subject: Re: [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Sep 2016 14:15:50 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 09/15/16 15:30, Ard Biesheuvel wrote: > Use the FDT client protocol rather than parsing the DT directly using > fdtlib. While we're at it, update the code so it deals correctly with > memory nodes that describe multiple disjoint regions in their "reg" > properties, and make the code work with #address-cells/#size-cells > properties of <1> as well as <2>. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/HighMemDxe/HighMemDxe.c | 120 +++++++++----------- > ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 16 ++- > 2 files changed, 62 insertions(+), 74 deletions(-) > > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > index 4d56e6236b54..08de3cbb7e9c 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > @@ -1,7 +1,7 @@ > /** @file > * High memory node enumeration DXE driver for ARM Virtual Machines > * > -* Copyright (c) 2015, Linaro Ltd. All rights reserved. > +* Copyright (c) 2015-2016, 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 > @@ -15,12 +15,12 @@ > **/ > > #include > -#include > #include > -#include > -#include > -#include > #include > +#include > +#include > + > +#include > > EFI_STATUS > EFIAPI > @@ -29,76 +29,66 @@ InitializeHighMemDxe ( > 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; > - } > + FDT_CLIENT_PROTOCOL *FdtClient; > + EFI_STATUS Status, FindNodeStatus; > + INT32 Node; > + CONST UINT32 *Reg; > + UINT32 RegSize; > + UINTN AddressCells, SizeCells; > + UINT64 CurBase; > + UINT64 CurSize; > > - DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase)); > + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, > + (VOID **)&FdtClient); > + ASSERT_EFI_ERROR (Status); > > // > - // Check for memory node and add the memory spaces expect the lowest one > + // Check for memory node and add the memory spaces except the lowest one > // > - for (Prev = 0;; Prev = Node) { > - Node = fdt_next_node (DeviceTreeBase, Prev, NULL); > - if (Node < 0) { > - break; > - } > + for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node, > + (CONST VOID **) &Reg, &AddressCells, > + &SizeCells, &RegSize); > + !EFI_ERROR (FindNodeStatus); > + FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node, > + &Node, (CONST VOID **) &Reg, &AddressCells, > + &SizeCells, &RegSize)) { > + I think we can remove this empty line. Not too important of course. > + ASSERT (AddressCells <= 2); > + ASSERT (SizeCells <= 2); > > - 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 != NULL && Len == (2 * sizeof (UINT64))) { > + while (RegSize > 0) { > This one too. (Also just a matter of taste.) > - CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]); > - CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]); > + CurBase = SwapBytes32 (*Reg++); > + if (AddressCells > 1) { > + CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++); The operator |= seems wrong; it should be just =. (You could do { CurBase <<= 32; CurBase |= SwapBytes32 (*Reg++); }. The above is more compact, but the operator should be fixed.) > + } > + CurSize = SwapBytes32 (*Reg++); > + if (SizeCells > 1) { > + CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++); Same here. With these fixed: Reviewed-by: Laszlo Ersek