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

["Laszlo Ersek" <lersek@redhat.com>]

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.

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.)


(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