From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (NAM12-DM6-obe.outbound.protection.outlook.com [40.107.243.57]) by mx.groups.io with SMTP id smtpd.web08.292.1631221307145529119 for ; Thu, 09 Sep 2021 14:01:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nvidia.com header.s=selector2 header.b=MZ86n4vd; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: nvidia.com, ip: 40.107.243.57, mailfrom: jbrasen@nvidia.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=EWQhWDdKZ7mUuyzlq4c+oxNkkYFGU/1+xdHoOjXMrlx/qx1mp5u2LNNabbH10dRLnTNjKMgUFF4PW5ptdPGGeZMtaKIuYxg8wSHz/fQX8kusj0wjqMsS7SBkiEEBt0CmrHG8OEZuuni2fMe7/ah46on2yCWQp2qbBDY9NB0CRyaGKfn0KdCtDiOWpaHI49FYkT6/SYX3HvQ0eEHIewXLySt2sNamFqERuWdcdaPuiZsCIl5FGYmHlav9IWCECXjzeJO6/Ua9CCPf7bXQeAyQXDGQudDHg7qBJFc3rnIbEdqtid8LaT78WKNyKoHVBS5UNhw1RFkLh4R8feD2c2RAkg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=+oPMJ4T6mvBj36h5t28Urwn7crFxFAT/rZYH1H4m7u8=; b=UZlndXKf761u/H7LAj3yGs0T7hIkLTeOT5x4sbK/ESI3ui5c5T0hB86Jg+v2vGv/CcnQ1a9mjRgben9HILIA1NCcd4WUYcj3iTg6dJm+IwOR3WvkbU9Z8JOcvMsDZxsGdXNNul6llN1jQUb1hT2pTDE/iWx59d0CDmhGUzmcD6BMAvRwLbxHoSk0Gr6mM84TQ8ExhTkPtP6kR+pZZ5XK8FAjU+BHqazgfdU7IOY1aZBPMow6zCSwmdM1y4vzbPcy4DzmVdEDxKNmfZDkQ6RsOebt5w4NeCn0bcunRuLchB7CJ3MiLYmB4q1dYgoCsa/hFRXUm0Cj2vT7FX5liKqOnQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=+oPMJ4T6mvBj36h5t28Urwn7crFxFAT/rZYH1H4m7u8=; b=MZ86n4vd1ZDL8cQoWUBFRpg96ge7CZIOhlBZHtm9rUhtXLAAd7jgCf0mqEvlDLxs3/dfYk5mSXOhzoTJ/vUNFjYGLxU4LIXqB1syD0fB3VTFtGe0DEKRONnmXGrC2GksUg8V3SmQfeAzqb2qKKQjdpHDPchwyeqhhyTssUJzCIx9w/oTCmCVPVe6VSYBfPNDdhqaiYmUneZPdII7eDOz4/WBw97n+krPLJmxakKcz1EWdOR/f5dJfopBhVM6uQpRXLo+zDKLEaNTTHLQ0id2VwBPHFb3tV6JG8HB8go/HIIMKFAlb896LeJxQJfSre7RHTIX9PtIrIB4w8/bXjPrVA== Received: from DM6PR12MB3340.namprd12.prod.outlook.com (2603:10b6:5:3d::24) by DM5PR12MB1674.namprd12.prod.outlook.com (2603:10b6:4:11::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4500.14; Thu, 9 Sep 2021 21:01:45 +0000 Received: from DM6PR12MB3340.namprd12.prod.outlook.com ([fe80::68ed:797b:18d4:848c]) by DM6PR12MB3340.namprd12.prod.outlook.com ([fe80::68ed:797b:18d4:848c%3]) with mapi id 15.20.4500.016; Thu, 9 Sep 2021 21:01:45 +0000 From: "Jeff Brasen" To: Leif Lindholm 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 Thread-Topic: [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux initrd Thread-Index: AQHXn2/vFnbr4851HUSCyEcKDhs3fKuY4w0AgANWLQQ= Date: Thu, 9 Sep 2021 21:01:45 +0000 Message-ID: References: <20210907175120.5osd27md7isohizz@leviathan> In-Reply-To: <20210907175120.5osd27md7isohizz@leviathan> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: 08267c60-dbac-fc8d-943a-4c3393484973 authentication-results: nuviainc.com; dkim=none (message not signed) header.d=none;nuviainc.com; dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 756dae74-4f4e-4077-c2a6-08d973d5083a x-ms-traffictypediagnostic: DM5PR12MB1674: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3383; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: gwsr3+P73cQogIeCh8bZAT/n1wUjpeik1Ck/y4ZLHB4RngJ/8HEhblOHP6ctRddnbzJ8f+mkkM3h5tVn7sI+vEZXQBOl8WWVcY1ZFGE9yeNg/c/efInnyaGuSKj2TV39QzjKDM/CQP42LiXVNw4pDzuItbNuT7oW52PR0dubOWA0Hp8Sq/aKKROE5kfboxwbM2KAef4allmh0aAHtkmaJIQJSXC6L3U+0Vcw30FMJyByKFaKf0cZMCgQFIeJiZKDxN+Ya78Rr0e3rHeYjUr8k8ZhGIYMHmmTM+CMkWVSROaWVOjvaTnvLC2XB9kn6+OJSd1KQu60dzGtmdzjEnAvkZ5kLunhV5GWcTaiKNWYhcdp4ujhOFeH+yj5RHyy4C92euP+7LC48v8+nsyMOnEi3VdBDkW6+oHULDm192EfRXa/jE0G+smUcNtfAeeKPRSwiWfa19RudMKNCMyTrCyQQH5wLoUIQooBWTK+YYz9favIY7mgzCdh8UprWW5BQmNwjwkQXu1bI07C3KLRvYZNyMQHs44tW4o9lKG544yfrsRBTEIw0IQwaENdCS7Y2iRuB0VItJeXnR46MtoXWPVBc4d24Eg8HQKKT0hJNBXouaeJfVrHedNihK8/YeXTcYQI0x8zEx+ABivt9+Q7SMxj3cyGpfdd9F4U6j66Rqj79Cp7vc4F8ddGJC4hM6FdoGrk6iJSLTKPJsgR39Yi4nBZvdQ0KY1mg/HGUhwTynV+YAk= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR12MB3340.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(396003)(136003)(366004)(376002)(346002)(39860400002)(478600001)(86362001)(8676002)(6916009)(122000001)(5660300002)(19627405001)(33656002)(38100700002)(316002)(54906003)(55016002)(71200400001)(8936002)(9686003)(4326008)(66556008)(26005)(66446008)(64756008)(2906002)(66946007)(66476007)(186003)(76116006)(91956017)(19627235002)(38070700005)(6506007)(53546011)(7696005)(52536014)(83380400001)(44824005);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?KmbfaME9JaX57rqumVGY29BnwpOK6vMBPMhtqzyUTCqx7EJzkYO4i5FH+4?= =?iso-8859-1?Q?Fjz/wVECQT/TS8i/zVzAvmLIiJz5hnBrHqCAsqF6c/EWZs/0v8SDrwa0UT?= =?iso-8859-1?Q?xT8ze/T7MnJVirp7IieZwcUtk97v2V+zcda9zmgHGjYrw1Y052gpAeD65n?= =?iso-8859-1?Q?OMc3O7oW+OKnl75Vv4p19yDXyGxR/mgRitgWUstX2kxaDs/wzCYMTdCfM8?= =?iso-8859-1?Q?hNSVnDOcMaljYtV9TlAx5tDeB6L8/pR7HyP7gJabju0JmpVVd/I3xkI2S9?= =?iso-8859-1?Q?2vny6C7ZTMZkiDCvynAvJErscVzIF1qV+uHP4LdW+KZjMcrlMxLGvTOfBD?= =?iso-8859-1?Q?j0amduQpcdb4Hx13MocrBAfZPtog5t+yKlXR/67/c4QqduiaW4bI1uVIDa?= =?iso-8859-1?Q?UUDCEsT35NOM8ZtfuJ0vgaNnXR/gRYDci5zI8YUGCaR335W1yO+2uwSyLz?= =?iso-8859-1?Q?sUUJReLMyCuSaOncDfl6lYOl0JJkrdIFOOlrJWoCcZIivj4CoD5OhyYFfg?= =?iso-8859-1?Q?Fjzkv5toHcvlS9CeIwEDrkmVaz+U8capIC+7TYCeK9hBSOo5MD8e4qnibn?= =?iso-8859-1?Q?1OwM2UNMuA7pT5fg47tGF6eCMRTLRFN4xnI4dDKZ3lYd5KGvhq3/8cVbUp?= =?iso-8859-1?Q?3MjAuCiEU0MYtslLv/TVGiLtTPCdahAdAuGRYMfO8u3kJ0pHgsfeckASY4?= =?iso-8859-1?Q?1XFWtFw1lXQc9ohRsilLSSnXP7Hwt9U7XHdXqH+qFevwLmaayx4w2iD50e?= =?iso-8859-1?Q?9qSG2CHqkg1gsLtBNSrHrEhwUJTAhx9d+KvEPNj0IcHyd56hlGvD7Nt2Hn?= =?iso-8859-1?Q?5vZty75ccIqZvKCdyWSQBsmgGqvunahBnZx25aznhTZqi113AeizDdS8WA?= =?iso-8859-1?Q?HfhMimf4D6CmxDyVkiP0JgnalAfCbj8TQxQU9do2M7ts1xFaCJPttWhqGh?= =?iso-8859-1?Q?4VRN6QEBjYCNHkFgiNO5E3qHTg28SXorOBmdFWumErcBmRyAJJKboe+Xfx?= =?iso-8859-1?Q?JNHp8luukBrRgIYcXkqra7Crid5GISpFw4PXH47a51hwjVJ73p4uQp5ZC1?= =?iso-8859-1?Q?hdFSROfb4+RAMdZc+yM303J6ZRS1zU3QZ6rjxiqYEnata9AL9I0RZmXymZ?= =?iso-8859-1?Q?jXIQa5tp+JVHjkwTxzqRjyiNjj4hioiaGTFewh7WR2nL+w7es+XV6Dfabb?= =?iso-8859-1?Q?VJpCpNXZvO6MRyCdlR1mltHw389r+v6C/GPd3yANXLygJMqxN32KTbazAA?= =?iso-8859-1?Q?avV7F5NYb6WSz1Nh1kap0CNjY/Dv0IM+8H9l5EfaHrVCF+5mNLAM/Rq3hr?= =?iso-8859-1?Q?SBB0iIXgg2GNOhxAf2FGS2CBmUl27HNeNVyKUqwNqs5XEu8=3D?= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3340.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 756dae74-4f4e-4077-c2a6-08d973d5083a X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Sep 2021 21:01:45.1823 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 8hPA6UHH9EBcK+PplQXq/pmXrSFrrfext82S0SBaXc37ntq1bdqod7Np7SHGmh33eojmi7C14/QHB8CphircOA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1674 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_DM6PR12MB33401F9F574593CC531615CDCBD59DM6PR12MB3340namp_" --_000_DM6PR12MB33401F9F574593CC531615CDCBD59DM6PR12MB3340namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable 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 ; dan= iel.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|0x000000= 5b > > [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 =3D NULL; > +STATIC UINTN mRamdiskSize =3D 0; > > STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate =3D > { > @@ -48,6 +59,99 @@ STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTempl= ate =3D > } // End > }; > > +STATIC CONST RAMDISK_DEVICE_PATH mRamdiskDevicePath =3D > +{ > + { > + { > + 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 wit= h 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 request= ed file. > + @param Buffer The memory buffer to transfer the file to. IF Buffe= r 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 cance= led. > + @retval EFI_BUFFER_TOO_SMALL The BufferSize is too small to read the = current > + directory entry. BufferSize has been upd= ated 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 =3D=3D NULL || > + BufferSize =3D=3D NULL || > + FilePath =3D=3D 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 =3D=3D 0) { > + return EFI_NOT_FOUND; > + } > + if (Buffer =3D=3D NULL || *BufferSize < mRamdiskSize) { > + *BufferSize =3D mRamdiskSize; > + return EFI_BUFFER_TOO_SMALL; > + } > + > + // Copy InitRd > + CopyMem (Buffer, mRamdiskData, mRamdiskSize); > + *BufferSize =3D mRamdiskSize; > + > + return EFI_SUCCESS; > +} > + > +/// > +/// Load File Protocol instance > +/// > +STATIC EFI_LOAD_FILE2_PROTOCOL mAndroidBootImgLoadFile2 =3D { > + AndroidBootImgLoadFile2 > +}; > + > EFI_STATUS > AndroidBootImgGetImgSize ( > IN VOID *BootImg, > @@ -383,6 +487,7 @@ AndroidBootImgBoot ( > VOID *RamdiskData; > UINTN RamdiskSize; > IN VOID *FdtBase; > + EFI_HANDLE RamDiskLoadFileHandle; > > NewKernelArg =3D NULL; > ImageHandle =3D NULL; > @@ -423,14 +528,29 @@ AndroidBootImgBoot ( > goto Exit; > } > > - Status =3D AndroidBootImgLocateFdt (Buffer, &FdtBase); > - if (EFI_ERROR (Status)) { > - goto Exit; > - } > + if (FeaturePcdGet (PcdAndroidBootLoadFile2)) { > + RamDiskLoadFileHandle =3D NULL; > + mRamdiskData =3D RamdiskData; > + mRamdiskSize =3D RamdiskSize; > + Status =3D gBS->InstallMultipleProtocolInterfaces (&RamDiskLoadFileH= andle, > + &gEfiLoadFile2Proto= colGuid, > + &mAndroidBootImgLoa= dFile2, > + &gEfiDevicePathProt= ocolGuid, > + &mRamdiskDevicePath= , > + NULL); > + if (EFI_ERROR (Status)) { > + goto Exit; > + } > + } else { > + Status =3D AndroidBootImgLocateFdt (Buffer, &FdtBase); > + if (EFI_ERROR (Status)) { > + goto Exit; > + } > > - Status =3D AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, Ramd= iskSize); > - if (EFI_ERROR (Status)) { > - goto Exit; > + Status =3D AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, Ra= mdiskSize); > + 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 a= dding this to mainly support the case of ACPI boot with this format and none of my ima= ges 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 to= oling I use to create the boot image format I am not sure if it can embedde= d 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 initr= d entries in dt 4. If mAndroidBootImg->UpdateDtb is NULL allow update of initrd if featu= re pcd is not enabled One other question if there is a fdt in boot image and also in the system c= onfig table which should take priority -Jeff > } > > KernelDevicePath =3D mMemoryDevicePathTemplate; > @@ -474,5 +594,18 @@ AndroidBootImgBoot ( > NewKernelArg =3D NULL; > } > } > + if (FeaturePcdGet (PcdAndroidBootLoadFile2)) { > + mRamdiskData =3D NULL; > + mRamdiskSize =3D 0; > + if (RamDiskLoadFileHandle !=3D NULL) { > + gBS->UninstallMultipleProtocolInterfaces (RamDiskLoadFileHandle, > + &gEfiLoadFile2ProtocolGu= id, > + &mAndroidBootImgLoadFile= 2, > + &gEfiDevicePathProtocolG= uid, > + &mRamdiskDevicePath, > + NULL); > + RamDiskLoadFileHandle =3D NULL; > + } > + } > > return Status; > } > -- > 2.17.1 > --_000_DM6PR12MB33401F9F574593CC531615CDCBD59DM6PR12MB3340namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
Question below

Thanks,

Jeff


From: Leif Lindholm <lei= f@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+tianocor= e@kernel.org <ardb+tianocore@kernel.org>; abner.chang@hpe.com <abn= er.chang@hpe.com>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com&g= t;
Subject: Re: [PATCH v2 2/2] EmbeddedPkg: Add LoadFile2 for linux ini= trd
 
External email: Use caution opening links or attac= hments


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      &= nbsp;            |&n= bsp;  1 +
>  .../AndroidBootImgLib/AndroidBootImgLib.inf   | &= nbsp; 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.PcdPrePiProduceMemoryTypeInf= ormationHob|FALSE|BOOLEAN|0x0000001b
>    gEmbeddedTokenSpaceGuid.PcdGdbSerial|FALSE|BOOLEAN|0= x00000053
> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2|FALSE|BOOLEAN|= 0x0000005b
>
>  [PcdsFixedAtBuild.common]
>    gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0|UINT32|0= x0000000b
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.i= nf 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    &nbs= p;           End;
>  } MEMORY_DEVICE_PATH;
>
> +typedef struct {
> +  VENDOR_DEVICE_PATH       &n= bsp;            = ;  VenMediaNode;

VendorMediaNode

> +  EFI_DEVICE_PATH_PROTOCOL      &n= bsp;         EndNode;
> +} RAMDISK_DEVICE_PATH;
> +
>  STATIC ANDROID_BOOTIMG_PROTOCOL     &nb= sp;           *mAndroidBo= otImg;
> +STATIC VOID         &nbs= p;            &= nbsp;           &nbs= p;  *mRamdiskData =3D NULL;
> +STATIC UINTN         &nb= sp;            =             &nb= sp; mRamdiskSize =3D 0;
>
>  STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate =3D >  {
> @@ -48,6 +59,99 @@ STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTe= mplate =3D
>    } // End
>  };
>
> +STATIC CONST RAMDISK_DEVICE_PATH mRamdiskDevicePath =3D
> +{
> +  {
> +    {
> +      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
> +           &nb= sp;         code of EFI_SUCCESS, th= e amount of data transferred to
> +           &nb= sp;         Buffer. On output with = a return code of EFI_BUFFER_TOO_SMALL,
> +           &nb= sp;         the size of Buffer requ= ired to retrieve the requested file.
> +  @param  Buffer     The memory buffer = to transfer the file to. IF Buffer is NULL,
> +           &nb= sp;         then no the size of the= requested file is returned in
> +           &nb= sp;         BufferSize.
> +
> +  @retval EFI_SUCCESS       &= nbsp;   The file was loaded.
> +  @retval EFI_UNSUPPORTED       Bo= otPolicy is TRUE.
> +  @retval EFI_INVALID_PARAMETER FilePath is not a valid device p= ath, or
> +           &nb= sp;            =         BufferSize is NULL.
> +  @retval EFI_NO_MEDIA       =    No medium was present to load the file.
> +  @retval EFI_DEVICE_ERROR      The fil= e was not loaded due to a device error.
> +  @retval EFI_NO_RESPONSE       Th= e remote system did not respond.
> +  @retval EFI_NOT_FOUND       = ;  The file was not found
> +  @retval EFI_ABORTED       &= nbsp;   The file load process was manually canceled.
> +  @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small= to read the current
> +           &nb= sp;            =         directory entry. BufferSize has = been updated with
> +           &nb= sp;            =         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        &nbs= p;           BootPolicy,<= br> > +  IN OUT UINTN        &n= bsp;         *BufferSize,
> +  IN VOID         &= nbsp;           &nbs= p; *Buffer OPTIONAL
> +  )
> +
> +{
> +  // Verify if the valid parameters
> +  if (This =3D=3D NULL ||
> +      BufferSize =3D=3D NULL ||
> +      FilePath =3D=3D 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 b= uffer
> +  if (mRamdiskSize =3D=3D 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +  if (Buffer =3D=3D NULL || *BufferSize < mRamdiskSize) {
> +    *BufferSize =3D mRamdiskSize;
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  // Copy InitRd
> +  CopyMem (Buffer, mRamdiskData, mRamdiskSize);
> +  *BufferSize =3D mRamdiskSize;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +///
> +/// Load File Protocol instance
> +///
> +STATIC EFI_LOAD_FILE2_PROTOCOL  mAndroidBootImgLoadFile2 =3D { > +  AndroidBootImgLoadFile2
> +};
> +
>  EFI_STATUS
>  AndroidBootImgGetImgSize (
>    IN  VOID    *BootImg,
> @@ -383,6 +487,7 @@ AndroidBootImgBoot (
>    VOID        =             &nb= sp;          *RamdiskData;
>    UINTN        = ;            &n= bsp;          RamdiskSize;
>    IN  VOID      &nb= sp;            =         *FdtBase;
> +  EFI_HANDLE        &nbs= p;            &= nbsp;    RamDiskLoadFileHandle;
>
>    NewKernelArg =3D NULL;
>    ImageHandle =3D NULL;
> @@ -423,14 +528,29 @@ AndroidBootImgBoot (
>      goto Exit;
>    }
>
> -  Status =3D AndroidBootImgLocateFdt (Buffer, &FdtBase);
> -  if (EFI_ERROR (Status)) {
> -    goto Exit;
> -  }
> +  if (FeaturePcdGet (PcdAndroidBootLoadFile2)) {
> +    RamDiskLoadFileHandle =3D NULL;
> +    mRamdiskData =3D RamdiskData;
> +    mRamdiskSize =3D RamdiskSize;
> +    Status =3D gBS->InstallMultipleProtocolInterfac= es (&RamDiskLoadFileHandle,
> +           &nb= sp;            =             &nb= sp;            =     &gEfiLoadFile2ProtocolGuid,
> +           &nb= sp;            =             &nb= sp;            =     &mAndroidBootImgLoadFile2,
> +           &nb= sp;            =             &nb= sp;            =     &gEfiDevicePathProtocolGuid,
> +           &nb= sp;            =             &nb= sp;            =     &mRamdiskDevicePath,
> +           &nb= sp;            =             &nb= sp;            =     NULL);
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
> +  } else {
> +    Status =3D AndroidBootImgLocateFdt (Buffer, &F= dtBase);
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
>
> -  Status =3D AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskDa= ta, RamdiskSize);
> -  if (EFI_ERROR (Status)) {
> -    goto Exit;
> +    Status =3D AndroidBootImgUpdateFdt (Buffer, FdtBas= e, 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 w= ork 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->Upd= ateDtb is NULL then we never install a new FDT either loaded from file or w= ith initrd content in it
I can restructure this but want to make sure it me= ets peoples needs (The tooling I use to create the boot image format I am n= ot 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 re= turn failure
  2. If ACPI is installed use LoadFile2 methods to install=
  3. If FDT based allow PCD to dictate if it passed via Lo= adFile2 or initrd entries in dt
  4. If mAndroidBoo= tImg->UpdateDtb is NULL allow update of initrd if feature pcd is not ena= bled
One other question if there is a fdt in boot image and also i= n the system config table which should take priority

-Jeff

>    }
>
>    KernelDevicePath =3D mMemoryDevicePathTemplate;
> @@ -474,5 +594,18 @@ AndroidBootImgBoot (
>        NewKernelArg =3D NULL;
>      }
>    }
> +  if (FeaturePcdGet (PcdAndroidBootLoadFile2)) {
> +    mRamdiskData =3D NULL;
> +    mRamdiskSize =3D 0;
> +    if (RamDiskLoadFileHandle !=3D NULL) {
> +      gBS->UninstallMultipleProtocolInter= faces (RamDiskLoadFileHandle,
> +           &nb= sp;            =             &nb= sp;           &gEfiLo= adFile2ProtocolGuid,
> +           &nb= sp;            =             &nb= sp;           &mAndro= idBootImgLoadFile2,
> +           &nb= sp;            =             &nb= sp;           &gEfiDe= vicePathProtocolGuid,
> +           &nb= sp;            =             &nb= sp;           &mRamdi= skDevicePath,
> +           &nb= sp;            =             &nb= sp;           NULL);
> +      RamDiskLoadFileHandle =3D NULL;
> +    }
> +  }
>
>    return Status;
>  }
> --
> 2.17.1
>
--_000_DM6PR12MB33401F9F574593CC531615CDCBD59DM6PR12MB3340namp_--