Re: [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 15 September 2016 at 15:15, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  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 <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>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/FdtClient.h>
>>
>>  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 <lersek@redhat.com>
>

OK, thanks.