From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x235.google.com (mail-it0-x235.google.com [IPv6:2607:f8b0:4001:c0b::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BD95C1A1EFC for ; Thu, 15 Sep 2016 07:18:18 -0700 (PDT) Received: by mail-it0-x235.google.com with SMTP id r192so97190112ita.0 for ; Thu, 15 Sep 2016 07:18:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yhAAKH+kt7FvbUS9ZqG4ghTzfG8RAxuHoKCocpvZ0aU=; b=MqvTU9YFeqFT/uafaXImwtnfZL1cBERPZ/0NhjeAhYcnviyNlCcJMf7lkcAUAm+9+G biQ473NUV9TTpFzIKbc+WFRnvnCXtIP49zc5TAFA1mdG8UX+LOJ8DnPOISHM3zw//Oz5 KGeXp/laq9Em9Tfq20Ra/9eq2TSqNGLEW3ZtU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yhAAKH+kt7FvbUS9ZqG4ghTzfG8RAxuHoKCocpvZ0aU=; b=SlgxVW8MCG43Xs6tbKISWRzv7IwZibejyFPh2X1rbjZVFNEmMbm+EKuBDtl2RQVexY OMnzrOt0r45e4HUQhyK5Ie8ZlrL1KjhrpEGBoatSJDBMxt3Cxuw2QjRQlTTQz3lM5hQg Z8dE6H2QlI1oGTgw9j6gF4aBHU1SHz/idA25raaliJpPzRvJbK67rx10ES/9d+Zf/HWG /K1sXC6ksf8aWajgUy6cXlLdkuWPH/9lWnF3hjxAm+xJw0IBx5L0fv6U/hAqCBXNNS4i 1tEMqXazymEUAOJS6RyQr4xe0dZWPISej4Zy720nCOkX7T7vmqT6tq0F8eNlmZ7PFJkz tp6A== X-Gm-Message-State: AE9vXwPnptnQxA8Qfaz/PmPSd4C2wFI9B2EqcdbYvIgAuO+cDTDSx8HN1eVCH75RcUHw+rTOqfeHJKbkbulFxgB0 X-Received: by 10.107.154.7 with SMTP id c7mr1363872ioe.209.1473949097928; Thu, 15 Sep 2016 07:18:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Thu, 15 Sep 2016 07:18:17 -0700 (PDT) In-Reply-To: <6c55da74-6982-d31e-c487-0fb1d6ccda8f@redhat.com> References: <1473946233-10547-1-git-send-email-ard.biesheuvel@linaro.org> <1473946233-10547-5-git-send-email-ard.biesheuvel@linaro.org> <6c55da74-6982-d31e-c487-0fb1d6ccda8f@redhat.com> From: Ard Biesheuvel Date: Thu, 15 Sep 2016 15:18:17 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Shannon Zhao , Sakar Arora 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:18:19 -0000 Content-Type: text/plain; charset=UTF-8 On 15 September 2016 at 15:15, Laszlo Ersek wrote: > 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. > Sure >> + 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.) > If you prefer, I don't care either way. >> - 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.) > You are right. I got away with it due to the fact that I tested with base/size pairs <= 4GB, and so the leading cell == 0 in all cases. Thanks for spotting that! >> + } >> + CurSize = SwapBytes32 (*Reg++); >> + if (SizeCells > 1) { >> + CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++); > > Same here. > > With these fixed: > > Reviewed-by: Laszlo Ersek > OK, thanks.