From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56422) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLZS8-0006MG-PT for qemu-devel@nongnu.org; Tue, 19 Jan 2016 11:48:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLZS7-00053J-OL for qemu-devel@nongnu.org; Tue, 19 Jan 2016 11:48:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLZS7-00053F-Go for qemu-devel@nongnu.org; Tue, 19 Jan 2016 11:48:19 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 9D562C09FAA3 for ; Tue, 19 Jan 2016 16:48:18 +0000 (UTC) References: <20160118230413.2140.8336.stgit@gimli.home> <569DF9D4.3050206@gmail.com> <1453221507.32741.240.camel@redhat.com> From: Marcel Apfelbaum Message-ID: <569E68CE.9030809@redhat.com> Date: Tue, 19 Jan 2016 18:48:14 +0200 MIME-Version: 1.0 In-Reply-To: <1453221507.32741.240.camel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RE-RESEND PATCH] pci: Adjust PCI config limit based on bus topology List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson , mst@redhat.com Cc: qemu-devel@nongnu.org On 01/19/2016 06:38 PM, Alex Williamson wrote: > On Tue, 2016-01-19 at 10:54 +0200, Marcel Apfelbaum wrote: >> On 01/19/2016 01:06 AM, Alex Williamson wrote: >>> A conventional PCI bus does not support config space accesses above >>> the standard 256 byte configuration space. PCIe-to-PCI bridges are >>> not permitted to forward transactions if the extended register >>> address >>> field is non-zero and must handle it as an unsupported request >>> (PCIe >>> bridge spec rev 1.0, 4.1.3, 4.1.4). Therefore, we should not >>> support >>> extended config space if there is a conventional bus anywhere on >>> the >>> path to a device. >>> >>> Signed-off-by: Alex Williamson >>> --- >>> Previous postings: >>> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg05384.html >>> https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg02422.html >>> >>> hw/pci/pci_host.c | 26 ++++++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c >>> index 49f59a5..3a3e294 100644 >>> --- a/hw/pci/pci_host.c >>> +++ b/hw/pci/pci_host.c >>> @@ -19,6 +19,7 @@ >>> */ >>> >>> #include "hw/pci/pci.h" >>> +#include "hw/pci/pci_bridge.h" >>> #include "hw/pci/pci_host.h" >>> #include "hw/pci/pci_bus.h" >>> #include "trace.h" >>> @@ -49,9 +50,29 @@ static inline PCIDevice >>> *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) >>> return pci_find_device(bus, bus_num, devfn); >>> } >>> >>> +static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit) >>> +{ >>> + if (*limit > PCI_CONFIG_SPACE_SIZE) { >>> + if (!pci_bus_is_express(bus)) { >>> + *limit = PCI_CONFIG_SPACE_SIZE; >>> + return; >>> + } >>> + >>> + if (!pci_bus_is_root(bus)) { >>> + PCIDevice *bridge = pci_bridge_get_device(bus); >>> + pci_adjust_config_limit(bridge->bus, limit); >>> + } >>> + } >>> +} >>> + >>> void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t >>> addr, >>> uint32_t limit, uint32_t val, >>> uint32_t len) >>> { >>> + pci_adjust_config_limit(pci_dev->bus, &limit); >>> + if (limit <= addr) { >>> + return; >>> + } >>> + >>> assert(len <= 4); >>> /* non-zero functions are only exposed when function 0 is >>> present, >>> * allowing direct removal of unexposed functions. >>> @@ -70,6 +91,11 @@ uint32_t pci_host_config_read_common(PCIDevice >>> *pci_dev, uint32_t addr, >>> { >>> uint32_t ret; >>> >>> + pci_adjust_config_limit(pci_dev->bus, &limit); >>> + if (limit <= addr) { >>> + return ~0x0; >>> + } >>> + >>> assert(len <= 4); >>> /* non-zero functions are only exposed when function 0 is >>> present, >>> * allowing direct removal of unexposed functions. >>> >>> >> >> Quick question: could we check the limit as part of pci_config_size? > > If we plugin a physical PCIe card behind a bridge that masks access to > the extended configuration space, does the config size for that card > change? No, it's up to the bridge to drop the transactions, which > seems like how we probably want to handle it in QEMU as well. Is what I thought. Thanks, Marcel > >> Anyway, it looks OK to me. >> >> Reviewed-by: Marcel Apfelbaum > > Thanks, > Alex >