[PATCH 0/2] ExtPkg Updates

[PATCH 0/2] ExtPkg Updates

[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

v1 - Initial revision

Jeff Brasen (2):
  Ext4Pkg: Improve Binding support behavior
  Ext4Pkg: Support non-cleanlty unmounted filesystems

 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 45 +++++++++++++++++++--
 3 files changed, 99 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 1/2] Ext4Pkg: Improve Binding support behavior

[Jeff Brasen]

A couple improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to deduce start calls

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++
 3 files changed, 92 insertions(+), 13 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..9a3938e671 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
   OUT UINTN          *VolNameLen
   );
 
+/**
+   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
+  );
+
 #endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..cb1e6d532a 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,64 @@ 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_STATUS            Status;
+  EFI_DISK_IO_PROTOCOL  *DiskIo = NULL;
+  EFI_BLOCK_IO_PROTOCOL *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..1f8cdd3705 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,40 @@ 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

[PATCH 2/2] Ext4Pkg: Support non-cleanlty unmounted filesystems

[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 1f8cdd3705..444dd3ca97 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.
@@ -88,10 +88,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;
@@ -214,6 +212,11 @@ Ext4OpenSuperblock (
     return EFI_UNSUPPORTED;
   }
 
+  if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_RECOVER) == EXT4_FEATURE_INCOMPAT_RECOVER) {
+    DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery mount 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

Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior

[Pedro Falcato]

Hi Jeff,

Comments below.

The patch itself looks good.

Nitpick on the commit message: I'd reword the commit message to
something like "Improve Ext4IsBindingSupported() behavior"
or "Improve IsBindingSupported behavior", since this patch doesn't
change Ext4Bind(), which does the actual bind.

On Thu, Sep 9, 2021 at 9:41 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> A couple improvements to improve performance.
> Add check to return ACCESS_DENIED if already connected
> Add check to verify superblock magic during supported to deduce start calls
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++
>  3 files changed, 92 insertions(+), 13 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index 64eab455db..9a3938e671 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>    OUT UINTN          *VolNameLen
>    );
>
> +/**
> +   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
> +  );

I'd reword the first part of the comment into something like "Checks
the superblock's magic value.".
Aligning the two "Pointer to the ...Io" would be neat :)
Types and parameter names need to be separated by at least two
columns. So, something like:

 IN EFI_DISK_IO_PROTOCOL   *DiskIo,
 IN EFI_BLOCK_IO_PROTOCOL  *BlockIo

The feedback above also applies to the function's definition below, in
Superblock.c.

> +
>  #endif
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> index ea2e048d77..cb1e6d532a 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,64 @@ 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_STATUS            Status;
> +  EFI_DISK_IO_PROTOCOL  *DiskIo = NULL;
> +  EFI_BLOCK_IO_PROTOCOL *BlockIo = NULL;
>
Initialization of variables are always separate from the declaration. Example:
EFI_DISK_IO_PROTOCOL  *DiskIo;
...

DiskIo = 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..1f8cdd3705 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -34,6 +34,40 @@ 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
> +  )

Same as above.

> +{
> +  UINT16      Magic;
> +  EFI_STATUS Status;
> +
> +  Status = DiskIo->ReadDisk (DiskIo,
> +                             BlockIo->Media->MediaId,
> +                             EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
> +                             sizeof (Magic),
> +                             &Magic
> +                             );
Splitting a function call in multiple lines should be done like in
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name.

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

Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior

[Jeff Brasen]

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

Sounds good will update that in a v2 patch tomorrow

Get Outlook for Android<https://aka.ms/ghei36>
________________________________
From: Pedro Falcato <pedro.falcato@gmail.com>
Sent: Thursday, September 9, 2021 10:22:51 PM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior

External email: Use caution opening links or attachments


Hi Jeff,

Comments below.

The patch itself looks good.

Nitpick on the commit message: I'd reword the commit message to
something like "Improve Ext4IsBindingSupported() behavior"
or "Improve IsBindingSupported behavior", since this patch doesn't
change Ext4Bind(), which does the actual bind.

On Thu, Sep 9, 2021 at 9:41 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> A couple improvements to improve performance.
> Add check to return ACCESS_DENIED if already connected
> Add check to verify superblock magic during supported to deduce start calls
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++
>  3 files changed, 92 insertions(+), 13 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index 64eab455db..9a3938e671 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>    OUT UINTN          *VolNameLen
>    );
>
> +/**
> +   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
> +  );

I'd reword the first part of the comment into something like "Checks
the superblock's magic value.".
Aligning the two "Pointer to the ...Io" would be neat :)
Types and parameter names need to be separated by at least two
columns. So, something like:

 IN EFI_DISK_IO_PROTOCOL   *DiskIo,
 IN EFI_BLOCK_IO_PROTOCOL  *BlockIo

The feedback above also applies to the function's definition below, in
Superblock.c.

> +
>  #endif
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> index ea2e048d77..cb1e6d532a 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,64 @@ 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_STATUS            Status;
> +  EFI_DISK_IO_PROTOCOL  *DiskIo = NULL;
> +  EFI_BLOCK_IO_PROTOCOL *BlockIo = NULL;
>
Initialization of variables are always separate from the declaration. Example:
EFI_DISK_IO_PROTOCOL  *DiskIo;
...

DiskIo = 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..1f8cdd3705 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -34,6 +34,40 @@ 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
> +  )

Same as above.

> +{
> +  UINT16      Magic;
> +  EFI_STATUS Status;
> +
> +  Status = DiskIo->ReadDisk (DiskIo,
> +                             BlockIo->Media->MediaId,
> +                             EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
> +                             sizeof (Magic),
> +                             &Magic
> +                             );
Splitting a function call in multiple lines should be done like in
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2-docs.gitbook.io%2Fedk-ii-c-coding-standards-specification%2F5_source_files%2F52_spacing%235-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name&amp;data=04%7C01%7Cjbrasen%40nvidia.com%7C6e1a40882279499fbb0008d97412af02%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637668446481896854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=WZjbNs2Q3eU6QH%2F1RZbxakJnhVYOE4%2Bt%2BWF68JragVg%3D&amp;reserved=0.

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

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

Re: [PATCH 2/2] Ext4Pkg: Support non-cleanlty unmounted filesystems

[Pedro Falcato]

Comments below.

The patch itself also looks good.

Commit message issue: typo on "non-cleanlty".

On Thu, Sep 9, 2021 at 9:41 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> 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 1f8cdd3705..444dd3ca97 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.
> @@ -88,10 +88,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"));
Nitpick: filesystem should be capitalized.
>    }
>
>    return TRUE;
> @@ -214,6 +212,11 @@ Ext4OpenSuperblock (
>      return EFI_UNSUPPORTED;
>    }
>
> +  if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_RECOVER) == EXT4_FEATURE_INCOMPAT_RECOVER) {

New code that wants to test for features/feature-sets should use
EXT4_HAS_INCOMPAT, EXT4_HAS_COMPAT, EXT4_HAS_RO_COMPAT.
The code in this function that's testing for features using i.e:
FeaturesIncompat & FEATURE manually should likely be replaced by the
macros.

> +    DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery mount read-only\n"));
The debug message looks poorly worded; something like "[ext4] Needs
journal recovery, mounting read-only\n" sounds good?
> +    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
>


-- 
Pedro Falcato

Re: [PATCH 0/2] ExtPkg Updates

[Pedro Falcato]

Hi Jeff,

Thanks for the patches! It's great that Ext4Pkg is already getting used!

I've looked at each patch and replied with feedback.
In general, they look good, although there are some minor issues. I
hadn't thought of the 1st patch but the 2nd seemed inevitable
for real world systems that may not unmount cleanly.

Best regards,

Pedro


On Thu, Sep 9, 2021 at 9:41 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> 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
>
> v1 - Initial revision
>
> Jeff Brasen (2):
>   Ext4Pkg: Improve Binding support behavior
>   Ext4Pkg: Support non-cleanlty unmounted filesystems
>
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 45 +++++++++++++++++++--
>  3 files changed, 99 insertions(+), 17 deletions(-)
>
> --
> 2.17.1
>


-- 
Pedro Falcato

Re: [edk2-devel] [PATCH 1/2] Ext4Pkg: Improve Binding support behavior

[Marvin Häuser]

Good day,

On 09/09/2021 22:41, Jeff Brasen via groups.io wrote:
> A couple improvements to improve performance.
> Add check to return ACCESS_DENIED if already connected

This "performance improvement" actually aligns the load behaviour with 
the UEFI spec, maybe it should be mentioned?

> Add check to verify superblock magic during supported to deduce start calls
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
>   Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++
>   3 files changed, 92 insertions(+), 13 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index 64eab455db..9a3938e671 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>     OUT UINTN          *VolNameLen
>     );
>   
> +/**
> +   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
> +  );
> +
>   #endif
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> index ea2e048d77..cb1e6d532a 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,64 @@ 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_STATUS            Status;
> +  EFI_DISK_IO_PROTOCOL  *DiskIo = NULL;
> +  EFI_BLOCK_IO_PROTOCOL *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;

This change is not required as nothing was opened on failure; see below.

>     }
> -
> +  //
> +  // 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

Why do the open modes mismatch if both protocols are equally stored in 
the private partition data? Is the owner of Block I/O a different one 
than the one of Disk I/O?

>                     );
> +  if (EFI_ERROR (Status)) {
> +    goto Exit;
> +  }
> +
> +  if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
> +    Status = EFI_UNSUPPORTED;
> +  }

This can easily be rewritten to not require "goto":

   if (!EFI_ERROR (Status) && !Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
     Status = EFI_UNSUPPORTED;
   }

With the change above this allows to drop the label and any "goto" code.

> +
> +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
> +          );
> +  }

Protocols retrieved by GET_PROTOCOL are not closed.

>     return Status;
>   }
>   
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> index c321d8c3d8..1f8cdd3705 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -34,6 +34,40 @@ 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.

Whether it "only" checks the magic is more of a functional detail than 
an actual description.

> +
> +   @param[in] DiskIo     Pointer to the DiskIo.
> +   @param[in] BlockIo     Pointer to the BlockIo.
> +
> +   @return TRUE if a valid ext4 superblock, else FALSE.

Maybe use "@returns  Whether the partition has a valid EXT4 superblock 
magic" for readability?

Best regards,
Marvin

> +**/
> +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.
>