External email: Use caution opening links or attachments
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 <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 reduce 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 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
>>
>