On 30 July 2015 at 15:44, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
On 28 July 2015 at 19:10, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> BdsLib was using fixed PCDs where constants could easily perform that
> same function.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>
> ---
>  .../Application/LinuxLoader/AArch64/LinuxStarter.c |  8 +++---
>  ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c     |  2 +-
>  ArmPkg/Application/LinuxLoader/Arm/LinuxStarter.c  | 22 +++++++--------
>  ArmPkg/Application/LinuxLoader/LinuxLoader.h       | 32 ++++++++++++++++++++--
>  ArmPkg/Application/LinuxLoader/LinuxLoader.inf     |  8 ------
>  ArmPkg/Application/LinuxLoader/LinuxLoaderFdt.c    | 14 +++-------
>  ArmPkg/ArmPkg.dec                                  | 19 -------------
>  7 files changed, 49 insertions(+), 56 deletions(-)
>
> diff --git a/ArmPkg/Application/LinuxLoader/AArch64/LinuxStarter.c b/ArmPkg/Application/LinuxLoader/AArch64/LinuxStarter.c
> index e72537f..317e73e 100644
> --- a/ArmPkg/Application/LinuxLoader/AArch64/LinuxStarter.c
> +++ b/ArmPkg/Application/LinuxLoader/AArch64/LinuxStarter.c
> @@ -229,12 +229,12 @@ BootLinuxFdt (
>    //
>
>    // Try to put the kernel at the start of RAM so as to give it access to all memory.
> -  // If that fails fall back to try loading it within LINUX_KERNEL_MAX_OFFSET of memory start.
> +  // If that fails fall back to try loading it within LINUX_KERNEL_MAX_ADDR of memory start.
>    LinuxImage = SystemMemoryBase + 0x80000;
>    Status = BdsLoadImage (LinuxKernelDevicePath, AllocateAddress, &LinuxImage, &LinuxImageSize);
>    if (EFI_ERROR (Status)) {
>      // Try again but give the loader more freedom of where to put the image.
> -    LinuxImage = LINUX_KERNEL_MAX_OFFSET;
> +    LinuxImage = LINUX_KERNEL_MAX_ADDR;
>      Status = BdsLoadImage (LinuxKernelDevicePath, AllocateMaxAddress, &LinuxImage, &LinuxImageSize);
>      if (EFI_ERROR (Status)) {
>        Print (L"ERROR: Did not find Linux kernel (%r).\n", Status);
> @@ -248,7 +248,7 @@ BootLinuxFdt (
>    }
>
>    if (InitrdDevicePath) {
> -    InitrdImageBase = LINUX_FDT_MAX_OFFSET;
> +    InitrdImageBase = LINUX_INITRD_MAX_ADDR;
>      Status = BdsLoadImage (InitrdDevicePath, AllocateMaxAddress, &InitrdImageBase, &InitrdImageBaseSize);
>      if (Status == EFI_OUT_OF_RESOURCES) {
>        Status = BdsLoadImage (InitrdDevicePath, AllocateAnyPages, &InitrdImageBase, &InitrdImageBaseSize);
> @@ -287,7 +287,7 @@ BootLinuxFdt (
>      // FDT device path explicitly defined. The FDT is relocated later to a
>      // more appropriate location for the Linux kernel.
>      //
> -    FdtBlobBase = LINUX_KERNEL_MAX_OFFSET;
> +    FdtBlobBase = LINUX_KERNEL_MAX_ADDR;
>      Status = BdsLoadImage (FdtDevicePath, AllocateMaxAddress, &FdtBlobBase, &FdtBlobSize);
>      if (EFI_ERROR (Status)) {
>        Print (L"ERROR: Did not find Device Tree blob (%r).\n", Status);

I'd recommend dropping all hunks against this file. First of all, the
preferred offset is as low as possible as long as it is 2 MB aligned.
But the other logic in this file is severely flawed as well:
- TEXT_OFFSET is usually 0x80000 but we should really read the actual
value from the Image header
- the CopyMem () that occurs may move the Image outside of the allocated area
- we should allocate some slack above the kernel Image: the header has
an image_size field these days, but for older kernels, we should have
at least around half a meg for the page tables
- the spin table code puts the mailboxes outside of the allocation it
performs for the pen code

I am in two minds between proposing a fix for all of this and removing
the LinuxLoader altogether from the AARCH64 platforms

This change is simply replacing PCDs with constants.  It's not attempting to change the logic, which as you point out, is severely flawed.  So the problems you point out already exist.

To my mind, if I'm going to replace the PCDs, I have to do it everywhere.

I'd expect fixes to the logic - or removing the LinuxLoader from Aarch64 - to be implemented as a separate patch.  But unless that patch goes in before these, I think I need to keep the hunks against this file.

I'd be happy to see the LinuxLoader removed from Aarch64, except some people use it for Aarch64 TFTP booting because the EFI stub is unable to fetch an initrd or device tree via TFTP, but Olivier reckoned I could work around that issue in a better way.  But I haven't out worked if I can or not.

I know the device tree can be pre-loaded via TFTP, but I am not sure how to handle the initrd.



--
Ard.


> diff --git a/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c b/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
> index fd7ee9c..a6dbf7a 100644
> --- a/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
> +++ b/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
> @@ -129,7 +129,7 @@ PrepareAtagList (
>    EFI_PHYSICAL_ADDRESS        AtagStartAddress;
>    SYSTEM_MEMORY_RESOURCE      *Resource;
>
> -  AtagStartAddress = LINUX_ATAG_MAX_OFFSET;
> +  AtagStartAddress = LINUX_ATAG_MAX_ADDR;
>    Status = gBS->AllocatePages (AllocateMaxAddress, EfiBootServicesData, EFI_SIZE_TO_PAGES (ATAG_MAX_SIZE), &AtagStartAddress);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_WARN, "Warning: Failed to allocate Atag at 0x%lX (%r). The Atag will be allocated somewhere else in System Memory.\n", AtagStartAddress, Status));
> diff --git a/ArmPkg/Application/LinuxLoader/Arm/LinuxStarter.c b/ArmPkg/Application/LinuxLoader/Arm/LinuxStarter.c
> index ca6331a..11be291 100644
> --- a/ArmPkg/Application/LinuxLoader/Arm/LinuxStarter.c
> +++ b/ArmPkg/Application/LinuxLoader/Arm/LinuxStarter.c
> @@ -84,18 +84,18 @@ StartLinux (
>    // physical memory.
>    //Note: There is no requirement on the alignment
>    if (MachineType != ARM_FDT_MACHINE_TYPE) {
> -    if (((UINTN)KernelParamsAddress > LINUX_ATAG_MAX_OFFSET) && (KernelParamsSize < PcdGet32 (PcdArmLinuxAtagMaxOffset))) {
> -      KernelParamsAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)CopyMem (ALIGN32_BELOW (LINUX_ATAG_MAX_OFFSET - KernelParamsSize), (VOID*)(UINTN)KernelParamsAddress, KernelParamsSize);
> +    if (((UINTN)KernelParamsAddress > LINUX_ATAG_MAX_ADDR) && (KernelParamsSize < LINUX_ATAG_MAX_OFFSET)) {
> +      KernelParamsAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)CopyMem (ALIGN32_BELOW (LINUX_ATAG_MAX_ADDR - KernelParamsSize), (VOID*)(UINTN)KernelParamsAddress, KernelParamsSize);
>      }
>    } else {
> -    if (((UINTN)KernelParamsAddress > LINUX_FDT_MAX_OFFSET) && (KernelParamsSize < PcdGet32 (PcdArmLinuxFdtMaxOffset))) {
> -      KernelParamsAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)CopyMem (ALIGN32_BELOW (LINUX_FDT_MAX_OFFSET - KernelParamsSize), (VOID*)(UINTN)KernelParamsAddress, KernelParamsSize);
> +    if (((UINTN)KernelParamsAddress > LINUX_FDT_MAX_ADDR) && (KernelParamsSize < LINUX_FDT_MAX_OFFSET)) {
> +      KernelParamsAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)CopyMem (ALIGN32_BELOW (LINUX_FDT_MAX_ADDR - KernelParamsSize), (VOID*)(UINTN)KernelParamsAddress, KernelParamsSize);
>      }
>    }
>
> -  if ((UINTN)LinuxImage > LINUX_KERNEL_MAX_OFFSET) {
> +  if ((UINTN)LinuxImage > LINUX_KERNEL_MAX_ADDR) {
>      //Note: There is no requirement on the alignment
> -    LinuxKernel = (LINUX_KERNEL)CopyMem (ALIGN32_BELOW (LINUX_KERNEL_MAX_OFFSET - LinuxImageSize), (VOID*)(UINTN)LinuxImage, LinuxImageSize);
> +    LinuxKernel = (LINUX_KERNEL)CopyMem (ALIGN32_BELOW (LINUX_KERNEL_MAX_ADDR - LinuxImageSize), (VOID*)(UINTN)LinuxImage, LinuxImageSize);
>    } else {
>      LinuxKernel = (LINUX_KERNEL)(UINTN)LinuxImage;
>    }
> @@ -183,7 +183,7 @@ BootLinuxAtag (
>    PERF_START (NULL, "BDS", NULL, 0);
>
>    // Load the Linux kernel from a device path
> -  LinuxImage = LINUX_KERNEL_MAX_OFFSET;
> +  LinuxImage = LINUX_KERNEL_MAX_ADDR;
>    Status = BdsLoadImage (LinuxKernelDevicePath, AllocateMaxAddress, &LinuxImage, &LinuxImageSize);
>    if (EFI_ERROR (Status)) {
>      Print (L"ERROR: Did not find Linux kernel.\n");
> @@ -192,7 +192,7 @@ BootLinuxAtag (
>
>    if (InitrdDevicePath) {
>      // Load the initrd near to the Linux kernel
> -    InitrdImageBase = LINUX_FDT_MAX_OFFSET;
> +    InitrdImageBase = LINUX_FDT_MAX_ADDR;
>      Status = BdsLoadImage (InitrdDevicePath, AllocateMaxAddress, &InitrdImageBase, &InitrdImageBaseSize);
>      if (Status == EFI_OUT_OF_RESOURCES) {
>        Status = BdsLoadImage (InitrdDevicePath, AllocateAnyPages, &InitrdImageBase, &InitrdImageBaseSize);
> @@ -272,7 +272,7 @@ BootLinuxFdt (
>    PERF_START (NULL, "BDS", NULL, 0);
>
>    // Load the Linux kernel from a device path
> -  LinuxImage = LINUX_KERNEL_MAX_OFFSET;
> +  LinuxImage = LINUX_KERNEL_MAX_ADDR;
>    Status = BdsLoadImage (LinuxKernelDevicePath, AllocateMaxAddress, &LinuxImage, &LinuxImageSize);
>    if (EFI_ERROR (Status)) {
>      Print (L"ERROR: Did not find Linux kernel.\n");
> @@ -280,7 +280,7 @@ BootLinuxFdt (
>    }
>
>    if (InitrdDevicePath) {
> -    InitrdImageBase = LINUX_FDT_MAX_OFFSET;
> +    InitrdImageBase = LINUX_FDT_MAX_ADDR;
>      Status = BdsLoadImage (InitrdDevicePath, AllocateMaxAddress, &InitrdImageBase, &InitrdImageBaseSize);
>      if (Status == EFI_OUT_OF_RESOURCES) {
>        Status = BdsLoadImage (InitrdDevicePath, AllocateAnyPages, &InitrdImageBase, &InitrdImageBaseSize);
> @@ -319,7 +319,7 @@ BootLinuxFdt (
>      // FDT device path explicitly defined. The FDT is relocated later to a
>      // more appropriate location for the Linux kernel.
>      //
> -    FdtBlobBase = LINUX_KERNEL_MAX_OFFSET;
> +    FdtBlobBase = LINUX_KERNEL_MAX_ADDR;
>      Status = BdsLoadImage (FdtDevicePath, AllocateMaxAddress, &FdtBlobBase, &FdtBlobSize);
>      if (EFI_ERROR (Status)) {
>        Print (L"ERROR: Did not find Device Tree blob (%r).\n", Status);
> diff --git a/ArmPkg/Application/LinuxLoader/LinuxLoader.h b/ArmPkg/Application/LinuxLoader/LinuxLoader.h
> index 8a23d7f..2544147 100644
> --- a/ArmPkg/Application/LinuxLoader/LinuxLoader.h
> +++ b/ArmPkg/Application/LinuxLoader/LinuxLoader.h
> @@ -37,9 +37,35 @@
>  #define MAX_MSG_LEN 80
>
>  #define LINUX_UIMAGE_SIGNATURE    0x56190527
> -#define LINUX_KERNEL_MAX_OFFSET   (SystemMemoryBase + PcdGet32(PcdArmLinuxKernelMaxOffset))
> -#define LINUX_ATAG_MAX_OFFSET     (SystemMemoryBase + PcdGet32(PcdArmLinuxAtagMaxOffset))
> -#define LINUX_FDT_MAX_OFFSET      (SystemMemoryBase + PcdGet32(PcdArmLinuxFdtMaxOffset))
> +
> +#if defined (MDE_CPU_ARM)
> +  // On ARM, the kernel recommendations say that the kernel should live before
> +  // the 128MiB (0x08000000) boundary.
> +  #define LINUX_KERNEL_MAX_OFFSET 0x08000000
> +
> +  // ATAGS data should  be in the first 16KB
> +  #define LINUX_ATAG_MAX_OFFSET   0x00004000
> +
> +  // The device tree and initrd should live after the 128MiB boundary.
> +  #define LINUX_FDT_MAX_OFFSET    0x08400000
> +  #define LINUX_INITRD_MAX_OFFSET 0x08400000
> +  #define LINUX_FDT_ALIGNMENT     0x00000008
> +#elif defined (MDE_CPU_AARCH64)
> +  #define LINUX_KERNEL_MAX_OFFSET 0x08000000
> +
> +  // arm64 Kernels prior to v4.2 required the device tree to reside in the
> +  // first 512MB of memory
> +  #define LINUX_FDT_MAX_OFFSET    0x20000000
> +  #define LINUX_INITRD_MAX_OFFSET 0x20000000
> +
> +  // Recommendations are to align the FDT to 2MiB
> +  #define LINUX_FDT_ALIGNMENT     0x00200000
> +#endif
> +
> +#define LINUX_KERNEL_MAX_ADDR   (SystemMemoryBase + LINUX_KERNEL_MAX_OFFSET)
> +#define LINUX_ATAG_MAX_ADDR     (SystemMemoryBase + LINUX_ATAG_MAX_OFFSET)
> +#define LINUX_FDT_MAX_ADDR      (SystemMemoryBase + LINUX_FDT_MAX_OFFSET)
> +#define LINUX_INITRD_MAX_ADDR   (SystemMemoryBase + LINUX_INITRD_MAX_OFFSET)
>
>  #define ARM_FDT_MACHINE_TYPE      0xFFFFFFFF
>
> diff --git a/ArmPkg/Application/LinuxLoader/LinuxLoader.inf b/ArmPkg/Application/LinuxLoader/LinuxLoader.inf
> index 59ab99c..3b640b8 100644
> --- a/ArmPkg/Application/LinuxLoader/LinuxLoader.inf
> +++ b/ArmPkg/Application/LinuxLoader/LinuxLoader.inf
> @@ -78,14 +78,6 @@
>  [FeaturePcd]
>    gArmTokenSpaceGuid.PcdArmLinuxSpinTable
>
> -[FixedPcd]
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtAlignment
> -  gArmTokenSpaceGuid.PcdArmLinuxKernelMaxOffset
> -
> -[FixedPcd.ARM]
> -  gArmTokenSpaceGuid.PcdArmLinuxAtagMaxOffset
> -
>  [Pcd.AARCH64]
>    gArmTokenSpaceGuid.PcdGicDistributorBase
>    gArmTokenSpaceGuid.PcdGicSgiIntId
> diff --git a/ArmPkg/Application/LinuxLoader/LinuxLoaderFdt.c b/ArmPkg/Application/LinuxLoader/LinuxLoaderFdt.c
> index 0f53784..6f7bcdc 100644
> --- a/ArmPkg/Application/LinuxLoader/LinuxLoaderFdt.c
> +++ b/ArmPkg/Application/LinuxLoader/LinuxLoaderFdt.c
> @@ -78,20 +78,16 @@ RelocateFdt (
>  {
>    EFI_STATUS            Status;
>    INTN                  Error;
> -  UINT64                FdtAlignment;
>
>    *RelocatedFdtSize = OriginalFdtSize + FDT_ADDITIONAL_ENTRIES_SIZE;
>
>    // If FDT load address needs to be aligned, allocate more space.
> -  FdtAlignment = PcdGet32 (PcdArmLinuxFdtAlignment);
> -  if (FdtAlignment != 0) {
> -    *RelocatedFdtSize += FdtAlignment;
> -  }
> +  *RelocatedFdtSize += LINUX_FDT_ALIGNMENT;
>
>    // Try below a watermark address.
>    Status = EFI_NOT_FOUND;
> -  if (PcdGet32 (PcdArmLinuxFdtMaxOffset) != 0) {
> -    *RelocatedFdt = LINUX_FDT_MAX_OFFSET;
> +  if (LINUX_FDT_MAX_OFFSET != 0) {
> +    *RelocatedFdt = LINUX_FDT_MAX_ADDR;
>      Status = gBS->AllocatePages (AllocateMaxAddress, EfiBootServicesData,
>                      EFI_SIZE_TO_PAGES (*RelocatedFdtSize), RelocatedFdt);
>      if (EFI_ERROR (Status)) {
> @@ -112,9 +108,7 @@ RelocateFdt (
>    }
>
>    *RelocatedFdtAlloc = *RelocatedFdt;
> -  if (FdtAlignment != 0) {
> -    *RelocatedFdt = ALIGN (*RelocatedFdt, FdtAlignment);
> -  }
> +  *RelocatedFdt = ALIGN (*RelocatedFdt, LINUX_FDT_ALIGNMENT);
>
>    // Load the Original FDT tree into the new region
>    Error = fdt_open_into ((VOID*)(UINTN) OriginalFdt,
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index b2122b1..59916d2 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -115,8 +115,6 @@
>    #
>    # BdsLib
>    #
> -  # The compressed Linux kernel is expected to be under 128MB from the beginning of the System Memory
> -  gArmTokenSpaceGuid.PcdArmLinuxKernelMaxOffset|0x08000000|UINT32|0x0000001F
>    # Maximum file size for TFTP servers that do not support 'tsize' extension
>    gArmTokenSpaceGuid.PcdMaxTftpFileSize|0x01000000|UINT32|0x00000000
>
> @@ -152,16 +150,6 @@
>    # By default we do not do a transition to non-secure mode
>    gArmTokenSpaceGuid.PcdArmNonSecModeTransition|0x0|UINT32|0x0000003E
>
> -  # The Linux ATAGs are expected to be under 0x4000 (16KB) from the beginning of the System Memory
> -  gArmTokenSpaceGuid.PcdArmLinuxAtagMaxOffset|0x4000|UINT32|0x00000020
> -
> -  # If the fixed FDT address is not available, then it should be loaded below the kernel.
> -  # The recommendation from the Linux kernel is to have the FDT below 16KB.
> -  # (see the kernel doc: Documentation/arm/Booting)
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset|0x08400000|UINT32|0x00000023
> -  # The FDT blob must be loaded at a 64bit aligned address.
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtAlignment|0x8|UINT32|0x00000026
> -
>    # Non Secure Access Control Register
>    # - BIT15 : NSASEDIS - Disable Non-secure Advanced SIMD functionality
>    # - BIT14 : NSD32DIS - Disable Non-secure use of D16-D31
> @@ -200,13 +188,6 @@
>    # Other modes include using SP0 or switching to Aarch32, but these are
>    # not currently supported.
>    gArmTokenSpaceGuid.PcdArmNonSecModeTransition|0x3c9|UINT32|0x0000003E
> -  # If the fixed FDT address is not available, then it should be loaded above the kernel.
> -  # The recommendation from the AArch64 Linux kernel is to have the FDT below 512MB.
> -  # (see the kernel doc: Documentation/arm64/booting.txt)
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset|0x20000000|UINT32|0x00000023
> -  # The FDT blob must be loaded at a 2MB aligned address.
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtAlignment|0x00200000|UINT32|0x00000026
> -
>
>  #
>  # These PCDs are also defined as 'PcdsDynamic' or 'PcdsPatchableInModule' to be
> --
> 2.1.0
>