Re: [Qemu-devel] [RFC PATCH] pci: Differentiate PCI Express bus

[Alex Williamson]

On Tue, 2013-03-12 at 16:46 +0200, Michael S. Tsirkin wrote:
> 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 <address@hidden>
> > ---
> > 
> > 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.

Yeah, that's true.

> > 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.

Ok, it just means either duplicating a lot of interfaces to make legacy
vs express functions or adding ugly is_express options to the functions
(ex. pci_bus_new).  Preference?

> > Any better ideas?  Thanks,
> > 
> > Alex
> 
> If I understand correctly, the issue is that the root bus does not
> have a parent device?

No, that's actually a feature that lets us determine if the express bus
is a root complex bus or normal express bus.  I'm worried about things
like xhci which calls pcie_cap_init() to give itself an endpoint
capability.  If it's connected to the root complex, that actually needs
to be an integrated endpoint or else windows won't use it.  Devices
probably don't want to care whether they're an endpoint or integrated
endpoint, so pcie_cap_init() should probably transparently change types
and drop the link capabilities.

We also need to add pseudo link negotiation.  Root ports and switches
should report maximum width and transfer rate capability and downstream
devices should call a function to negotiate link to the device
capabilities.  I don't think we can just look for is_express on
bus->parent_dev because the parent_dev could be a PCIe-to-PCI bridge.
So there seem to be a number of points where it would be convenient to
test express vs legacy on the bus.

> >  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()

Yep, this is just an example of the minimum footprint for such a change
pushing it as far out from the core as we can.  If you're onboard that
we need a way to differentiate the bus type I can make it more
integrated into the core.  Thanks,

Alex

> > 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,