From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web09.10031.1603100119546482790 for ; Mon, 19 Oct 2020 02:35:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=aZ+jYNDh; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: zhichao.gao@intel.com) IronPort-SDR: lNq890t7nuUvHAVv8Fwi/ktCahoheo+byEqOd6v0PPnKabYYIaAdxQdHQYMIfseI1OQLb0UDok pJTvHYzUK0AA== X-IronPort-AV: E=McAfee;i="6000,8403,9778"; a="154788283" X-IronPort-AV: E=Sophos;i="5.77,394,1596524400"; d="scan'208";a="154788283" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2020 02:35:17 -0700 IronPort-SDR: XJ0p190wYZgCWWBPt8Pz/GrJV6/gxCkwHVNG4taED04OBhEeFnYAw8pt80Qf6e3fhHxFPOwhPG 7+m/9/J1Lo+Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,394,1596524400"; d="scan'208";a="532555339" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by orsmga005.jf.intel.com with ESMTP; 19 Oct 2020 02:35:17 -0700 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 19 Oct 2020 02:35:16 -0700 Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 19 Oct 2020 02:35:16 -0700 Received: from NAM12-BN8-obe.outbound.protection.outlook.com (104.47.55.169) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1713.5; Mon, 19 Oct 2020 02:35:15 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iyVFsFtQzWFSvYeNWx7/yKJ7wSsD2QAnJzIktmLJWMQ3pC4Y+ge40o7UAagMduqRp9d1vyWrZDOR3JZ3V2lffPTM7nwQwJ7iTMVCxaqh7VRnvaudLgqPN9YYEEoNG5IQh76RK5iJtp7lZfAN3ceBtPGyw1I3pE9VGSqCRX60G0ICrbijxY/qwOAJmT16NOknw4JiRuEEapVMbPqJBCtAyDr75NPxSXH6PYtVhPQCK+qhCevncADoVte0DO2lrKBm8bzN5w1tACwO8gUWSUw0j5M57fxfUbu5V8Q8rgCtGT44+6/wfIYX7HHld/g5XzlitPkICAjm6ReS5PkbDIJvhw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Ye/iAs3HR1YDsdzmQRXTpfeAI85p7WJ5LSAA+DDX/8g=; b=XS42lz5htXAu3AelPI0IMn6S6ON1ajNzQggqiJJsVXaF5dzaSPHPzxAeau67ROU2608C8JzaMi6cPzcDFvAmCVdcMgJS+dXNQJ+jGYke6nze1e3E/6tTLU2zKPXPLR2C4ThZqmQ6NXaul0vOXS9kTs7hP8zvglMAXMKVX6EIalqw7XQPFEknn8/1rGBGZ9H1qsG2z/T0qUVeWwW2EmX8rX66oA5NS/D1iOfVVc7WGWJWrB4LRC+2f9spEHklJDlGb95RM7+JJQopEKWfY2/PSg1kokDmVnvwk8y+HdH1YJCDAWZPB+i7K+GJfX+Nstrp0UZUuBH/zKW8cEXYL0YUWQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Ye/iAs3HR1YDsdzmQRXTpfeAI85p7WJ5LSAA+DDX/8g=; b=aZ+jYNDh1pZsAdLAmWc82DEcDg4eGILp4a33TVz8cJHrTl/YpCZqCCKZUsFvFulLdFash112szt0ZiHaj70nZH3zdaKNopZIFHPRuMegfQERLb2wH9zYeaJkpPRZugpcohijpn8/rO1tbsAAb1PNEB13gcKLa/5FacJ0WFAfu1k= Received: from MWHPR11MB1647.namprd11.prod.outlook.com (2603:10b6:301:d::12) by MWHPR11MB2045.namprd11.prod.outlook.com (2603:10b6:300:29::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3477.22; Mon, 19 Oct 2020 09:35:13 +0000 Received: from MWHPR11MB1647.namprd11.prod.outlook.com ([fe80::b96a:621b:54b6:c8ea]) by MWHPR11MB1647.namprd11.prod.outlook.com ([fe80::b96a:621b:54b6:c8ea%8]) with mapi id 15.20.3477.028; Mon, 19 Oct 2020 09:35:13 +0000 From: "Gao, Zhichao" To: "Ni, Ray" , "devel@edk2.groups.io" , "glin@suse.com" , Laszlo Ersek CC: "Wu, Hao A" Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change Thread-Topic: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change Thread-Index: AQHWotxvCXf9EMfMvUaKOPLqN2LlJamZyTOAgARzt0CAADZygIAAO+7g Date: Mon, 19 Oct 2020 09:35:13 +0000 Message-ID: References: <20201012072230.46152-1-zhichao.gao@intel.com> <4f20aa01-793a-8477-53c1-56a5899caa14@redhat.com> <20201016064205.GI19552@GaryWorkstation> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.102.204.37] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 36b745b5-76d2-4ae2-8707-08d8741247eb x-ms-traffictypediagnostic: MWHPR11MB2045: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: A0IAgM6WoOJaYJHOLvgL+fZFPjvH5cS5wIHPnIYUG5jJ1X0RE5038lfZ+hckdWgHf6NA1DpM0Vg2l0zuXZJZNBTR56djct5wkNodQb7vsvIu1LicVa88XRgMns/3kPW4zUxXRzdBvB7xiSxy9B74Wjbrqn+ZZwSgYcF3VTj9hwI7xbm3duaLkrt1omh+8R1JnEBT9HtFznOUalNM4iAuZEzbSzpEcFxoqUUfGkh3/J8IKCgPmllrdRCm6z6s38Val/+ROreAwKxobaIs42bv4CIpKEnVXy6TgTosxmdfp2/21/rvCBvYB6XJI70qBPjYtKjHQMqHX0D2vGviy+qIkznrRSwB3LkNNYSRbPNpkc6+3cfUa5kFGDgq+1IW4A/TVlJLPe+p7xz1CZnuOkDSGQ== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR11MB1647.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(376002)(39860400002)(346002)(396003)(136003)(366004)(33656002)(55016002)(53546011)(83380400001)(6506007)(8936002)(8676002)(107886003)(966005)(71200400001)(478600001)(5660300002)(186003)(76116006)(26005)(86362001)(9686003)(4326008)(2906002)(64756008)(66446008)(66476007)(66556008)(316002)(110136005)(66946007)(7696005)(52536014);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: kmm21OXJPUjQNoQIMBMAaYMZG2kn5dTRmQ7auYMHI9Z7L6N5mhbXiu4OdLMdGTV5f1ShQNLwNiURuyx03AYF7xjFvL3ucWYy1i3lWyvtsStUjjkJxOJ0RLdxv8O+mcZBWX1bKAQdifeOknLeGsbzdMKOV0jcNDWrOcqekzFoM7OWCTmp3diAhrjUPJf4ww69Um1/dIH2mrmn/uAfUs9M6KMoQ3bZDaSckDqbUMFmTsKpW1Fs7f6Vp98iuHkoOkLrfyMhKKEipfK8ZePPnNVDovsxNdePGcdIrMhnQaBq1pIWRJSEnE68EZfLEGcCCiZjPxoXS232Gb05+6qW28PaiQl6OXfHill3sUFb4oT+R4zkrtIUU3aY16RA0hjmdtE4PBVu72otAK7rwb8ygimz4kuP4htXeCnOBTaZvay1RR/XllckO+JzkOda97fAUVtwLBqs7Nzwfqfa+paR85EW3/RUkWj0v97BhMXghkjbxjsnP+gPkjNaluWsVKV/pTkNdZrlkCzq//WcXltLg1+z0giFINPNSjO+vx8kcZVmToCVTMnpRqmTc59OtAzMt5befA6Y3HL2j6emlcCKsuY/T1q+fuS30wAaY0OmHp9dxZzkVTFPZbHODE5buuy32C1qdd/lL2XtdmASmkzvLa08NQ== MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1647.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 36b745b5-76d2-4ae2-8707-08d8741247eb X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Oct 2020 09:35:13.7026 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: /MxCZJNE7Iq/OvTWJx7oIrz5Us6v3dhFFf0jHOKhewCLFQ0VmC6SHOKC6sEgXnhh2sYzE0WzPSjhxIyMBbndnw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB2045 Return-Path: zhichao.gao@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Yes. But I need to confirm the comment #3. The directly revert would make t= he title of the commit message larger which may be larger than 72 chars. Th= at is another reason I didn't use revert directly. Thanks, Zhichao > -----Original Message----- > From: Ni, Ray > Sent: Monday, October 19, 2020 1:56 PM > To: Gao, Zhichao ; devel@edk2.groups.io; > glin@suse.com; Laszlo Ersek > Cc: Wu, Hao A > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the = child > handler blocksize change >=20 > Zhichao, > Can you please update the commit message to address Laszlo's comments? >=20 > Thanks, > Ray >=20 > > -----Original Message----- > > From: Gao, Zhichao > > Sent: Monday, October 19, 2020 10:46 AM > > To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek > > > > Cc: Ni, Ray ; Wu, Hao A > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > > the child handler blocksize change > > > > Thanks Gary for your test. I have give my comments base on Laszlo's > > reply. I don't think the regression would affect Linux ISO image > > except there is one Linux image with boot catalog media type not > > NO_EMULATOR. Anyway, thanks for your test and your quickly response. > > > > Thanks, > > Zhichao > > > > > -----Original Message----- > > > From: devel@edk2.groups.io On Behalf Of Gary > > > Lin > > > Sent: Friday, October 16, 2020 2:42 PM > > > To: Laszlo Ersek > > > Cc: Gao, Zhichao ; devel@edk2.groups.io; Ni, > > > Ray ; Wu, Hao A > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > > > the > > child > > > handler blocksize change > > > > > > 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=3D2843 > > > > > > > > > > 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 =3D PARTITION_PRIVATE_DATA_SIGNATUR= E; > > > > > > > > > > - Private->Start =3D MultU64x32 (Start, BlockSize); > > > > > - Private->End =3D MultU64x32 (End + 1, BlockSize)= ; > > > > > + Private->Start =3D MultU64x32 (Start, ParentBlockI= o->Media- > > > >BlockSize); > > > > > + Private->End =3D MultU64x32 (End + 1, ParentBloc= kIo->Media- > > > >BlockSize); > > > > > > > > > > Private->BlockSize =3D BlockSize; > > > > > Private->ParentBlockIo =3D ParentBlockIo; > > > > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > > > > > > > Private->Media.IoAlign =3D 0; > > > > > Private->Media.LogicalPartition =3D TRUE; > > > > > - Private->Media.LastBlock =3D End - Start; > > > > > + Private->Media.LastBlock =3D DivU64x32 ( > > > > > + MultU64x32 ( > > > > > + End - Start + 1, > > > > > + ParentBlockIo->Media->BlockSiz= e > > > > > + ), > > > > > + BlockSize > > > > > + ) - 1; > > > > > > > > > > Private->Media.BlockSize =3D (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 th= e > > > > 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 expecte= d. > > > > [...] > > > > > > > > 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 ori= ginally. > > > > > > > > (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=3D2823#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 > > > > > > 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 > > > > > > > > > > > > > > > >=20 > > >