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.
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)