From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 2546E21D0918C for ; Thu, 27 Jul 2017 04:21:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3F41AABB31; Thu, 27 Jul 2017 11:23:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3F41AABB31 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-71.phx2.redhat.com [10.3.116.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id 08B145C269; Thu, 27 Jul 2017 11:23:25 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Star Zeng , Marcel Apfelbaum , Aleksandr Bezzubikov References: <20170727030537.51784-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <8efaf70a-ac43-5f29-5ff3-cd0dbb0f5d91@redhat.com> Date: Thu, 27 Jul 2017 13:23:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170727030537.51784-1-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 27 Jul 2017 11:23:29 +0000 (UTC) Subject: 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: Thu, 27 Jul 2017 11:21:26 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Ray, please excuse me for hijacking this thread a little bit, but I'm very happy that you happen to be looking at this exact part of the code, which is also what I've been discussing with Marcel and Aleksandr (CC'd) due to a different reason. (The relevant sub-thread is at Allow RedHat PCI bridges reserve more buses than necessary during init http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f-2ceb6180f69a@redhat.com 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.) In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource paddings, from the EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() function. QEMU has no "root" hotplug controllers, only "non-root" hotplug controllers (using the terminology from "12.6 PCI Hot Plug PCI Initialization Protocol" 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. According to the PI spec, this is a valid thing to do: * From "12.5 Sample Implementation for a Platform Containing PCI Hot Plug* Slots": > [...] 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. [...] * From "12.6 PCI Hot Plug PCI Initialization Protocol, EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()": > [...] 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. [...] I searched PciBusDxe for GetResourcePadding() call sites, and there are two: (1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function GetResourcePaddingForHpb(). This call site seems to work as follows, for our purposes: 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 // 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 // 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. (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. 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. The end result is that bus number paddings are not considered at all for "non-root" hotplug controllers: - 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. This doesn't seem to match what the PI spec says. Is this behavior intentional in PciBusDxe? If so, what is the reason for it? Thank you! Laszlo 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 = 0; > + NextDescriptors = Descriptors; > Status = PciGetBusRange ( > - &Descriptors, > + &NextDescriptors, > NULL, > NULL, > &BusRange >