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

[Michael S. Tsirkin]

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.

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