From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-vs1-f47.google.com (mail-vs1-f47.google.com [209.85.217.47]) by mx.groups.io with SMTP id smtpd.web08.911.1628203826438492557 for ; Thu, 05 Aug 2021 15:50:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=b44KFoLn; spf=pass (domain: gmail.com, ip: 209.85.217.47, mailfrom: pedro.falcato@gmail.com) Received: by mail-vs1-f47.google.com with SMTP id n22so4046594vsq.11 for ; Thu, 05 Aug 2021 15:50:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=YsVIHpo+5VDdBbupwKtpybJrIcux+i750gEA3Nsg9/w=; b=b44KFoLnoB0tMg9mGlACXn9HDS0ABQUteaULZXcTGgeC1LyVyEvM7SITKPkZDof0TI GPQeV9TDH0ETkp6G4D/LGHQXKj4RLKxO7qdlBK8/37yU78NyS64txJrQkhhvtYeiiyk5 TLf9PN6xWFo9Tw4Q+3urmhAlVaL/yw36X/+THTQMFy1VHoD7I8puh5ASlO6S7D8dfVd6 dFvoZe/qcVob0U9s5cu9ubz7NdaCvQi7Gdkl0qJEhDCTC/ZhEd77h6DF/5aJX+eR8D7s i878RNS4j2fIqQnRda0DSq8IXvZ80UFsuZSHCO1NZOzQSiZ+kSsKvNsM4CXe+iKyCHzZ azkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=YsVIHpo+5VDdBbupwKtpybJrIcux+i750gEA3Nsg9/w=; b=da7PX8jP0IX5NX8O22VYau4L3iRbj9OmS5pUpm1lLc9J3xoBc3dfTQy87lUMkKaLbX VPmy5RR1FSbUlqc7Ym98xHGxdb5mZU7WEAODSuzgpUTMgnET59PfXJBYojG+nIEjX8fU 1WV/GZBUvkhpndyTfPoljljDNoE16u4oWnUJb0p/wccFHOXL3z+qx3CNiE/7Fw6NG2Nj TJ06hYEry9NgZ1t0x+S1DUGug7i3k6QLyvdWv3/MxGPklSkV3dyBuZBylFlA1IEKNcNg tSKyQp+OsPC14kUvHojVGo+6vG1wftpqteMT4zg0SkJ7ACyGQdg1IUX4YhCChqeLpQIT 1niQ== X-Gm-Message-State: AOAM530ZTL6AWUFHkm7Y7VyQwHuARupgvIS5fChFngwVvjlFA1zV4rD7 8DLXHa5XgK5qbJszW2zxIXxZjTBQLxfzRZdkM2k= X-Google-Smtp-Source: ABdhPJxjVg2xcYFuKwvjak/WTeKmhJzzM5WzUjFjVCh68yDNhffpGGqqEHgIk+5LJ28iPkdT7Gr2oQVPDlwTVnn1RaA= X-Received: by 2002:a67:16c1:: with SMTP id 184mr6929897vsw.14.1628203825408; Thu, 05 Aug 2021 15:50:25 -0700 (PDT) MIME-Version: 1.0 References: <20210730161641.7245-1-pedro.falcato@gmail.com> In-Reply-To: From: "Pedro Falcato" Date: Thu, 5 Aug 2021 23:50:15 +0100 Message-ID: Subject: Re: [Patch 0/3] Ext4Pkg: Add Ext4Pkg To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: Leif Lindholm , Bret Barkelew Content-Type: text/plain; charset="UTF-8" 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 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 > > Sent: Friday, July 30, 2021 9:17 AM > > To: devel@edk2.groups.io > > Cc: Pedro Falcato ; Leif Lindholm ; Kinney, Michael D > > ; Bret Barkelew > > 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 > > Cc: Michael D Kinney > > Cc: Bret Barkelew > > > > 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