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 <[email protected]>
> ---
> .../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/
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
