Re: [Xen-devel] [PATCH v2 07/29] ArmVirtualizationPkg: use a HOB to store device tree blob

["Olivier Martin" <olivier.martin@xxxxxxx>]

I do not have a strong opinion on this patch.
It would be better to keep the dynamic PCD support in this patch. But I am
aware it is not possible with PrePi (I had the issue a couple of weeks ago).
Dynamic Pcds are actually supported when you use the PeiCore with PcdPeim.
But the PeiCore only make sense at the moment on platforms that have DRAM
initialized by the UEFI firmware.
I would like to extend the PI spec to also be able to use PeiCore in the
case where the DRAM is already initialized at the time of the UEFI firmware.
That would mean we could use the PcdPeim and Dynamic Pcd. But it will take
time before we have support for it.
And I do not want to gate the patch set for this reason.

I am ok to accept it if no one reject it.


Anyway, this patch breaks the ARM Toolchain build:

"armlink" --partial -o
/tianocore/Build/ArmVirtualizationQemu-ARM/DEBUG_RVCTLINUX/ARM/ArmPlatformPk
g/PrePeiCore/PrePeiCoreUniCore/OUTPUT/ArmPlatformPrePeiCore.lib --via
/tianocore/Build/ArmVirtualizationQemu-ARM/DEBUG_RVCTLINUX/ARM/ArmPlatformPk
g/PrePeiCore/PrePeiCoreUniCore/OUTPUT/object_files.lst
"armlink"  --ro-base 0 --no_scanlib --reloc --no_exceptions --datacompressor
off --strict --symbols --diag_style=ide --entry _ModuleEntryPoint --map
--list
/tianocore/Build/ArmVirtualizationQemu-ARM/DEBUG_RVCTLINUX/ARM/ArmPlatformPk
g/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.map -o
/tianocore/Build/ArmVirtualizationQemu-ARM/DEBUG_RVCTLINUX/ARM/ArmPlatformPk
g/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll  --via
/tianocore/Build/ArmVirtualizationQemu-ARM/DEBUG_RVCTLINUX/ARM/ArmPlatformPk
g/PrePeiCore/PrePeiCoreUniCore/OUTPUT/static_library_files.lst
armlink : error L6218:  Undefined symbol AllocatePages (referred from
ArmVirtualizationPlatformLib.lib).
armlink : Not enough information to list image symbols.
armlink : Finished: 1 information, 0 warning and 1 error messages.


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@xxxxxxxxxx]
> Sent: 26 January 2015 19:03
> To: edk2-devel@xxxxxxxxxxxxxxxxxxxxx; lersek@xxxxxxxxxx; Olivier
> Martin; roy.franz@xxxxxxxxxx; leif.lindholm@xxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> anthony.perard@xxxxxxxxxx; christoffer.dall@xxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxx; ilias.biris@xxxxxxxxxx
> Cc: Ard Biesheuvel
> Subject: [PATCH v2 07/29] ArmVirtualizationPkg: use a HOB to store
> device tree blob
> 
> 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>
> ---
>  ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> |  2 --
>  ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc
> |  3 ---
> 
> ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLi
> b/ArmVirtualizationPlatformLib.inf |  2 --
> 
> ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiL
> ib.c                               | 11 ++++++++---
> 
> ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiL
> ib.inf                             |  4 +---
>  ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> | 10 ++++++++--
>  ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
> |  3 ++-
>  EmbeddedPkg/EmbeddedPkg.dec
> |  2 ++
>  EmbeddedPkg/Include/Guid/FdtHob.h
> | 26 ++++++++++++++++++++++++++
>  9 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> index d83117fc6abe..868488906643 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> @@ -44,8 +44,6 @@
> 
> gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x0|UI
> NT64|0x00000001
> 
>  [PcdsDynamic, PcdsFixedAtBuild]
> -
> gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress|0x0|UINT64|0x
> 00000002
> -
>    #
>    # 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/Library/ArmVirtualizationPlatform
> Lib/ArmVirtualizationPlatformLib.inf
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatform
> Lib/ArmVirtualizationPlatformLib.inf
> index 43b3c6ca1bef..c57002f3e9da 100644
> ---
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatform
> Lib/ArmVirtualizationPlatformLib.inf
> +++
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatform
> Lib/ArmVirtualizationPlatformLib.inf
> @@ -33,8 +33,6 @@
>    ArmLib
>    PrintLib
>    FdtLib
> -  SerialPortLib
> -  HobLib
> 
>  [Sources.common]
>    Virt.c
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.c
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.c
> index 58bc2b828dcd..c500d5964b25 100644
> ---
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.c
> +++
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.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;
> +  UINT64             *FdtHobData;
>    UINT64             *UartHobData;
>    INT32              Node, Prev;
>    CONST CHAR8        *Compatible;
> @@ -41,15 +43,18 @@ PlatformPeim (
>    UINT64             UartBase;
> 
> 
> -  Base = (VOID*)(UINTN)FixedPcdGet64
> (PcdDeviceTreeInitialBaseAddress);
> +  Base = (VOID*)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (Base != NULL);
>    ASSERT (fdt_check_header (Base) == 0);
> 
>    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);
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.inf
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.inf
> index a376fbd1f345..96019e4009ff 100644
> ---
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.inf
> +++
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPe
> iLib.inf
> @@ -41,11 +41,9 @@
>    gArmTokenSpaceGuid.PcdFvSize
>    gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> 
> -[Pcd]
> -  gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress
> -
>  [Guids]
>    gEarlyPL011BaseAddressGuid
> +  gFdtHobGuid
> 
>  [Depex]
>    gEfiPeiMemoryDiscoveredPpiGuid
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> index 31164905d34e..34fac40fa803 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 (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));
> 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
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 600d0e54c4b3..2f261ece9212 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -52,6 +52,8 @@
>    ## FDT Configuration Table
>    # Include/Guid/Fdt.h
>    gFdtTableGuid = { 0xb1b621d5, 0xf19c, 0x41a5, { 0x83, 0x0b, 0xd9,
> 0x15, 0x2c, 0x69, 0xaa, 0xe0 } }
> +  # Include/Guid/FdtHob.h
> +  gFdtHobGuid   = { 0x16958446, 0x19B7, 0x480B, { 0xB0, 0x47, 0x74,
> 0x85, 0xAD, 0x3F, 0x71, 0x6D } }
> 
>  [Protocols.common]
>    gHardwareInterruptProtocolGuid =  { 0x2890B3EA, 0x053D, 0x1643, {
> 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
> diff --git a/EmbeddedPkg/Include/Guid/FdtHob.h
> b/EmbeddedPkg/Include/Guid/FdtHob.h
> new file mode 100644
> index 000000000000..287729e0c350
> --- /dev/null
> +++ b/EmbeddedPkg/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
> --
> 1.8.3.2
> 





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