From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.12167.1603111613807610055 for ; Mon, 19 Oct 2020 05:46:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FvicXNBK; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1603111613; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kdUMs9bLXamndjuFVCeXdz3UoWlY79MeJ7NLKTtHuBM=; b=FvicXNBKnJmEaSb5zOCHj0flAjPyDsM09fRiE2v7i4j0bHD0pp+S950WZkxYqKkmvpti7B ele3NTF++c3TJzB4scKFDoME9KPqhseP4TgKOVWHWF0plZUg4Z87/Hu/CqMruWQ90BDzE3 uaCFa4Rz+m+EYC8krOKqcN5V2hhf12E= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-516-ehcu7V8-NwizUzyykqV9qQ-1; Mon, 19 Oct 2020 08:46:45 -0400 X-MC-Unique: ehcu7V8-NwizUzyykqV9qQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5CE2218BE17D; Mon, 19 Oct 2020 12:46:44 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-119.ams2.redhat.com [10.36.113.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id CF43B5882A; Mon, 19 Oct 2020 12:46:42 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change To: Gary Lin Cc: zhichao.gao@intel.com, devel@edk2.groups.io, Ray Ni , Hao A Wu References: <20201012072230.46152-1-zhichao.gao@intel.com> <4f20aa01-793a-8477-53c1-56a5899caa14@redhat.com> <20201016064205.GI19552@GaryWorkstation> From: "Laszlo Ersek" Message-ID: <75899afd-4e6d-37b7-1cfe-9939e59ae082@redhat.com> Date: Mon, 19 Oct 2020 14:46:41 +0200 MIME-Version: 1.0 In-Reply-To: <20201016064205.GI19552@GaryWorkstation> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>> Cc: Hao A Wu >>> Signed-off-by: Zhichao Gao >>> --- >>> 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 >> , 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 >> 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 >> >> 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 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 >> >