> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> Sent: Thursday, August 5, 2021 3:50 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; devel@edk2.groups.io
> Cc: Leif Lindholm <l...@nuviainc.com>; Bret Barkelew
> <bret.barke...@microsoft.com>
> Subject: Re: [edk2-devel] [Patch 0/3] Ext4Pkg: Add Ext4Pkg
>
> 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
This is ok since this is a structure that is based on the EXT4
documentation,
> 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.kin...@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.falc...@gmail.com>
> > > Sent: Friday, July 30, 2021 9:17 AM
> > > To: devel@edk2.groups.io
> > > Cc: Pedro Falcato <pedro.falc...@gmail.com>; Leif Lindholm
> > > <l...@nuviainc.com>; Kinney, Michael D
> > > <michael.d.kin...@intel.com>; Bret Barkelew <bret.barke...@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 <l...@nuviainc.com>
> > > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > > Cc: Bret Barkelew <bret.barke...@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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78816): https://edk2.groups.io/g/devel/message/78816
Mute This Topic: https://groups.io/mt/84553674/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
