1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec
  4. View patch

[edk2-devel] [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

Gao, Zhichao posted 3 patches 5 years, 6 months ago
[edk2-devel] [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
Posted by Gao, Zhichao 5 years, 6 months ago 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843

PartitionInstallChildHandle's parameters Start and End is counted
by the BlockSize, but in the implementation it uses the parent
device's BlockSize to calculate the new Start, End and LastBlock.
It would cause the driver report incorrect block scope and the file
system would fail to be found with right block scope.
So correct it to the right value.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Gary Lin <glin@suse.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
index 473e091320..f10ce7c65b 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
@@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (
 
   Private->Signature        = PARTITION_PRIVATE_DATA_SIGNATURE;
 
-  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media->BlockSize);
-  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize);
+  Private->Start            = MultU64x32 (Start, BlockSize);
+  Private->End              = MultU64x32 (End + 1, BlockSize);
 
   Private->BlockSize        = BlockSize;
   Private->ParentBlockIo    = ParentBlockIo;
@@ -1187,13 +1187,7 @@ PartitionInstallChildHandle (
 
   Private->Media.IoAlign   = 0;
   Private->Media.LogicalPartition = TRUE;
-  Private->Media.LastBlock = DivU64x32 (
-                               MultU64x32 (
-                                 End - Start + 1,
-                                 ParentBlockIo->Media->BlockSize
-                                 ),
-                                BlockSize
-                               ) - 1;
+  Private->Media.LastBlock = End - Start;
 
   Private->Media.BlockSize = (UINT32) BlockSize;
 
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64026): https://edk2.groups.io/g/devel/message/64026
Mute This Topic: https://groups.io/mt/76139484/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
Posted by Wu, Hao A 5 years, 6 months ago 
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA
> size in child hander
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
> 
> PartitionInstallChildHandle's parameters Start and End is counted by the
> BlockSize, but in the implementation it uses the parent device's BlockSize to
> calculate the new Start, End and LastBlock.
> It would cause the driver report incorrect block scope and the file system
> would fail to be found with right block scope.
> So correct it to the right value.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index 473e091320..f10ce7c65b 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (
> 
>    Private->Signature        = PARTITION_PRIVATE_DATA_SIGNATURE;
> 
> -  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media-
> >BlockSize);
> -  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media-
> >BlockSize);
> +  Private->Start            = MultU64x32 (Start, BlockSize);
> +  Private->End              = MultU64x32 (End + 1, BlockSize);


For function PartitionInstallChildHandle(), the input parameters 'Start' and
'End' (which denote the block index) should be of unit indicated by the input
parameter 'BlockSize'.

I checked all the callers of PartitionInstallChildHandle(), it looks to me that
GPT, MBR and UDF follow the above rule. While I am not able to get a straight-
forward match for the El Torito case. It seems fine with me, but I am not sure.
Anyway, if the CDROM content can be properly identified then I think the change
will be good.

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
>    Private->BlockSize        = BlockSize;
>    Private->ParentBlockIo    = ParentBlockIo;
> @@ -1187,13 +1187,7 @@ PartitionInstallChildHandle (
> 
>    Private->Media.IoAlign   = 0;
>    Private->Media.LogicalPartition = TRUE;
> -  Private->Media.LastBlock = DivU64x32 (
> -                               MultU64x32 (
> -                                 End - Start + 1,
> -                                 ParentBlockIo->Media->BlockSize
> -                                 ),
> -                                BlockSize
> -                               ) - 1;
> +  Private->Media.LastBlock = End - Start;
> 
>    Private->Media.BlockSize = (UINT32) BlockSize;
> 
> --
> 2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64048): https://edk2.groups.io/g/devel/message/64048
Mute This Topic: https://groups.io/mt/76139484/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
Posted by Gao, Zhichao 5 years, 6 months ago 
For additional info,

I have test patch #3 without #1 and #2, and revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM". The CD's FAT can be observed with MBR path and I can load the EFI application successfully. But the behavior is different as I mention in patch #1 commit message. Test with open SUSE 15.2, RedHat 8.2, Ubuntu 18.04 and Fedora 20.
Only Ubuntu can enter the installation selection but it output a "error: unknown filesystem".
Anyway, CDROM's MBR can be checked and handled correctly.

Thanks,
Zhichao

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, August 12, 2020 2:35 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA
> size in child hander
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Wednesday, August 12, 2020 9:21 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> > <glin@suse.com>; Andrew Fish <afish@apple.com>
> > Subject: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect
> > LBA size in child hander
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
> >
> > PartitionInstallChildHandle's parameters Start and End is counted by
> > the BlockSize, but in the implementation it uses the parent device's
> > BlockSize to calculate the new Start, End and LastBlock.
> > It would cause the driver report incorrect block scope and the file
> > system would fail to be found with right block scope.
> > So correct it to the right value.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Gary Lin <glin@suse.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12
> > +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > index 473e091320..f10ce7c65b 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (
> >
> >    Private->Signature        = PARTITION_PRIVATE_DATA_SIGNATURE;
> >
> > -  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media-
> > >BlockSize);
> > -  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media-
> > >BlockSize);
> > +  Private->Start            = MultU64x32 (Start, BlockSize);
> > +  Private->End              = MultU64x32 (End + 1, BlockSize);
> 
> 
> For function PartitionInstallChildHandle(), the input parameters 'Start' and 'End'
> (which denote the block index) should be of unit indicated by the input parameter
> 'BlockSize'.
> 
> I checked all the callers of PartitionInstallChildHandle(), it looks to me that GPT,
> MBR and UDF follow the above rule. While I am not able to get a straight-
> forward match for the El Torito case. It seems fine with me, but I am not sure.
> Anyway, if the CDROM content can be properly identified then I think the change
> will be good.
> 
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> >    Private->BlockSize        = BlockSize;
> >    Private->ParentBlockIo    = ParentBlockIo;
> > @@ -1187,13 +1187,7 @@ PartitionInstallChildHandle (
> >
> >    Private->Media.IoAlign   = 0;
> >    Private->Media.LogicalPartition = TRUE;
> > -  Private->Media.LastBlock = DivU64x32 (
> > -                               MultU64x32 (
> > -                                 End - Start + 1,
> > -                                 ParentBlockIo->Media->BlockSize
> > -                                 ),
> > -                                BlockSize
> > -                               ) - 1;
> > +  Private->Media.LastBlock = End - Start;
> >
> >    Private->Media.BlockSize = (UINT32) BlockSize;
> >
> > --
> > 2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64136): https://edk2.groups.io/g/devel/message/64136
Mute This Topic: https://groups.io/mt/76139484/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.