Re: [Xen-devel] [PATCH v1 05/21] ArmVirtualizationPkg: use a HOB to store device tree blob

[Laszlo Ersek <lersek@xxxxxxxxxx>]

I reviewed the discussion under

http://lists.linaro.org/pipermail/linaro-uefi/2014-December/000601.html

and I can see that you addressed all points there. I have some new comments:

On 01/23/15 16:02, Ard Biesheuvel wrote:
> Instead of using a dynamic PCD, store the device tree address in a HOB
> so that we can also run under a configuration that does not support
> dynamic PCDs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  .../ArmVirtualizationPkg/ArmVirtualizationPkg.dec  |  3 +--
>  .../ArmVirtualizationPkg/ArmVirtualizationQemu.dsc |  3 ---
>  .../ArmVirtualizationPkg/Include/Guid/FdtHob.h     | 26 
> ++++++++++++++++++++++
>  .../ArmVirtualizationPlatformLib.inf               |  1 +
>  .../Library/PlatformPeiLib/PlatformPeiLib.c        | 12 ++++++----
>  .../Library/PlatformPeiLib/PlatformPeiLib.inf      |  3 ---
>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c   | 10 +++++++--
>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf |  3 ++-
>  8 files changed, 46 insertions(+), 15 deletions(-)
>  create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/Include/Guid/FdtHob.h
> 
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec 
> b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> index 99411548aff6..cc7d31d62d6c 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> @@ -33,6 +33,7 @@
>  [Guids.common]
>    gArmVirtualizationTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 
> 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>    gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 
> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
> +  gFdtHobGuid                      = { 0x16958446, 0x19B7, 0x480B, { 0xB0, 
> 0x47, 0x74, 0x85, 0xAD, 0x3F, 0x71, 0x6D } }
>  
>  [PcdsFixedAtBuild]
>    #
> @@ -44,8 +45,6 @@
>    
> gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x0|UINT64|0x00000001
>  
>  [PcdsDynamic, PcdsFixedAtBuild]
> -  
> gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress|0x0|UINT64|0x00000002
> -
>    #
>    # ARM PSCI function invocations can be done either through hypervisor
>    # calls (HVC) or secure monitor calls (SMC).
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc 
> b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc
> index dff4e2507058..4f8eb632143c 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc
> @@ -160,9 +160,6 @@
>    # System Memory Size -- 1 MB initially, actual size will be fetched from DT
>    gArmTokenSpaceGuid.PcdSystemMemorySize|0x00100000
>  
> -  # location of the device tree blob passed by QEMU
> -  gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress|0x0
> -
>    gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum|0x0
>    gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|0x0
>    gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|0x0
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Include/Guid/FdtHob.h 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Include/Guid/FdtHob.h
> new file mode 100644
> index 000000000000..287729e0c350
> --- /dev/null
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Include/Guid/FdtHob.h
> @@ -0,0 +1,26 @@
> +/** @file
> +  GUID for the HOB that contains the copy of the flattened device tree blob
> +
> +  Copyright (C) 2014, Linaro Ltd.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License that 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.
> +
> +**/
> +
> +#ifndef __FDT_HOB_H__
> +#define __FDT_HOB_H__
> +
> +#define FDT_HOB_GUID { \
> +          0x16958446, 0x19B7, 0x480B, \
> +          { 0xB0, 0x47, 0x74, 0x85, 0xAD, 0x3F, 0x71, 0x6D } \
> +        }
> +
> +extern EFI_GUID gFdtHobGuid;
> +
> +#endif
> diff --git 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf
>  
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf
> index d1572882af1b..cb048232c0c0 100644
> --- 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf
> +++ 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf
> @@ -65,3 +65,4 @@
>  
>  [Guids]
>    gEarlyPL011BaseAddressGuid
> +  gFdtHobGuid

I think that this change should not occur to this file (you're not using
the new GUID in this module), but to
"ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.inf"
(because that's where you add the new code using the GUID as well).

Similarly, the previous patch (v1 04/21) should have moved over the
other GUID too (gEarlyPL011BaseAddressGuid), meaning that v1 05/21
should ultimately eliminate the [Guid] section from
ArmVirtualizationPlatformLib.inf.

> diff --git 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> index 58bc2b828dcd..f2404f89d152 100644
> --- 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> +++ 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> @@ -22,6 +22,7 @@
>  #include <libfdt.h>
>  
>  #include <Guid/EarlyPL011BaseAddress.h>
> +#include <Guid/FdtHob.h>
>  
>  EFI_STATUS
>  EFIAPI
> @@ -32,6 +33,7 @@ PlatformPeim (
>    VOID               *Base;
>    VOID               *NewBase;
>    UINTN              FdtSize;
> +  UINTN              *FdtHobData;

Okay, so the new HOB in question will carry an address from the PEI
phase to the DXE phase. In such cases you must be extra careful and
avoid (VOID *) and UINTN, because their sizes are word-size dependent,
and PEI and DXE can *theoretically* have different word size. (See eg.
OvmfPkgIa32X64.dsc.) I think it would be cleaner to stick with (UINT64*)
here.

>    UINT64             *UartHobData;
>    INT32              Node, Prev;
>    CONST CHAR8        *Compatible;
> @@ -41,15 +43,17 @@ PlatformPeim (
>    UINT64             UartBase;
>  
>  
> -  Base = (VOID*)(UINTN)FixedPcdGet64 (PcdDeviceTreeInitialBaseAddress);
> -  ASSERT (fdt_check_header (Base) == 0);
> +  Base = (VOID*)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (Base != NULL && fdt_check_header (Base) == 0);

Please always break up conjunctions in ASSERT() predicates to separate
ASSERT() statements; that way if either fails the message gives more
precise information.

>  
>    FdtSize = fdt_totalsize (Base);
>    NewBase = AllocatePages (EFI_SIZE_TO_PAGES (FdtSize));
>    ASSERT (NewBase != NULL);
> -
>    CopyMem (NewBase, Base, FdtSize);
> -  PcdSet64 (PcdDeviceTreeBaseAddress, (UINT64)(UINTN)NewBase);
> +
> +  FdtHobData = BuildGuidHob (&gFdtHobGuid, sizeof *FdtHobData);
> +  ASSERT (FdtHobData != NULL);
> +  *FdtHobData = (UINTN)NewBase;
>  
>    UartHobData = BuildGuidHob (&gEarlyPL011BaseAddressGuid, sizeof 
> *UartHobData);
>    ASSERT (UartHobData != NULL);

Okay, you'll do the padding in a separate patch (the next one), good.

> diff --git 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
>  
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
> index e544b528d261..12b24db63313 100644
> --- 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
> +++ 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
> @@ -41,8 +41,5 @@
>    gArmTokenSpaceGuid.PcdFvSize
>    gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
>  
> -[Pcd]
> -  gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress
> -
>  [Depex]
>    gEfiPeiMemoryDiscoveredPpiGuid
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c 
> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> index 8953f78f5fe4..96aeec61ee7f 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -24,10 +24,12 @@
>  #include <Library/DevicePathLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/DxeServicesLib.h>
> +#include <Library/HobLib.h>
>  #include <libfdt.h>
>  
>  #include <Guid/Fdt.h>
>  #include <Guid/VirtioMmioTransport.h>
> +#include <Guid/FdtHob.h>
>  
>  #pragma pack (1)
>  typedef struct {
> @@ -105,6 +107,7 @@ InitializeVirtFdtDxe (
>    IN EFI_SYSTEM_TABLE     *SystemTable
>    )
>  {
> +  VOID                           *Hob;
>    VOID                           *DeviceTreeBase;
>    INT32                          Node, Prev;
>    INT32                          RtcNode;
> @@ -125,8 +128,11 @@ InitializeVirtFdtDxe (
>    UINT64                         FwCfgDataAddress;
>    UINT64                         FwCfgDataSize;
>  
> -  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
> -  ASSERT (DeviceTreeBase != NULL);
> +  Hob = GetFirstGuidHob(&gFdtHobGuid);
> +  if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof DeviceTreeBase) {
> +    return EFI_NOT_FOUND;
> +  }

Again, in theory, sizeof(VOID*) could be different here in DXE from the
size in PEI.

> +  DeviceTreeBase = (VOID *)*(UINTN *)GET_GUID_HOB_DATA (Hob);

So this should be

  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));
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf 
> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> index 514ce2fdf658..1392c7c3fa45 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> @@ -40,13 +40,14 @@
>    DxeServicesLib
>    FdtLib
>    VirtioMmioDeviceLib
> +  HobLib
>  
>  [Guids]
>    gFdtTableGuid
>    gVirtioMmioTransportGuid
> +  gFdtHobGuid
>  
>  [Pcd]
> -  gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress
>    gArmVirtualizationTokenSpaceGuid.PcdArmPsciMethod
>    gArmVirtualizationTokenSpaceGuid.PcdFwCfgSelectorAddress
>    gArmVirtualizationTokenSpaceGuid.PcdFwCfgDataAddress
> 

Looks okay otherwise.

Thanks,
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel