From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.8963.1602757085943620923 for ; Thu, 15 Oct 2020 03:18:06 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Ib2Ir/1t; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602757085; 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=J5F+zx2YQFujAwDy/Ch7cRlXOcfVVpvUtZbotR4bT20=; b=Ib2Ir/1tT1r0tWg9l7VTaJZ1aW5AH+tr0MXEOXDiB7DPqtLoozIz5KQJGJEsZxfXMcw6uo cjhZ2EsX+BnZ++XNJVBIT7x487xxqqViihi9ufu8vK9UJNtdAZMHt797mcFjeAymywUvkA X8HMlFnR6kXBG7pqPe6cHYuXRMSSWuk= 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-253-XFj7QICMM5WF0RLI-xBuNg-1; Thu, 15 Oct 2020 06:17:55 -0400 X-MC-Unique: XFj7QICMM5WF0RLI-xBuNg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 72048101962A; Thu, 15 Oct 2020 10:17:53 +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 07C616EF61; Thu, 15 Oct 2020 10:17:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change To: zhichao.gao@intel.com Cc: devel@edk2.groups.io, Ray Ni , Hao A Wu , Gary Lin References: <20201012072230.46152-1-zhichao.gao@intel.com> From: "Laszlo Ersek" Message-ID: <4f20aa01-793a-8477-53c1-56a5899caa14@redhat.com> Date: Thu, 15 Oct 2020 12:17:50 +0200 MIME-Version: 1.0 In-Reply-To: <20201012072230.46152-1-zhichao.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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/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; > > (1) Adding Gary Lin to the CC list. (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.) (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