We actually can't open the BlockIo protocol as BY_DRIVER as it is already owned by the DiskIoDxe driver. Will upload a v3 with the other feedback though. Thanks, Jeff ________________________________ From: Marvin Häuser 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 Ext4IsBindingSupported() 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äuser 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 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 >>>> >