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