On Wed, 10 Dec 2014 13:53:13 +0100 Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 10 December 2014 at 13:48, Laszlo Ersek lersek@redhat.com wrote:
On 12/10/14 13:25, Peter Maydell wrote:
On 10 December 2014 at 11:40, Laszlo Ersek lersek@redhat.com wrote:
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.
Hmm. That seems like a bug in QEMU to me -- we should probably be trimming down the dtb we create ourselves. In any case I wouldn't recommend making an guest assumptions about the presence or absence of that padding or whether it might end up more than 64K in future.
Ard, if you want to prepare for the case that the current padding disappears and want to add some reserve to the DTB, I think it would be best done in PEI, where you copy from the initial place to the dynamically allocated place anyway.
(ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c, PlatformPeim().)
Because, elsewhere, you could only do it in VirtFdtDxe, which would introduce an extra allocation there. Let's avoid that. Also, you can't FreePages() in DXE what you have AllocatePages()'d in PEI; the MemoryAllocationLib instances implementing those interfaces in PEI vs. DXE are not compatible. Which means that beyond having to do the extra allocation in VirtFdtDxe, you'd also have to leak the first copy (made in PEI).
So I propose
- to use the new HOB to carry only the address of the copy of the DTB
that's made in PEI
- the current AllocatePages() in PEI should account for a larger
(expanded) size DTB, and after copying the initial DTB over there, the copy should be at once extended in-place to the new size (I think this is doable in libfdt). This is arguably a small separate patch though.
Yes, I was just implementing that, along the lines of:
// // Add some padding to FdtSize, so that we can add properties later if needed. // FdtSize = fdt_totalsize (Base) + PADDING; *FdtHobData = (UINT64)AllocatePages (EFI_SIZE_TO_PAGES (FdtSize)); ASSERT (*FdtHobData != 0); fdt_open_into (Base, (VOID *)*FdtHobData, EFI_SIZE_TO_PAGES (FdtSize) * EFI_PAGE_SIZE);
Looks sensible from my p.o.v. As I guess you've realised, the fdt_open_into() function does both a copy and expand. It's basically designed for exactly this use case.