[edk2,v4,10/16] ArmVirtualizationPkg: add VirtFdtDxe driver

This driver enumerates the device nodes in the device tree located at the
base address passed in gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, and
installs drivers for devices it cares about (GIC interrupt controller, RTC,
architected timer interrupt)

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c   | 261 +++++++++++++++++++++
 .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf |  59 +++++
 2 files changed, 320 insertions(+)
 create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
 create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf

splendid updates, and I'm *almost* giving my R-b :) , but:

On 08/28/14 16:13, Ard Biesheuvel wrote:
> This driver enumerates the device nodes in the device tree located at the
> base address passed in gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, and
> installs drivers for devices it cares about (GIC interrupt controller, RTC,
> architected timer interrupt)
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c   | 261 +++++++++++++++++++++
>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf |  59 +++++
>  2 files changed, 320 insertions(+)
>  create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
>  create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> 
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> new file mode 100644
> index 000000000000..28c34c1cb094
> --- /dev/null
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -0,0 +1,261 @@
> +/** @file
> +*  Device tree enumeration DXE driver for ARM Virtual Machines
> +*
> +*  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
> +*  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/DebugLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/VirtioMmioDeviceLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/DxeServicesLib.h>
> +#include <libfdt.h>
> +
> +#include <Guid/Fdt.h>
> +
> +CONST UINT32 mUint32Max = 0xFFFFFFFFU;
> +
> +#pragma pack (1)
> +typedef struct {
> +  VENDOR_DEVICE_PATH                  Vendor;
> +  UINT64                              PhysBase;
> +  EFI_DEVICE_PATH_PROTOCOL            End;
> +} VIRTIO_TRANSPORT_DEVICE_PATH;
> +#pragma pack ()
> +
> +typedef enum {
> +  PropertyTypeUnknown,
> +  PropertyTypeGic,
> +  PropertyTypeRtc,
> +  PropertyTypeVirtio,
> +  PropertyTypeUart,
> +  PropertyTypeTimer,
> +} PROPERTY_TYPE;
> +
> +typedef struct {
> +  PROPERTY_TYPE Type;
> +  CHAR8         Compatible[20];
> +} PROPERTY;
> +
> +STATIC CONST PROPERTY CompatibleProperties[] = {
> +  { PropertyTypeGic,     "arm,cortex-a15-gic"  },
> +  { PropertyTypeRtc,     "arm,pl031"           },
> +  { PropertyTypeVirtio,  "virtio,mmio"         },
> +  { PropertyTypeUart,    "arm,pl011"           },
> +  { PropertyTypeTimer,   "arm,armv7-timer"     },
> +  { PropertyTypeTimer,   "arm,armv8-timer"     },
> +  { PropertyTypeUnknown, ""                    }
> +};
> +
> +typedef struct {
> +  UINT32  Type;
> +  UINT32  Number;
> +  UINT32  Flags;
> +} INTERRUPT_PROP;
> +
> +STATIC
> +PROPERTY_TYPE
> +GetTypeFromNode (
> +  IN CONST CHAR8 *NodeType,
> +  IN UINTN       Size
> +  )
> +{
> +  CONST CHAR8 *Compatible;
> +
> +  //
> +  // A 'compatible' node may contain a sequence of NULL terminated
> +  // compatible strings so check each one
> +  //
> +  for (Compatible = NodeType; Compatible < NodeType + Size && *Compatible;
> +       Compatible += 1 + AsciiStrLen (Compatible)) {
> +
> +    CONST PROPERTY *CompProp;
> +
> +    for (CompProp = CompatibleProperties; CompProp->Compatible[0]; CompProp++) {
> +      if (AsciiStrCmp (CompProp->Compatible, Compatible) == 0) {
> +        return CompProp->Type;
> +      }
> +    }
> +  }
> +  return PropertyTypeUnknown;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +InitializeVirtFdtDxe (
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_SYSTEM_TABLE     *SystemTable
> +  )
> +{
> +  VOID         *DeviceTreeBase;
> +  INT32        Node, Prev;
> +  EFI_STATUS   Status;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  if (fdt_check_header (DeviceTreeBase) != 0) {
> +    DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n",
> +           __FUNCTION__, DeviceTreeBase));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
> +
> +  //
> +  // Now enumerate the nodes and install peripherals that we are interested in,
> +  // i.e., GIC, RTC and virtio MMIO nodes
> +  //
> +  for (Prev = 0;; Prev = Node) {
> +    CONST CHAR8                    *Type;
> +    INT32                          Len;
> +    PROPERTY_TYPE                  PropType;
> +    CONST VOID                     *RegProp;
> +    VIRTIO_TRANSPORT_DEVICE_PATH   *DevicePath;
> +    EFI_HANDLE                     Handle;
> +    UINT64                         RegBase;
> +    UINT64                         DistBase, CpuBase;
> +    CONST INTERRUPT_PROP           *InterruptProp;
> +    INT32                          SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
> +
> +    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> +    if (Node < 0) {
> +      break;
> +    }
> +
> +    Type = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
> +    if (Type == NULL) {
> +      continue;
> +    }
> +
> +    PropType = GetTypeFromNode (Type, Len);
> +    if (PropType == PropertyTypeUnknown) {
> +      continue;
> +    }
> +
> +    //
> +    // Get the 'reg' property of this node. For now, we will assume
> +    // 8 byte quantities for base and size, respectively.
> +    // TODO use #cells root properties instead
> +    //
> +    RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +    ASSERT (RegProp != NULL || PropType == PropertyTypeTimer);
> +
> +    switch (PropType) {
> +
> +    case PropertyTypeVirtio:
> +      ASSERT (Len == 16);
> +      //
> +      // Create a unique device path for this transport on the fly
> +      //
> +      RegBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +      DevicePath = (VIRTIO_TRANSPORT_DEVICE_PATH *)CreateDeviceNode (
> +                                    HARDWARE_DEVICE_PATH,
> +                                    HW_VENDOR_DP,
> +                                    sizeof (VIRTIO_TRANSPORT_DEVICE_PATH));

Please check if DevicePath is NULL here. I've missed all this time that
CreateDeviceNode() allocates memory dynamically. If DevicePath is NULL,
just log an error ("out of memory"), and break out of the switch.

> +
> +      CopyMem (&DevicePath->Vendor.Guid, &gEfiCallerIdGuid, sizeof (EFI_GUID));
> +      DevicePath->PhysBase = RegBase;
> +      SetDevicePathNodeLength (&DevicePath->Vendor,
> +                              sizeof (*DevicePath) - sizeof (DevicePath->End));
> +      SetDevicePathEndNode (&DevicePath->End);
> +
> +      Handle = NULL;
> +      Status = gBS->InstallProtocolInterface (&Handle,
> +                     &gEfiDevicePathProtocolGuid, EFI_NATIVE_INTERFACE,
> +                     DevicePath);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "%a: Failed to install the EFI_DEVICE_PATH "
> +               "protocol on a new handle (Status == %r)\n",
> +               __FUNCTION__, Status));
> +        break;
> +      }

Please 'FreePool (DevicePath);' here, in case of error. We need to undo
the memory allocation of CreateDeviceNode(). Apologies for noticing it
just now :( Please see more lower.

> +
> +      Status = VirtioMmioInstallDevice (RegBase, Handle);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "%a: Failed to install VirtIO transport @ 0x%Lx "
> +               "on handle %p\n (Status == %r)",
> +               __FUNCTION__, RegBase, Handle, Status));
> +
> +        Status = gBS->UninstallProtocolInterface (Handle,
> +                                                  &gEfiDevicePathProtocolGuid,
> +                                                  EFI_NATIVE_INTERFACE);
> +        ASSERT_EFI_ERROR (Status);
> +      }
> +      break;

A number of slight issues here.

First, you added "(Status == %r)" to the error message (thanks for
that), but you inserted it *after* the newline character ("\n"). Just a
typo.

Second, the UninstallProtocolInterface() boot service is used
incorrectly; the 3rd parameter needs to be the *original* structure
address that you associated with the protocol GUID. In this case, the
3rd parameter must be the "DevicePath" variable.

(You didn't get an error during compilation because EFI_NATIVE_INTERFACE
happens to be an integer constant expression with value 0, hence it
doubles as a NULL pointer constant.)

Third, once you uninstall the device path protocol interface, the
storage pointed-to by "DevicePath" must be released too. You allocated
that storage dynamically with CreateDeviceNode() -- of memory type
EfiBootServicesData --, so now you must release it with FreePool().

Fourth, you should either ignore the retval of
gBS->UninstallProtocolInterface(), or call that function directly in the
argument list of ASSERT_EFI_ERROR(). The way you've written it clobbers
Status.

Hm, wait a second! Scratch issue #4; that's not an issue actually.
Instead, please see another request lower down, concerning the retval of
this function.

Anyway, for handling the error of VirtioMmioInstallDevice(), I propose
the following, addressing #1 to #3 above:

      if (EFI_ERROR (Status)) {
        DEBUG ((EFI_D_ERROR, "%a: Failed to install VirtIO transport "
          "@ 0x%Lx on handle %p (Status == %r)\n", __FUNCTION__,
          RegBase, Handle, Status));

        Status = gBS->UninstallProtocolInterface (Handle,
                        &gEfiDevicePathProtocolGuid, DevicePath);
        ASSERT_EFI_ERROR (Status);
        FreePool (DevicePath);
      }

> +
> +    case PropertyTypeGic:
> +      ASSERT (Len == 32);
> +
> +      DistBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +      CpuBase = fdt64_to_cpu (((UINT64 *)RegProp)[2]);
> +      ASSERT (DistBase < mUint32Max);
> +      ASSERT (CpuBase < mUint32Max);
> +
> +      PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
> +      PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase);
> +
> +      DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
> +      break;
> +
> +    case PropertyTypeRtc:
> +      ASSERT (Len == 16);
> +
> +      RegBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> +      ASSERT (RegBase < mUint32Max);
> +
> +      PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
> +
> +      DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
> +      break;
> +
> +    case PropertyTypeTimer:
> +
> +      //
> +      // - interrupts : Interrupt list for secure, non-secure, virtual and
> +      //  hypervisor timers, in that order.
> +      //
> +      InterruptProp = fdt_getprop (DeviceTreeBase, Node, "interrupts", &Len);
> +      ASSERT (Len == 48);
> +
> +      SecIntrNum = fdt32_to_cpu (InterruptProp[0].Number)
> +                   + (InterruptProp[0].Type ? 16 : 0);
> +      IntrNum = fdt32_to_cpu (InterruptProp[1].Number)
> +                + (InterruptProp[1].Type ? 16 : 0);
> +      VirtIntrNum = fdt32_to_cpu (InterruptProp[2].Number)
> +                    + (InterruptProp[2].Type ? 16 : 0);
> +      HypIntrNum = fdt32_to_cpu (InterruptProp[3].Number)
> +                   + (InterruptProp[3].Type ? 16 : 0);
> +
> +      DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
> +        SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
> +
> +      PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
> +      PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
> +      PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
> +      PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
> +      break;
> +
> +    default:
> +      break;
> +    }
> +  }
> +  return Status;
> +}

Please return constant EFI_SUCCESS here.

... Apologies about the churn. With the changes above, you can add my
R-b, when posting v6.

Thanks!
Laszlo

> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> new file mode 100644
> index 000000000000..4c16d0032704
> --- /dev/null
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> @@ -0,0 +1,59 @@
> +## @file
> +#  Device tree enumeration DXE driver for ARM Virtual Machines
> +#
> +#  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
> +#  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                      = VirtFdtDxe
> +  FILE_GUID                      = 837DCA9E-E874-4D82-B29A-23FE0E23D1E2
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +
> +  ENTRY_POINT                    = InitializeVirtFdtDxe
> +
> +[Sources]
> +  VirtFdtDxe.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  PcdLib
> +  UefiDriverEntryPoint
> +  DxeServicesLib
> +  FdtLib
> +  VirtioMmioDeviceLib
> +
> +[Guids]
> +  gFdtTableGuid
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress
> +  gArmTokenSpaceGuid.PcdGicDistributorBase
> +  gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
> +  gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum
> +  gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
> +  gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
> +  gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum
> +  gArmPlatformTokenSpaceGuid.PcdPL031RtcBase
> +
> +[Protocols]
> +  gEfiDevicePathProtocolGuid
> +
> +[Depex]
> +  TRUE
> 


------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/