From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 44E612117FD4C for ; Thu, 6 Dec 2018 04:50:10 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Dec 2018 04:50:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,322,1539673200"; d="scan'208";a="107792299" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 06 Dec 2018 04:50:09 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 6 Dec 2018 04:50:08 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 6 Dec 2018 04:50:07 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.203]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.201]) with mapi id 14.03.0415.000; Thu, 6 Dec 2018 20:50:05 +0800 From: "Wu, Hao A" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH] MdeModulePkg/PciBus: Shadow option ROM after BARs are programmed Thread-Index: AQHUjHwpSGyi5ynx60eKJ9OMr7ENxKVxqjgA Date: Thu, 6 Dec 2018 12:50:05 +0000 Message-ID: References: <20181205092433.136328-1-ruiyu.ni@intel.com> In-Reply-To: <20181205092433.136328-1-ruiyu.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: Shadow option ROM after BARs are programmed 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, 06 Dec 2018 12:50:10 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Ni, Ruiyu > Sent: Wednesday, December 05, 2018 5:25 PM > To: edk2-devel@lists.01.org > Cc: Wu, Hao A > Subject: [PATCH] MdeModulePkg/PciBus: Shadow option ROM after BARs > are programmed >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1376 >=20 > Today's implementation reuses the 32bit MMIO resource requested by > all PCI devices MMIO BARs when shadowing the option ROM. > Take a simple example, a system has only one PCI device. It requires > 8MB 32bit MMIO and contains a 4MB option ROM. Today's implementation > only requests 8MB (max of 4M and 8M) 32bit MMIO from > PciHostBridgeResourceAllocation protocol. Let's assume the MMIO range > [3GB, 3GB+8MB) is allocated. The 3GB base address is firstly > programmed to the option ROM BAR for option ROM shadow. Then the > option ROM decoding is turned off and 3GB base address is programmed > to the 32bit MMIO BAR. >=20 > It doesn't cause issues when the device doesn't request too much > MMIO. > But when the device contains a 64bit MMIO BAR which requests 4GB MMIO > and a 4MB option ROM. Let's assume [3GB, 3GB+8MB) 32bit MMIO range is "[3GB, 3GB+4MB)" here? > allocated for the option ROM. When the option ROM is being shadowed, > 64bit MMIO BAR is programmed to value 0, which means [0, 4GB) MMIO is > given to the 64bit BAR. > The range overlaps with the option ROM range which may cause the > device malfunction (e.g.: option ROM cannot be read out) when the > device has two separate decoders: one for MMIO BAR, the other for > option ROM. >=20 > The patch requests dedicated MEM32 resource for Option ROMs and > moves the Option ROM shadow logic after all MMIO BARs are programmed. > The MMIO BAR setting to 0 when shadowing Option ROM is also skipped > because the MMIO BAR already contains the correct value. Reviewed-by: Hao Wu Best Regards, Hao Wu >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Hao A Wu > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 3 +- > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 71 ++++++++--------= ------ > .../Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 13 ---- > .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 37 ++++++++++- > 4 files changed, 62 insertions(+), 62 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > index f9eebdd5a8..6938802857 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > @@ -1,7 +1,7 @@ > /** @file > Header files and data structures needed by PCI Bus module. >=20 > -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2018, 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 > @@ -67,6 +67,7 @@ typedef enum { > PciBarTypePMem32, > PciBarTypeMem64, > PciBarTypePMem64, > + PciBarTypeOpRom, > PciBarTypeIo, > PciBarTypeMem, > PciBarTypeMaxType > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > index b81f81a136..7255bcfbbc 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > @@ -24,6 +24,7 @@ CHAR16 *mBarTypeStr[] =3D { > L"PMem32", > L" Mem64", > L"PMem64", > + L" OpRom", > L" Io", > L" Mem", > L"Unknow" > @@ -425,15 +426,9 @@ PciHostBridgeResourceAllocator ( > PCI_RESOURCE_NODE PMem32Pool; > PCI_RESOURCE_NODE Mem64Pool; > PCI_RESOURCE_NODE PMem64Pool; > - BOOLEAN ReAllocate; > EFI_DEVICE_HANDLE_EXTENDED_DATA_PAYLOAD HandleExtendedData; > EFI_RESOURCE_ALLOC_FAILURE_ERROR_DATA_PAYLOAD > AllocFailExtendedData; >=20 > - // > - // Reallocate flag > - // > - ReAllocate =3D FALSE; > - > // > // It may try several times if the resource allocation fails > // > @@ -514,6 +509,17 @@ PciHostBridgeResourceAllocator ( > PciResUsageTypical > ); >=20 > + // > + // Get the max ROM size that the root bridge can process > + // Insert to resource map so that there will be dedicate MEM32 res= ource > range for Option ROM. > + // All devices' Option ROM share the same MEM32 resource. > + // > + MaxOptionRomSize =3D GetMaxOptionRomSize (RootBridgeDev); > + RootBridgeDev->PciBar[0].BarType =3D PciBarTypeOpRom; > + RootBridgeDev->PciBar[0].Length =3D MaxOptionRomSize; > + RootBridgeDev->PciBar[0].Alignment =3D MaxOptionRomSize - 1; > + GetResourceFromDevice (RootBridgeDev, IoBridge, Mem32Bridge, > PMem32Bridge, Mem64Bridge, PMem64Bridge); > + > // > // Create resourcemap by going through all the devices subject to = this > root bridge > // > @@ -526,38 +532,6 @@ PciHostBridgeResourceAllocator ( > PMem64Bridge > ); >=20 > - // > - // Get the max ROM size that the root bridge can process > - // > - RootBridgeDev->RomSize =3D Mem32Bridge->Length; > - > - // > - // Skip to enlarge the resource request during realloction > - // > - if (!ReAllocate) { > - // > - // Get Max Option Rom size for current root bridge > - // > - MaxOptionRomSize =3D GetMaxOptionRomSize (RootBridgeDev); > - > - // > - // Enlarger the mem32 resource to accomdate the option rom > - // if the mem32 resource is not enough to hold the rom > - // > - if (MaxOptionRomSize > Mem32Bridge->Length) { > - > - Mem32Bridge->Length =3D MaxOptionRomSize; > - RootBridgeDev->RomSize =3D MaxOptionRomSize; > - > - // > - // Alignment should be adjusted as well > - // > - if (Mem32Bridge->Alignment < MaxOptionRomSize - 1) { > - Mem32Bridge->Alignment =3D MaxOptionRomSize - 1; > - } > - } > - } > - > // > // Based on the all the resource tree, construct ACPI resource nod= e to > // submit the resource aperture to pci host bridge protocol > @@ -760,8 +734,6 @@ PciHostBridgeResourceAllocator ( > if (EFI_ERROR (Status)) { > return Status; > } > - > - ReAllocate =3D TRUE; > } > } > // > @@ -828,11 +800,6 @@ PciHostBridgeResourceAllocator ( > &PMem64Base > ); >=20 > - // > - // Process option rom for this root bridge > - // > - ProcessOptionRom (RootBridgeDev, Mem32Base, RootBridgeDev- > >RomSize); > - > // > // Create the entire system resource map from the information collec= ted > by > // enumerator. Several resource tree was created > @@ -889,6 +856,20 @@ PciHostBridgeResourceAllocator ( > PMem64Bridge > ); >=20 > + // > + // Process Option ROM for this root bridge after all BARs are progra= mmed. > + // The PPB's MEM32 RANGE BAR is re-programmed to the Option ROM > BAR Base in order to > + // shadow the Option ROM of the devices under the PPB. > + // After the shadow, Option ROM BAR decoding is turned off and the > PPB's MEM32 RANGE > + // BAR is restored back to the original value. > + // The original value is programmed by ProgramResource() above. > + // > + DEBUG (( > + DEBUG_INFO, "Process Option ROM: BAR Base/Length =3D %lx/%lx\n", > + RootBridgeDev->PciBar[0].BaseAddress, RootBridgeDev- > >PciBar[0].Length > + )); > + ProcessOptionRom (RootBridgeDev, RootBridgeDev- > >PciBar[0].BaseAddress, RootBridgeDev->PciBar[0].Length); > + > IoBridge ->PciDev->PciBar[IoBridge ->Bar].BaseAddress =3D IoBa= se; > Mem32Bridge ->PciDev->PciBar[Mem32Bridge ->Bar].BaseAddress =3D > Mem32Base; > PMem32Bridge->PciDev->PciBar[PMem32Bridge->Bar].BaseAddress =3D > PMem32Base; > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c > index c2be85a906..aa314474dd 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c > @@ -583,23 +583,10 @@ RomDecode ( > ) > { > UINT32 Value32; > - UINT32 Offset; > - UINT32 OffsetMax; > EFI_PCI_IO_PROTOCOL *PciIo; >=20 > PciIo =3D &PciDevice->PciIo; > if (Enable) { > - // > - // Clear all bars > - // > - OffsetMax =3D 0x24; > - if (IS_PCI_BRIDGE(&PciDevice->Pci)) { > - OffsetMax =3D 0x14; > - } > - > - for (Offset =3D 0x10; Offset <=3D OffsetMax; Offset +=3D sizeof (UIN= T32)) { > - PciIo->Pci.Write (PciIo, EfiPciIoWidthUint32, Offset, 1, &gAllZero= ); > - } >=20 > // > // set the Rom base address: now is hardcode > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > index f3e51d6150..d3cbefbadf 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > @@ -446,13 +446,14 @@ GetResourceFromDevice ( > switch ((PciDev->PciBar)[Index].BarType) { >=20 > case PciBarTypeMem32: > + case PciBarTypeOpRom: >=20 > Node =3D CreateResourceNode ( > PciDev, > (PciDev->PciBar)[Index].Length, > (PciDev->PciBar)[Index].Alignment, > Index, > - PciBarTypeMem32, > + (PciDev->PciBar)[Index].BarType, > PciResUsageTypical > ); >=20 > @@ -1307,7 +1308,13 @@ ProgramBar ( > 1, > &Address > ); > + // > + // Continue to the case PciBarTypeOpRom to set the BaseAddress. > + // PciBarTypeOpRom is a virtual BAR only in root bridge, to capture > + // the MEM32 resource requirement for Option ROM shadow. > + // >=20 > + case PciBarTypeOpRom: > Node->PciDev->PciBar[Node->Bar].BaseAddress =3D Address; >=20 > break; > @@ -1656,6 +1663,8 @@ ProgrameUpstreamBridgeForRom ( > { > PCI_IO_DEVICE *Parent; > PCI_RESOURCE_NODE Node; > + UINT64 Base; > + UINT64 Length; > // > // For root bridge, just return. > // > @@ -1667,7 +1676,6 @@ ProgrameUpstreamBridgeForRom ( > } >=20 > Node.PciDev =3D Parent; > - Node.Length =3D PciDevice->RomSize; > Node.Alignment =3D 0; > Node.Bar =3D PPB_MEM32_RANGE; > Node.ResType =3D PciBarTypeMem32; > @@ -1677,10 +1685,33 @@ ProgrameUpstreamBridgeForRom ( > // Program PPB to only open a single <=3D 16MB apperture > // > 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; > + > PCI_ENABLE_COMMAND_REGISTER (Parent, > EFI_PCI_COMMAND_MEMORY_SPACE); > } else { > - InitializePpb (Parent); > + // > + // Cover 32bit MMIO for devices below the bridge. > + // > + Node.Length =3D Parent->PciBar[Node.Bar].Length; > + ProgramPpbApperture (Parent->PciBar[Node.Bar].BaseAddress, > &Node); > PCI_DISABLE_COMMAND_REGISTER (Parent, > EFI_PCI_COMMAND_MEMORY_SPACE); > } >=20 > -- > 2.16.1.windows.1