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.151; helo=mga17.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (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 754B0202E53E6 for ; Wed, 6 Feb 2019 18:17:54 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Feb 2019 18:17:53 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,342,1544515200"; d="scan'208";a="273097233" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 06 Feb 2019 18:17:53 -0800 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 6 Feb 2019 18:17:52 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 6 Feb 2019 18:17:52 -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; Thu, 7 Feb 2019 10:17:50 +0800 From: "Wu, Hao A" To: "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+6iGmBJ6uVu6XTnQWw Date: Thu, 7 Feb 2019 02:17:50 +0000 Message-ID: References: <20190201085910.241544-1-ray.ni@intel.com> In-Reply-To: <20190201085910.241544-1-ray.ni@intel.com> 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: Thu, 07 Feb 2019 02:17:54 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 >=20 > From: Ruiyu Ni >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1505 >=20 > 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. >=20 > 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. >=20 > This patch fixes this issue by not calling ProgramPpbApperture(). > Instead, it directly programs the PPB MEM32 RANGE BAR. >=20 > 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(-) >=20 > 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. >=20 > -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 BS= D > License > which accompanies this distribution. The full text of the license may b= e found > at > @@ -1661,57 +1661,52 @@ ProgrameUpstreamBridgeForRom ( Not directly related to the purpose of the patch, seems to me there is a typo for the function name: Programe -> Program > 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; > } >=20 > - 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; >=20 > // > // Program PPB to only open a single <=3D 16MB apperture Also not directly related: apperture -> aperture There seems lots of 'apperture' within this driver. I would suggest to propose another patch to address those typos. > // > 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 th= e > 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= ); > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE0= 1, > Bridge.MemoryBase), 1, &Base); > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE0= 1, > Bridge.MemoryLimit), 1, &Limit); >=20 > 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].BaseAddress = >> > 16); > + Limit =3D (UINT16) > + ((Parent->PciBar[PPB_MEM32_RANGE].BaseAddress + Pare= nt- > >PciBar[PPB_MEM32_RANGE].Length - 1) >> 16); > + } > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE0= 1, > Bridge.MemoryBase), 1, &Base); > + PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE0= 1, > Bridge.MemoryLimit), 1, &Limit); > + Patch itself seems good to me, Reviewed-by: Hao Wu Best Regards, Hao Wu=20 > PCI_DISABLE_COMMAND_REGISTER (Parent, > EFI_PCI_COMMAND_MEMORY_SPACE); > } >=20 > -- > 2.20.1.windows.1