Re: [Qemu-devel] [RFC for-2.10 1/3] pci/pcie: Make a consistent helper for switching PCI/PCIe "hybrid" devices

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

On Wed, Apr 26, 2017 at 06:23:58PM +0300, Michael S. Tsirkin wrote:
> On Tue, Mar 28, 2017 at 01:16:49PM +1100, David Gibson wrote:
> > virtio-pci and XHCI are "hybrid" devices in the sense that they can present
> > themselves as either PCIe or plain PCI devices depending on the machine
> > and bus they're connected to.
> > 
> > For virtio-pci to present as PCIe it requires that it's connected to a PCIe
> > bus and that it's not a root bus - this is to ensure that the device is
> > connected via a PCIe root port or downstream port rather than being a
> > integrated endpoint.  Some guests (Windows in particular AIUI) don't really
> > cope with PCIe integrated endpoints.
> > 
> > For XHCI it only checks that the bus is PCIe, but that probably means it
> > would cause problems if attached as an integrated devices directly to a
> > PCIe root bus.
> > 
> > This patch makes the test consistent between XHCI and virtio-pci, and
> > clarifies things by having them both use a new 'pci_allow_hybrid_pcie()'
> > helper which performs the same check as virtio-pci.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/pci/pci.c           | 7 +++++++
> >  hw/usb/hcd-xhci.c      | 2 +-
> >  hw/virtio/virtio-pci.c | 3 +--
> >  include/hw/pci/pci.h   | 1 +
> >  4 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bd8043c..779787b 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -390,6 +390,13 @@ bool pci_bus_is_root(PCIBus *bus)
> >      return PCI_BUS_GET_CLASS(bus)->is_root(bus);
> >  }
> >  
> > +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> > +{
> > +    PCIBus *bus = pci_dev->bus;
> > +
> > +    return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
> > +}
> > +
> >  void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> >                           const char *name,
> >                           MemoryRegion *address_space_mem,
> 
> I'd prefer pci_allow_hybrid_pci_pcie.

Ok, I've made that change for the next spin (aimed at 2.11, obviously).

> 
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > index f0af852..a7ff4fd 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -3619,7 +3619,7 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
> >                       PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64,
> >                       &xhci->mem);
> >  
> > -    if (pci_bus_is_express(dev->bus) ||
> > +    if (pci_allow_hybrid_pcie(dev) ||
> >          xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
> >          ret = pcie_endpoint_cap_init(dev, 0xa0);
> >          assert(ret >= 0);
> 
> This seems to change the behaviour for xhci on a root bus - what
> am I missing?

Nothing.  I didn't consider the backwards compat implications; I'll
fix it for the next spin.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]