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

[Laszlo Ersek <lersek@redhat.com>]

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.

> +    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 <lersek@redhat.com>