* [PATCH v2 0/2] ExtPkg Updates
@ 2021-09-10 15:58 Jeff Brasen
2021-09-10 15:58 ` [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Jeff Brasen
2021-09-10 15:58 ` [PATCH v2 2/2] Ext4Pkg: Support uncleanly unmounted filesystems Jeff Brasen
0 siblings, 2 replies; 9+ messages in thread
From: Jeff Brasen @ 2021-09-10 15:58 UTC (permalink / raw)
To: devel; +Cc: pedro.falcato, Jeff Brasen
I have been using the new Ext4Pkg and been pretty successful and it is solving a use case we had.
Had a couple updates to propose
1. Changed the implementation of the binding protocol to both check if the driver is already bound
to the partition as well as added a really quick check to validate the magic value in supported.
This improves performance when you have a large number of non-ext4 partitions on the system.
2. As we are planning on using this for boot support we want to support unclean filesystem states in
case the user doesn't reset cleanly. I added a check if the recovery journal is present and if so treat
the filesystem as read-only (I know the driver is only RO at this point, but figured if you added write
support prior to recovery journal support we would want that). With this everything seems to work great.
I can add this under a FeaturePcd if desired as well.
Change log
v2 - Minor code review comments
v1 - Initial revision
Jeff Brasen (2):
Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Ext4Pkg: Support uncleanly unmounted filesystems
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 46 +++++++++++++++++++--
3 files changed, 102 insertions(+), 16 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 15:58 [PATCH v2 0/2] ExtPkg Updates Jeff Brasen
@ 2021-09-10 15:58 ` Jeff Brasen
2021-09-10 16:52 ` Pedro Falcato
2021-09-10 15:58 ` [PATCH v2 2/2] Ext4Pkg: Support uncleanly unmounted filesystems Jeff Brasen
1 sibling, 1 reply; 9+ messages in thread
From: Jeff Brasen @ 2021-09-10 15:58 UTC (permalink / raw)
To: devel; +Cc: pedro.falcato, Jeff Brasen
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] Ext4Pkg: Support uncleanly unmounted filesystems
2021-09-10 15:58 [PATCH v2 0/2] ExtPkg Updates Jeff Brasen
2021-09-10 15:58 ` [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Jeff Brasen
@ 2021-09-10 15:58 ` Jeff Brasen
1 sibling, 0 replies; 9+ messages in thread
From: Jeff Brasen @ 2021-09-10 15:58 UTC (permalink / raw)
To: devel; +Cc: pedro.falcato, Jeff Brasen
Support for uncleanly mounted filesystems, if there is a recovery
journal mark filesystem as read-only.
Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 1ceb0d5cbb..251babb320 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -18,7 +18,7 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
EXT4_FEATURE_INCOMPAT_64BIT | EXT4_FEATURE_INCOMPAT_DIRDATA |
EXT4_FEATURE_INCOMPAT_FLEX_BG | EXT4_FEATURE_INCOMPAT_FILETYPE |
EXT4_FEATURE_INCOMPAT_EXTENTS | EXT4_FEATURE_INCOMPAT_LARGEDIR |
- EXT4_FEATURE_INCOMPAT_MMP;
+ EXT4_FEATURE_INCOMPAT_MMP | EXT4_FEATURE_INCOMPAT_RECOVER;
// Future features that may be nice additions in the future:
// 1) Btree support: Required for write support and would speed up lookups in large directories.
@@ -89,10 +89,8 @@ Ext4SuperblockValidate (
return FALSE;
}
- // Is this correct behaviour? Imagine the power cuts out, should the system fail to boot because
- // we're scared of touching something corrupt?
if ((Sb->s_state & EXT4_FS_STATE_UNMOUNTED) == 0) {
- return FALSE;
+ DEBUG ((DEBUG_WARN, "[ext4] Filesystem was not unmounted cleanly\n"));
}
return TRUE;
@@ -215,6 +213,11 @@ Ext4OpenSuperblock (
return EFI_UNSUPPORTED;
}
+ if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_RECOVER)) {
+ DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery, mounting read-only\n"));
+ Partition->ReadOnly = TRUE;
+ }
+
// At the time of writing, it's the only supported checksum.
if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM &&
Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C) {
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 15:58 ` [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Jeff Brasen
@ 2021-09-10 16:52 ` Pedro Falcato
2021-09-10 16:56 ` [edk2-devel] " Marvin Häuser
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Falcato @ 2021-09-10 16:52 UTC (permalink / raw)
To: Jeff Brasen; +Cc: edk2-devel-groups-io
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.
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
>
--
Pedro Falcato
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 16:52 ` Pedro Falcato
@ 2021-09-10 16:56 ` Marvin Häuser
2021-09-10 17:05 ` Jeff Brasen
2021-09-10 17:08 ` Pedro Falcato
0 siblings, 2 replies; 9+ messages in thread
From: Marvin Häuser @ 2021-09-10 16:56 UTC (permalink / raw)
To: devel, pedro.falcato, Jeff Brasen
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 16:56 ` [edk2-devel] " Marvin Häuser
@ 2021-09-10 17:05 ` Jeff Brasen
2021-09-10 17:08 ` Pedro Falcato
1 sibling, 0 replies; 9+ messages in thread
From: Jeff Brasen @ 2021-09-10 17:05 UTC (permalink / raw)
To: Marvin Häuser, devel@edk2.groups.io, pedro.falcato@gmail.com
[-- Attachment #1: Type: text/plain, Size: 7975 bytes --]
Yeah we need by_driver on at least one protocol to prevent re-binding to the controller. It will return ALREADY_STARTED if this driver already has it open (from in Start) or ACCESS_DENIED if another driver is holding this.
I can covert both to by_driver if that is desired.
Will make the other little changes as well once I get a desired direction on how to open BlockIo.
Thanks,
Jeff
________________________________
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Friday, September 10, 2021 10:56 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; pedro.falcato@gmail.com <pedro.falcato@gmail.com>; 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 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: 16175 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 16:56 ` [edk2-devel] " Marvin Häuser
2021-09-10 17:05 ` Jeff Brasen
@ 2021-09-10 17:08 ` Pedro Falcato
2021-09-10 18:09 ` Marvin Häuser
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Falcato @ 2021-09-10 17:08 UTC (permalink / raw)
To: Marvin Häuser; +Cc: edk2-devel-groups-io, Jeff Brasen
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.
So, change it to BY_DRIVER. The performance should be the same and
it's more consistent.
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
> >>
> >
>
--
Pedro Falcato
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 17:08 ` Pedro Falcato
@ 2021-09-10 18:09 ` Marvin Häuser
2021-09-10 21:07 ` Jeff Brasen
0 siblings, 1 reply; 9+ messages in thread
From: Marvin Häuser @ 2021-09-10 18:09 UTC (permalink / raw)
To: Pedro Falcato; +Cc: edk2-devel-groups-io, Jeff Brasen
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
>>>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
2021-09-10 18:09 ` Marvin Häuser
@ 2021-09-10 21:07 ` Jeff Brasen
0 siblings, 0 replies; 9+ messages in thread
From: Jeff Brasen @ 2021-09-10 21:07 UTC (permalink / raw)
To: Marvin Häuser, Pedro Falcato; +Cc: edk2-devel-groups-io
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-10 21:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-10 15:58 [PATCH v2 0/2] ExtPkg Updates Jeff Brasen
2021-09-10 15:58 ` [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Jeff Brasen
2021-09-10 16:52 ` Pedro Falcato
2021-09-10 16:56 ` [edk2-devel] " Marvin Häuser
2021-09-10 17:05 ` Jeff Brasen
2021-09-10 17:08 ` Pedro Falcato
2021-09-10 18:09 ` Marvin Häuser
2021-09-10 21:07 ` Jeff Brasen
2021-09-10 15:58 ` [PATCH v2 2/2] Ext4Pkg: Support uncleanly unmounted filesystems Jeff Brasen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox