[Qemu-devel] [RFC for-2.10 0/3] Rework handling of PCI/PCIe "hybrid" devices

[Qemu-devel] [RFC for-2.10 0/3] Rework handling of PCI/PCIe "hybrid" devices

[David Gibson]

A couple of devices - virtio-pci and XHCI - can present themselves to
the guest as either PCI or PCIe devices depending on how they're
attached.  However, the logic is a little different between the two
devices.  In addition the logic in virtio makes it difficult to put a
PCIe virtio device into a "pseries" guest because of the unusual way
the paravirtualized PCI bus works there.

This series makes the logic more consistent, and allows per-machine
overrides to address that.

Currently patch 3/3 shows a non-obvious side effect of this change.  A
PCIe virtio device is, by default, modern mode only, but the qtest
logic doesn't handle modern-only virtio devices correctly.  We work
around this by explicitly adding disable-legacy=off to the testcases.
It would probably be better to update libqos so that it can handle
modern virtio devices.

David Gibson (3):
  pci/pcie: Make a consistent helper for switching PCI/PCIe "hybrid"
    devices
  pci: Allow host bridges to override PCI/PCIe hybrid device behaviour
  pseries: Allow PCIe virtio and XHCI on pseries machine type

 hw/pci/pci.c              | 14 ++++++++++++++
 hw/ppc/spapr_pci.c        |  9 +++++++++
 hw/usb/hcd-xhci.c         |  2 +-
 hw/virtio/virtio-pci.c    |  3 +--
 include/hw/pci/pci.h      |  1 +
 include/hw/pci/pci_host.h |  1 +
 tests/virtio-9p-test.c    |  2 +-
 tests/virtio-blk-test.c   |  4 ++--
 tests/virtio-net-test.c   |  2 +-
 tests/virtio-scsi-test.c  |  2 +-
 10 files changed, 32 insertions(+), 8 deletions(-)

-- 
2.9.3

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

[David Gibson]

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,
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);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..9b34673 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
     VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
-    bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
-                     !pci_bus_is_root(pci_dev->bus);
+    bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
 
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..7b9a40f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
 
 PCIBus *pci_find_primary_bus(void);
 PCIBus *pci_device_root_bus(const PCIDevice *d);
+bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
 const char *pci_root_bus_path(PCIDevice *dev);
 PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
 int pci_qdev_find_device(const char *id, PCIDevice **pdev);
-- 
2.9.3

[Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[David Gibson]

Currently PCI/PCIe hybrid devices - that is, devices which can appear as
either plain PCI or PCIe depending on where they're attached - will only
appear in PCIe mode if they're attached to a PCIe bus via a root port or
downstream port.

This is correct for "standard" PCIe setups, but there are some platforms
which need different behaviour (notably "pseries" whose paravirtualized
PCI host bridges have some idiosyncracies).

This patch allows the host bridge to override the normal behaviour.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pci.c              | 11 +++++++++--
 include/hw/pci/pci_host.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 779787b..ac68065 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
 
 bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
 {
-    PCIBus *bus = pci_dev->bus;
+    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
+    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
+
+    if (hc->allow_hybrid_pcie) {
+        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
+    } else {
+        PCIBus *bus = pci_dev->bus;
 
-    return pci_bus_is_express(bus) && !pci_bus_is_root(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,
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index ba31595..ad03cca 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -54,6 +54,7 @@ typedef struct PCIHostBridgeClass {
     SysBusDeviceClass parent_class;
 
     const char *(*root_bus_path)(PCIHostState *, PCIBus *);
+    bool (*allow_hybrid_pcie)(PCIHostState *, PCIDevice *);
 } PCIHostBridgeClass;
 
 /* common internal helpers for PCI/PCIe hosts, cut off overflows */
-- 
2.9.3

[Qemu-devel] [RFC for-2.10 3/3] pseries: Allow PCIe virtio and XHCI on pseries machine type

[David Gibson]

pseries now allows PCIe devices (both emulated and VFIO), although its
PCI bus is in most respects a plain PCI bus - this uses paravirtualized
access methods to PCIe extended config space defined in the PAPR spec.

However, because the bus is not PCIe, it means that virtio-pci and XHCI
devices will present themselves as plain PCI rather than PCIe, which would
be preferable.

This patch uses the new hook to override the behaviour for such PCI/PCIe
"hybrid" devices to allow PCIe virtio-pci and XHCI on pseries.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c       | 9 +++++++++
 tests/virtio-9p-test.c   | 2 +-
 tests/virtio-blk-test.c  | 4 ++--
 tests/virtio-net-test.c  | 2 +-
 tests/virtio-scsi-test.c | 2 +-
 5 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 98c52e4..7686f7f 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1979,6 +1979,14 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
     return sphb->dtbusname;
 }
 
+static bool spapr_phb_allow_hybrid_pcie(PCIHostState *host_bridge,
+                                        PCIDevice *pci_dev)
+{
+    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(host_bridge);
+
+    return sphb->pcie_ecs;
+}
+
 static void spapr_phb_class_init(ObjectClass *klass, void *data)
 {
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
@@ -1986,6 +1994,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     HotplugHandlerClass *hp = HOTPLUG_HANDLER_CLASS(klass);
 
     hc->root_bus_path = spapr_phb_root_bus_path;
+    hc->allow_hybrid_pcie = spapr_phb_allow_hybrid_pcie;
     dc->realize = spapr_phb_realize;
     dc->props = spapr_phb_properties;
     dc->reset = spapr_phb_reset;
diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 43a1ad8..ae0d51e 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -32,7 +32,7 @@ static QVirtIO9P *qvirtio_9p_start(const char *driver)
 {
     const char *arch = qtest_get_arch();
     const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s "
-                      "-device %s,fsdev=fsdev0,mount_tag=%s";
+                      "-device %s,fsdev=fsdev0,mount_tag=%s,disable-legacy=off";
     QVirtIO9P *v9p = g_new0(QVirtIO9P, 1);
 
     v9p->test_share = g_strdup("/tmp/qtest.XXXXXX");
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 1eee95d..5fb7882 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -65,7 +65,7 @@ static QOSState *pci_test_start(void)
     const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
                       "-drive if=none,id=drive1,file=/dev/null,format=raw "
                       "-device virtio-blk-pci,id=drv0,drive=drive0,"
-                      "addr=%x.%x";
+                      "addr=%x.%x,disable-legacy=off";
 
     tmp_path = drive_create();
 
@@ -656,7 +656,7 @@ static void pci_hotplug(void)
 
     /* plug secondary disk */
     qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
-                          "'drive': 'drive1'");
+                          "'drive': 'drive1', 'disable-legacy': 'off'");
 
     dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
     g_assert(dev);
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 8f94360..a35d87b 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -55,7 +55,7 @@ static QOSState *pci_test_start(int socket)
 {
     const char *arch = qtest_get_arch();
     const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
-                      "virtio-net-pci,netdev=hs0";
+                      "virtio-net-pci,netdev=hs0,disable-legacy=off";
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         return qtest_pc_boot(cmd, socket);
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 0eabd56..5a802d9 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -36,7 +36,7 @@ static QOSState *qvirtio_scsi_start(const char *extra_opts)
 {
     const char *arch = qtest_get_arch();
     const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
-                      "-device virtio-scsi-pci,id=vs0 "
+                      "-device virtio-scsi-pci,id=vs0,disable-legacy=off "
                       "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-- 
2.9.3

Re: [Qemu-devel] [RFC for-2.10 3/3] pseries: Allow PCIe virtio and XHCI on pseries machine type

[Alexey Kardashevskiy]

On 28/03/17 13:16, David Gibson wrote:
> pseries now allows PCIe devices (both emulated and VFIO), although its
> PCI bus is in most respects a plain PCI bus - this uses paravirtualized
> access methods to PCIe extended config space defined in the PAPR spec.
> 
> However, because the bus is not PCIe, it means that virtio-pci and XHCI
> devices will present themselves as plain PCI rather than PCIe, which would
> be preferable.
> 
> This patch uses the new hook to override the behaviour for such PCI/PCIe
> "hybrid" devices to allow PCIe virtio-pci and XHCI on pseries.


Not clear what all these tests/virtio-*.c changes are for and why here -
does "make check" break if you do not enforce disable-legacy=off?


> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c       | 9 +++++++++
>  tests/virtio-9p-test.c   | 2 +-
>  tests/virtio-blk-test.c  | 4 ++--
>  tests/virtio-net-test.c  | 2 +-
>  tests/virtio-scsi-test.c | 2 +-
>  5 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 98c52e4..7686f7f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1979,6 +1979,14 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
>      return sphb->dtbusname;
>  }
>  
> +static bool spapr_phb_allow_hybrid_pcie(PCIHostState *host_bridge,
> +                                        PCIDevice *pci_dev)
> +{
> +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(host_bridge);
> +
> +    return sphb->pcie_ecs;
> +}
> +
>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> @@ -1986,6 +1994,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>      HotplugHandlerClass *hp = HOTPLUG_HANDLER_CLASS(klass);
>  
>      hc->root_bus_path = spapr_phb_root_bus_path;
> +    hc->allow_hybrid_pcie = spapr_phb_allow_hybrid_pcie;
>      dc->realize = spapr_phb_realize;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> index 43a1ad8..ae0d51e 100644
> --- a/tests/virtio-9p-test.c
> +++ b/tests/virtio-9p-test.c
> @@ -32,7 +32,7 @@ static QVirtIO9P *qvirtio_9p_start(const char *driver)
>  {
>      const char *arch = qtest_get_arch();
>      const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s "
> -                      "-device %s,fsdev=fsdev0,mount_tag=%s";
> +                      "-device %s,fsdev=fsdev0,mount_tag=%s,disable-legacy=off";
>      QVirtIO9P *v9p = g_new0(QVirtIO9P, 1);
>  
>      v9p->test_share = g_strdup("/tmp/qtest.XXXXXX");
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 1eee95d..5fb7882 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -65,7 +65,7 @@ static QOSState *pci_test_start(void)
>      const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
>                        "-drive if=none,id=drive1,file=/dev/null,format=raw "
>                        "-device virtio-blk-pci,id=drv0,drive=drive0,"
> -                      "addr=%x.%x";
> +                      "addr=%x.%x,disable-legacy=off";
>  
>      tmp_path = drive_create();
>  
> @@ -656,7 +656,7 @@ static void pci_hotplug(void)
>  
>      /* plug secondary disk */
>      qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
> -                          "'drive': 'drive1'");
> +                          "'drive': 'drive1', 'disable-legacy': 'off'");
>  
>      dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
>      g_assert(dev);
> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index 8f94360..a35d87b 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -55,7 +55,7 @@ static QOSState *pci_test_start(int socket)
>  {
>      const char *arch = qtest_get_arch();
>      const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
> -                      "virtio-net-pci,netdev=hs0";
> +                      "virtio-net-pci,netdev=hs0,disable-legacy=off";
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          return qtest_pc_boot(cmd, socket);
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index 0eabd56..5a802d9 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -36,7 +36,7 @@ static QOSState *qvirtio_scsi_start(const char *extra_opts)
>  {
>      const char *arch = qtest_get_arch();
>      const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
> -                      "-device virtio-scsi-pci,id=vs0 "
> +                      "-device virtio-scsi-pci,id=vs0,disable-legacy=off "
>                        "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> 


-- 
Alexey

Re: [Qemu-devel] [RFC for-2.10 3/3] pseries: Allow PCIe virtio and XHCI on pseries machine type

[David Gibson]

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

On Wed, Mar 29, 2017 at 01:20:50PM +1100, Alexey Kardashevskiy wrote:
> On 28/03/17 13:16, David Gibson wrote:
> > pseries now allows PCIe devices (both emulated and VFIO), although its
> > PCI bus is in most respects a plain PCI bus - this uses paravirtualized
> > access methods to PCIe extended config space defined in the PAPR spec.
> > 
> > However, because the bus is not PCIe, it means that virtio-pci and XHCI
> > devices will present themselves as plain PCI rather than PCIe, which would
> > be preferable.
> > 
> > This patch uses the new hook to override the behaviour for such PCI/PCIe
> > "hybrid" devices to allow PCIe virtio-pci and XHCI on pseries.
> 
> 
> Not clear what all these tests/virtio-*.c changes are for and why here -
> does "make check" break if you do not enforce disable-legacy=off?

Yes it does.  There's an explanation in the 0/3 message.

> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c       | 9 +++++++++
> >  tests/virtio-9p-test.c   | 2 +-
> >  tests/virtio-blk-test.c  | 4 ++--
> >  tests/virtio-net-test.c  | 2 +-
> >  tests/virtio-scsi-test.c | 2 +-
> >  5 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 98c52e4..7686f7f 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1979,6 +1979,14 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
> >      return sphb->dtbusname;
> >  }
> >  
> > +static bool spapr_phb_allow_hybrid_pcie(PCIHostState *host_bridge,
> > +                                        PCIDevice *pci_dev)
> > +{
> > +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(host_bridge);
> > +
> > +    return sphb->pcie_ecs;
> > +}
> > +
> >  static void spapr_phb_class_init(ObjectClass *klass, void *data)
> >  {
> >      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> > @@ -1986,6 +1994,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
> >      HotplugHandlerClass *hp = HOTPLUG_HANDLER_CLASS(klass);
> >  
> >      hc->root_bus_path = spapr_phb_root_bus_path;
> > +    hc->allow_hybrid_pcie = spapr_phb_allow_hybrid_pcie;
> >      dc->realize = spapr_phb_realize;
> >      dc->props = spapr_phb_properties;
> >      dc->reset = spapr_phb_reset;
> > diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> > index 43a1ad8..ae0d51e 100644
> > --- a/tests/virtio-9p-test.c
> > +++ b/tests/virtio-9p-test.c
> > @@ -32,7 +32,7 @@ static QVirtIO9P *qvirtio_9p_start(const char *driver)
> >  {
> >      const char *arch = qtest_get_arch();
> >      const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s "
> > -                      "-device %s,fsdev=fsdev0,mount_tag=%s";
> > +                      "-device %s,fsdev=fsdev0,mount_tag=%s,disable-legacy=off";
> >      QVirtIO9P *v9p = g_new0(QVirtIO9P, 1);
> >  
> >      v9p->test_share = g_strdup("/tmp/qtest.XXXXXX");
> > diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> > index 1eee95d..5fb7882 100644
> > --- a/tests/virtio-blk-test.c
> > +++ b/tests/virtio-blk-test.c
> > @@ -65,7 +65,7 @@ static QOSState *pci_test_start(void)
> >      const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
> >                        "-drive if=none,id=drive1,file=/dev/null,format=raw "
> >                        "-device virtio-blk-pci,id=drv0,drive=drive0,"
> > -                      "addr=%x.%x";
> > +                      "addr=%x.%x,disable-legacy=off";
> >  
> >      tmp_path = drive_create();
> >  
> > @@ -656,7 +656,7 @@ static void pci_hotplug(void)
> >  
> >      /* plug secondary disk */
> >      qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
> > -                          "'drive': 'drive1'");
> > +                          "'drive': 'drive1', 'disable-legacy': 'off'");
> >  
> >      dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
> >      g_assert(dev);
> > diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> > index 8f94360..a35d87b 100644
> > --- a/tests/virtio-net-test.c
> > +++ b/tests/virtio-net-test.c
> > @@ -55,7 +55,7 @@ static QOSState *pci_test_start(int socket)
> >  {
> >      const char *arch = qtest_get_arch();
> >      const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
> > -                      "virtio-net-pci,netdev=hs0";
> > +                      "virtio-net-pci,netdev=hs0,disable-legacy=off";
> >  
> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> >          return qtest_pc_boot(cmd, socket);
> > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> > index 0eabd56..5a802d9 100644
> > --- a/tests/virtio-scsi-test.c
> > +++ b/tests/virtio-scsi-test.c
> > @@ -36,7 +36,7 @@ static QOSState *qvirtio_scsi_start(const char *extra_opts)
> >  {
> >      const char *arch = qtest_get_arch();
> >      const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
> > -                      "-device virtio-scsi-pci,id=vs0 "
> > +                      "-device virtio-scsi-pci,id=vs0,disable-legacy=off "
> >                        "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
> >  
> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> > 
> 
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[Eduardo Habkost]

On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> either plain PCI or PCIe depending on where they're attached - will only
> appear in PCIe mode if they're attached to a PCIe bus via a root port or
> downstream port.
> 
> This is correct for "standard" PCIe setups, but there are some platforms
> which need different behaviour (notably "pseries" whose paravirtualized
> PCI host bridges have some idiosyncracies).
> 
> This patch allows the host bridge to override the normal behaviour.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/pci/pci.c              | 11 +++++++++--
>  include/hw/pci/pci_host.h |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 779787b..ac68065 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
>  
>  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)

Why are we asking the device about allowing hybrid pcie, and not
the bus itself? Shouldn't we be able to query this before the
device is plugged, and before pci_dev->bus is even set?

In other words, why pci_allow_hyberid_pcie() and
pci_device_get_bus() get a PCIDevice* argument instead of a
PCIBus* argument?

>  {
> -    PCIBus *bus = pci_dev->bus;
> +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);

There's something I don't understand completely: what exactly
guarantees that pci_device_root_bus(...)->qbus.parent is always
going to be a PCI_HOST_BRIDGE?

> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> +
> +    if (hc->allow_hybrid_pcie) {
> +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> +    } else {
> +        PCIBus *bus = pci_dev->bus;
>  
> -    return pci_bus_is_express(bus) && !pci_bus_is_root(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,
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index ba31595..ad03cca 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -54,6 +54,7 @@ typedef struct PCIHostBridgeClass {
>      SysBusDeviceClass parent_class;
>  
>      const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> +    bool (*allow_hybrid_pcie)(PCIHostState *, PCIDevice *);
>  } PCIHostBridgeClass;
>  
>  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> -- 
> 2.9.3
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[David Gibson]

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

On Mon, Apr 17, 2017 at 03:30:46PM -0300, Eduardo Habkost wrote:
> On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> > Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> > either plain PCI or PCIe depending on where they're attached - will only
> > appear in PCIe mode if they're attached to a PCIe bus via a root port or
> > downstream port.
> > 
> > This is correct for "standard" PCIe setups, but there are some platforms
> > which need different behaviour (notably "pseries" whose paravirtualized
> > PCI host bridges have some idiosyncracies).
> > 
> > This patch allows the host bridge to override the normal behaviour.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/pci/pci.c              | 11 +++++++++--
> >  include/hw/pci/pci_host.h |  1 +
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 779787b..ac68065 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
> >  
> >  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> 
> Why are we asking the device about allowing hybrid pcie, and not
> the bus itself? Shouldn't we be able to query this before the
> device is plugged, and before pci_dev->bus is even set?
> 
> In other words, why pci_allow_hyberid_pcie() and
> pci_device_get_bus() get a PCIDevice* argument instead of a
> PCIBus* argument?

Ah good point.  I made it a PCIDevice simply for convenience, but
you're right we should be able to query before the device is plugged.

> 
> >  {
> > -    PCIBus *bus = pci_dev->bus;
> > +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
> 
> There's something I don't understand completely: what exactly
> guarantees that pci_device_root_bus(...)->qbus.parent is always
> going to be a PCI_HOST_BRIDGE?

Well, by definition whatever is above the root bus isn't PCI, which
pretty much means it has to be a PCI host bridge.  A machine could
break this assumption, but I think that would be a bug.  We use this
same pattern to find a PCI device's (or bus's) host bridge in other
places - there doesn't appear to be another way.

> > +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> > +
> > +    if (hc->allow_hybrid_pcie) {
> > +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> > +    } else {
> > +        PCIBus *bus = pci_dev->bus;
> >  
> > -    return pci_bus_is_express(bus) && !pci_bus_is_root(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,
> > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> > index ba31595..ad03cca 100644
> > --- a/include/hw/pci/pci_host.h
> > +++ b/include/hw/pci/pci_host.h
> > @@ -54,6 +54,7 @@ typedef struct PCIHostBridgeClass {
> >      SysBusDeviceClass parent_class;
> >  
> >      const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> > +    bool (*allow_hybrid_pcie)(PCIHostState *, PCIDevice *);
> >  } PCIHostBridgeClass;
> >  
> >  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[Eduardo Habkost]

On Tue, Apr 18, 2017 at 12:21:51PM +1000, David Gibson wrote:
> On Mon, Apr 17, 2017 at 03:30:46PM -0300, Eduardo Habkost wrote:
> > On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> > > Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> > > either plain PCI or PCIe depending on where they're attached - will only
> > > appear in PCIe mode if they're attached to a PCIe bus via a root port or
> > > downstream port.
> > > 
> > > This is correct for "standard" PCIe setups, but there are some platforms
> > > which need different behaviour (notably "pseries" whose paravirtualized
> > > PCI host bridges have some idiosyncracies).
> > > 
> > > This patch allows the host bridge to override the normal behaviour.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/pci/pci.c              | 11 +++++++++--
> > >  include/hw/pci/pci_host.h |  1 +
> > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 779787b..ac68065 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
> > >  
> > >  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> > 
> > Why are we asking the device about allowing hybrid pcie, and not
> > the bus itself? Shouldn't we be able to query this before the
> > device is plugged, and before pci_dev->bus is even set?
> > 
> > In other words, why pci_allow_hyberid_pcie() and
> > pci_device_get_bus() get a PCIDevice* argument instead of a
> > PCIBus* argument?
> 
> Ah good point.  I made it a PCIDevice simply for convenience, but
> you're right we should be able to query before the device is plugged.

Understood.

> 
> > 
> > >  {
> > > -    PCIBus *bus = pci_dev->bus;
> > > +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
> > 
> > There's something I don't understand completely: what exactly
> > guarantees that pci_device_root_bus(...)->qbus.parent is always
> > going to be a PCI_HOST_BRIDGE?
> 
> Well, by definition whatever is above the root bus isn't PCI, which
> pretty much means it has to be a PCI host bridge.  A machine could
> break this assumption, but I think that would be a bug.  We use this
> same pattern to find a PCI device's (or bus's) host bridge in other
> places - there doesn't appear to be another way.

After taking a better look at the way PCI buses are created, I
agree it's OK. Maybe a
  PCIHostState *pci_host_bridge(PCIBus *bus)
helper would be useful?

Anyway, both pci_bus_root_bus() and pci_host_bridge() could be
implemented as follow-ups if you prefer, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But I still have another suggestion:

> 
> > > +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> > > +
> > > +    if (hc->allow_hybrid_pcie) {
> > > +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> > > +    } else {
> > > +        PCIBus *bus = pci_dev->bus;
> > >  
> > > -    return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
> > > +        return pci_bus_is_express(bus) && !pci_bus_is_root(bus);

Maybe it's a matter of personal taste, but instead of requiring
an explicit check for NULL hc->allow_hybrid_pcie, why not set a
default hc->allow_hybrid_pcie() implementation on
TYPE_PCI_HOST_BRIDGE that returns
(pci_bus_is_express(bus) && !pci_bus_is_root(bus))?

-- 
Eduardo

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

[Marcel Apfelbaum]

On 03/28/2017 05:16 AM, 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);
>  }
>

Hi David,

> +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)

I agree with the patch but I think the function name
may be better.
It actually queries if a hybrid device is pcie on this slot.

> +{
> +    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,
> 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) ||

Gerd agreed with this change, but I suppose we need a compat property since
is a behavior change.

Thanks,
Marcel


>          xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
>          ret = pcie_endpoint_cap_init(dev, 0xa0);
>          assert(ret >= 0);
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..9b34673 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
>      VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> -    bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
> -                     !pci_bus_is_root(pci_dev->bus);
> +    bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
>
>      if (!kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..7b9a40f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
>
>  PCIBus *pci_find_primary_bus(void);
>  PCIBus *pci_device_root_bus(const PCIDevice *d);
> +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
>  const char *pci_root_bus_path(PCIDevice *dev);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[Marcel Apfelbaum]

On 04/18/2017 05:33 PM, Eduardo Habkost wrote:
> On Tue, Apr 18, 2017 at 12:21:51PM +1000, David Gibson wrote:
>> On Mon, Apr 17, 2017 at 03:30:46PM -0300, Eduardo Habkost wrote:
>>> On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
>>>> Currently PCI/PCIe hybrid devices - that is, devices which can appear as
>>>> either plain PCI or PCIe depending on where they're attached - will only
>>>> appear in PCIe mode if they're attached to a PCIe bus via a root port or
>>>> downstream port.
>>>>
>>>> This is correct for "standard" PCIe setups, but there are some platforms
>>>> which need different behaviour (notably "pseries" whose paravirtualized
>>>> PCI host bridges have some idiosyncracies).
>>>>
>>>> This patch allows the host bridge to override the normal behaviour.
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>>  hw/pci/pci.c              | 11 +++++++++--
>>>>  include/hw/pci/pci_host.h |  1 +
>>>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index 779787b..ac68065 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
>>>>
>>>>  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
>>>
>>> Why are we asking the device about allowing hybrid pcie, and not
>>> the bus itself? Shouldn't we be able to query this before the
>>> device is plugged, and before pci_dev->bus is even set?
>>>
>>> In other words, why pci_allow_hyberid_pcie() and
>>> pci_device_get_bus() get a PCIDevice* argument instead of a
>>> PCIBus* argument?
>>
>> Ah good point.  I made it a PCIDevice simply for convenience, but
>> you're right we should be able to query before the device is plugged.
>
> Understood.
>
>>
>>>
>>>>  {
>>>> -    PCIBus *bus = pci_dev->bus;
>>>> +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
>>>
>>> There's something I don't understand completely: what exactly
>>> guarantees that pci_device_root_bus(...)->qbus.parent is always
>>> going to be a PCI_HOST_BRIDGE?
>>
>> Well, by definition whatever is above the root bus isn't PCI, which
>> pretty much means it has to be a PCI host bridge.  A machine could
>> break this assumption, but I think that would be a bug.  We use this
>> same pattern to find a PCI device's (or bus's) host bridge in other
>> places - there doesn't appear to be another way.
>

Agreed, the root bus is always exposed by (some kind of) a host bridge.

> After taking a better look at the way PCI buses are created, I
> agree it's OK. Maybe a
>   PCIHostState *pci_host_bridge(PCIBus *bus)
> helper would be useful?
>

For sure.

> Anyway, both pci_bus_root_bus() and pci_host_bridge() could be
> implemented as follow-ups if you prefer, so:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>

Thanks,
Marcel

> But I still have another suggestion:
>
>>
>>>> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
>>>> +
>>>> +    if (hc->allow_hybrid_pcie) {
>>>> +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
>>>> +    } else {
>>>> +        PCIBus *bus = pci_dev->bus;
>>>>
>>>> -    return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
>>>> +        return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
>
> Maybe it's a matter of personal taste, but instead of requiring
> an explicit check for NULL hc->allow_hybrid_pcie, why not set a
> default hc->allow_hybrid_pcie() implementation on
> TYPE_PCI_HOST_BRIDGE that returns
> (pci_bus_is_express(bus) && !pci_bus_is_root(bus))?
>

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

[Michael S. Tsirkin]

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.

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

> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..9b34673 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
>      VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> -    bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
> -                     !pci_bus_is_root(pci_dev->bus);
> +    bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
>  
>      if (!kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

I don't actually remember why do we disable pcie on a root port.
Marcel, do you happen to know?

> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..7b9a40f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
>  
>  PCIBus *pci_find_primary_bus(void);
>  PCIBus *pci_device_root_bus(const PCIDevice *d);
> +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
>  const char *pci_root_bus_path(PCIDevice *dev);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> -- 
> 2.9.3

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[Michael S. Tsirkin]

On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> either plain PCI or PCIe depending on where they're attached - will only
> appear in PCIe mode if they're attached to a PCIe bus via a root port or
> downstream port.
> 
> This is correct for "standard" PCIe setups, but there are some platforms
> which need different behaviour (notably "pseries" whose paravirtualized
> PCI host bridges have some idiosyncracies).
> 
> This patch allows the host bridge to override the normal behaviour.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/pci/pci.c              | 11 +++++++++--
>  include/hw/pci/pci_host.h |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 779787b..ac68065 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
>  
>  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
>  {
> -    PCIBus *bus = pci_dev->bus;
> +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> +
> +    if (hc->allow_hybrid_pcie) {
> +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> +    } else {
> +        PCIBus *bus = pci_dev->bus;
>  
> -    return pci_bus_is_express(bus) && !pci_bus_is_root(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,

I think I'd prefer adding some flags in PCIBus. While we are at it,
is_root can become a flag too.

> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index ba31595..ad03cca 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -54,6 +54,7 @@ typedef struct PCIHostBridgeClass {
>      SysBusDeviceClass parent_class;
>  
>      const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> +    bool (*allow_hybrid_pcie)(PCIHostState *, PCIDevice *);
>  } PCIHostBridgeClass;
>  
>  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> -- 
> 2.9.3

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4791 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.

So, I agree the current name isn't great, but that suggestion doesn't
make sense to me.  The function is determining if the hybrid device
should present as PCIe rather than PCI, not whether it's permitted at
all.

> 
> > 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 need to fix this for the next spin.

> 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f9b7244..9b34673 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> >  {
> >      VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> >      VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> > -    bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
> > -                     !pci_bus_is_root(pci_dev->bus);
> > +    bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
> >  
> >      if (!kvm_has_many_ioeventfds()) {
> >          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> 
> I don't actually remember why do we disable pcie on a root port.
> Marcel, do you happen to know?

I assume you mean on a root bus rather than on a root port (in the
sense of under the root port's fake P2P bridge).  I was told it breaks
firmware and/or some guests on x86.

> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index a37a2d5..7b9a40f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
> >  
> >  PCIBus *pci_find_primary_bus(void);
> >  PCIBus *pci_device_root_bus(const PCIDevice *d);
> > +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
> >  const char *pci_root_bus_path(PCIDevice *dev);
> >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> >  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[David Gibson]

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

On Wed, Apr 26, 2017 at 06:29:05PM +0300, Michael S. Tsirkin wrote:
> On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> > Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> > either plain PCI or PCIe depending on where they're attached - will only
> > appear in PCIe mode if they're attached to a PCIe bus via a root port or
> > downstream port.
> > 
> > This is correct for "standard" PCIe setups, but there are some platforms
> > which need different behaviour (notably "pseries" whose paravirtualized
> > PCI host bridges have some idiosyncracies).
> > 
> > This patch allows the host bridge to override the normal behaviour.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/pci/pci.c              | 11 +++++++++--
> >  include/hw/pci/pci_host.h |  1 +
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 779787b..ac68065 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
> >  
> >  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> >  {
> > -    PCIBus *bus = pci_dev->bus;
> > +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
> > +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> > +
> > +    if (hc->allow_hybrid_pcie) {
> > +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> > +    } else {
> > +        PCIBus *bus = pci_dev->bus;
> >  
> > -    return pci_bus_is_express(bus) && !pci_bus_is_root(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,
> 
> I think I'd prefer adding some flags in PCIBus

I'm not really sure what you have in mind from that.  Remember that we
need to be able to set this based on machine type for backwards
compatibility.

>  While we are at it,
> is_root can become a flag too.

Seems a bit odd to me, since it can be derived from the existing
information.

> 
> > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> > index ba31595..ad03cca 100644
> > --- a/include/hw/pci/pci_host.h
> > +++ b/include/hw/pci/pci_host.h
> > @@ -54,6 +54,7 @@ typedef struct PCIHostBridgeClass {
> >      SysBusDeviceClass parent_class;
> >  
> >      const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> > +    bool (*allow_hybrid_pcie)(PCIHostState *, PCIDevice *);
> >  } PCIHostBridgeClass;
> >  
> >  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> 

-- 
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: 819 bytes --]

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

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [RFC for-2.10 0/3] Rework handling of PCI/PCIe "hybrid" devices

[Eduardo Habkost]

On Tue, Mar 28, 2017 at 01:16:48PM +1100, David Gibson wrote:
> A couple of devices - virtio-pci and XHCI - can present themselves to
> the guest as either PCI or PCIe devices depending on how they're
> attached.  However, the logic is a little different between the two
> devices.  In addition the logic in virtio makes it difficult to put a
> PCIe virtio device into a "pseries" guest because of the unusual way
> the paravirtualized PCI bus works there.

virtio-pci and xhci are not the only hybrid devices.  What about
vmxnet3, pvscsi, and vfio-pci?

> 
> This series makes the logic more consistent, and allows per-machine
> overrides to address that.
> 
> Currently patch 3/3 shows a non-obvious side effect of this change.  A
> PCIe virtio device is, by default, modern mode only, but the qtest
> logic doesn't handle modern-only virtio devices correctly.  We work
> around this by explicitly adding disable-legacy=off to the testcases.
> It would probably be better to update libqos so that it can handle
> modern virtio devices.
> 
> David Gibson (3):
>   pci/pcie: Make a consistent helper for switching PCI/PCIe "hybrid"
>     devices
>   pci: Allow host bridges to override PCI/PCIe hybrid device behaviour
>   pseries: Allow PCIe virtio and XHCI on pseries machine type
> 
>  hw/pci/pci.c              | 14 ++++++++++++++
>  hw/ppc/spapr_pci.c        |  9 +++++++++
>  hw/usb/hcd-xhci.c         |  2 +-
>  hw/virtio/virtio-pci.c    |  3 +--
>  include/hw/pci/pci.h      |  1 +
>  include/hw/pci/pci_host.h |  1 +
>  tests/virtio-9p-test.c    |  2 +-
>  tests/virtio-blk-test.c   |  4 ++--
>  tests/virtio-net-test.c   |  2 +-
>  tests/virtio-scsi-test.c  |  2 +-
>  10 files changed, 32 insertions(+), 8 deletions(-)
> 
> -- 
> 2.9.3
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Aug 29, 2017 at 09:42:39PM +1000, David Gibson wrote:
> 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).

I'm a bit confused by the naming: by looking at the function
name, I don't know if "allow hybrid" means "this bus+device can
(also) work as Conventional PCI" or "this bus+device can (also)
work as PCI Express".

What about just naming it pci_allow_pcie() or
pci_bus_allow_pcie()?  It looks like the function doesn't care if
the device is hybrid or PCIe-only: it's only checking if the
device can work as PCIe on that bus.  It's up to the device to
decide if it should switch to Conventional PCI or report an error
if the function returns false.

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



-- 
Eduardo

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

[David Gibson]

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

On Tue, Aug 29, 2017 at 11:12:36AM -0300, Eduardo Habkost wrote:
> On Tue, Aug 29, 2017 at 09:42:39PM +1000, David Gibson wrote:
> > 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).
> 
> I'm a bit confused by the naming: by looking at the function
> name, I don't know if "allow hybrid" means "this bus+device can
> (also) work as Conventional PCI" or "this bus+device can (also)
> work as PCI Express".

Neither, actually.  It means "should this device, which is capable of
both PCI and PCIe operation, operate as PCIe in this context".  It's
only expected to be called by devices which support both modes of
operation.

I have yet to think of a succinct name which conveys that :(.

> What about just naming it pci_allow_pcie() or
> pci_bus_allow_pcie()?  It looks like the function doesn't care if
> the device is hybrid or PCIe-only: it's only checking if the
> device can work as PCIe on that bus.  It's up to the device to
> decide if it should switch to Conventional PCI or report an error
> if the function returns false.

Hmm.. that would mean changing *every* existing PCIe device to call
this, which I don't think I want to do.

Also it's _not* really saying if a device can operate as PCIe.  AIUI,
a device _can_ operate as PCIe on a root bus (without a port) although
it's unusual.  Integrated PCIe devices would do so, IIUC.  For that
matter I believe current devices which only support PCIe mode will
also operate in PCIe mode on a root bus right now; but doing so
without inserting a root port might make guests unhappy, at least on
PC.

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

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

[Eduardo Habkost]

On Wed, Aug 30, 2017 at 03:54:32PM +1000, David Gibson wrote:
> On Tue, Aug 29, 2017 at 11:12:36AM -0300, Eduardo Habkost wrote:
> > On Tue, Aug 29, 2017 at 09:42:39PM +1000, David Gibson wrote:
> > > 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).
> > 
> > I'm a bit confused by the naming: by looking at the function
> > name, I don't know if "allow hybrid" means "this bus+device can
> > (also) work as Conventional PCI" or "this bus+device can (also)
> > work as PCI Express".
> 
> Neither, actually.  It means "should this device, which is capable of
> both PCI and PCIe operation, operate as PCIe in this context".  It's
> only expected to be called by devices which support both modes of
> operation.
> 
> I have yet to think of a succinct name which conveys that :(.

Based on this description, maybe pci_hybrid_allow_pcie() would be
clearer.

But based on the comments below, I have another suggestion:

> 
> > What about just naming it pci_allow_pcie() or
> > pci_bus_allow_pcie()?  It looks like the function doesn't care if
> > the device is hybrid or PCIe-only: it's only checking if the
> > device can work as PCIe on that bus.  It's up to the device to
> > decide if it should switch to Conventional PCI or report an error
> > if the function returns false.
> 
> Hmm.. that would mean changing *every* existing PCIe device to call
> this, which I don't think I want to do.

Maybe calling it from the common PCI realize function won't be a
bad idea.  But let's discuss that after we clean up the existing
hybrid devices.

> 
> Also it's _not* really saying if a device can operate as PCIe.  AIUI,
> a device _can_ operate as PCIe on a root bus (without a port) although
> it's unusual.  Integrated PCIe devices would do so, IIUC.  For that
> matter I believe current devices which only support PCIe mode will
> also operate in PCIe mode on a root bus right now; but doing so
> without inserting a root port might make guests unhappy, at least on
> PC.

If that's the case, I would change the name and documentation to
say "defaults to", "should", "recommend", or "prefer".

What about pci_bus_prefers_pcie() or pci_hybrid_prefers_pcie()?

In either case, we really need a doc comment clearly explaining
the function purpose and semantics.

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



-- 
Eduardo

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

[David Gibson]

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

On Wed, Aug 30, 2017 at 09:23:59AM -0300, Eduardo Habkost wrote:
> On Wed, Aug 30, 2017 at 03:54:32PM +1000, David Gibson wrote:
> > On Tue, Aug 29, 2017 at 11:12:36AM -0300, Eduardo Habkost wrote:
> > > On Tue, Aug 29, 2017 at 09:42:39PM +1000, David Gibson wrote:
> > > > 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).
> > > 
> > > I'm a bit confused by the naming: by looking at the function
> > > name, I don't know if "allow hybrid" means "this bus+device can
> > > (also) work as Conventional PCI" or "this bus+device can (also)
> > > work as PCI Express".
> > 
> > Neither, actually.  It means "should this device, which is capable of
> > both PCI and PCIe operation, operate as PCIe in this context".  It's
> > only expected to be called by devices which support both modes of
> > operation.
> > 
> > I have yet to think of a succinct name which conveys that :(.
> 
> Based on this description, maybe pci_hybrid_allow_pcie() would be
> clearer.
> 
> But based on the comments below, I have another suggestion:
> 
> > 
> > > What about just naming it pci_allow_pcie() or
> > > pci_bus_allow_pcie()?  It looks like the function doesn't care if
> > > the device is hybrid or PCIe-only: it's only checking if the
> > > device can work as PCIe on that bus.  It's up to the device to
> > > decide if it should switch to Conventional PCI or report an error
> > > if the function returns false.
> > 
> > Hmm.. that would mean changing *every* existing PCIe device to call
> > this, which I don't think I want to do.
> 
> Maybe calling it from the common PCI realize function won't be a
> bad idea.  But let's discuss that after we clean up the existing
> hybrid devices.
> 
> > 
> > Also it's _not* really saying if a device can operate as PCIe.  AIUI,
> > a device _can_ operate as PCIe on a root bus (without a port) although
> > it's unusual.  Integrated PCIe devices would do so, IIUC.  For that
> > matter I believe current devices which only support PCIe mode will
> > also operate in PCIe mode on a root bus right now; but doing so
> > without inserting a root port might make guests unhappy, at least on
> > PC.
> 
> If that's the case, I would change the name and documentation to
> say "defaults to", "should", "recommend", or "prefer".
> 
> What about pci_bus_prefers_pcie() or pci_hybrid_prefers_pcie()?

Ok, I'm going with pci_hybrid_prefer_pcie() in the next spin.

> In either case, we really need a doc comment clearly explaining
> the function purpose and semantics.

I've put something on the function itself.  Possibly it should move to
the header.  Anyway, you can comment again on the next spin.

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

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[David Gibson]

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

On Wed, Apr 26, 2017 at 06:29:05PM +0300, Michael S. Tsirkin wrote:
> On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> > Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> > either plain PCI or PCIe depending on where they're attached - will only
> > appear in PCIe mode if they're attached to a PCIe bus via a root port or
> > downstream port.
> > 
> > This is correct for "standard" PCIe setups, but there are some platforms
> > which need different behaviour (notably "pseries" whose paravirtualized
> > PCI host bridges have some idiosyncracies).
> > 
> > This patch allows the host bridge to override the normal behaviour.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/pci/pci.c              | 11 +++++++++--
> >  include/hw/pci/pci_host.h |  1 +
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 779787b..ac68065 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
> >  
> >  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> >  {
> > -    PCIBus *bus = pci_dev->bus;
> > +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
> > +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> > +
> > +    if (hc->allow_hybrid_pcie) {
> > +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> > +    } else {
> > +        PCIBus *bus = pci_dev->bus;
> >  
> > -    return pci_bus_is_express(bus) && !pci_bus_is_root(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,
> 
> I think I'd prefer adding some flags in PCIBus. While we are at it,
> is_root can become a flag too.

Good idea!  Looking at it the is_root() method really is bogus, I have
a draft patch which replaces it with a flag.  (In fact the other
PCIBusClass methods look kind of bogus too, I'll see if I can find
time to do something about that).

> 
> > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> > index ba31595..ad03cca 100644
> > --- a/include/hw/pci/pci_host.h
> > +++ b/include/hw/pci/pci_host.h
> > @@ -54,6 +54,7 @@ typedef struct PCIHostBridgeClass {
> >      SysBusDeviceClass parent_class;
> >  
> >      const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> > +    bool (*allow_hybrid_pcie)(PCIHostState *, PCIDevice *);
> >  } PCIHostBridgeClass;
> >  
> >  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> 

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