Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

["Marvin Häuser" <mhaeuser@posteo.de>]

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
>>
>