* [PATCH v2 0/2] AndroidBootImgLib improvements
@ 2021-09-01 20:28 Jeff Brasen
2021-09-01 20:28 ` [PATCH v2 1/2] EmbeddedPkg: AndroidBootImgBoot error handling updates Jeff Brasen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jeff Brasen @ 2021-09-01 20:28 UTC (permalink / raw)
To: devel; +Cc: leif, ardb+tianocore, abner.chang, daniel.schaefer, Jeff Brasen
Added support for using loadfile2 approach for passing ramdisk to linux.
Created patch series for general error handling improvments based on
review feedback.
[v2]
-Added review feedback
-General improvements to error handling
[v1]
- Intial revision
Jeff Brasen (2):
EmbeddedPkg: AndroidBootImgBoot error handling updates
EmbeddedPkg: Add LoadFile2 for linux initrd
EmbeddedPkg/EmbeddedPkg.dec | 1 +
.../AndroidBootImgLib/AndroidBootImgLib.inf | 3 +
.../AndroidBootImgLib/AndroidBootImgLib.c | 193 +++++++++++++++---
3 files changed, 171 insertions(+), 26 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/2] EmbeddedPkg: AndroidBootImgBoot error handling updates 2021-09-01 20:28 [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen @ 2021-09-01 20:28 ` Jeff Brasen 2021-09-01 20:28 ` [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd Jeff Brasen 2021-09-07 15:44 ` [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen 2 siblings, 0 replies; 6+ messages in thread From: Jeff Brasen @ 2021-09-01 20:28 UTC (permalink / raw) To: devel; +Cc: leif, ardb+tianocore, abner.chang, daniel.schaefer, Jeff Brasen Update AndroidBootImgBoot to use a single return point Make sure Kernel args are freed and Image is unloaded. Signed-off-by: Jeff Brasen <jbrasen@nvidia.com> --- .../AndroidBootImgLib/AndroidBootImgLib.c | 50 +++++++++++-------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c index bbc240c3632a..3c617649f735 100644 --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c @@ -384,10 +384,13 @@ AndroidBootImgBoot ( UINTN RamdiskSize; IN VOID *FdtBase; + NewKernelArg = NULL; + ImageHandle = NULL; + Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL, (VOID **) &mAndroidBootImg); if (EFI_ERROR (Status)) { - return Status; + goto Exit; } Status = AndroidBootImgGetKernelInfo ( @@ -396,19 +399,19 @@ AndroidBootImgBoot ( &KernelSize ); if (EFI_ERROR (Status)) { - return Status; + goto Exit; } NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE); if (NewKernelArg == NULL) { DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n")); - return EFI_OUT_OF_RESOURCES; + Status = EFI_OUT_OF_RESOURCES; + goto Exit; } Status = AndroidBootImgUpdateArgs (Buffer, NewKernelArg); if (EFI_ERROR (Status)) { - FreePool (NewKernelArg); - return Status; + goto Exit; } Status = AndroidBootImgGetRamdiskInfo ( @@ -417,19 +420,17 @@ AndroidBootImgBoot ( &RamdiskSize ); if (EFI_ERROR (Status)) { - return Status; + goto Exit; } Status = AndroidBootImgLocateFdt (Buffer, &FdtBase); if (EFI_ERROR (Status)) { - FreePool (NewKernelArg); - return Status; + goto Exit; } Status = AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, RamdiskSize); if (EFI_ERROR (Status)) { - FreePool (NewKernelArg); - return Status; + goto Exit; } KernelDevicePath = mMemoryDevicePathTemplate; @@ -442,21 +443,15 @@ AndroidBootImgBoot ( (EFI_DEVICE_PATH *)&KernelDevicePath, (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle); if (EFI_ERROR (Status)) { - // - // With EFI_SECURITY_VIOLATION retval, the Image was loaded and an ImageHandle was created - // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image can not be started right now. - // If the caller doesn't have the option to defer the execution of an image, we should - // unload image for the EFI_SECURITY_VIOLATION to avoid resource leak. - // - if (Status == EFI_SECURITY_VIOLATION) { - gBS->UnloadImage (ImageHandle); - } - return Status; + goto Exit; } // Set kernel arguments Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo); + if (EFI_ERROR (Status)) { + goto Exit; + } ImageInfo->LoadOptions = NewKernelArg; ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16); @@ -466,5 +461,18 @@ AndroidBootImgBoot ( Status = gBS->StartImage (ImageHandle, NULL, NULL); // Clear the Watchdog Timer if the image returns gBS->SetWatchdogTimer (0, 0x10000, 0, NULL); - return EFI_SUCCESS; + +Exit: + //Unload image as it will not be used anymore + if (ImageHandle != NULL) { + gBS->UnloadImage (ImageHandle); + ImageHandle = NULL; + } + if (EFI_ERROR (Status)) { + if (NewKernelArg != NULL) { + FreePool (NewKernelArg); + NewKernelArg = NULL; + } + } + return Status; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd 2021-09-01 20:28 [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen 2021-09-01 20:28 ` [PATCH v2 1/2] EmbeddedPkg: AndroidBootImgBoot error handling updates Jeff Brasen @ 2021-09-01 20:28 ` Jeff Brasen 2021-09-07 17:51 ` Leif Lindholm 2021-09-07 15:44 ` [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen 2 siblings, 1 reply; 6+ messages in thread From: Jeff Brasen @ 2021-09-01 20:28 UTC (permalink / raw) To: devel; +Cc: leif, ardb+tianocore, abner.chang, daniel.schaefer, Jeff Brasen 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> #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> @@ -25,7 +29,14 @@ typedef struct { EFI_DEVICE_PATH_PROTOCOL End; } MEMORY_DEVICE_PATH; +typedef struct { + VENDOR_DEVICE_PATH VenMediaNode; + 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; + } } 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd 2021-09-01 20:28 ` [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd Jeff Brasen @ 2021-09-07 17:51 ` Leif Lindholm 2021-09-09 21:01 ` Jeff Brasen 0 siblings, 1 reply; 6+ messages in thread From: Leif Lindholm @ 2021-09-07 17:51 UTC (permalink / raw) To: Jeff Brasen; +Cc: devel, ardb+tianocore, abner.chang, daniel.schaefer 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 > } > > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd 2021-09-07 17:51 ` Leif Lindholm @ 2021-09-09 21:01 ` Jeff Brasen 0 siblings, 0 replies; 6+ messages in thread From: Jeff Brasen @ 2021-09-09 21:01 UTC (permalink / raw) To: Leif Lindholm Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org, abner.chang@hpe.com, daniel.schaefer@hpe.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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] AndroidBootImgLib improvements 2021-09-01 20:28 [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen 2021-09-01 20:28 ` [PATCH v2 1/2] EmbeddedPkg: AndroidBootImgBoot error handling updates Jeff Brasen 2021-09-01 20:28 ` [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd Jeff Brasen @ 2021-09-07 15:44 ` Jeff Brasen 2 siblings, 0 replies; 6+ messages in thread From: Jeff Brasen @ 2021-09-07 15:44 UTC (permalink / raw) To: devel@edk2.groups.io Cc: leif@nuviainc.com, ardb+tianocore@kernel.org, abner.chang@hpe.com, daniel.schaefer@hpe.com [-- Attachment #1: Type: text/plain, Size: 1161 bytes --] Any additional feedback on this patchset? Thanks, Jeff ________________________________ From: Jeff Brasen <jbrasen@nvidia.com> Sent: Wednesday, September 1, 2021 2:28 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: leif@nuviainc.com <leif@nuviainc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; abner.chang@hpe.com <abner.chang@hpe.com>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; Jeff Brasen <jbrasen@nvidia.com> Subject: [PATCH v2 0/2] AndroidBootImgLib improvements Added support for using loadfile2 approach for passing ramdisk to linux. Created patch series for general error handling improvments based on review feedback. [v2] -Added review feedback -General improvements to error handling [v1] - Intial revision Jeff Brasen (2): EmbeddedPkg: AndroidBootImgBoot error handling updates EmbeddedPkg: Add LoadFile2 for linux initrd EmbeddedPkg/EmbeddedPkg.dec | 1 + .../AndroidBootImgLib/AndroidBootImgLib.inf | 3 + .../AndroidBootImgLib/AndroidBootImgLib.c | 193 +++++++++++++++--- 3 files changed, 171 insertions(+), 26 deletions(-) -- 2.17.1 [-- Attachment #2: Type: text/html, Size: 2356 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-09 21:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-01 20:28 [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen 2021-09-01 20:28 ` [PATCH v2 1/2] EmbeddedPkg: AndroidBootImgBoot error handling updates Jeff Brasen 2021-09-01 20:28 ` [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd Jeff Brasen 2021-09-07 17:51 ` Leif Lindholm 2021-09-09 21:01 ` Jeff Brasen 2021-09-07 15:44 ` [PATCH v2 0/2] AndroidBootImgLib improvements Jeff Brasen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox