Question below Thanks, Jeff ________________________________ From: Leif Lindholm Sent: Tuesday, September 7, 2021 11:51 AM To: Jeff Brasen Cc: devel@edk2.groups.io ; ardb+tianocore@kernel.org ; abner.chang@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 > --- > 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 > #include > #include > +#include Move that inserted line up one to maintain alphabetical sorting. > #include > #include > > #include > #include > +#include > + > +#include > > #include 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 >