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


Michael D Kinney
 

-----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.kinney@intel.com>; devel@edk2.groups.io
Cc: Leif Lindholm <leif@nuviainc.com>; Bret Barkelew <Bret.Barkelew@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.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



Join devel@edk2.groups.io to automatically receive all group messages.