From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BF7CB208EB41A for ; Sun, 10 Feb 2019 17:26:05 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Feb 2019 17:26:04 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,357,1544515200"; d="scan'208";a="121410438" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga007.fm.intel.com with ESMTP; 10 Feb 2019 17:26:04 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 10 Feb 2019 17:26:04 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.102]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.172]) with mapi id 14.03.0415.000; Mon, 11 Feb 2019 09:25:35 +0800 From: "Wu, Hao A" To: "Wu, Hao A" , "Ni, Ray" , "edk2-devel@lists.01.org" CC: "Bi, Dandan" Thread-Topic: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes Thread-Index: AQHUugwWyeKHFpIZCE+6iGmBJ6uVu6XTnQWwgAY+llA= Date: Mon, 11 Feb 2019 01:25:35 +0000 Message-ID: References: <20190201085910.241544-1-ray.ni@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Feb 2019 01:26:06 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wu= , > Hao A > Sent: Thursday, February 07, 2019 10:18 AM > To: Ni, Ray; edk2-devel@lists.01.org > Cc: Bi, Dandan > Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR > isn't restored sometimes >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Friday, February 01, 2019 4:59 PM > > To: edk2-devel@lists.01.org > > Cc: Ni, Ray; Wu, Hao A; Bi, Dandan > > Subject: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't > > restored sometimes > > > > From: Ruiyu Ni > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1505 > > > > When a device under PPB contains option ROM but doesn't require 32bit > > MMIO, ProgrameUpstreamBridgeForRom() cannot correctly restore the > > PPB MEM32 RANGE BAR. It causes the 32bit MMIO conflict which may > > cause system hangs in boot. > > > > The root cause is when ProgrameUpstreamBridgeForRom() calls > > ProgramPpbApperture() to restore the PPB MEM32 RANGE BAR, the > > ProgramPpbApperture() skips to program the BAR when the resource > > length is 0. > > > > This patch fixes this issue by not calling ProgramPpbApperture(). > > Instead, it directly programs the PPB MEM32 RANGE BAR. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ray Ni > > Cc: Hao Wu > > Cc: Dandan Bi > > --- > > .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 53 +++++++++---------- > > 1 file changed, 24 insertions(+), 29 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > index d3cbefbadf..77cdc3e844 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > @@ -1,7 +1,7 @@ > > /** @file > > PCI resouces support functions implemntation for PCI Bus module. > > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> > This program and the accompanying materials > > are licensed and made available under the terms and conditions of the = BSD > > License > > which accompanies this distribution. The full text of the license may= be > found > > at > > @@ -1661,57 +1661,52 @@ ProgrameUpstreamBridgeForRom ( >=20 > Not directly related to the purpose of the patch, seems to me there is a > typo for the function name: > Programe -> Program >=20 > > IN BOOLEAN Enable > > ) > > { > > - PCI_IO_DEVICE *Parent; > > - PCI_RESOURCE_NODE Node; > > - UINT64 Base; > > - UINT64 Length; > > + PCI_IO_DEVICE *Parent; > > + EFI_PCI_IO_PROTOCOL *PciIo; > > + UINT16 Base; > > + UINT16 Limit; > > // > > // For root bridge, just return. > > // > > Parent =3D PciDevice->Parent; > > - ZeroMem (&Node, sizeof (Node)); > > while (Parent !=3D NULL) { > > if (!IS_PCI_BRIDGE (&Parent->Pci)) { > > break; > > } > > > > - Node.PciDev =3D Parent; > > - Node.Alignment =3D 0; > > - Node.Bar =3D PPB_MEM32_RANGE; > > - Node.ResType =3D PciBarTypeMem32; > > - Node.Offset =3D 0; > > + PciIo =3D &Parent->PciIo; > > > > // > > // Program PPB to only open a single <=3D 16MB apperture >=20 > Also not directly related: > apperture -> aperture >=20 > There seems lots of 'apperture' within this driver. > I would suggest to propose another patch to address those typos. >=20 > > // > > if (Enable) { > > - // > > - // Save the original PPB_MEM32_RANGE BAR. > > - // The values will be changed by ProgramPpbApperture(). > > - // > > - Base =3D Parent->PciBar[Node.Bar].BaseAddress; > > - Length =3D Parent->PciBar[Node.Bar].Length; > > - > > // > > // Only cover MMIO for Option ROM. > > // > > - Node.Length =3D PciDevice->RomSize; > > - ProgramPpbApperture (OptionRomBase, &Node); > > - > > - // > > - // Restore the original PPB_MEM32_RANGE BAR. > > - // So the MEM32 RANGE BAR register can be restored when disable = the > > decoding. > > - // > > - Parent->PciBar[Node.Bar].BaseAddress =3D Base; > > - Parent->PciBar[Node.Bar].Length =3D Length; > > + Base =3D (UINT16) (OptionRomBase >> 16); > > + Limit =3D (UINT16) ((OptionRomBase + PciDevice->RomSize - 1) >> = 16); Sorry for not spotting this earlier. 'PciDevice->RomSize' is of type UINT64 here, RShiftU64() should be used her= e. Two more similar cases below. > > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYP= E01, > > Bridge.MemoryBase), 1, &Base); > > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYP= E01, > > Bridge.MemoryLimit), 1, &Limit); > > > > PCI_ENABLE_COMMAND_REGISTER (Parent, > > EFI_PCI_COMMAND_MEMORY_SPACE); > > } else { > > // > > // Cover 32bit MMIO for devices below the bridge. > > // > > - Node.Length =3D Parent->PciBar[Node.Bar].Length; > > - ProgramPpbApperture (Parent->PciBar[Node.Bar].BaseAddress, &Node= ); > > + if (Parent->PciBar[PPB_MEM32_RANGE].Length =3D=3D 0) { > > + // > > + // When devices under the bridge contains Option ROM and doesn= 't > > require 32bit MMIO. > > + // > > + Base =3D (UINT16) gAllOne; > > + Limit =3D (UINT16) gAllZero; > > + } else { > > + Base =3D (UINT16) (Parent->PciBar[PPB_MEM32_RANGE].BaseAddres= s >> > > 16); Please use RShiftU64() here. > > + Limit =3D (UINT16) > > + ((Parent->PciBar[PPB_MEM32_RANGE].BaseAddress + Pa= rent- > > >PciBar[PPB_MEM32_RANGE].Length - 1) >> 16); Please use RShiftU64() here. Best Regards, Hao Wu > > + } > > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYP= E01, > > Bridge.MemoryBase), 1, &Base); > > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYP= E01, > > Bridge.MemoryLimit), 1, &Limit); > > + >=20 > Patch itself seems good to me, > Reviewed-by: Hao Wu >=20 > Best Regards, > Hao Wu >=20 > > PCI_DISABLE_COMMAND_REGISTER (Parent, > > EFI_PCI_COMMAND_MEMORY_SPACE); > > } > > > > -- > > 2.20.1.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel