Re: [Patch 0/3] Ext4Pkg: Add Ext4Pkg

["Pedro Falcato" <pedro.falcato@gmail.com>]

Hi Mike,

Thanks for the helpful pointers. I'll consider everything for V2,
which I'll submit as soon as possible (hopefully tomorrow).

RE: Code style. I'll re-run ECC and try to solve the issues. One thing
though: Is it possible to make an exception for the naming of
ext4-specific struct members?
Example: Members' names like "bg_block_bitmap_lo" in
EXT4_BLOCK_GROUP_DESC. I'd like to make a case for it; from my
experience with my own hobby project's ext2 driver, having names
similar to what's used in the documentation/other source code is
incredibly helpful when trying to work on the code; with the original
docs' names, which are admittedly not compliant with the EDK2 coding
style, it really makes everything much clearer when using other code
or documentation as reference. Of course, if it's not possible I'll
rename them all.

Thanks,

Pedro


On Thu, 5 Aug 2021 at 19:33, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Pedro,
>
> 1) Ext4Pkg/Ext4Dxe/Ext4Dxe.inf:
>
>   * To be consistent with other drivers, BASE_NAME should be changed from Ext4 to Ext4Dxe.
>   * For proper dependency checking in incremental builds, please add the .h files to the [Sources] section
>
>       Ext4Disk.h
>       Ext4Dxe.h
>
> 2) There are a number of code style issues that need to be addressed.  Can you fix those for V2?
>
> 3) I did a quick pass to find the IA32 NOOPT VS2019 issues.  With the following changes, I can get it to build.  Do not know if I introduced any functional changes by mistake.
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
> index 10a82d40a0..f2db93f02c 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
> @@ -61,7 +61,7 @@ Ext4ReadInode (
>               Partition,
>               Inode,
>               Partition->InodeSize,
> -             Ext4BlockToByteOffset (Partition, InodeTableStart) + InodeOffset * Partition->InodeSize
> +             Ext4BlockToByteOffset (Partition, InodeTableStart) + MultU64x32 (InodeOffset, Partition->InodeSize)
>               );
>
>    if (EFI_ERROR (Status)) {
> diff --git a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
> index 1cafdd64cd..65109809c0 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
> @@ -45,7 +45,7 @@ Ext4ReadBlocks (
>    IN EXT4_BLOCK_NR BlockNumber
>    )
>  {
> -  return Ext4ReadDiskIo (Partition, Buffer, NumberBlocks * Partition->BlockSize, BlockNumber * Partition->BlockSize);
> +  return Ext4ReadDiskIo (Partition, Buffer, NumberBlocks * Partition->BlockSize, MultU64x32 (BlockNumber, Partition->BlockSize));
>  }
>
>  /**
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index d790e70be1..8aa584df14 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -445,6 +445,6 @@ typedef struct {
>  typedef UINT64  EXT4_BLOCK_NR;
>  typedef UINT32  EXT4_INO_NR;
>
> -#define EXT4_INODE_SIZE(ino)  (((UINT64)ino->i_size_hi << 32) | ino->i_size_lo)
> +#define EXT4_INODE_SIZE(ino)  (LShiftU64 (ino->i_size_hi, 32) | ino->i_size_lo)
>
>  #endif
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index f6875c919e..a055a139e1 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -244,7 +244,7 @@ Ext4MakeBlockNumberFromHalfs (
>    )
>  {
>    // High might have garbage if it's not a 64 bit filesystem
> -  return Ext4Is64Bit (Partition) ? Low | ((UINT64)High << 32) : Low;
> +  return Ext4Is64Bit (Partition) ? (Low | LShiftU64 (High, 32)) : Low;
>  }
>
>  /**
> @@ -297,7 +297,7 @@ Ext4BlockToByteOffset (
>    IN EXT4_BLOCK_NR Block
>    )
>  {
> -  return Partition->BlockSize * Block;
> +  return MultU64x32 (Block, Partition->BlockSize);
>  }
>
>  /**
> @@ -333,7 +333,7 @@ Ext4InodeSize (
>    CONST EXT4_INODE *Inode
>    )
>  {
> -  return ((UINT64)Inode->i_size_hi << 32) | Inode->i_size_lo;
> +  return (LShiftU64 (Inode->i_size_hi, 32) | Inode->i_size_lo);
>  }
>
>  /**
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
> index 102b12d613..fc0185285e 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
> @@ -111,6 +111,8 @@ [Sources]
>    Collation.c
>    Crc32c.c
>    Crc16.c
> +  Ext4Disk.h
> +  Ext4Dxe.h
>
>  [Packages]
>    MdePkg/MdePkg.dec
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> index db4bf5aa3f..8c9b4a4c75 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> @@ -210,7 +210,7 @@ Ext4ExtentIdxLeafBlock (
>    IN EXT4_EXTENT_INDEX *Index
>    )
>  {
> -  return ((UINT64)Index->ei_leaf_hi << 32) | Index->ei_leaf_lo;
> +  return LShiftU64(Index->ei_leaf_hi, 32) | Index->ei_leaf_lo;
>  }
>
>  STATIC UINTN  GetExtentRequests  = 0;
> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
> index 10dda64b16..71d36d1990 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -487,8 +487,8 @@ Ext4GetFilesystemInfo (
>    Info->BlockSize = Part->BlockSize;
>    Info->Size       = NeededLength;
>    Info->ReadOnly   = Part->ReadOnly;
> -  Info->VolumeSize = TotalBlocks * Part->BlockSize;
> -  Info->FreeSpace  = FreeBlocks * Part->BlockSize;
> +  Info->VolumeSize = MultU64x32 (TotalBlocks, Part->BlockSize);
> +  Info->FreeSpace  = MultU64x32 (FreeBlocks, Part->BlockSize);
>
>    if (VolumeName != NULL) {
>      StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 304bf0c4a9..2a9f534d7e 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -154,7 +154,7 @@ Ext4Read (
>        UINT64  ExtentOffset;
>        UINTN   ExtentMayRead;
>
> -      ExtentStartBytes   = (((UINT64)Extent.ee_start_hi << 32) | Extent.ee_start_lo) * Partition->BlockSize;
> +      ExtentStartBytes   = MultU64x32 (LShiftU64 (Extent.ee_start_hi, 32) | Extent.ee_start_lo, Partition->BlockSize);
>        ExtentLengthBytes  = Extent.ee_len * Partition->BlockSize;
>        ExtentLogicalBytes = (UINT64)Extent.ee_block * Partition->BlockSize;
>        ExtentOffset  = CurrentSeek - ExtentLogicalBytes;
> @@ -276,17 +276,17 @@ Ext4FilePhysicalSpace (
>    Blocks   = File->Inode->i_blocks;
>
>    if(HugeFile) {
> -    Blocks |= ((UINT64)File->Inode->i_osd2.data_linux.l_i_blocks_high) << 32;
> +    Blocks |= LShiftU64 (File->Inode->i_osd2.data_linux.l_i_blocks_high, 32);
>
>      // If HUGE_FILE is enabled and EXT4_HUGE_FILE_FL is set in the inode's flags, each unit
>      // in i_blocks corresponds to an actual filesystem block
>      if(File->Inode->i_flags & EXT4_HUGE_FILE_FL) {
> -      return Blocks * File->Partition->BlockSize;
> +      return MultU64x32 (Blocks, File->Partition->BlockSize);
>      }
>    }
>
>    // Else, each i_blocks unit corresponds to 512 bytes
> -  return Blocks * 512;
> +  return MultU64x32 (Blocks, 512);
>  }
>
>  // Copied from EmbeddedPkg at my mentor's request.
> @@ -368,7 +368,7 @@ EpochToEfiTime (
>      UINT32      Nanoseconds  = 0;                                \
>                                                             \
>      if (Ext4InodeHasField (Inode, Field ## _extra)) {          \
> -      SecondsEpoch |= ((UINT64)(Inode->Field ## _extra & EXT4_EXTRA_TIMESTAMP_MASK)) << 32; \
> +      SecondsEpoch |= LShiftU64 ((UINT64)(Inode->Field ## _extra & EXT4_EXTRA_TIMESTAMP_MASK), 32); \
>        Nanoseconds   = Inode->Field ## _extra >> 2;                                            \
>      }                                                                                       \
>      EpochToEfiTime ((UINTN)SecondsEpoch, Time);                                                     \
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> index 18d8295a1f..88d01b62a8 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -161,7 +161,7 @@ Ext4OpenSuperblock (
>
>    DEBUG ((EFI_D_INFO, "Read only = %u\n", Partition->ReadOnly));
>
> -  Partition->BlockSize = 1024 << Sb->s_log_block_size;
> +  Partition->BlockSize = (UINT32)LShiftU64 (1024, Sb->s_log_block_size);
>
>    // The size of a block group can also be calculated as 8 * Partition->BlockSize
>    if(Sb->s_blocks_per_group != 8 * Partition->BlockSize) {
> @@ -195,7 +195,7 @@ Ext4OpenSuperblock (
>    }
>
>    NrBlocks = (UINTN)DivU64x32Remainder (
> -                              Partition->NumberBlockGroups * Partition->DescSize,
> +                              MultU64x32 (Partition->NumberBlockGroups, Partition->DescSize),
>                                Partition->BlockSize,
>                                &NrBlocksRem
>                                );
> diff --git a/Features/Ext4Pkg/Ext4Pkg.dsc b/Features/Ext4Pkg/Ext4Pkg.dsc
> index 62cb4e69cf..57f279a4d9 100644
> --- a/Features/Ext4Pkg/Ext4Pkg.dsc
> +++ b/Features/Ext4Pkg/Ext4Pkg.dsc
> @@ -20,6 +20,8 @@ [Defines]
>    BUILD_TARGETS                  = DEBUG|RELEASE|NOOPT
>    SKUID_IDENTIFIER               = DEFAULT
>
> +!include MdePkg/MdeLibs.dsc.inc
> +
>  [BuildOptions]
>    *_*_*_CC_FLAGS                       = -D DISABLE_NEW_DEPRECATED_INTERFACES
>
>
>
>
> Thanks,
>
> Mike
>
>
>
>
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Friday, July 30, 2021 9:17 AM
> > To: devel@edk2.groups.io
> > Cc: Pedro Falcato <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Subject: [Patch 0/3] Ext4Pkg: Add Ext4Pkg
> >
> > This patch-set adds Ext4Pkg, a package designed to hold various drivers and
> > utilities related to the EXT4 filesystem.
> >
> > Right now, it holds a single read-only UEFI EXT4 driver (Ext4Dxe), which consumes the
> > DISK_IO, BLOCK_IO and DISK_IO2 protocols and produce EFI_FILE_PROTOCOL and
> > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL; this driver allows the mounting of EXT4 partitions and
> > the reading of their contents.
> >
> > Relevant RFC discussion, which includes a more in-depth walkthrough of EXT4 internals and
> > driver limitations is available at https://edk2.groups.io/g/devel/topic/84368561.
> >
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >
> > Pedro Falcato (3):
> >   Ext4Pkg: Add Ext4Pkg.dec and Ext4Pkg.uni.
> >   Ext4Pkg: Add Ext4Dxe driver.
> >   Ext4Pkg: Add .DSC file.
> >
> >  Features/Ext4Pkg/Ext4Dxe/BlockGroup.c | 208 ++++++
> >  Features/Ext4Pkg/Ext4Dxe/Collation.c  | 157 +++++
> >  Features/Ext4Pkg/Ext4Dxe/Crc16.c      |  75 ++
> >  Features/Ext4Pkg/Ext4Dxe/Crc32c.c     |  84 +++
> >  Features/Ext4Pkg/Ext4Dxe/Directory.c  | 492 ++++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/DiskUtil.c   |  83 +++
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   | 450 ++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 454 +++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 942 ++++++++++++++++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf  | 147 ++++
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.uni  |  15 +
> >  Features/Ext4Pkg/Ext4Dxe/Extents.c    | 616 +++++++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/File.c       | 583 ++++++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/Inode.c      | 468 +++++++++++++
> >  Features/Ext4Pkg/Ext4Dxe/Partition.c  | 120 ++++
> >  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 257 +++++++
> >  Features/Ext4Pkg/Ext4Pkg.dec          |  17 +
> >  Features/Ext4Pkg/Ext4Pkg.dsc          |  68 ++
> >  Features/Ext4Pkg/Ext4Pkg.uni          |  14 +
> >  19 files changed, 5250 insertions(+)
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Collation.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Crc16.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Crc32c.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Directory.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.uni
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Extents.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/File.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Inode.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Partition.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Dxe/Superblock.c
> >  create mode 100644 Features/Ext4Pkg/Ext4Pkg.dec
> >  create mode 100644 Features/Ext4Pkg/Ext4Pkg.dsc
> >  create mode 100644 Features/Ext4Pkg/Ext4Pkg.uni
> >
> > --
> > 2.32.0
>


-- 
Pedro Falcato