From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by mx.groups.io with SMTP id smtpd.web12.353.1631037083611862832 for ; Tue, 07 Sep 2021 10:51:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=dAzHHp18; spf=pass (domain: nuviainc.com, ip: 209.85.128.50, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f50.google.com with SMTP id k5-20020a05600c1c8500b002f76c42214bso2492851wms.3 for ; Tue, 07 Sep 2021 10:51:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=MaSnCwEw3wHXYMx966hQrJMkDMCyI3qvToLfZr2/Lao=; b=dAzHHp18Sv+OVM/gdKsoKJcsSX4L75E3w0LmH1+zKdnsJcFpx4xmBmUyl0bOwh6bSN CM6gYGMmxi6qhyMtXyiOYEeWvjfDnRwyiweIIMqif6Ha97yi0HaAsyXqEeW30WbgZR94 OHaE+ABH4CTArq5Uqs+NYIwuucul13IfoOy6S1FbBP8oGjHPgx37XwQ2CZ/27VlzoTV1 ymSWwTLOGvho41e2OgHNqT7s2lGUn2VfnITbyaTkyjjd2qm0nb87PTttD8p7bi6ZzWSk U6hwmDvshl6DEy7jFpl4BuemVfr6dU0EYoelgY0umxr11uCZX5gTpsRePjZnnC0Yujee O5eA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=MaSnCwEw3wHXYMx966hQrJMkDMCyI3qvToLfZr2/Lao=; b=Q3IZLB2TngyXTlR/sVcGHZqA+25+2gEavvITSksdXJ/QpeZdUm41tnvXSpvl+biav8 Un9JBMXYE+hx+Z0Bk5TIfYcC22dNMDsreIgqAYeJ+wV00HOZE5XxBNN9dZeSMBnGSiQJ ZVIccVJHIoa8m6aaUelA+H5P+0X/n36HQ7YY6XCSIUdSbIkvw1huPTP/B6cKEkhrQqDa n+8UvICYhTguIn97wgVOYCHzdIeLI9I4cFz3zojbx9mHFxbAID2XO2z6gqDzMSUVrEGE 6nDhQHtrbsbBDSHps278vWAHXQkWfIwJbUhg5HHOeoXGjSUUKdZZi6NjBLPLK91AsxAW cwtw== X-Gm-Message-State: AOAM5313RAoZKV7NH+rWSxCAljTF2BrIyt5iZd44hu117QQOweLW9aIO EVj76AinUl1mPKKABXuuMCJfFQ== X-Google-Smtp-Source: ABdhPJx1GU+mHYQegeF0dIWoeBWCSyr82BNmfn2ih8vfTiNJz9/4s6ncpS/2pjZn4QYKtpUt8MqElQ== X-Received: by 2002:a05:600c:2252:: with SMTP id a18mr112088wmm.64.1631037082168; Tue, 07 Sep 2021 10:51:22 -0700 (PDT) Return-Path: Received: from leviathan (cpc92314-cmbg19-2-0-cust559.5-4.cable.virginm.net. [82.11.186.48]) by smtp.gmail.com with ESMTPSA id g1sm12246299wrc.65.2021.09.07.10.51.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Sep 2021 10:51:21 -0700 (PDT) Date: Tue, 7 Sep 2021 18:51:20 +0100 From: "Leif Lindholm" 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 Message-ID: <20210907175120.5osd27md7isohizz@leviathan> References: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > } > > 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 >