[Qemu-devel] [PATCH 0/2] Some small cleanups and corrections to PCI-E handling

[Qemu-devel] [PATCH 0/2] Some small cleanups and corrections to PCI-E handling

[David Gibson]

Here are a couple of cleanups for PCI-E handling which arose out of
some other work I have in progress.  At least the first of these
patches has been posted before as part of a different series, but
wasn't ready to go at the time.

David Gibson (2):
  pci: Simplify pci_bus_is_root()
  pcie: Don't allow extended config space access via conventional PCI
    bridges

 hw/pci-bridge/pci_expander_bridge.c |  6 -----
 hw/pci/pci.c                        | 40 ++++++++++++++++++++++-------
 hw/virtio/virtio-pci.c              |  1 +
 include/hw/pci/pci.h                |  5 ++--
 include/hw/pci/pci_bus.h            | 13 ++++++++++
 5 files changed, 48 insertions(+), 17 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] pci: Simplify pci_bus_is_root()

[David Gibson]

pci_bus_is_root() currently relies on a method in the PCIBusClass.
But it's always known if a PCI bus is a root bus when we create it, so
using a dynamic method is overkill.

This replaces it with an IS_ROOT bit in a new flags field, which is set on
root buses and otherwise clear.  As a bonus this removes the special
is_root logic from pci_expander_bridge, since it already creates its bus
as a root bus.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 hw/pci-bridge/pci_expander_bridge.c |  6 ------
 hw/pci/pci.c                        | 14 ++------------
 hw/virtio/virtio-pci.c              |  1 +
 include/hw/pci/pci.h                |  1 -
 include/hw/pci/pci_bus.h            | 11 +++++++++++
 5 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index e62de4218f..ca66bc721a 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -66,11 +66,6 @@ static int pxb_bus_num(PCIBus *bus)
     return pxb->bus_nr;
 }
 
-static bool pxb_is_root(PCIBus *bus)
-{
-    return true; /* by definition */
-}
-
 static uint16_t pxb_bus_numa_node(PCIBus *bus)
 {
     PXBDev *pxb = convert_to_pxb(bus->parent_dev);
@@ -83,7 +78,6 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
     PCIBusClass *pbc = PCI_BUS_CLASS(class);
 
     pbc->bus_num = pxb_bus_num;
-    pbc->is_root = pxb_is_root;
     pbc->numa_node = pxb_bus_numa_node;
 }
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c9fc2fbe19..f6d8b337db 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -129,14 +129,9 @@ static void pci_bus_unrealize(BusState *qbus, Error **errp)
     vmstate_unregister(NULL, &vmstate_pcibus, bus);
 }
 
-static bool pcibus_is_root(PCIBus *bus)
-{
-    return !bus->parent_dev;
-}
-
 static int pcibus_num(PCIBus *bus)
 {
-    if (pcibus_is_root(bus)) {
+    if (pci_bus_is_root(bus)) {
         return 0; /* pci host bridge */
     }
     return bus->parent_dev->config[PCI_SECONDARY_BUS];
@@ -159,7 +154,6 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
     k->unrealize = pci_bus_unrealize;
     k->reset = pcibus_reset;
 
-    pbc->is_root = pcibus_is_root;
     pbc->bus_num = pcibus_num;
     pbc->numa_node = pcibus_numa_node;
 }
@@ -379,6 +373,7 @@ static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
     bus->slot_reserved_mask = 0x0;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
+    bus->flags |= PCI_BUS_IS_ROOT;
 
     /* host bridge */
     QLIST_INIT(&bus->child);
@@ -396,11 +391,6 @@ bool pci_bus_is_express(PCIBus *bus)
     return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
 }
 
-bool pci_bus_is_root(PCIBus *bus)
-{
-    return PCI_BUS_GET_CLASS(bus)->is_root(bus);
-}
-
 void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
                               const char *name,
                               MemoryRegion *address_space_mem,
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e978bfe760..a3e43ebe31 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -20,6 +20,7 @@
 #include "standard-headers/linux/virtio_pci.h"
 #include "hw/virtio/virtio.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "hw/pci/msi.h"
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d87f5f93e9..1273deb740 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -395,7 +395,6 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 #define TYPE_PCIE_BUS "PCIE"
 
 bool pci_bus_is_express(PCIBus *bus);
-bool pci_bus_is_root(PCIBus *bus);
 void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
                               const char *name,
                               MemoryRegion *address_space_mem,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index dfb75752cb..3a4d599da3 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -20,8 +20,14 @@ typedef struct PCIBusClass {
     uint16_t (*numa_node)(PCIBus *bus);
 } PCIBusClass;
 
+enum PCIBusFlags {
+    /* This bus is the root of a PCI domain */
+    PCI_BUS_IS_ROOT                                         = 0x0001,
+};
+
 struct PCIBus {
     BusState qbus;
+    enum PCIBusFlags flags;
     PCIIOMMUFunc iommu_fn;
     void *iommu_opaque;
     uint8_t devfn_min;
@@ -46,4 +52,9 @@ struct PCIBus {
     Notifier machine_done;
 };
 
+static inline bool pci_bus_is_root(PCIBus *bus)
+{
+    return !!(bus->flags & PCI_BUS_IS_ROOT);
+}
+
 #endif /* QEMU_PCI_BUS_H */
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] pcie: Don't allow extended config space access via conventional PCI bridges

[David Gibson]

In hardware it's possible, if odd, to have a configuration like:

PCIe host bridge
\- PCIe to PCI bridge
   \- PCI to PCIe bridge
      \- PCIe device

The PCIe extended configuration space on the device won't be
accessible to the host, because the cycles can't traverse the
conventional PCI bus on the way there.

However, if we attempt to model that configuration under qemu,
extended config access on the device *will* work, because
pci_config_size() depends only on whether the device itself is PCIe
capable.

This patch fixes that modelling error by adding a flag to each
PCI/PCIe bus instance indicating whether extended config space
accesses are possible on it.  It will always be false for conventional
PCI buses, for PCIe buses it will be true if and only if the parent
bus also has the flag set.

AIUI earlier attempts to correct this have been rejected, because they
involved expensively traversing the whole bus hierarchy on each config
access.  This approach avoids that by computing the value as the bus
hierarchy is constructed, meaning we only need a single bit check when
we actually attempt the config access.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pci.c             | 32 ++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h     |  4 +++-
 include/hw/pci/pci_bus.h |  2 ++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index f6d8b337db..f2d9dff9ee 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -120,6 +120,25 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
 }
 
+static void pcie_bus_realize(BusState *qbus, Error **errp)
+{
+    PCIBus *bus = PCI_BUS(qbus);
+
+    pci_bus_realize(qbus, errp);
+
+    /* a PCI-E bus can supported extended config space if it's the
+     * root bus, or if the bus/bridge above it does as well */
+    if (pci_bus_is_root(bus)) {
+        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
+    } else {
+        PCIBus *parent_bus = pci_get_bus(bus->parent_dev);
+
+        if (pci_bus_extended_config_space(parent_bus)) {
+            bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
+        }
+    }
+}
+
 static void pci_bus_unrealize(BusState *qbus, Error **errp)
 {
     PCIBus *bus = PCI_BUS(qbus);
@@ -166,6 +185,13 @@ static const TypeInfo pci_bus_info = {
     .class_init = pci_bus_class_init,
 };
 
+static void pcie_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+
+    k->realize = pcie_bus_realize;
+}
+
 static const TypeInfo pcie_interface_info = {
     .name          = INTERFACE_PCIE_DEVICE,
     .parent        = TYPE_INTERFACE,
@@ -174,6 +200,7 @@ static const TypeInfo pcie_interface_info = {
 static const TypeInfo conventional_pci_interface_info = {
     .name          = INTERFACE_CONVENTIONAL_PCI_DEVICE,
     .parent        = TYPE_INTERFACE,
+    .class_init    = pcie_bus_class_init,
 };
 
 static const TypeInfo pcie_bus_info = {
@@ -391,6 +418,11 @@ bool pci_bus_is_express(PCIBus *bus)
     return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
 }
 
+bool pci_bus_extended_config_space(PCIBus *bus)
+{
+    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
+}
+
 void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
                               const char *name,
                               MemoryRegion *address_space_mem,
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 1273deb740..919e8a6f5f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -395,6 +395,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 #define TYPE_PCIE_BUS "PCIE"
 
 bool pci_bus_is_express(PCIBus *bus);
+bool pci_bus_extended_config_space(PCIBus *bus);
 void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
                               const char *name,
                               MemoryRegion *address_space_mem,
@@ -754,7 +755,8 @@ static inline int pci_is_express_downstream_port(const PCIDevice *d)
 
 static inline uint32_t pci_config_size(const PCIDevice *d)
 {
-    return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
+    return (pci_is_express(d) && pci_bus_extended_config_space(pci_get_bus(d)))
+        ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
 }
 
 static inline uint16_t pci_get_bdf(PCIDevice *dev)
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 3a4d599da3..8b1e849c34 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -23,6 +23,8 @@ typedef struct PCIBusClass {
 enum PCIBusFlags {
     /* This bus is the root of a PCI domain */
     PCI_BUS_IS_ROOT                                         = 0x0001,
+    /* PCIe extended configuration space is accessible on this bus */
+    PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
 };
 
 struct PCIBus {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/2] pcie: Don't allow extended config space access via conventional PCI bridges

[Alexey Kardashevskiy]

On 14/02/2019 16:08, David Gibson wrote:
> In hardware it's possible, if odd, to have a configuration like:
> 
> PCIe host bridge
> \- PCIe to PCI bridge
>    \- PCI to PCIe bridge
>       \- PCIe device
> 
> The PCIe extended configuration space on the device won't be
> accessible to the host, because the cycles can't traverse the
> conventional PCI bus on the way there.
> 
> However, if we attempt to model that configuration under qemu,
> extended config access on the device *will* work, because
> pci_config_size() depends only on whether the device itself is PCIe
> capable.
> 
> This patch fixes that modelling error by adding a flag to each
> PCI/PCIe bus instance indicating whether extended config space
> accesses are possible on it.  It will always be false for conventional
> PCI buses, for PCIe buses it will be true if and only if the parent
> bus also has the flag set.
> 
> AIUI earlier attempts to correct this have been rejected, because they
> involved expensively traversing the whole bus hierarchy on each config
> access.  This approach avoids that by computing the value as the bus
> hierarchy is constructed, meaning we only need a single bit check when
> we actually attempt the config access.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/pci/pci.c             | 32 ++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h     |  4 +++-
>  include/hw/pci/pci_bus.h |  2 ++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index f6d8b337db..f2d9dff9ee 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -120,6 +120,25 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
>      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
>  }
>  
> +static void pcie_bus_realize(BusState *qbus, Error **errp)
> +{
> +    PCIBus *bus = PCI_BUS(qbus);
> +
> +    pci_bus_realize(qbus, errp);
> +
> +    /* a PCI-E bus can supported extended config space if it's the
> +     * root bus, or if the bus/bridge above it does as well */
> +    if (pci_bus_is_root(bus)) {
> +        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> +    } else {
> +        PCIBus *parent_bus = pci_get_bus(bus->parent_dev);
> +
> +        if (pci_bus_extended_config_space(parent_bus)) {
> +            bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> +        }
> +    }
> +}
> +
>  static void pci_bus_unrealize(BusState *qbus, Error **errp)
>  {
>      PCIBus *bus = PCI_BUS(qbus);
> @@ -166,6 +185,13 @@ static const TypeInfo pci_bus_info = {
>      .class_init = pci_bus_class_init,
>  };
>  
> +static void pcie_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +
> +    k->realize = pcie_bus_realize;
> +}
> +
>  static const TypeInfo pcie_interface_info = {
>      .name          = INTERFACE_PCIE_DEVICE,
>      .parent        = TYPE_INTERFACE,
> @@ -174,6 +200,7 @@ static const TypeInfo pcie_interface_info = {
>  static const TypeInfo conventional_pci_interface_info = {
>      .name          = INTERFACE_CONVENTIONAL_PCI_DEVICE,
>      .parent        = TYPE_INTERFACE,
> +    .class_init    = pcie_bus_class_init,
>  };
>  
>  static const TypeInfo pcie_bus_info = {
> @@ -391,6 +418,11 @@ bool pci_bus_is_express(PCIBus *bus)
>      return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
>  }
>  
> +bool pci_bus_extended_config_space(PCIBus *bus)
> +{
> +    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
> +}
> +
>  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
>                                const char *name,
>                                MemoryRegion *address_space_mem,
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 1273deb740..919e8a6f5f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -395,6 +395,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
>  #define TYPE_PCIE_BUS "PCIE"
>  
>  bool pci_bus_is_express(PCIBus *bus);
> +bool pci_bus_extended_config_space(PCIBus *bus);
>  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
>                                const char *name,
>                                MemoryRegion *address_space_mem,
> @@ -754,7 +755,8 @@ static inline int pci_is_express_downstream_port(const PCIDevice *d)
>  
>  static inline uint32_t pci_config_size(const PCIDevice *d)
>  {
> -    return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
> +    return (pci_is_express(d) && pci_bus_extended_config_space(pci_get_bus(d)))
> +        ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;


Since there is a selfnack anyway, I'll ask out of curiosity - can a
device sit on PCIe bus and not be PCIe itself?

The pci_is_express(d) check above just seems a little redundant,
g_assert() could probably do just that.




>  }
>  
>  static inline uint16_t pci_get_bdf(PCIDevice *dev)
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 3a4d599da3..8b1e849c34 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -23,6 +23,8 @@ typedef struct PCIBusClass {
>  enum PCIBusFlags {
>      /* This bus is the root of a PCI domain */
>      PCI_BUS_IS_ROOT                                         = 0x0001,
> +    /* PCIe extended configuration space is accessible on this bus */
> +    PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
>  };
>  
>  struct PCIBus {
> 

-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] pcie: Don't allow extended config space access via conventional PCI bridges

[David Gibson]

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

On Thu, Feb 14, 2019 at 05:04:03PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 14/02/2019 16:08, David Gibson wrote:
> > In hardware it's possible, if odd, to have a configuration like:
> > 
> > PCIe host bridge
> > \- PCIe to PCI bridge
> >    \- PCI to PCIe bridge
> >       \- PCIe device
> > 
> > The PCIe extended configuration space on the device won't be
> > accessible to the host, because the cycles can't traverse the
> > conventional PCI bus on the way there.
> > 
> > However, if we attempt to model that configuration under qemu,
> > extended config access on the device *will* work, because
> > pci_config_size() depends only on whether the device itself is PCIe
> > capable.
> > 
> > This patch fixes that modelling error by adding a flag to each
> > PCI/PCIe bus instance indicating whether extended config space
> > accesses are possible on it.  It will always be false for conventional
> > PCI buses, for PCIe buses it will be true if and only if the parent
> > bus also has the flag set.
> > 
> > AIUI earlier attempts to correct this have been rejected, because they
> > involved expensively traversing the whole bus hierarchy on each config
> > access.  This approach avoids that by computing the value as the bus
> > hierarchy is constructed, meaning we only need a single bit check when
> > we actually attempt the config access.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/pci/pci.c             | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci.h     |  4 +++-
> >  include/hw/pci/pci_bus.h |  2 ++
> >  3 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index f6d8b337db..f2d9dff9ee 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -120,6 +120,25 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
> >      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> >  }
> >  
> > +static void pcie_bus_realize(BusState *qbus, Error **errp)
> > +{
> > +    PCIBus *bus = PCI_BUS(qbus);
> > +
> > +    pci_bus_realize(qbus, errp);
> > +
> > +    /* a PCI-E bus can supported extended config space if it's the
> > +     * root bus, or if the bus/bridge above it does as well */
> > +    if (pci_bus_is_root(bus)) {
> > +        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +    } else {
> > +        PCIBus *parent_bus = pci_get_bus(bus->parent_dev);
> > +
> > +        if (pci_bus_extended_config_space(parent_bus)) {
> > +            bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +        }
> > +    }
> > +}
> > +
> >  static void pci_bus_unrealize(BusState *qbus, Error **errp)
> >  {
> >      PCIBus *bus = PCI_BUS(qbus);
> > @@ -166,6 +185,13 @@ static const TypeInfo pci_bus_info = {
> >      .class_init = pci_bus_class_init,
> >  };
> >  
> > +static void pcie_bus_class_init(ObjectClass *klass, void *data)
> > +{
> > +    BusClass *k = BUS_CLASS(klass);
> > +
> > +    k->realize = pcie_bus_realize;
> > +}
> > +
> >  static const TypeInfo pcie_interface_info = {
> >      .name          = INTERFACE_PCIE_DEVICE,
> >      .parent        = TYPE_INTERFACE,
> > @@ -174,6 +200,7 @@ static const TypeInfo pcie_interface_info = {
> >  static const TypeInfo conventional_pci_interface_info = {
> >      .name          = INTERFACE_CONVENTIONAL_PCI_DEVICE,
> >      .parent        = TYPE_INTERFACE,
> > +    .class_init    = pcie_bus_class_init,
> >  };
> >  
> >  static const TypeInfo pcie_bus_info = {
> > @@ -391,6 +418,11 @@ bool pci_bus_is_express(PCIBus *bus)
> >      return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
> >  }
> >  
> > +bool pci_bus_extended_config_space(PCIBus *bus)
> > +{
> > +    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
> > +}
> > +
> >  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> >                                const char *name,
> >                                MemoryRegion *address_space_mem,
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 1273deb740..919e8a6f5f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -395,6 +395,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> >  #define TYPE_PCIE_BUS "PCIE"
> >  
> >  bool pci_bus_is_express(PCIBus *bus);
> > +bool pci_bus_extended_config_space(PCIBus *bus);
> >  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> >                                const char *name,
> >                                MemoryRegion *address_space_mem,
> > @@ -754,7 +755,8 @@ static inline int pci_is_express_downstream_port(const PCIDevice *d)
> >  
> >  static inline uint32_t pci_config_size(const PCIDevice *d)
> >  {
> > -    return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
> > +    return (pci_is_express(d) && pci_bus_extended_config_space(pci_get_bus(d)))
> > +        ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
> 
> 
> Since there is a selfnack anyway, I'll ask out of curiosity - can a
> device sit on PCIe bus and not be PCIe itself?

I believe so.  I think the most common case is plain-PCI integrated
devices in an otherwise PCI-E host bridge / root complex.

> The pci_is_express(d) check above just seems a little redundant,
> g_assert() could probably do just that.
> 
> 
> 
> 
> >  }
> >  
> >  static inline uint16_t pci_get_bdf(PCIDevice *dev)
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 3a4d599da3..8b1e849c34 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -23,6 +23,8 @@ typedef struct PCIBusClass {
> >  enum PCIBusFlags {
> >      /* This bus is the root of a PCI domain */
> >      PCI_BUS_IS_ROOT                                         = 0x0001,
> > +    /* PCIe extended configuration space is accessible on this bus */
> > +    PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
> >  };
> >  
> >  struct PCIBus {
> > 
> 

-- 
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] [PATCH 0/2] Some small cleanups and corrections to PCI-E handling

[David Gibson]

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

On Thu, Feb 14, 2019 at 04:08:06PM +1100, David Gibson wrote:
> Here are a couple of cleanups for PCI-E handling which arose out of
> some other work I have in progress.  At least the first of these
> patches has been posted before as part of a different series, but
> wasn't ready to go at the time.

Self NACK.

Sorry, made a sloppy error in my testing and this breaks make check.
I'll repost once I've fixed it.

> 
> David Gibson (2):
>   pci: Simplify pci_bus_is_root()
>   pcie: Don't allow extended config space access via conventional PCI
>     bridges
> 
>  hw/pci-bridge/pci_expander_bridge.c |  6 -----
>  hw/pci/pci.c                        | 40 ++++++++++++++++++++++-------
>  hw/virtio/virtio-pci.c              |  1 +
>  include/hw/pci/pci.h                |  5 ++--
>  include/hw/pci/pci_bus.h            | 13 ++++++++++
>  5 files changed, 48 insertions(+), 17 deletions(-)
> 

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