Re: [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd

["Jeff Brasen" <jbrasen@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 11176 bytes --]

Question below


Thanks,

Jeff

________________________________
From: Leif Lindholm <leif@nuviainc.com>
Sent: Tuesday, September 7, 2021 11:51 AM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; abner.chang@hpe.com <abner.chang@hpe.com>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>
Subject: Re: [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd

External email: Use caution opening links or attachments


Minor style comments below.

On Wed, Sep 01, 2021 at 20:28:27 +0000, Jeff Brasen wrote:
> Add support under a pcd feature for using the new interface to pass
> initrd to the linux kernel.
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
>  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   3 +
>  .../AndroidBootImgLib/AndroidBootImgLib.c     | 147 +++++++++++++++++-
>  3 files changed, 144 insertions(+), 7 deletions(-)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index c2068ce5a8ce..7638aaaadeb8 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -86,6 +86,7 @@ [Ppis]
>  [PcdsFeatureFlag.common]
>    gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob|FALSE|BOOLEAN|0x0000001b
>    gEmbeddedTokenSpaceGuid.PcdGdbSerial|FALSE|BOOLEAN|0x00000053
> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2|FALSE|BOOLEAN|0x0000005b
>
>  [PcdsFixedAtBuild.common]
>    gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0|UINT32|0x0000000b
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> index bfed4ba9dcd4..39d8abe72cd1 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> @@ -42,3 +42,6 @@ [Protocols]
>
>  [Guids]
>    gFdtTableGuid
> +
> +[FeaturePcd]
> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index 3c617649f735..635965690dc0 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -10,11 +10,15 @@
>  #include <libfdt.h>
>  #include <Library/AndroidBootImgLib.h>
>  #include <Library/PrintLib.h>
> +#include <Library/DevicePathLib.h>

Move that inserted line up one to maintain alphabetical sorting.

>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>
>  #include <Protocol/AndroidBootImg.h>
>  #include <Protocol/LoadedImage.h>
> +#include <Protocol/LoadFile2.h>
> +
> +#include <Guid/LinuxEfiInitrdMedia.h>
>
>  #include <libfdt.h>

Glad to see we're *really* including libfdt.h (also included above and
completely unrelated to this patch).

> @@ -25,7 +29,14 @@ typedef struct {
>    EFI_DEVICE_PATH_PROTOCOL                End;
>  } MEMORY_DEVICE_PATH;
>
> +typedef struct {
> +  VENDOR_DEVICE_PATH                      VenMediaNode;

VendorMediaNode

> +  EFI_DEVICE_PATH_PROTOCOL                EndNode;
> +} RAMDISK_DEVICE_PATH;
> +
>  STATIC ANDROID_BOOTIMG_PROTOCOL                 *mAndroidBootImg;
> +STATIC VOID                                     *mRamdiskData = NULL;
> +STATIC UINTN                                    mRamdiskSize = 0;
>
>  STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate =
>  {
> @@ -48,6 +59,99 @@ STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate =
>    } // End
>  };
>
> +STATIC CONST RAMDISK_DEVICE_PATH mRamdiskDevicePath =
> +{
> +  {
> +    {
> +      MEDIA_DEVICE_PATH,
> +      MEDIA_VENDOR_DP,
> +      { sizeof (VENDOR_DEVICE_PATH), 0 }
> +    },
> +    LINUX_EFI_INITRD_MEDIA_GUID
> +  },
> +  {
> +    END_DEVICE_PATH_TYPE,
> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> +  }
> +};
> +
> +/**
> +  Causes the driver to load a specified file.
> +
> +  @param  This       Protocol instance pointer.
> +  @param  FilePath   The device specific path of the file to load.
> +  @param  BootPolicy Should always be FALSE.
> +  @param  BufferSize On input the size of Buffer in bytes. On output with a return
> +                     code of EFI_SUCCESS, the amount of data transferred to
> +                     Buffer. On output with a return code of EFI_BUFFER_TOO_SMALL,
> +                     the size of Buffer required to retrieve the requested file.
> +  @param  Buffer     The memory buffer to transfer the file to. IF Buffer is NULL,
> +                     then no the size of the requested file is returned in
> +                     BufferSize.
> +
> +  @retval EFI_SUCCESS           The file was loaded.
> +  @retval EFI_UNSUPPORTED       BootPolicy is TRUE.
> +  @retval EFI_INVALID_PARAMETER FilePath is not a valid device path, or
> +                                BufferSize is NULL.
> +  @retval EFI_NO_MEDIA          No medium was present to load the file.
> +  @retval EFI_DEVICE_ERROR      The file was not loaded due to a device error.
> +  @retval EFI_NO_RESPONSE       The remote system did not respond.
> +  @retval EFI_NOT_FOUND         The file was not found
> +  @retval EFI_ABORTED           The file load process was manually canceled.
> +  @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current
> +                                directory entry. BufferSize has been updated with
> +                                the size needed to complete the request.
> +
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +AndroidBootImgLoadFile2 (
> +  IN EFI_LOAD_FILE2_PROTOCOL    *This,
> +  IN EFI_DEVICE_PATH_PROTOCOL   *FilePath,
> +  IN BOOLEAN                    BootPolicy,
> +  IN OUT UINTN                  *BufferSize,
> +  IN VOID                       *Buffer OPTIONAL
> +  )
> +
> +{
> +  // Verify if the valid parameters
> +  if (This == NULL ||
> +      BufferSize == NULL ||
> +      FilePath == NULL ||
> +      !IsDevicePathValid (FilePath, 0)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (BootPolicy) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  // Check if the given buffer size is big enough
> +  // EFI_BUFFER_TOO_SMALL to allow caller to allocate a bigger buffer
> +  if (mRamdiskSize == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +  if (Buffer == NULL || *BufferSize < mRamdiskSize) {
> +    *BufferSize = mRamdiskSize;
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  // Copy InitRd
> +  CopyMem (Buffer, mRamdiskData, mRamdiskSize);
> +  *BufferSize = mRamdiskSize;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +///
> +/// Load File Protocol instance
> +///
> +STATIC EFI_LOAD_FILE2_PROTOCOL  mAndroidBootImgLoadFile2 = {
> +  AndroidBootImgLoadFile2
> +};
> +
>  EFI_STATUS
>  AndroidBootImgGetImgSize (
>    IN  VOID    *BootImg,
> @@ -383,6 +487,7 @@ AndroidBootImgBoot (
>    VOID                               *RamdiskData;
>    UINTN                               RamdiskSize;
>    IN  VOID                           *FdtBase;
> +  EFI_HANDLE                          RamDiskLoadFileHandle;
>
>    NewKernelArg = NULL;
>    ImageHandle = NULL;
> @@ -423,14 +528,29 @@ AndroidBootImgBoot (
>      goto Exit;
>    }
>
> -  Status = AndroidBootImgLocateFdt (Buffer, &FdtBase);
> -  if (EFI_ERROR (Status)) {
> -    goto Exit;
> -  }
> +  if (FeaturePcdGet (PcdAndroidBootLoadFile2)) {
> +    RamDiskLoadFileHandle = NULL;
> +    mRamdiskData = RamdiskData;
> +    mRamdiskSize = RamdiskSize;
> +    Status = gBS->InstallMultipleProtocolInterfaces (&RamDiskLoadFileHandle,
> +                                                     &gEfiLoadFile2ProtocolGuid,
> +                                                     &mAndroidBootImgLoadFile2,
> +                                                     &gEfiDevicePathProtocolGuid,
> +                                                     &mRamdiskDevicePath,
> +                                                     NULL);
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
> +  } else {
> +    Status = AndroidBootImgLocateFdt (Buffer, &FdtBase);
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
>
> -  Status = AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, RamdiskSize);
> -  if (EFI_ERROR (Status)) {
> -    goto Exit;
> +    Status = AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, RamdiskSize);
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }

It looks to me like this changes the functionality so that if
PcdAndroidBootLoadFile2 is set, we now don't locate/update an FDT,
which was not clear to me from the cover letter and commit message.

This may well be intentional and correct, but since this function was
already a bit messy, I'd kind of prefer shunting some of this off into
helper functions now we're adding to the complexity.

/
    Leif


[JB] Actually do we know how this is supposed to work in all cases, I was adding this
to mainly support the case of ACPI boot with this format and none of my images have
FDT in them.

Current behavior seems a bit odd

  1.  If we don't have an fdt in either the system config table or file we exit with an error
  2.  If mAndroidBootImg->UpdateDtb is NULL then we never install a new FDT either loaded from file or with initrd content in it

I can restructure this but want to make sure it meets peoples needs (The tooling I use to create the boot image format I am not sure if it can embedded a device tree so testing might be hard)

Does this seem right

  1.  If ACPI is installed and Feature PCD is not enabled ASSERT and return failure
  2.  If ACPI is installed use LoadFile2 methods to install
  3.  If FDT based allow PCD to dictate if it passed via LoadFile2 or initrd entries in dt
  4.  If mAndroidBootImg->UpdateDtb is NULL allow update of initrd if feature pcd is not enabled

One other question if there is a fdt in boot image and also in the system config table which should take priority

-Jeff

>    }
>
>    KernelDevicePath = mMemoryDevicePathTemplate;
> @@ -474,5 +594,18 @@ AndroidBootImgBoot (
>        NewKernelArg = NULL;
>      }
>    }
> +  if (FeaturePcdGet (PcdAndroidBootLoadFile2)) {
> +    mRamdiskData = NULL;
> +    mRamdiskSize = 0;
> +    if (RamDiskLoadFileHandle != NULL) {
> +      gBS->UninstallMultipleProtocolInterfaces (RamDiskLoadFileHandle,
> +                                                &gEfiLoadFile2ProtocolGuid,
> +                                                &mAndroidBootImgLoadFile2,
> +                                                &gEfiDevicePathProtocolGuid,
> +                                                &mRamdiskDevicePath,
> +                                                NULL);
> +      RamDiskLoadFileHandle = NULL;
> +    }
> +  }
>
>    return Status;
>  }
> --
> 2.17.1
>

[-- Attachment #2: Type: text/html, Size: 22026 bytes --]