Re: [Xen-devel] [PATCH v1 06/21] ArmVirtualizationPkg: add padding to FDT allocation

[Laszlo Ersek <lersek@xxxxxxxxxx>]

On 01/23/15 16:02, Ard Biesheuvel wrote:
> Our primary user QEMU/mach-virt presents us with a FDT blob padded
> to 64 KB with plenty of room to set additional properties. However,
> in the general case, we should only add properties after making sure
> there is enough room available.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  .../Library/PlatformPeiLib/PlatformPeiLib.c             | 17 
> +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> index f2404f89d152..540474608deb 100644
> --- 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> +++ 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> @@ -24,6 +24,15 @@
>  #include <Guid/EarlyPL011BaseAddress.h>
>  #include <Guid/FdtHob.h>
>  
> +//
> +// We may want to apply some changes to the device tree before passing it
> +// to the OS: for instance, if we find a PL031 RTC node and attach our
> +// runtime driver to it, we should disable it in the device tree by setting
> +// its status property to "disabled". Add some padding to make sure this is
> +// possible.
> +//
> +#define FDT_PADDING   256
> +
>  EFI_STATUS
>  EFIAPI
>  PlatformPeim (
> @@ -32,7 +41,7 @@ PlatformPeim (
>  {
>    VOID               *Base;
>    VOID               *NewBase;
> -  UINTN              FdtSize;
> +  UINTN              FdtPages;
>    UINTN              *FdtHobData;
>    UINT64             *UartHobData;
>    INT32              Node, Prev;
> @@ -46,10 +55,10 @@ PlatformPeim (
>    Base = (VOID*)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
>    ASSERT (Base != NULL && fdt_check_header (Base) == 0);
>  
> -  FdtSize = fdt_totalsize (Base);
> -  NewBase = AllocatePages (EFI_SIZE_TO_PAGES (FdtSize));
> +  FdtPages = EFI_SIZE_TO_PAGES (fdt_totalsize (Base) + FDT_PADDING);
> +  NewBase = AllocatePages (FdtPages);
>    ASSERT (NewBase != NULL);
> -  CopyMem (NewBase, Base, FdtSize);
> +  fdt_open_into (Base, NewBase, EFI_PAGES_TO_SIZE (FdtPages));
>  
>    FdtHobData = BuildGuidHob (&gFdtHobGuid, sizeof *FdtHobData);
>    ASSERT (FdtHobData != NULL);
> 

One nit: please keep the original FdtSize assignment (with type UINTN)
and use that inside the EFI_SIZE_TO_PAGES() expression, when calculating
FdtPages.

Two reasons:
- EFI_SIZE_TO_PAGES evaluates its argument twice. Although
  fdt_totalsize() is r/o and probably quick even, it's not very nice to
  call it twice.
- I tried to figure out the return type of fdt_totalsize(), but I gave
  up quickly after looking at the header file. However,
  EFI_SIZE_TO_PAGES() should strictly take UINTN, which FdtSize covers
  at once.

Sth like:

  FdtSize = fdt_totalsize (Base) + FDT_PADDING;
  FdtPages = EFI_SIZE_TO_PAGES (FdtSize);
  NewBase = AllocatePages (FdtPages);
  ...
  fdt_open_into (Base, NewBase, EFI_PAGES_TO_SIZE (FdtPages));

Thanks
Laszlo

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