[edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change
Laszlo Ersek
lersek at redhat.com
Mon Oct 19 12:46:41 UTC 2020
On 10/16/20 08:42, Gary Lin wrote:
> 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 at intel.com>
>>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>> Signed-off-by: Zhichao Gao <zhichao.gao at 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 at 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 at suse.com>
Thank you, Gary!
Laszlo
>
> 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
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66386): https://edk2.groups.io/g/devel/message/66386
Mute This Topic: https://groups.io/mt/77455892/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list