Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

["Gao, Zhichao" <zhichao.gao@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, October 15, 2020 6:18 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>
> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Gary Lin <glin@suse.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child
> handler blocksize change
> 
> On 10/12/20 09:22, Gao, Zhichao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
> >
> > Revert the patch to change the block size in child handler. It would
> > block the CD (Eltorito) Hard disk media type's sub partition being
> > observed.
> > The blocksize patch used to fix the CD image's MBR table issue. The CD
> > MBR table would always be ignored because it would be handled by the
> > Eltorito partition handler first and never go into the MBR handler. So
> > directly revert it.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12
> > +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > index f10ce7c65b..473e091320 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, BlockSize);
> > -  Private->End              = MultU64x32 (End + 1, BlockSize);
> > +  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media->BlockSize);
> > +  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media-
> >BlockSize);
> >
> >    Private->BlockSize        = BlockSize;
> >    Private->ParentBlockIo    = ParentBlockIo;
> > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle (
> >
> >    Private->Media.IoAlign   = 0;
> >    Private->Media.LogicalPartition = TRUE;
> > -  Private->Media.LastBlock = End - Start;
> > +  Private->Media.LastBlock = DivU64x32 (
> > +                               MultU64x32 (
> > +                                 End - Start + 1,
> > +                                 ParentBlockIo->Media->BlockSize
> > +                                 ),
> > +                                BlockSize
> > +                               ) - 1;
> >
> >    Private->Media.BlockSize = (UINT32) BlockSize;
> >
> >
> 
> (1) Adding Gary Lin to the CC list.
> 
> 
> (2) I can see that the TianoCore bugzilla ticket, namely
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened.
> 
> That's wrong.
> 
> TianoCore#2843 was about calculating the starting and ending LBAs with
> incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore
> TianoCore#2843 should stay in RESOLVED|FIXED status.

My bad. I want the keep all the description in one page. Seems it is not a good way to do this. I would open another BZ and change 2843 as it previous status.

> 
> Now that we have realized that commit e0eacd7daa6f caused a regression, a
> *new BZ* should be filed, stating the particular compatibility issue (regression).
> It is a different symptom from the symptom originally reported under
> TianoCore#2843, so it belongs in a different ticket.
> 
> In particular, the statement in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the original
> commit (which now should be reverted) "doesn't fix any specific issue", is
> *completely wrong*. If you look at commit e0eacd7daa6f, it contains the tag
> 
>     Tested-by: Gary Lin <glin@suse.com>
> 
> Furthermore, if you look at the mailing list archive, you will find the following
> confirmation from Gary:
> 
>     After applying this patch series, the firmware recognizes
>     openSUSE/SUSE iso images again.
> 
> In the v1 thread at
> 
>   [edk2-devel] [PATCH 0/3]
>   MdeModulePkg/PartitionDxe: Make the parition driver match the spec
> 
>   https://edk2.groups.io/g/devel/message/63972
>   http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation
> 
> And then, in the v2 thread, Gary wrote
> 
>     I've tested the following ISO images and all booted as expected.
>     [...]
> 
> again giving a Tested-by:
> 
>   [edk2-devel] [PATCH V2 0/3]
>   MdeModulePkg/PartitionDxe: Make the parition driver match the spec
> 
>   https://edk2.groups.io/g/devel/message/64047
>   http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation
> 
> 
> Now that you are proposing a revert, you have missed all of the above feedback
> from Gary. That's because you never bothered to link the v1 and
> v2 mailing list threads into the bugzilla ticket.
> 
> So this patch risks reintroducing the issue that Gary reported originally.
> 
> (Of course, the original bug report from Gary is *also* not linked into
> TianoCore#2843:
> 
>   https://edk2.groups.io/g/devel/message/62648
>   https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6
>   http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation
> 
> so it's no wonder we have no idea whose use case we could regress with a
> revert!)
> 
> This patch should *NOT* be merged until Gary confirms it's OK.
> 
> (And if it's not OK, then a solution is needed that fixes both Gary's use case, and
> the compatibility regression. It might even need a PCD, if there is media out
> there that needs one kind of logic, and other media that needs the other kind of
> logic.)

The patch V2 #3 is an optimization. There is no issue report related to it. I find it when I analysis the Linux CD iso image. Before the patch set, the MBR handler is ahead of the UDF (Eltorito compatible). And the ISO FAT cannot be found with the ISO's MBR table. But this issue is already gone with patch V2 #1 because the MBR of ISO image would be ignored and the ISO image would always be handled as UDF/Eltorito.
The regression is for the CD image with Media type ELTORITO_12_DISKETTE/ELTORITO_14_DISKETTE/ELTORITO_28_DISKETTE/ELTORITO_HARD_DISK which is not the normal Linux iso image. I cannot find all these types of ISO image. But there is one type ELTORITO_12_DISKETTE can be treat as an example, it is DOS6.22_bootdisk.iso. Now that the legacy boot is dropped from edk2 but the FAT disk should be found in shell.
I suggest to revert the patch because of above regression and I already tested with the DOS image. The DOS image's FAT can be found after reverting. Another reason to revert it is the code logic has been used for more than ten years and seems no issue is reported related to the handler. That is why I think the original logic would be more compatible.

> 
> 
> (3) If this patch is a revert of commit e0eacd7daa6f, then the revert should be
> prepared with the "git revert" command. In particular, the commit message
> should very clearly state that this patch reverts commit e0eacd7daa6f.

Can we? Directly revert cannot give the detail and the reason.

Thanks,
Zhichao

> 
> 
> Thanks
> Laszlo