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

["Jeff Brasen" <jbrasen@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 8825 bytes --]

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 <mhaeuser@posteo.de>
Sent: Friday, September 10, 2021 12:09 PM
To: Pedro Falcato <pedro.falcato@gmail.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Jeff Brasen <jbrasen@nvidia.com>
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 <mhaeuser@posteo.de> 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 <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
>>>>
>


[-- Attachment #2: Type: text/html, Size: 18109 bytes --]