From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM02-SN1-obe.outbound.protection.outlook.com (NAM02-SN1-obe.outbound.protection.outlook.com [40.107.96.71]) by mx.groups.io with SMTP id smtpd.web09.15180.1631308022633470162 for ; Fri, 10 Sep 2021 14:07:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nvidia.com header.s=selector2 header.b=g92HcOGN; 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.96.71, mailfrom: jbrasen@nvidia.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=aVikT1fw+tXEribFm+JbEw1V0ppTbrQgJHO6fdjdjpD/4HgKV3VsKscHu4tAINo32A9zTugi5uqYSSjPztbdCwEol8gTa2r9phecdB+6yohs/LHHWV7vI+iPeAP0FHusWltTTv3jIJxTKNIX8n2PeBwau7zok7APL9I7xHy7FyCFwuiudhv8SyjjPHl8o8ptOi1Ne1IvXb9fOo1lprGqfRVw+s2l4bwfjUg4Bl5uGw1i0f7v7RJw/l/NH0NNsMMQuwnlTFr9yuPiXRVzuLyqcsIMrHVN2RzG3wZ0GJHJqRc3M2HmFT8vYDcqF/TWQRWVeKCyEc46DidNo7LQXIt0Cg== 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=AIzOHU/I/hVjTpeA3nXSvdbLPzCzXbehoSyEdRn6IW0=; b=Q6K/CJabEhWR9Z/+4PGzw+rITRYUifvvrKpuYuo3kajkIrjuT3jnV2H5VnCBF3FvIlGM7qyVx36kOE7sfudn4KAMqvhO8wkk9nAGrTear8uiBoHER5MUrPF3udZ9cCA21neYcUzSh6UrPWckSD3GB2fZswP6rX5VXYHlAtevpafz99m5OHFusGGCBvSQ+hhEHvk/i9vZq3FxTKBiCqZn7y5EU1xNzOL+EdY7RQwj6u6tmSLaAgE8WBprbN3AyfzUkRq+dyHKg+BUlt+L1prI//IOWOdFawIrwNSz2TTtqDdrFbFgyshthQfn0+dKIF3qEECZC+7dkzsvSaAPdAJD0w== 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=AIzOHU/I/hVjTpeA3nXSvdbLPzCzXbehoSyEdRn6IW0=; b=g92HcOGNLKK5RM+Esv/n0diNu6CY+6tkHn9+B5Gn1CmjcGss38BBb+2HqmCktcsJDOPsk+icI9IOCKQZce2s1TYri4K2Xu3Amw8OGaQ86fKjsGZm1basmfLAYPgVro/ziWjfWZyVxk+5HrGJ1yfITMTzvu4aUTgvg0S6OXghUh2Nr/n5q4cZiJTGUyPdR2Yu921ZHCEZwhoVobG4JPck9PxYG2J1f/P2R/ciqt7YLSDyWFJvl/nRMFhfXq1SOY91vwBBlWl8O56DeGWK9eY5L3dJxXyHoIE8RDD/ATdUVFuq3Dny74f4np8eXbGvYx505O+QNH5jdW1fYLf1ECtc5w== Received: from DM6PR12MB3340.namprd12.prod.outlook.com (2603:10b6:5:3d::24) by DM5PR1201MB0250.namprd12.prod.outlook.com (2603:10b6:4:54::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4500.14; Fri, 10 Sep 2021 21:07:00 +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.017; Fri, 10 Sep 2021 21:07:00 +0000 From: "Jeff Brasen" To: =?iso-8859-1?Q?Marvin_H=E4user?= , Pedro Falcato CC: edk2-devel-groups-io Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Thread-Topic: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Thread-Index: AQHXplzAmzS2XdAeyEeDaa29U/4086ude7iAgAABG4CAAANjAIAAEPCAgAAxFS0= Date: Fri, 10 Sep 2021 21:07:00 +0000 Message-ID: References: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> <22ccf198-457e-f77f-d241-30b2a72c1de7@posteo.de> In-Reply-To: <22ccf198-457e-f77f-d241-30b2a72c1de7@posteo.de> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: b17973ee-338e-6ad8-3bbb-6e05f78b7ec6 authentication-results: posteo.de; dkim=none (message not signed) header.d=none;posteo.de; dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 21400730-0325-4a0f-11c8-08d9749eeeb0 x-ms-traffictypediagnostic: DM5PR1201MB0250: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:4502; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 0JrImcottSGDriJxbYcEk/D7iCC56WgeN2S9wN5X7t0rGccV/h1noyoDxNtMBgx6uQSDw3NUMRvPtVxdVkE6/OQAIjKmHvZYZnmzFYUUMjtRbmSbq8JIBDwXPw8b2tn/aIHD45f88iOyYD39jUW+rcpq+q0EfgCZNZze+1PlaqJBkE3735vTV/Q9jiEWf/DeRXUoddmY7bHGvZ9OIUA2dcU/GviPXTh0Uhw0hRqcot5LnyypmXlg0aDYcbK5PGYDs9FnT6MUun17v//kTLsn3JShdrkUvuFJpRDDNN7rjU5X/z2ztCuVvqz5U759iLRT2+AX/Ic5DzSayt4/x9res/Fjd91ZBpW7VRDWGdXRvzAWm9eHGw1n4iOKlYIl2asqysh6KCS4GhYWd5Yd+lE2NjO4Y6pb3tv4x/O73oxFCv1RMnbSn/zuQLqTv2Mp9YJZpH604LSIDufue7ybuhaKy+ZMX/gTJGaoFnCbRtdT/iYfPPwL78qXTMCsAffviF+yWXPZCowYyTURCS8h7lOgXCGPDWsBbHb8QhYI/BHBYDU4AkhAsx83N9kAVySvVM8wOYWzYHrGc/A18H/vtfsUfuEELT3aaRVYAqiwQEq90BlOUZsbslU2H8X3GcSnvnuD/BM4g5YIO38UDWUaLIoEdS+umZwKzlEjAqu+Ydw7bBk8cFoQCjlFnUaYuIuP9GEHmt93NEPSWj+Vzz4JzhoCGoNzoMQIhNfAguAN4dfK2c8= 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:(6029001)(4636009)(366004)(7696005)(52536014)(4326008)(66556008)(86362001)(19627235002)(38070700005)(6506007)(66476007)(83380400001)(66574015)(71200400001)(55016002)(8936002)(66446008)(8676002)(5660300002)(122000001)(316002)(64756008)(508600001)(110136005)(186003)(76116006)(38100700002)(91956017)(26005)(66946007)(9686003)(53546011)(33656002)(2906002)(19627405001)(32563001);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?C7w1BfI43wb7twV7GLb7A4Sxc5VlzUu9KzXX4uRi48CShfTjLiRwq/gGJM?= =?iso-8859-1?Q?Vu3C9+CQthspeWaNyHGSmnmRSbQD71YUypA9+eGc9NqMImbyG/Qb9qPaO6?= =?iso-8859-1?Q?RcvTKOptp4EJiGkBibISqYDTcaZKhjaC74U8IbEpK8IZL+pglZDyT29FO9?= =?iso-8859-1?Q?68RDQRv+3a45/8w0S5WI24C2iEMJsNOBKS3V09/BE85dmL4pthHMLZV8+w?= =?iso-8859-1?Q?2aQahFL8gG/v4XejjsZUGDQnp0jkwakPHi+NjHX+zjOrPVCzSlMKpLxBdY?= =?iso-8859-1?Q?jX/fIU4mcWwGLdSHsImRJ80B3btD29ep15nx4qK+PkrhSNo77b01kzQKyC?= =?iso-8859-1?Q?gdDtKEejdZ2ZTtrH0BocP9Mqj1Ok4BMvZTmwmm46mR0j25dPl58M46a8ye?= =?iso-8859-1?Q?Gu2unzekAoj4/Wnst8P/3ABGwNjl4I23aAebMPpftdP2WfrQ0bu89pVyfW?= =?iso-8859-1?Q?fM3x7tM0f02RyJYkwC/B4xgEVZ2xpbuh8fEMWp1VOiP+097jMbogyBDA0M?= =?iso-8859-1?Q?kUOtvCwSs8ho3ZngPECTWBAYo6Y5X4RfaUxS/gYAOOeyvC1TFuhHScJKhd?= =?iso-8859-1?Q?/B5dzsfPU5oi8wuLdocJDI17YwT9WgITJOLOijwotallRTfzU+KzfE1qov?= =?iso-8859-1?Q?ot/g9kgbmzDVXZUJnZRNWcC001g7RQJBnv+UYl8FQePwRv8pZVIG9keZ4M?= =?iso-8859-1?Q?cgvbehxF5GZjIPrdXwxWiZ362yxf5Oegtjx4Pc2tZDjEyHlA7Vnu9+Byji?= =?iso-8859-1?Q?aPT/o8o55fU3rd7TXfOli9kiKrWec5fVOnwMAFqoU//qAq62MbU1gzQ2t2?= =?iso-8859-1?Q?IfuY7EIzjf9vQ2ep1UKUurxBuEbtf6UIvyFI+kcZCzSHn550VM1gsyW6gb?= =?iso-8859-1?Q?DPqkKc1PCQRl7zeL3eoZkV7cnoOi+tCXd8HlPAafklcIs6XQY/2suFNuaM?= =?iso-8859-1?Q?riAMRe0r4T5x4I6z6yMjDX+qIiBnIF0If9/95ACa46TExwXQ4DFOFctoSC?= =?iso-8859-1?Q?/gJnOYYbgk4fWnsw9yTwF2zg1zxJbUj0zxddtUqTuemYqQFDIsXfPT+MEa?= =?iso-8859-1?Q?XE1Yux2d79PmgxJV9lXrN51ekSRMm3ZV1E67jnGtxVIKf9ccLEwWxVR2e5?= =?iso-8859-1?Q?x3xhU82nfSuUYCuMPerVT/8ezJUyJ8CUU11aL0t7Bg8h5YvRVXQ7WDUm2R?= =?iso-8859-1?Q?twxUDjqcVo3cp7Jeez4ElNSv8KkQyTlQ53KWwOtvncppwSA7Ea9J3hHpaK?= =?iso-8859-1?Q?CCGJ2EjnwW69mRUKBWEy2FdYjBwUe2O0mJ5OmZqqANm7zVWs79P19HWicD?= =?iso-8859-1?Q?53Qi6Kd1iFh3tLULv2wifyswT2Ma46W/HxMtUvB3ovkOt54=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: 21400730-0325-4a0f-11c8-08d9749eeeb0 X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Sep 2021 21:07:00.6647 (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: ZaVwSA/SpfIohN9IQdAc3ppcl/kTTg+I7sPnS2qTJV6nBDrKGS4h2pf8SELpunixtTe4doSujDLiOANL6dJ6BQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1201MB0250 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_DM6PR12MB334062EEDC24D03159A953BBCBD69DM6PR12MB3340namp_" --_000_DM6PR12MB334062EEDC24D03159A953BBCBD69DM6PR12MB3340namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable We actually can't open the BlockIo protocol as BY_DRIVER as it is already o= wned by the DiskIoDxe driver. Will upload a v3 with the other feedback thou= gh. Thanks, Jeff ________________________________ From: Marvin H=E4user Sent: Friday, September 10, 2021 12:09 PM To: Pedro Falcato Cc: edk2-devel-groups-io ; Jeff Brasen Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupp= orted() behavior External email: Use caution opening links or attachments On 10/09/2021 19:08, Pedro Falcato wrote: > Ah yes, thanks! Although I didn't find the part where they say that > you need to use the same attributes, > after re-reading the spec it seems they recommend using BY_DRIVER. UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute value that was used in Supported()." > So, change it to BY_DRIVER. The performance should be the same and > it's more consistent. Should be better because all handles opened by BY_DRIVER already will immediately fail. Best regards, Marvin > > On Fri, Sep 10, 2021 at 5:56 PM Marvin H=E4user wrot= e: >> On 10/09/2021 18:52, Pedro Falcato wrote: >>> Like Marvin raised in v1, it's a good idea to change the first >>> OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER >>> into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake. >>> Since we're not keeping these protocols open, using BY_DRIVER makes no >>> difference. >> No, the UEFI spec demands Supported() to use the same Attributes as >> Start() (likely for these very performance reasons), actually I thought >> both need BY_DRIVER. >> >> Best regards, >> Marvin >> >>> You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c >>> when you changed it in the header. >>> Just a nitpick, but if you could quickly align the >>> Ext4SuperblockCheckMagic's parameters' descriptions like >>> >>> @param[in] DiskIo Pointer to the DiskIo. >>> @param[in] BlockIo Pointer to the BlockIo. >>> >>> it would be excellent. >>> >>> Apart from that, looks great to me and the patch series will get my RB >>> after that quick change. >>> >>> Best regards, >>> >>> Pedro >>> >>> On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen wrote: >>>> A couple of improvements to improve performance. >>>> Add check to return ACCESS_DENIED if already connected >>>> Add check to verify superblock magic during supported to reduce start = calls >>>> >>>> Signed-off-by: Jeff Brasen >>>> --- >>>> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++ >>>> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++---= --- >>>> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++ >>>> 3 files changed, 95 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext= 4Dxe/Ext4Dxe.h >>>> index 64eab455db..5c60860894 100644 >>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >>>> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName ( >>>> OUT UINTN *VolNameLen >>>> ); >>>> >>>> +/** >>>> + Checks the superblock's magic value. >>>> + >>>> + @param[in] DiskIo Pointer to the DiskIo. >>>> + @param[in] BlockIo Pointer to the BlockIo. >>>> + >>>> + @return TRUE if a valid ext4 superblock, else FALSE. >>>> +**/ >>>> +BOOLEAN >>>> +Ext4SuperblockCheckMagic ( >>>> + IN EFI_DISK_IO_PROTOCOL *DiskIo, >>>> + IN EFI_BLOCK_IO_PROTOCOL *BlockIo >>>> + ); >>>> + >>>> #endif >>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext= 4Dxe/Ext4Dxe.c >>>> index ea2e048d77..5ae93d0484 100644 >>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >>>> @@ -631,7 +631,6 @@ Ext4Unload ( >>>> @retval EFI_ACCESS_DENIED The device specified by Controll= erHandle and >>>> RemainingDevicePath is already b= eing managed by a different >>>> driver or an application that re= quires exclusive access. >>>> - Currently not implemented. >>>> @retval EFI_UNSUPPORTED The device specified by Controll= erHandle and >>>> RemainingDevicePath is not suppo= rted by the driver specified by This. >>>> **/ >>>> @@ -643,32 +642,67 @@ Ext4IsBindingSupported ( >>>> IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL >>>> ) >>>> { >>>> - // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close = the >>>> - // protocol and ignore the output argument entirely >>>> + EFI_STATUS Status; >>>> + EFI_DISK_IO_PROTOCOL *DiskIo; >>>> + EFI_BLOCK_IO_PROTOCOL *BlockIo; >>>> >>>> - EFI_STATUS Status; >>>> + DiskIo =3D NULL; >>>> + BlockIo =3D NULL; >>>> >>>> + // >>>> + // Open the IO Abstraction(s) needed to perform the supported test >>>> + // >>>> Status =3D gBS->OpenProtocol ( >>>> ControllerHandle, >>>> &gEfiDiskIoProtocolGuid, >>>> - NULL, >>>> - BindingProtocol->ImageHandle, >>>> + (VOID **) &DiskIo, >>>> + BindingProtocol->DriverBindingHandle, >>>> ControllerHandle, >>>> - EFI_OPEN_PROTOCOL_TEST_PROTOCOL >>>> + EFI_OPEN_PROTOCOL_BY_DRIVER >>>> ); >>>> >>>> if (EFI_ERROR (Status)) { >>>> - return Status; >>>> + goto Exit; >>>> } >>>> - >>>> + // >>>> + // Open the IO Abstraction(s) needed to perform the supported test >>>> + // >>>> Status =3D gBS->OpenProtocol ( >>>> ControllerHandle, >>>> &gEfiBlockIoProtocolGuid, >>>> - NULL, >>>> - BindingProtocol->ImageHandle, >>>> + (VOID **) &BlockIo, >>>> + BindingProtocol->DriverBindingHandle, >>>> ControllerHandle, >>>> - EFI_OPEN_PROTOCOL_TEST_PROTOCOL >>>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL >>>> ); >>>> + if (EFI_ERROR (Status)) { >>>> + goto Exit; >>>> + } >>>> + >>>> + if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) { >>>> + Status =3D EFI_UNSUPPORTED; >>>> + } >>>> + >>>> +Exit: >>>> + // >>>> + // Close the I/O Abstraction(s) used to perform the supported test >>>> + // >>>> + if (DiskIo !=3D NULL) { >>>> + gBS->CloseProtocol ( >>>> + ControllerHandle, >>>> + &gEfiDiskIoProtocolGuid, >>>> + BindingProtocol->DriverBindingHandle, >>>> + ControllerHandle >>>> + ); >>>> + } >>>> + if (BlockIo !=3D NULL) { >>>> + gBS->CloseProtocol ( >>>> + ControllerHandle, >>>> + &gEfiBlockIoProtocolGuid, >>>> + BindingProtocol->DriverBindingHandle, >>>> + ControllerHandle >>>> + ); >>>> + } >>>> return Status; >>>> } >>>> >>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/= Ext4Dxe/Superblock.c >>>> index c321d8c3d8..1ceb0d5cbb 100644 >>>> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c >>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c >>>> @@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =3D >>>> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORR= UPTED >>>> // references and add some Ext4SignalCorruption function + function= call. >>>> >>>> +/** >>>> + Checks only superblock magic value. >>>> + >>>> + @param[in] DiskIo Pointer to the DiskIo. >>>> + @param[in] BlockIo Pointer to the BlockIo. >>>> + >>>> + @return TRUE if a valid ext4 superblock, else FALSE. >>>> +**/ >>>> +BOOLEAN >>>> +Ext4SuperblockCheckMagic ( >>>> + IN EFI_DISK_IO_PROTOCOL *DiskIo, >>>> + IN EFI_BLOCK_IO_PROTOCOL *BlockIo >>>> + ) >>>> +{ >>>> + UINT16 Magic; >>>> + EFI_STATUS Status; >>>> + >>>> + Status =3D DiskIo->ReadDisk ( >>>> + DiskIo, >>>> + BlockIo->Media->MediaId, >>>> + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBL= OCK, s_magic), >>>> + sizeof (Magic), >>>> + &Magic >>>> + ); >>>> + if (EFI_ERROR (Status)) { >>>> + return FALSE; >>>> + } >>>> + >>>> + if (Magic !=3D EXT4_SIGNATURE) { >>>> + return FALSE; >>>> + } >>>> + >>>> + return TRUE; >>>> +} >>>> + >>>> /** >>>> Does brief validation of the ext4 superblock. >>>> >>>> -- >>>> 2.17.1 >>>> > --_000_DM6PR12MB334062EEDC24D03159A953BBCBD69DM6PR12MB3340namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
We actually can't open the BlockIo protocol as BY_DRIVER as it is already o= wned by the DiskIoDxe driver. Will upload a v3 with the other feedback thou= gh.

Thanks,

Jeff

From: Marvin H=E4user <m= haeuser@posteo.de>
Sent: Friday, September 10, 2021 12:09 PM
To: Pedro Falcato <pedro.falcato@gmail.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Jeff Brasen &= lt;jbrasen@nvidia.com>
Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBind= ingSupported() behavior
 
External email: Use caution opening links or attac= hments


On 10/09/2021 19:08, Pedro Falcato wrote:
> Ah yes, thanks! Although I didn't find the part where they say that > you need to use the same attributes,
> after re-reading the spec it seems they recommend using BY_DRIVER.

UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same A= ttribute
value that was used in Supported()."

> So, change it to BY_DRIVER. The performance should be the same and
> it's more consistent.

Should be better because all handles opened by BY_DRIVER already will
immediately fail.

Best regards,
Marvin

>
> On Fri, Sep 10, 2021 at 5:56 PM Marvin H=E4user <mhaeuser@posteo.de= > wrote:
>> On 10/09/2021 18:52, Pedro Falcato wrote:
>>> Like Marvin raised in v1, it's a good idea to change the first=
>>> OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
>>> into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake. >>> Since we're not keeping these protocols open, using BY_DRIVER = makes no
>>> difference.
>> No, the UEFI spec demands Supported() to use the same Attributes a= s
>> Start() (likely for these very performance reasons), actually I th= ought
>> both need BY_DRIVER.
>>
>> Best regards,
>> Marvin
>>
>>> You didn't update Ext4SuperblockCheckMagic's comment in Superb= lock.c
>>> when you changed it in the header.
>>> Just a nitpick, but if you could quickly align the
>>> Ext4SuperblockCheckMagic's parameters' descriptions like
>>>
>>>      @param[in] DiskIo  &nb= sp;   Pointer to the DiskIo.
>>>      @param[in] BlockIo  &n= bsp;  Pointer to the BlockIo.
>>>
>>> it would be excellent.
>>>
>>> Apart from that, looks great to me and the patch series will g= et my RB
>>> after that quick change.
>>>
>>> Best regards,
>>>
>>> Pedro
>>>
>>> On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@nvidia= .com> wrote:
>>>> A couple of improvements to improve performance.
>>>> Add check to return ACCESS_DENIED if already connected
>>>> Add check to verify superblock magic during supported to r= educe start calls
>>>>
>>>> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
>>>> ---
>>>>    Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h =    | 14 +++++++
>>>>    Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c =    | 58 +++++++++++++++++++++------
>>>>    Features/Ext4Pkg/Ext4Dxe/Superblock.c | = 35 ++++++++++++++++
>>>>    3 files changed, 95 insertions(+), 12 de= letions(-)
>>>>
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features= /Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>>>> index 64eab455db..5c60860894 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>>>> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>>>>      OUT UINTN   &= nbsp;      *VolNameLen
>>>>      );
>>>>
>>>> +/**
>>>> +   Checks the superblock's magic value.
>>>> +
>>>> +   @param[in] DiskIo     Po= inter to the DiskIo.
>>>> +   @param[in] BlockIo     P= ointer to the BlockIo.
>>>> +
>>>> +   @return TRUE if a valid ext4 superblock, els= e FALSE.
>>>> +**/
>>>> +BOOLEAN
>>>> +Ext4SuperblockCheckMagic (
>>>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>>>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>>>> +  );
>>>> +
>>>>    #endif
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features= /Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>>>> index ea2e048d77..5ae93d0484 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>>>> @@ -631,7 +631,6 @@ Ext4Unload (
>>>>      @retval EFI_ACCESS_DENIED&nb= sp;       The device specified by ControllerH= andle and
>>>>          = ;            &n= bsp;            = ;    RemainingDevicePath is already being managed by a diffe= rent
>>>>          = ;            &n= bsp;            = ;    driver or an application that requires exclusive access= .
>>>> -         &nb= sp;            =              Cu= rrently not implemented.
>>>>      @retval EFI_UNSUPPORTED = ;         The device specified by C= ontrollerHandle and
>>>>          = ;            &n= bsp;            = ;    RemainingDevicePath is not supported by the driver spec= ified by This.
>>>>    **/
>>>> @@ -643,32 +642,67 @@ Ext4IsBindingSupported (
>>>>      IN EFI_DEVICE_PATH *Remainin= gDevicePath  OPTIONAL
>>>>      )
>>>>    {
>>>> -  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL l= ets us not close the
>>>> -  // protocol and ignore the output argument entirel= y
>>>> +  EFI_STATUS      &nbs= p;     Status;
>>>> +  EFI_DISK_IO_PROTOCOL  *DiskIo;
>>>> +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
>>>>
>>>> -  EFI_STATUS  Status;
>>>> +  DiskIo =3D NULL;
>>>> +  BlockIo =3D NULL;
>>>>
>>>> +  //
>>>> +  // Open the IO Abstraction(s) needed to perform th= e supported test
>>>> +  //
>>>>      Status =3D gBS->OpenProto= col (
>>>>          = ;            Control= lerHandle,
>>>>          = ;            &gE= fiDiskIoProtocolGuid,
>>>> -         &nb= sp;        NULL,
>>>> -         &nb= sp;        BindingProtocol->ImageHand= le,
>>>> +         &nb= sp;        (VOID **) &DiskIo,
>>>> +         &nb= sp;        BindingProtocol->DriverBin= dingHandle,
>>>>          = ;            Control= lerHandle,
>>>> -         &nb= sp;        EFI_OPEN_PROTOCOL_TEST_PROTOC= OL
>>>> +         &nb= sp;        EFI_OPEN_PROTOCOL_BY_DRIVER >>>>          = ;            );
>>>>
>>>>      if (EFI_ERROR (Status)) { >>>> -    return Status;
>>>> +    goto Exit;
>>>>      }
>>>> -
>>>> +  //
>>>> +  // Open the IO Abstraction(s) needed to perform th= e supported test
>>>> +  //
>>>>      Status =3D gBS->OpenProto= col (
>>>>          = ;            Control= lerHandle,
>>>>          = ;            &gE= fiBlockIoProtocolGuid,
>>>> -         &nb= sp;        NULL,
>>>> -         &nb= sp;        BindingProtocol->ImageHand= le,
>>>> +         &nb= sp;        (VOID **) &BlockIo,
>>>> +         &nb= sp;        BindingProtocol->DriverBin= dingHandle,
>>>>          = ;            Control= lerHandle,
>>>> -         &nb= sp;        EFI_OPEN_PROTOCOL_TEST_PROTOC= OL
>>>> +         &nb= sp;        EFI_OPEN_PROTOCOL_GET_PROTOCO= L
>>>>          = ;            );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    goto Exit;
>>>> +  }
>>>> +
>>>> +  if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {=
>>>> +    Status =3D EFI_UNSUPPORTED;
>>>> +  }
>>>> +
>>>> +Exit:
>>>> +  //
>>>> +  // Close the I/O Abstraction(s) used to perform th= e supported test
>>>> +  //
>>>> +  if (DiskIo !=3D NULL) {
>>>> +    gBS->CloseProtocol (
>>>> +          Co= ntrollerHandle,
>>>> +          &a= mp;gEfiDiskIoProtocolGuid,
>>>> +          Bi= ndingProtocol->DriverBindingHandle,
>>>> +          Co= ntrollerHandle
>>>> +          );=
>>>> +  }
>>>> +  if (BlockIo !=3D NULL) {
>>>> +    gBS->CloseProtocol (
>>>> +          Co= ntrollerHandle,
>>>> +          &a= mp;gEfiBlockIoProtocolGuid,
>>>> +          Bi= ndingProtocol->DriverBindingHandle,
>>>> +          Co= ntrollerHandle
>>>> +          );=
>>>> +  }
>>>>      return Status;
>>>>    }
>>>>
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Featu= res/Ext4Pkg/Ext4Dxe/Superblock.c
>>>> index c321d8c3d8..1ceb0d5cbb 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>>>> @@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedInc= ompatFeat =3D
>>>>    // this is desired, it's fairly trivial = to look for EFI_VOLUME_CORRUPTED
>>>>    // references and add some Ext4SignalCor= ruption function + function call.
>>>>
>>>> +/**
>>>> +   Checks only superblock magic value.
>>>> +
>>>> +   @param[in] DiskIo     Po= inter to the DiskIo.
>>>> +   @param[in] BlockIo     P= ointer to the BlockIo.
>>>> +
>>>> +   @return TRUE if a valid ext4 superblock, els= e FALSE.
>>>> +**/
>>>> +BOOLEAN
>>>> +Ext4SuperblockCheckMagic (
>>>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>>>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>>>> +  )
>>>> +{
>>>> +  UINT16      Magic;
>>>> +  EFI_STATUS  Status;
>>>> +
>>>> +  Status =3D DiskIo->ReadDisk (
>>>> +         &nb= sp;           DiskIo,
>>>> +         &nb= sp;           BlockIo->= ;Media->MediaId,
>>>> +         &nb= sp;           EXT4_SUPERB= LOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
>>>> +         &nb= sp;           sizeof (Mag= ic),
>>>> +         &nb= sp;           &Magic<= br> >>>> +         &nb= sp;           );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return FALSE;
>>>> +  }
>>>> +
>>>> +  if (Magic !=3D EXT4_SIGNATURE) {
>>>> +    return FALSE;
>>>> +  }
>>>> +
>>>> +  return TRUE;
>>>> +}
>>>> +
>>>>    /**
>>>>       Does brief validation = of the ext4 superblock.
>>>>
>>>> --
>>>> 2.17.1
>>>>
>

--_000_DM6PR12MB334062EEDC24D03159A953BBCBD69DM6PR12MB3340namp_--