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

["Gary Lin" <glin@suse.com>]

On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote:
> 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;
> >  
> > 
> 
Hi Laszlo,

> (1) Adding Gary Lin to the CC list.
> 
Thanks for noticing me :)

> 
> (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.
> 
> 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.)
> 
I just tested the patch with the ISO files with SLE15-SP2, openSUSE Leap
15.2, Fedora 32, and ubuntu 20.04, and the VM loads them without any
problem, so there is no regression I had before.

I'd give it my Tested-by.

Tested-by: Gary Lin <glin@suse.com>

Gary Lin

> 
> (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.
> 
> 
> Thanks
> Laszlo
>