Re: [PATCH 1/3] EmbeddedPkg/AndroidFastboot: drop dependency on the LinuxLoader

[Ryan Harkin <ryan.harkin@linaro.org>]

On 21 November 2016 at 12:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When booting the kernel via Fastboot, invoke the kernel image directly
> rather than passing it to the LinuxLoader app. This requires the kernel
> image to be built with UEFI stub support.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Build tested only.
>
>  EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c | 70 ++++++--------------
>  1 file changed, 22 insertions(+), 48 deletions(-)
>
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> index acedd3e0e3cd..46a7ceb3a41c 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> @@ -21,11 +21,6 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>
> -#define LINUX_LOADER_COMMAND_LINE       L"%s -f %s -c %s"
> -
> -// This GUID is defined in the INGF file of ArmPkg/Application/LinuxLoader
> -CONST EFI_GUID mLinuxLoaderAppGuid = { 0x701f54f2, 0x0d70, 0x4b89, { 0xbc, 0x0a, 0xd9, 0xca, 0x25, 0x37, 0x90, 0x59 }};
> -
>  // Device Path representing an image in memory
>  #pragma pack(1)
>  typedef struct {
> @@ -68,11 +63,7 @@ BootAndroidBootImg (
>    VOID                               *Ramdisk;
>    UINTN                               RamdiskSize;
>    MEMORY_DEVICE_PATH                  KernelDevicePath;
> -  MEMORY_DEVICE_PATH*                 RamdiskDevicePath;
> -  CHAR16*                             KernelDevicePathTxt;
> -  CHAR16*                             RamdiskDevicePathTxt;
> -  EFI_DEVICE_PATH*                    LinuxLoaderDevicePath;
> -  CHAR16*                             LoadOptions;
> +  CHAR16                              *LoadOptions, *NewLoadOptions;
>
>    Status = ParseAndroidBootImg (
>              Buffer,
> @@ -93,55 +84,38 @@ BootAndroidBootImg (
>    KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
>    KernelDevicePath.Node1.EndingAddress   = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel + KernelSize;
>
> -  RamdiskDevicePath = NULL;
> -  if (RamdiskSize != 0) {
> -    RamdiskDevicePath = (MEMORY_DEVICE_PATH*)DuplicateDevicePath ((EFI_DEVICE_PATH_PROTOCOL*) &MemoryDevicePathTemplate);
> -
> -    RamdiskDevicePath->Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Ramdisk;
> -    RamdiskDevicePath->Node1.EndingAddress   = ((EFI_PHYSICAL_ADDRESS)(UINTN) Ramdisk) + RamdiskSize;
> -  }
> -
> -  //
> -  // Boot Linux using the Legacy Linux Loader
> -  //
> -
> -  Status = LocateEfiApplicationInFvByGuid (&mLinuxLoaderAppGuid, &LinuxLoaderDevicePath);
> -  if (EFI_ERROR (Status)) {
> -    Print (L"Couldn't Boot Linux: %d\n", Status);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  KernelDevicePathTxt = ConvertDevicePathToText ((EFI_DEVICE_PATH_PROTOCOL *) &KernelDevicePath, FALSE, FALSE);
> -  if (KernelDevicePathTxt == NULL) {
> -    return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  RamdiskDevicePathTxt = ConvertDevicePathToText ((EFI_DEVICE_PATH_PROTOCOL *) RamdiskDevicePath, FALSE, FALSE);
> -  if (RamdiskDevicePathTxt == NULL) {
> +  // Initialize Linux command line
> +  LoadOptions = CatSPrint (NULL, L"%a", KernelArgs);
> +  if (LoadOptions == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>
> -  // Initialize Legacy Linux loader command line
> -  LoadOptions = CatSPrint (NULL, LINUX_LOADER_COMMAND_LINE, KernelDevicePathTxt, RamdiskDevicePathTxt, KernelArgs);
> -  if (LoadOptions == NULL) {
> -    return EFI_OUT_OF_RESOURCES;
> +  if (RamdiskSize != 0) {
> +    NewLoadOptions = CatSPrint (LoadOptions, L" initrd=0x%x,0x%x",
> +                       (UINTN)Ramdisk, RamdiskSize);
> +    FreePool (LoadOptions);
> +    if (NewLoadOptions == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    LoadOptions = NewLoadOptions;
>    }
>
> -  Status = BdsStartEfiApplication (gImageHandle, LinuxLoaderDevicePath, StrSize (LoadOptions), LoadOptions);
> +  Status = BdsStartEfiApplication (gImageHandle,
> +             (EFI_DEVICE_PATH_PROTOCOL *) &KernelDevicePath,
> +             StrSize (LoadOptions),
> +             LoadOptions);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "Couldn't Boot Linux: %d\n", Status));
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (RamdiskDevicePath) {
> -    FreePool (RamdiskDevicePathTxt);
> -    FreePool (RamdiskDevicePath);
> +    Status = EFI_DEVICE_ERROR;
> +    goto FreeLoadOptions;
>    }
>
> -  FreePool (KernelDevicePathTxt);
> -
>    // If we got here we do a confused face because BootLinuxFdt returned,
>    // reporting success.
>    DEBUG ((EFI_D_ERROR, "WARNING: BdsBootLinuxFdt returned EFI_SUCCESS.\n"));

This comment has nothing to do with your change, but this DEBUG
message hasn't been true since commit
bd9a5182a59696870690b54aaa63632c80694000 removed the call to
BdsBootLinuxFdt :-/


>    return EFI_SUCCESS;
> +
> +FreeLoadOptions:
> +  FreePool (LoadOptions);
> +  return Status;
>  }
> --
> 2.7.4
>