From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.11913.1631292984220814740 for ; Fri, 10 Sep 2021 09:56:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=U+Z1vuZU; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id 1BA7F240027 for ; Fri, 10 Sep 2021 18:56:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1631292981; bh=Qgm03OX2hv1wFmq300bJ4NDAuwea+/J0itjuy1DDRgM=; h=Subject:To:From:Date:From; b=U+Z1vuZU+rU3itvdcUAgwte5F6mYx3htG6ldFhtc5/uTafQMUjDOoN79a91hc3A4I tt5gWVMOJ1I9A3pBrZgNfL6queawktlKXtvIjpXBGbZycKu9YIh7z/sAm22uDh6Ccw cgffJpIuja+od/NFD13l16hhmR7qY1DISVP+9oC9a1VS+d/DAaIWLle9uJ33fhO0+T 8YuQa2Ivhfvrfk74yhB/yZvd9LRKMXNd31xR98qvdOXmC+ZXG01qoLmwcIBVqwi3wP o6m0XZCgTJgkfZN9IhvuLNZ3B+nsclUHAjq0SvMtw6ECGVq7nn/QZRv5Egatgg+2ZC qcxfFXtzNdgqw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4H5hpq6vXLz9rxL; Fri, 10 Sep 2021 18:56:19 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior To: devel@edk2.groups.io, pedro.falcato@gmail.com, Jeff Brasen References: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> Date: Fri, 10 Sep 2021 16:56:19 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB 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/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 *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/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 The device specified by ControllerHandle 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 ControllerHandle and >> RemainingDevicePath is not supported 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 = NULL; >> + BlockIo = NULL; >> >> + // >> + // Open the IO Abstraction(s) needed to perform the supported test >> + // >> Status = 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 = 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 = EFI_UNSUPPORTED; >> + } >> + >> +Exit: >> + // >> + // Close the I/O Abstraction(s) used to perform the supported test >> + // >> + if (DiskIo != NULL) { >> + gBS->CloseProtocol ( >> + ControllerHandle, >> + &gEfiDiskIoProtocolGuid, >> + BindingProtocol->DriverBindingHandle, >> + ControllerHandle >> + ); >> + } >> + if (BlockIo != 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 = >> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED >> // 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 = DiskIo->ReadDisk ( >> + DiskIo, >> + BlockIo->Media->MediaId, >> + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic), >> + sizeof (Magic), >> + &Magic >> + ); >> + if (EFI_ERROR (Status)) { >> + return FALSE; >> + } >> + >> + if (Magic != EXT4_SIGNATURE) { >> + return FALSE; >> + } >> + >> + return TRUE; >> +} >> + >> /** >> Does brief validation of the ext4 superblock. >> >> -- >> 2.17.1 >> >