From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:32807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFQSX-0006mN-FQ for qemu-devel@nongnu.org; Tue, 12 Mar 2013 10:45:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UFQSS-0003rv-12 for qemu-devel@nongnu.org; Tue, 12 Mar 2013 10:45:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17584) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFQSR-0003rU-QL for qemu-devel@nongnu.org; Tue, 12 Mar 2013 10:45:39 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2CEjct1005197 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 12 Mar 2013 10:45:38 -0400 Date: Tue, 12 Mar 2013 16:46:02 +0200 From: "Michael S. Tsirkin" Message-ID: <20130312144602.GA8837@redhat.com> References: <20130311210254.5171.78669.stgit@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130311210254.5171.78669.stgit@bling.home> Subject: Re: [Qemu-devel] [RFC PATCH] pci: Differentiate PCI Express bus List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org On Mon, Mar 11, 2013 at 03:18:49PM -0600, Alex Williamson wrote: > When creating capabilities devices need to know what kind of bus > they're on. If we're on an express bus without a parent_dev, then > we're on the root complex and need to use integrated endpoints > rather than standard endpoints. When we're on an express bus with > a parent_dev we need to negotiate link parameters so that the > endpoint doesn't claim it's running x16, 8GT/s while the root port > above it claims x1, 2.5GT/s capability. > > Signed-off-by: Alex Williamson > --- > > This feels a bit kludgy so I'm sending this out as an RFC looking for > suggestions. I played a little with creating a PCIBusClass, putting > is_express on the class, You actually don't even need is_express if you do this, just add a wrapper that checks the type. > and instantiating a TYPE_PCIE_BUS that uses > TYPE_PCI_BUS as it's parent, but that gets overly complicated and > means that any time we instantiate a bus we need to figure out whether > to use legacy or express. This last is probably a plus, not a minus. > Any better ideas? Thanks, > > Alex If I understand correctly, the issue is that the root bus does not have a parent device? > hw/ioh3420.c | 2 ++ > hw/pci/pci_bus.h | 2 ++ > hw/q35.c | 1 + > hw/xio3130_downstream.c | 2 ++ > hw/xio3130_upstream.c | 2 ++ > 5 files changed, 9 insertions(+) This isn't too bad though I'd prefer accessing is_express through an API. E.g. pci_bus_set_type() > diff --git a/hw/ioh3420.c b/hw/ioh3420.c > index 95bceb5..186a46f 100644 > --- a/hw/ioh3420.c > +++ b/hw/ioh3420.c > @@ -102,6 +102,8 @@ static int ioh3420_initfn(PCIDevice *d) > return rc; > } > > + br->sec_bus->is_express = true; > + > pcie_port_init_reg(d); > > rc = pci_bridge_ssvid_init(d, IOH_EP_SSVID_OFFSET, > diff --git a/hw/pci/pci_bus.h b/hw/pci/pci_bus.h > index aef559a..a325f46 100644 > --- a/hw/pci/pci_bus.h > +++ b/hw/pci/pci_bus.h > @@ -34,6 +34,8 @@ struct PCIBus { > Keep a count of the number of devices with raised IRQs. */ > int nirq; > int *irq_count; > + > + bool is_express; /* PCI Express bus or Legacy bus? */ > }; > > typedef struct PCIBridgeWindows PCIBridgeWindows; > diff --git a/hw/q35.c b/hw/q35.c > index efebc27..f5fdcb0 100644 > --- a/hw/q35.c > +++ b/hw/q35.c > @@ -55,6 +55,7 @@ static int q35_host_init(SysBusDevice *dev) > } > b = pci_bus_new(&s->host.pci.busdev.qdev, "pcie.0", > s->mch.pci_address_space, s->mch.address_space_io, 0); > + b->is_express = true; > s->host.pci.bus = b; > qdev_set_parent_bus(DEVICE(&s->mch), BUS(b)); > qdev_init_nofail(DEVICE(&s->mch)); > diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c > index 7f00bc8..600ec06 100644 > --- a/hw/xio3130_downstream.c > +++ b/hw/xio3130_downstream.c > @@ -66,6 +66,8 @@ static int xio3130_downstream_initfn(PCIDevice *d) > return rc; > } > > + br->sec_bus->is_express = true; > + > pcie_port_init_reg(d); > > rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, > diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c > index 70b15d3..b6fea60 100644 > --- a/hw/xio3130_upstream.c > +++ b/hw/xio3130_upstream.c > @@ -62,6 +62,8 @@ static int xio3130_upstream_initfn(PCIDevice *d) > return rc; > } > > + br->sec_bus->is_express = true; > + > pcie_port_init_reg(d); > > rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,