Re: [Linaro-uefi] [RFC PATCH 2/8] ArmVirtualizationPkg: use a HOB to store device tree blob

comments below

On 12/08/14 19:22, Ard Biesheuvel wrote:

Instead of using AllocatePages() and a dynamic PCD, store the device tree data 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@linaro.org

---

.../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 | 2 +- 8 files changed, 43 insertions(+), 17 deletions(-) create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/Include/Guid/FdtHob.h

suggestion (that I should keep in mind myself as well): when formatting edk2 patchsets, please pass --stat=150 or something similar to git-format-patch. The project uses unreasonably long filenames and deep directory nesting, so without --stat=150 the diffstat is not really useful.

diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec index b581add024aa..55a9b8ce7562 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).

If I'm counting right, all 6 uses of PcdDeviceTreeBaseAddress are removed, good.

diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc index c7066b091cb8..395bf217e149 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc @@ -154,9 +154,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..79b06e935700 --- /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_H_GUID { \

•      0x16958446, 0x19B7, 0x480B, \
  

•      { 0xB0, 0x47, 0x74, 0x85, 0xAD, 0x3F, 0x71, 0x6D } \
  

•    }
  

+extern EFI_GUID gFdtHobGuid;

+#endif

Looks good, except the structure initializer's name should not contain the "_H" substring. FDT_HOB_H_GUID --> FDT_HOB_GUID

diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf index 57bebff13ef8..679192bb0485 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/ArmVirtualizationPlatformLib.inf @@ -65,3 +65,4 @@ [Guids] gEarlyPL011BaseAddressGuid

• gFdtHobGuid

diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c index 58bc2b828dcd..d180a01415b0 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 @@ -30,8 +31,8 @@ PlatformPeim ( ) { VOID *Base;

• VOID *NewBase; UINTN FdtSize;

• UINT64 *FdtHobData;

I would prefer if you changed the type of this variable to (VOID *).

UINT64 *UartHobData; INT32 Node, Prev; CONST CHAR8 *Compatible; @@ -44,12 +45,11 @@ PlatformPeim ( Base = (VOID*)(UINTN)FixedPcdGet64 (PcdDeviceTreeInitialBaseAddress); ASSERT (fdt_check_header (Base) == 0);

• fdt_pack (Base);

This is okay, but please consider adding a *loud* comment here.

The dtb that QEMU exports at the moment is 64KB in size (FDT_MAX_SIZE in "device_tree.c"). See

qemu-system-aarch64 -M virt -machine dumpdtb=virt.dtb

I'm sure you've noticed that; I'll assume that your first new BuildGuidHob() call failed. That's because the BuildGuidHob() function in "MdePkg/Library/PeiHobLib/HobLib.c" limits the size to

(0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)))

Which is why you probably added fdt_pack().

The 64KB DTB that qemu currently exports for the virt machtype carries actually less than 5KB of payload, the rest is padding (for in-place updates I guess), which fdt_pack() trims in-place.

First question: can you guarantee that fdt_pack() trims the DTB in-place? CC'ing David Gibson.

This is the initial DTB we're talking about, the one that QEMU places at the base of DRAM in advance (and at reset).

Assuming the above packing (trimming) occurs in-place, we do have a nice margin here (58+ KB) for *normal* FDT nodes (ie. nodes that describe devices), but this change definitely shuts the door on any future ideas to place anything "blob-like" in the DTB. That's okay, and it does match our qemu plans, but you should still explain it in a loud comment at the top. (Also, CC'ing Peter.)

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, FdtSize);
• ASSERT (FdtHobData != NULL);
• CopyMem (FdtHobData, Base, FdtSize);

UartHobData = BuildGuidHob (&gEarlyPL011BaseAddressGuid, sizeof *UartHobData); ASSERT (UartHobData != NULL); 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 d002e668aa48..14879387864c 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c @@ -24,9 +24,11 @@ #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/FdtHob.h> #pragma pack (1) typedef struct { @@ -100,6 +102,7 @@ InitializeVirtFdtDxe ( IN EFI_SYSTEM_TABLE *SystemTable ) {

• VOID *Hob; VOID *DeviceTreeBase; INT32 Node, Prev; INT32 RtcNode;

@@ -116,8 +119,11 @@ InitializeVirtFdtDxe ( INT32 SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum; CONST CHAR8 *PsciMethod;

• DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
• ASSERT (DeviceTreeBase != NULL);

• Hob = GetFirstGuidHob(&gFdtHobGuid);
• if (Hob == NULL) {
• return EFI_NOT_FOUND;
• }
• DeviceTreeBase = GET_GUID_HOB_DATA (Hob);

Hm, I disagree.

You are going to do at least two things to the memory pointed-to by DeviceTreeBase: - link it into the UEFI system config table, with gBS->InstallConfigurationTable(), - you set the status of the RTC node to disabled, with fdt_setprop_string()

I'm probably just too paranoid, but I don't feel safe linking the guts of a HOB directly into the UEFI system table. Previously what we linked was an independently allocated memory area.

In addition, what you're modifying now with fdt_setprop_string is (a) the guts of a HOB, (b) that has even been packed.

Please *at least* do something like

DeviceTreeBase = AllocateCopyPool ( fdt_totalsize (GET_GUID_HOB_DATA (Hob)), GET_GUID_HOB_DATA (Hob) );

(and check the retval / return EFI_OUT_OF_RESOURCES).

But, preferably, allocate something bigger than the packed size (64KB could be a good choice), and "inflate" the packed FDT from the HOB contents into the newly allocated, bigger block, giving it again some "reserve" room, so that the later fdt_setprop_string() has a good chance to succeed.

(I might be wrong about how libfdt works.)

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 1c9dd20580c4..2ea6b6f8156d 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf @@ -43,9 +43,9 @@ [Guids] gFdtTableGuid

• gFdtHobGuid

[Pcd]

• gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress gArmVirtualizationTokenSpaceGuid.PcdArmPsciMethod gArmTokenSpaceGuid.PcdGicDistributorBase gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase

Looks okay other than that.

Thanks, Laszlo