From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 70D3F21D49C8C for ; Fri, 28 Jul 2017 03:18:12 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Jul 2017 03:20:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,425,1496127600"; d="scan'208";a="1177298295" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 28 Jul 2017 03:20:01 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 28 Jul 2017 03:19:28 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 28 Jul 2017 03:19:29 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.151]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.122]) with mapi id 14.03.0319.002; Fri, 28 Jul 2017 18:19:27 +0800 From: "Ni, Ruiyu" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: Marcel Apfelbaum , Aleksandr Bezzubikov , "Zeng, Star" Thread-Topic: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top] Thread-Index: AQHTBsrevAYgPYqg2EaiQTslqwnp8aJpBVXQ Date: Fri, 28 Jul 2017 10:19:26 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5B9C4316@SHSMSX104.ccr.corp.intel.com> References: <20170727030537.51784-1-ruiyu.ni@intel.com> <8efaf70a-ac43-5f29-5ff3-cd0dbb0f5d91@redhat.com> In-Reply-To: <8efaf70a-ac43-5f29-5ff3-cd0dbb0f5d91@redhat.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: bus number padding [was: MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top] X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Jul 2017 10:18:12 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Thanks for raising this questions. Please see my embedded reply in below Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Thursday, July 27, 2017 7:23 PM > To: Ni, Ruiyu ; edk2-devel@lists.01.org > Cc: Marcel Apfelbaum ; Aleksandr Bezzubikov > ; Zeng, Star > Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid > hang when BUS pad resource is not in top] >=20 > Hello Ray, >=20 > please excuse me for hijacking this thread a little bit, but I'm very hap= py that > you happen to be looking at this exact part of the code, which is also wh= at > I've been discussing with Marcel and Aleksandr (CC'd) due to a different > reason. >=20 > (The relevant sub-thread is at >=20 > Allow RedHat PCI bridges reserve more buses than necessary during init >=20 > http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f- > 2ceb6180f69a@redhat.com >=20 > but it's very long, so I'm not asking you to read that -- instead, please= let me > ask you our question independently and in a self-contained > form.) >=20 > In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource > paddings, from the > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() > function. >=20 > QEMU has no "root" hotplug controllers, only "non-root" hotplug controlle= rs > (using the terminology from "12.6 PCI Hot Plug PCI Initialization Protoco= l" in > the PI-1.6 spec). Therefore OVMF's > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty > array. > The BUS resource padding that we'd like to return from > GetResourcePadding() would be for "non-root" hotplug controllers. >=20 > According to the PI spec, this is a valid thing to do: >=20 > * From "12.5 Sample Implementation for a Platform Containing PCI > Hot Plug* Slots": >=20 > > [...] For all the root HPCs and the nonroot HPCs, call > > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() to obtain the > > amount of overallocation and add that amount to the requests from the > > physical devices. Reprogram the bus numbers by taking into account the > > bus resource padding information. [...] >=20 > * From "12.6 PCI Hot Plug PCI Initialization Protocol, > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()": >=20 > > [...] This member function is called for all the root HPCs and nonroot > > HPCs that are detected by the PCI bus enumerator. [...] The PCI bus > > driver is responsible for adding this resource request to the resource > > requests by the physical PCI devices. [...] >=20 > I searched PciBusDxe for GetResourcePadding() call sites, and there are > two: >=20 > (1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function > GetResourcePaddingForHpb(). This call site seems to work as follows, > for our purposes: >=20 > GetResourcePaddingPpb() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > GetResourcePaddingForHpb() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c] > gPciHotPlugInit->GetResourcePadding() > // > // and then save the resource descriptors, including the bus > // padding, in PciIoDevice->ResourcePaddingDescriptors > // >=20 > ResourcePaddingPolicy() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > ApplyResourcePadding() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > // > // translate IO and MMIO resources from > // PciDev->ResourcePaddingDescriptors to internal padding > // representation > // >=20 > This call site and processing seem to be active for "non-root" > hotplug controllers, but they do not consider bus numbers, only IO > and MMIO. [Ray] It's in bus allocation phase. So the PciBus driver only cares about t= he bus padding. PciBus driver firstly allocates the bus resource and programs the PCI-PCI b= ridges to set the bus numbers. After that, PciBus allocates the IO/MMIO resources and program the BARs in PCI devices & PCI-PCI bridges. So it's good here. >=20 > (2) In the PciScanBus() function, file > "MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another > GetResourcePadding() call -- your present patch is updating this > code part. >=20 > From the resource padding list returned by this GetResourcePadding() > invocation, PciBusDxe cares only about the bus numbers. I think > that's probably fine. However, in this location, > GetResourcePadding() is called *only* for "root" hotplug > controllers, of which QEMU has none. >=20 > The end result is that bus number paddings are not considered at all for > "non-root" hotplug controllers: [Ray] Bus number padding should be considered for "non-root" HPCs. So I agree it's a bug/gap I need to fix. I noticed that already before your= question. And I do have a plan to fix it though the priority is not very high. > - site (1) is active for "non-root" HPCs, but it ignores bus number > paddings, > - site (2) processes bus number paddings, but it is inactive for > "non-root" HPCs. >=20 > This doesn't seem to match what the PI spec says. Is this behavior intent= ional > in PciBusDxe? If so, what is the reason for it? [Ray] In summary, site(1) is good. Site(2) contains a bug/gap. Would you please submit a Bugzilla to record this issue? But can you just return the non-root HPCs as root for a workaround? What prevent you from reporting the HPC as non-root? My opinion is the differences between non-root and root HPCs are: 1. Explicitly initialization is needed for root HPCs 2. Root HPCs may not be detected as hot-pluggable by PciBus using standard = ways If your platform doesn't report any root HPCs, why not just report non-root= as root ones? >=20 > Thank you! > Laszlo >=20 > On 07/27/17 05:05, Ruiyu Ni wrote: > > PciScanBus() assumes the GetResourcePadding() puts BUS descriptor in > > the very beginning, if it's not, the Descriptors will be updated to > > point to middle of the pool buffer, which can cause > > FreePool(Descriptors) hang in DEBUG image. > > No functionality impact to RELEASE image. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Ruiyu Ni > > Cc: Star Zeng > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > index e1d62e8c21..8b076e8791 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > @@ -1,7 +1,7 @@ > > /** @file > > Internal library implementation for PCI Bus module. > > > > -Copyright (c) 2006 - 2016, Intel Corporation. All rights > > reserved.
> > +Copyright (c) 2006 - 2017, Intel Corporation. All rights > > +reserved.
> > (C) Copyright 2015 Hewlett Packard Enterprise Development LP
> > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License @@ -986,6 > > +986,7 @@ PciScanBus ( > > UINT64 PciAddress; > > EFI_HPC_PADDING_ATTRIBUTES Attributes; > > EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptors; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *NextDescriptors; > > UINT16 BusRange; > > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo; > > BOOLEAN BusPadding; > > @@ -1143,8 +1144,9 @@ PciScanBus ( > > } > > > > BusRange =3D 0; > > + NextDescriptors =3D Descriptors; > > Status =3D PciGetBusRange ( > > - &Descriptors, > > + &NextDescriptors, > > NULL, > > NULL, > > &BusRange > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel