From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web08.13000.1631297347789297600 for ; Fri, 10 Sep 2021 11:09:08 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=cX41xhwL; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 1EC6E240108 for ; Fri, 10 Sep 2021 20:09:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1631297345; bh=DB3o5SsvZOrR7mSj7JUtbJnB4TM9WyXnMPkP550xpbY=; h=Subject:To:Cc:From:Date:From; b=cX41xhwL7KJdx49a9xT4i/gkyyDo0CUKDBq/ZfYCiQYU2thBxImRdzOWAjhJaCQ4V fuoc9HdFsWL5VXtx4RYz9mzTMyk4TKiccPwk8mI2ERY8s7k5kRryy7UaXgsBEaq1c2 KSTExzsToElh1unzlOgAwpZzNEcZ4mMaY+uUdGPOhfVeQeNX/dc54sxMi5o3Fwk0Ei 1Hm9VZ2wCIn9A1LswzViebfffqzNBPvIlrpXYVGfjSN29M1XxXQ8djgWzrMaAmJT0O xD5ox7Txabt44MYdUQMhCTSPD+ZwchUAge/lcqdZbseEcVW1Qi++W8BB+rcu5GMIl5 GE2kAitIfFOxw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4H5kQm1mBjz6tmJ; Fri, 10 Sep 2021 20:09:04 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior To: Pedro Falcato Cc: edk2-devel-groups-io , Jeff Brasen References: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <22ccf198-457e-f77f-d241-30b2a72c1de7@posteo.de> Date: Fri, 10 Sep 2021 18:09:03 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable 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=20 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=20 immediately fail. Best regards, Marvin > > On Fri, Sep 10, 2021 at 5:56 PM Marvin H=C3=A4user = 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 n= o >>> difference. >> No, the UEFI spec demands Supported() to use the same Attributes as >> Start() (likely for these very performance reasons), actually I though= t >> 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 R= B >>> after that quick change. >>> >>> Best regards, >>> >>> Pedro >>> >>> On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen wrot= e: >>>> 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 star= t 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/E= xt4Dxe/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/E= xt4Dxe/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 Contro= llerHandle and >>>> RemainingDevicePath is already= being managed by a different >>>> driver or an application that = requires exclusive access. >>>> - Currently not implemented. >>>> @retval EFI_UNSUPPORTED The device specified by Contro= llerHandle and >>>> RemainingDevicePath is not sup= ported 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 clos= e 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 tes= t >>>> + // >>>> 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 tes= t >>>> + // >>>> 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 tes= t >>>> + // >>>> + 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/Ext4Pk= g/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_CO= RRUPTED >>>> // references and add some Ext4SignalCorruption function + functi= on 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_SUPER= BLOCK, 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 >>>> >