From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM04-DM6-obe.outbound.protection.outlook.com (NAM04-DM6-obe.outbound.protection.outlook.com [40.107.102.82]) by mx.groups.io with SMTP id smtpd.web08.470.1631638662814563441 for ; Tue, 14 Sep 2021 09:57:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nvidia.com header.s=selector2 header.b=NOa/0nMY; 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.102.82, mailfrom: jbrasen@nvidia.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BcHVmVb1oLqUozdjGdJiZQRQVyUvMSsvCvjTa0HMLfEPBiDQa2qrxNTg4j2R8kQ3QY1aDc6K1cZ5snezGc9Mx+kDZmz6AWO0LR35M2Zqoku53VdmqhXW49slbgGRJG9wmI9WCqJpLVIm0ztX/a31uMq2pnxeQMl22JLuqqfjf2I2hzCcq+tEsD8fZAVX/BuH+mwM7NbCA7bb2OyQ1LjCMctsQ/GCBeCL7F5QtuGwZDFQgcCnpWbUpfrmflHYQtU6YPpfF/M0HPYKWVzIc/ysfoc6QydfKN64qhKHox2cZpAEp3RdGZv0pA3erUORL6srm1lVcxY8Z3d5ZaErPNYvAw== 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=/kyx4ISmNxVX9K+RV54cAJsFzPB7JxfRJSpShG3zn80=; b=WqMkeJe7ApyW3e2klZqaHFP6I7ThepJXV0sIhUaLLLXvoMVAGRG3Ahw1O6Jyqfl7AuJxtI3R5bvmCYlFlWkojALSOIRGLr2e43N+mTrva7QO/pU4OgAnCcQqhoG7HffjULRo0hFxqwv+yTQby5qasgcShnjnca0pl4yS/2tsLIJFFqYqL00f8e6EccrDOgqHpTD3/Z5mrpATME/WHcz10p/V28Nt8mYBiOdGwBAHIgkUuPo/Uwhfim7l1sRR1hGF5qNwSxXYgMwW2a0GAy2/lUKl7deySx+2qRwUZerZleI4Ky1tMeYXjbZfqCJg4VJAe4B8F48sguG2YtqWcJT38A== 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=/kyx4ISmNxVX9K+RV54cAJsFzPB7JxfRJSpShG3zn80=; b=NOa/0nMYlPWtlln1JbQ3/5R3aTxRlPLdwuwUbrRUNHVzhcopbdbF0Oz9iD7lVXTafLeDJG4UTDqaSk8+yxltlbbcEWpiqRPgpCqOpUwlace3JsYEN9+RZQ65+V27GGx8pBtrQ52VQpjwZLy9TQLXCEZeTq5wZ0o3dlFTpr8gEydmKFYZ/TSltN/iHRB5fObBdJNfqlaSYlesWr0drVATeYQm8wS0UBQA+crhCUTcDYHvXs8o5BcROeoEooHVBqQXXy4w+05QZ9JT13FP7+MXivrf691UDr610XnjVaJTonpEYdqJnn3PYymXv9wGBjbmXc4OUx8dNkpKsT4wufPDjg== Received: from DM6PR12MB3340.namprd12.prod.outlook.com (2603:10b6:5:3d::24) by DM6PR12MB4089.namprd12.prod.outlook.com (2603:10b6:5:213::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4500.17; Tue, 14 Sep 2021 16:57:41 +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.019; Tue, 14 Sep 2021 16:57:41 +0000 From: "Jeff Brasen" To: Leif Lindholm CC: "devel@edk2.groups.io" , "daniel.schaefer@hpe.com" , "abner.chang@hpe.com" , "ardb+tianocore@kernel.org" , Jun Nie Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements Thread-Topic: [PATCH v3 0/4] AndroidBootImgLib improvements Thread-Index: AQHXqPXBNs0khKNEzUWnxYBc56ObZKujoLoAgAAfOqc= Date: Tue, 14 Sep 2021 16:57:41 +0000 Message-ID: References: <20210914150058.nb5z4hz2a3e7ndts@leviathan> In-Reply-To: <20210914150058.nb5z4hz2a3e7ndts@leviathan> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: 6f952be6-d8b5-95e9-10fb-b23eee792d9c 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: 6f41ac97-1065-4677-82ef-08d977a0c3ea x-ms-traffictypediagnostic: DM6PR12MB4089: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8273; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: O/usOwV0+LXRtgO5NpF15gHtLY10SQRDwCil8Qz2+bPffueRzJ7VQANdCUUdOuMhJkqL5ysyYo5kGZAui62K0HjUizGsO/Tjx09tteNd3H0liFrqhF5SJUSUeeKeixLU9UTS0U3hoHfonw1nZwoAqMJz9OUI4Dp8QNwAZ/cvUA0CbFBAKP+r/Rq4igFHlMdXEocKZISSGrO8TItd9JVuBUI/YjDyQnReZeui2epdIgd0j3ijJWeKBsrZ7XwsIORtCNGpmF1qvbx1M2Fr18N+d4jYaAkCy2SF8dq1pYTRok02zXWdfb9tcgxcy+GyMWa9iBfH4eRmjNOLKcTVCgMsafIGn9K026ZdFLr/SasjDXDkSPt5UZCurqr8y41qOKKp19z3IrTYCIzUfXg11ri1pAZ+VkddCyO6fSdfOn/PDru+c7zklZI8h04jZ4eH4/vnBuJxd21wIGCyUuL8BC+199duMBjW5FrEHlr3DlUKIz/d7ypVWNQ1G/eDiEcEqyAPGw3ASdtTlVbK8HhiPJVBNLxtzNreHPMTWG6bOyvuRVNi8rAf9sXE/jpmVpJrdEgrYmevH3ggWCEszN1isZ75rW5xr5fw0HLCVoHzKNPvTNXr3fSByi960KesmInu8QvRHLp6L3TDrflPywXfzPhdyOteukbLEK+3917bLXZY3v/nQIplDWv29MpHx9hUbgN5r9a/5mZzwF1m/OnioOS1Iw== 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)(366004)(7696005)(9686003)(53546011)(71200400001)(4326008)(5660300002)(38070700005)(316002)(52536014)(55016002)(2906002)(186003)(122000001)(8936002)(508600001)(26005)(76116006)(6506007)(83380400001)(19627405001)(54906003)(66446008)(86362001)(6916009)(8676002)(33656002)(64756008)(38100700002)(66946007)(66476007)(66556008);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?HcHDnZKKcxsZBpM5K4XpLJwmSfDf0+vA9VDcQQVdka0Ytpw4rAqzEzQnrqHG?= =?us-ascii?Q?LjwCinfTkgo2PARu2NVtcWRMiIL0n5h/FzKNfKY8vdKyUQMUmKVOHINgcjrC?= =?us-ascii?Q?afmD61g8Gm3ZRcLk5iqlCvWTNsR/vzYmBLASx81eWMHURLK2yKk0ayQdVRtu?= =?us-ascii?Q?2bU5addN3G8chjqSGBrfjbexWWeYB1JTAwVScfAffr7UxQuCR2F0OwwhGC0T?= =?us-ascii?Q?lNN8+NJE1aQ2yh32Xi/sNOSA/gkxhoXuLz6DoAZf660WF4idJFAeXA82Un0s?= =?us-ascii?Q?ZBL0qwCsbDVfjGc9F9ugGCJt3tlypmUAZJMOQjX02QldL+SQbgGvX/d+dxhm?= =?us-ascii?Q?Dvz89xI1djJfr8ehxXDYZwmQrf0SfBtUcIOc5yUlfGWqUVO2o2nAR12u6zV/?= =?us-ascii?Q?Is2qmdQi0b5S9ci6I4MaMVpc9e9QbfMC2Q7VBv+0ILT9GehtyPoSkkpcITVY?= =?us-ascii?Q?XWZfxKs/qDrDL86akWueA7UHc9qMZFcF5oJYTjpSkLhTwjNq6Rl2B+4U7F2r?= =?us-ascii?Q?2MXpRG44WZHgm2LsX7s7KvwaSNUmJX7pZTqc618lAGeAZuo43o/X9DQI9Odc?= =?us-ascii?Q?TZFpHipX3LdJc5mYSNOlYcVISeb/a/u9MHxZrDUYsNgXljBw7mteNTNCF3eO?= =?us-ascii?Q?98qBliKeDSgqo55GyEPHn2cY+X2LEdlxDYGqgKjlXIA+B1eR+qSvqCMGe96J?= =?us-ascii?Q?8nzIU3+xegLffwnUQ6B/XexpuG01mqrKFYgyZR7XU+H3KV2Kd8eZ6Czq7E3/?= =?us-ascii?Q?UZaICSwyiC2buSz1SZAG8Ayypwj7jNNt9as/Ox+P0t5HtfAioc44KG/7Fo7H?= =?us-ascii?Q?YyHIc4/XK/NxMMu6dBsTU7XTBhmqGmt74fqAnUTRokjYM0m8S1ZpUY7R7pic?= =?us-ascii?Q?tcqJm/qKTWbiFSVkxX2NUWNHnH0bbQWvKax5FvBCHcoPgrKsou4BGjJtmGL/?= =?us-ascii?Q?qxWkotGm/ykm+dAJ3ODjT3gGbSwuUXsoFIWlGBdH8FWXcFIbz9ir8P7TbwdJ?= =?us-ascii?Q?pA30gJ58DF9GucYD5vsfW5aQnCeConPsW3ct2vkuH/vpIh7598IVoc56SUW/?= =?us-ascii?Q?6p6u3+cxjijJbKF0aXvk2nX3REF5uWWJylNQhB1UviPUUgMoLQUX3tRtfLbB?= =?us-ascii?Q?UtPuCuOJxfNczvvJzaKnkICjz1zfUYY1ErX/gptCmPV80Sg4ij262HmR1oH1?= =?us-ascii?Q?QgsyRLBt+qE+diQosRKA8L8FC9zrywa3j7q5BpKI/zwMNLgj1gmBCK4Q6vpA?= =?us-ascii?Q?HRazXeyKOM7w5iq1zN+TIRPfmkHC8Psf0Rjnjo2IgDZEcTBE3OUzheCyTK3U?= =?us-ascii?Q?2Vs=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: 6f41ac97-1065-4677-82ef-08d977a0c3ea X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Sep 2021 16:57:41.3815 (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: VS/OkEo0Cj5V0zTlpum72zRfNH2E3M7UISGRlCqBUCIJDf3VdPu9t22naDrEKwa5WPuds0CJk5dTL3ZZpQ5L3w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4089 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_DM6PR12MB33407EEC50F0C74AF00032F4CBDA9DM6PR12MB3340namp_" --_000_DM6PR12MB33407EEC50F0C74AF00032F4CBDA9DM6PR12MB3340namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb =3D=3D = NULL. This seemed like a bug as we would not add the initrd values nor would we u= se the fdt from the BootImg if that is where the device tree was sourced fr= om. It seems like either we should require UpdateDtb to be implemented (which s= eems to cause greater compatibility issues) or we install the device tree w= e have if that function isn't implemented. As far as merging goes I am fine either way. Our particular flow won't hit = this path as we don't have a device tree in the bootimg (use the system con= fig table) and we will have the new pcd set, but this seemed like a bug whi= le I looking at this code. Thanks, Jeff ________________________________ From: Leif Lindholm Sent: Tuesday, September 14, 2021 9:00 AM To: Jeff Brasen Cc: devel@edk2.groups.io ; daniel.schaefer@hpe.com ; abner.chang@hpe.com ; ardb+ti= anocore@kernel.org ; Jun Nie Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements External email: Use caution opening links or attachments Hi Jeff, Thanks for this. This set looks good to me, with a slight question mark wrt behaviour compatibility with previous versions for 3/4. (I think it's fine, but I'm a bear of very little brain, and it's been several years since I reviewed this code, and even longer since I really interacted with Android. ^ | shameless plug for more EmbeddedPkg reviewer volunteers.) I've added Jun Nie, who wrote the original version of this code, to see if he has any comments. 1-2/4 are obviously unproblematic, and I could merge those ahead of time if preferred. You can add Reviewed-by: Leif Lindholm for those if there are any further revisions of the set. Best Regards, Leif On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote: > Added support for using loadfile2 approach for passing ramdisk to linux. > Created patch series for general error handling improvments based on > review feedback. > If ACPI tables are in system table or PCD is defined the LoadFile2 method > of passing initrd will be used. > > [v3] > -Code review cleanup > -Removed duplicate header file > -Added change to allow FDT to install if UpdateDtb function is not define= d > -Added specific ACPI check > -Moved install functions to subfunctions > > [v2] > -Added review feedback > -General improvements to error handling > > [v1] > - Intial revision > > > Jeff Brasen (4): > EmbeddedPkg: Remove duplicate libfdt.h include > EmbeddedPkg: AndroidBootImgBoot error handling updates > EmbeddedPkg: Install FDT if UpdateDtb is not present > EmbeddedPkg: Add LoadFile2 for linux initrd > > EmbeddedPkg/EmbeddedPkg.dec | 1 + > .../AndroidBootImgLib/AndroidBootImgLib.inf | 4 + > .../AndroidBootImgLib/AndroidBootImgLib.c | 275 +++++++++++++++--- > 3 files changed, 233 insertions(+), 47 deletions(-) > > -- > 2.17.1 > --_000_DM6PR12MB33407EEC50F0C74AF00032F4CBDA9DM6PR12MB3340namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb= =3D=3D NULL. 

This seemed like a bug as we would not add the initrd values nor would we u= se the fdt from the BootImg if that is where the device tree was sourced fr= om. 

It seems like either we should require UpdateDtb to be implemented (which s= eems to cause greater compatibility issues) or we install the device tree w= e have if that function isn't implemented.

As far as merging goes I am fine either way. Our particular flow won't hit = this path as we don't have a device tree in the bootimg (use the system con= fig table) and we will have the new pcd set, but this seemed like a bug whi= le I looking at this code.

Thanks,

Jeff

From: Leif Lindholm <lei= f@nuviainc.com>
Sent: Tuesday, September 14, 2021 9:00 AM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaef= er@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.c= hang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org&g= t;; Jun Nie <jun.nie@linaro.org>
Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
 
External email: Use caution opening links or attac= hments


Hi Jeff,

Thanks for this.
This set looks good to me, with a slight question mark wrt behaviour
compatibility with previous versions for 3/4.
(I think it's fine, but I'm a bear of very little brain, and it's been
several years since I reviewed this code, and even longer since I
really interacted with Android.
^
| shameless plug for more EmbeddedPkg reviewer volunteers.)

I've added Jun Nie, who wrote the original version of this code, to
see if he has any comments.

1-2/4 are obviously unproblematic, and I could merge those ahead of
time if preferred. You can add
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
for those if there are any further revisions of the set.

Best Regards,

Leif

On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> Added support for using loadfile2 approach for passing ramdisk to linu= x.
> Created patch series for general error handling improvments based on > review feedback.
> If ACPI tables are in system table or PCD is defined the LoadFile2 met= hod
> of passing initrd will be used.
>
> [v3]
> -Code review cleanup
> -Removed duplicate header file
> -Added change to allow FDT to install if UpdateDtb function is not def= ined
> -Added specific ACPI check
> -Moved install functions to subfunctions
>
> [v2]
> -Added review feedback
> -General improvements to error handling
>
> [v1]
> - Intial revision
>
>
> Jeff Brasen (4):
>   EmbeddedPkg: Remove duplicate libfdt.h include
>   EmbeddedPkg: AndroidBootImgBoot error handling updates
>   EmbeddedPkg: Install FDT if UpdateDtb is not present
>   EmbeddedPkg: Add LoadFile2 for linux initrd
>
>  EmbeddedPkg/EmbeddedPkg.dec      &= nbsp;            |&n= bsp;  1 +
>  .../AndroidBootImgLib/AndroidBootImgLib.inf   | &= nbsp; 4 +
>  .../AndroidBootImgLib/AndroidBootImgLib.c    = ; | 275 +++++++++++++++---
>  3 files changed, 233 insertions(+), 47 deletions(-)
>
> --
> 2.17.1
>
--_000_DM6PR12MB33407EEC50F0C74AF00032F4CBDA9DM6PR12MB3340namp_--