[Qemu-devel] [RFC for-4.1 0/5] Simplify some not-really-necessary PCI bus callbacks

[Qemu-devel] [RFC for-4.1 0/5] Simplify some not-really-necessary PCI bus callbacks

[David Gibson]

c2077e2c "pci: Adjust PCI config limit based on bus topology"
introduced checking the availability of extended config space for
PCI-E devices which are in a bus topology that doesn't permit extended
config space access (e.g. under PCI-E to PCI then PCI to PCI-E
bridges).

This caused some problems for the spapr para-virtual PCI bus which
_does_ allow extended config space access, despite acting in most ways
like a vanilla PCI bus.

Greg Kurz has posted a proposed fix for that against qemu-4.0 - for
easy reference I've included that as the first two patches of this
series.

The rest of the series simplfies both the PCI-E config space access
and pci_bus_is_root() logic, using a new flags field on the PCI Bus
instance.

David Gibson (3):
  pcie: Remove redundant test in pcie_mmcfg_data_read()
  pci: Simplify pci_bus_is_root()
  pcie: Simplify pci_adjust_config_limit()

Greg Kurz (2):
  pci: Allow PCI bus subtypes to support extended config space accesses
  spapr_pci: Fix extended config space accesses

 hw/pci-bridge/pci_expander_bridge.c |  6 -----
 hw/pci/pci.c                        | 40 ++++++++++++++++++++++-------
 hw/pci/pci_host.c                   | 13 +++-------
 hw/pci/pcie_host.c                  |  5 ----
 hw/ppc/spapr_pci.c                  | 20 ++++++++++++++-
 hw/virtio/virtio-pci.c              |  1 +
 include/hw/pci/pci.h                |  3 ++-
 include/hw/pci/pci_bus.h            | 14 +++++++++-
 8 files changed, 69 insertions(+), 33 deletions(-)

-- 
2.20.1

[Qemu-devel] [RFC for-4.1 1/5] pci: Allow PCI bus subtypes to support extended config space accesses

[David Gibson]

From: Greg Kurz <groug@kaod.org>

Some PHB implementations, eg. PAPR used on pseries machine, act like
a regular PCI bus rather than a PCIe bus, but allow access to the
PCIe extended config space anyway.

Introduce a new PCI bus class method to modelize this behaviour and
use it when adjusting the config space size limit during accesses.

No behaviour change for existing PCI bus types.

Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <155414130271.574858.4253514266378127489.stgit@bahia.lan>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pci.c             | 24 ++++++++++++++++++++++++
 hw/pci/pci_host.c        |  2 +-
 include/hw/pci/pci.h     |  2 ++
 include/hw/pci/pci_bus.h |  1 +
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 35451c1e99..6d13ef877b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -147,6 +147,11 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
     return NUMA_NODE_UNASSIGNED;
 }
 
+static bool pcibus_allows_extended_config_space(PCIBus *bus)
+{
+    return false;
+}
+
 static void pci_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
@@ -162,6 +167,7 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
     pbc->is_root = pcibus_is_root;
     pbc->bus_num = pcibus_num;
     pbc->numa_node = pcibus_numa_node;
+    pbc->allows_extended_config_space = pcibus_allows_extended_config_space;
 }
 
 static const TypeInfo pci_bus_info = {
@@ -182,9 +188,22 @@ static const TypeInfo conventional_pci_interface_info = {
     .parent        = TYPE_INTERFACE,
 };
 
+static bool pciebus_allows_extended_config_space(PCIBus *bus)
+{
+    return true;
+}
+
+static void pcie_bus_class_init(ObjectClass *klass, void *data)
+{
+    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
+
+    pbc->allows_extended_config_space = pciebus_allows_extended_config_space;
+}
+
 static const TypeInfo pcie_bus_info = {
     .name = TYPE_PCIE_BUS,
     .parent = TYPE_PCI_BUS,
+    .class_init = pcie_bus_class_init,
 };
 
 static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
@@ -401,6 +420,11 @@ bool pci_bus_is_root(PCIBus *bus)
     return PCI_BUS_GET_CLASS(bus)->is_root(bus);
 }
 
+bool pci_bus_allows_extended_config_space(PCIBus *bus)
+{
+    return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(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/pci/pci_host.c b/hw/pci/pci_host.c
index 5f5345dbac..9d64b2e12f 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -54,7 +54,7 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
 {
     if (*limit > PCI_CONFIG_SPACE_SIZE) {
-        if (!pci_bus_is_express(bus)) {
+        if (!pci_bus_allows_extended_config_space(bus)) {
             *limit = PCI_CONFIG_SPACE_SIZE;
             return;
         }
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d87f5f93e9..0abb06b357 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -396,6 +396,8 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 
 bool pci_bus_is_express(PCIBus *bus);
 bool pci_bus_is_root(PCIBus *bus);
+bool pci_bus_allows_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,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index dfb75752cb..f6df834170 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -18,6 +18,7 @@ typedef struct PCIBusClass {
     bool (*is_root)(PCIBus *bus);
     int (*bus_num)(PCIBus *bus);
     uint16_t (*numa_node)(PCIBus *bus);
+    bool (*allows_extended_config_space)(PCIBus *bus);
 } PCIBusClass;
 
 struct PCIBus {
-- 
2.20.1

[Qemu-devel] [RFC for-4.1 2/5] spapr_pci: Fix extended config space accesses

[David Gibson]

From: Greg Kurz <groug@kaod.org>

The PAPR PHB acts as a legacy PCI bus but it allows PCIe extended
config space accesses anyway (for pseries-2.9 and newer machine
types).

Introduce a specific PCI bus subtype to inform the common PCI code
about that.

Fixes: c2077e2ca0da7
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <155414130834.574858.16502276132110219890.stgit@bahia.lan>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index b63ed9d8da..2e76d8cbd8 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1638,6 +1638,28 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
     memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
 }
 
+static bool spapr_phb_allows_extended_config_space(PCIBus *bus)
+{
+    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(BUS(bus)->parent);
+
+    return sphb->pcie_ecs;
+}
+
+static void spapr_phb_root_bus_class_init(ObjectClass *klass, void *data)
+{
+    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
+
+    pbc->allows_extended_config_space = spapr_phb_allows_extended_config_space;
+}
+
+#define TYPE_SPAPR_PHB_ROOT_BUS "spapr-pci-host-bridge-root-bus"
+
+static const TypeInfo spapr_phb_root_bus_info = {
+    .name = TYPE_SPAPR_PHB_ROOT_BUS,
+    .parent = TYPE_PCI_BUS,
+    .class_init = spapr_phb_root_bus_class_init,
+};
+
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
@@ -1742,7 +1764,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     bus = pci_register_root_bus(dev, NULL,
                                 pci_spapr_set_irq, pci_spapr_map_irq, sphb,
                                 &sphb->memspace, &sphb->iospace,
-                                PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
+                                PCI_DEVFN(0, 0), PCI_NUM_PINS,
+                                TYPE_SPAPR_PHB_ROOT_BUS);
     phb->bus = bus;
     qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb), NULL);
 
@@ -2325,6 +2348,7 @@ void spapr_pci_rtas_init(void)
 static void spapr_pci_register_types(void)
 {
     type_register_static(&spapr_phb_info);
+    type_register_static(&spapr_phb_root_bus_info);
 }
 
 type_init(spapr_pci_register_types)
-- 
2.20.1

[Qemu-devel] [RFC for-4.1 3/5] pcie: Remove redundant test in pcie_mmcfg_data_read()

[David Gibson]

This function has an explicit test for accesses above the device's config
size, in which case it returns ~0x0.  But pci_host_config_read_common()
which it is about to call already has checks against the config space
limit and likewise returns ~0x0 in that case.  So, remove the redundant
test.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pcie_host.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 553db56778..8f4435838e 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -70,11 +70,6 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
     limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return ~0x0;
-    }
     return pci_host_config_read_common(pci_dev, addr, limit, len);
 }
 
-- 
2.20.1

[Qemu-devel] [RFC for-4.1 4/5] 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            | 12 +++++++++++-
 5 files changed, 14 insertions(+), 20 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 6d13ef877b..ea5941fb22 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];
@@ -164,7 +159,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;
     pbc->allows_extended_config_space = pcibus_allows_extended_config_space;
@@ -398,6 +392,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);
@@ -415,11 +410,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);
-}
-
 bool pci_bus_allows_extended_config_space(PCIBus *bus)
 {
     return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(bus);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index cb44e19b67..942173d830 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 0abb06b357..33ccce320c 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);
 bool pci_bus_allows_extended_config_space(PCIBus *bus);
 
 void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index f6df834170..aea98d5040 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -15,14 +15,19 @@ typedef struct PCIBusClass {
     BusClass parent_class;
     /*< public >*/
 
-    bool (*is_root)(PCIBus *bus);
     int (*bus_num)(PCIBus *bus);
     uint16_t (*numa_node)(PCIBus *bus);
     bool (*allows_extended_config_space)(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;
@@ -47,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] [RFC for-4.1 5/5] pcie: Simplify pci_adjust_config_limit()

[David Gibson]

Since c2077e2c "pci: Adjust PCI config limit based on bus topology",
pci_adjust_config_limit() has been used in the config space read and write
paths to only permit access to extended config space on buses which permit
it.  Specifically it prevents access on devices below a vanilla-PCI bus via
some combination of bridges, even if both the host bridge and the device
itself are PCI-E.

It accomplishes this with a somewhat complex call up the chain of bridges
to see if any of them prohibit extended config space access.  This is
overly complex, since we can always know if the bus will support such
access at the point it is constructed.

This patch simplifies the test by using a flag in the PCIBus instance
indicating wither extended configuration space is accessible.  It is always
false for vanilla PCI buses.  For PCI-E buses, it is true for root buses
and equal to the parent bus's's capability otherwise.

This should cause no behavioural change.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci/pci.c             | 36 ++++++++++++++++++++++--------------
 hw/pci/pci_host.c        | 13 +++----------
 hw/ppc/spapr_pci.c       | 24 +++++++++---------------
 include/hw/pci/pci_bus.h |  3 ++-
 4 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ea5941fb22..420496571e 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_allows_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);
@@ -142,11 +161,6 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
     return NUMA_NODE_UNASSIGNED;
 }
 
-static bool pcibus_allows_extended_config_space(PCIBus *bus)
-{
-    return false;
-}
-
 static void pci_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
@@ -161,7 +175,6 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
 
     pbc->bus_num = pcibus_num;
     pbc->numa_node = pcibus_numa_node;
-    pbc->allows_extended_config_space = pcibus_allows_extended_config_space;
 }
 
 static const TypeInfo pci_bus_info = {
@@ -182,16 +195,11 @@ static const TypeInfo conventional_pci_interface_info = {
     .parent        = TYPE_INTERFACE,
 };
 
-static bool pciebus_allows_extended_config_space(PCIBus *bus)
-{
-    return true;
-}
-
 static void pcie_bus_class_init(ObjectClass *klass, void *data)
 {
-    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
+    BusClass *k = BUS_CLASS(klass);
 
-    pbc->allows_extended_config_space = pciebus_allows_extended_config_space;
+    k->realize = pcie_bus_realize;
 }
 
 static const TypeInfo pcie_bus_info = {
@@ -412,7 +420,7 @@ bool pci_bus_is_express(PCIBus *bus)
 
 bool pci_bus_allows_extended_config_space(PCIBus *bus)
 {
-    return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(bus);
+    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
 }
 
 void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 9d64b2e12f..5f3497256c 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -53,16 +53,9 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 
 static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
 {
-    if (*limit > PCI_CONFIG_SPACE_SIZE) {
-        if (!pci_bus_allows_extended_config_space(bus)) {
-            *limit = PCI_CONFIG_SPACE_SIZE;
-            return;
-        }
-
-        if (!pci_bus_is_root(bus)) {
-            PCIDevice *bridge = pci_bridge_get_device(bus);
-            pci_adjust_config_limit(pci_get_bus(bridge), limit);
-        }
+    if ((*limit > PCI_CONFIG_SPACE_SIZE) &&
+        !pci_bus_allows_extended_config_space(bus)) {
+        *limit = PCI_CONFIG_SPACE_SIZE;
     }
 }
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2e76d8cbd8..23d70ca6fe 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1638,26 +1638,11 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
     memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
 }
 
-static bool spapr_phb_allows_extended_config_space(PCIBus *bus)
-{
-    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(BUS(bus)->parent);
-
-    return sphb->pcie_ecs;
-}
-
-static void spapr_phb_root_bus_class_init(ObjectClass *klass, void *data)
-{
-    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
-
-    pbc->allows_extended_config_space = spapr_phb_allows_extended_config_space;
-}
-
 #define TYPE_SPAPR_PHB_ROOT_BUS "spapr-pci-host-bridge-root-bus"
 
 static const TypeInfo spapr_phb_root_bus_info = {
     .name = TYPE_SPAPR_PHB_ROOT_BUS,
     .parent = TYPE_PCI_BUS,
-    .class_init = spapr_phb_root_bus_class_init,
 };
 
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
@@ -1766,6 +1751,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
                                 &sphb->memspace, &sphb->iospace,
                                 PCI_DEVFN(0, 0), PCI_NUM_PINS,
                                 TYPE_SPAPR_PHB_ROOT_BUS);
+
+    /*
+     * Despite resembling a vanilla PCI bus in most ways, the PAPR
+     * para-virtualized PCI bus *does* permit PCI-E extended config
+     * space access
+     */
+    if (sphb->pcie_ecs) {
+        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
+    }
     phb->bus = bus;
     qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb), NULL);
 
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index aea98d5040..3c0be4c420 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -17,12 +17,13 @@ typedef struct PCIBusClass {
 
     int (*bus_num)(PCIBus *bus);
     uint16_t (*numa_node)(PCIBus *bus);
-    bool (*allows_extended_config_space)(PCIBus *bus);
 } 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] [RFC for-4.1 3/5] pcie: Remove redundant test in pcie_mmcfg_data_read()

[Greg Kurz]

On Tue,  2 Apr 2019 16:40:26 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This function has an explicit test for accesses above the device's config
> size, in which case it returns ~0x0.  But pci_host_config_read_common()
> which it is about to call already has checks against the config space
> limit and likewise returns ~0x0 in that case.  So, remove the redundant
> test.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/pci/pcie_host.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index 553db56778..8f4435838e 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -70,11 +70,6 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
>      }
>      addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
>      limit = pci_config_size(pci_dev);
> -    if (limit <= addr) {
> -        /* conventional pci device can be behind pcie-to-pci bridge.
> -           256 <= addr < 4K has no effects. */
> -        return ~0x0;
> -    }
>      return pci_host_config_read_common(pci_dev, addr, limit, len);
>  }
>  

It looks good but what about pcie_mmcfg_data_write(), which follows
the same pattern ?

Re: [Qemu-devel] [RFC for-4.1 3/5] pcie: Remove redundant test in pcie_mmcfg_data_read()

[David Gibson]

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

On Tue, Apr 02, 2019 at 09:10:55AM +0200, Greg Kurz wrote:
> On Tue,  2 Apr 2019 16:40:26 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This function has an explicit test for accesses above the device's config
> > size, in which case it returns ~0x0.  But pci_host_config_read_common()
> > which it is about to call already has checks against the config space
> > limit and likewise returns ~0x0 in that case.  So, remove the redundant
> > test.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/pci/pcie_host.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> > index 553db56778..8f4435838e 100644
> > --- a/hw/pci/pcie_host.c
> > +++ b/hw/pci/pcie_host.c
> > @@ -70,11 +70,6 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
> >      }
> >      addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
> >      limit = pci_config_size(pci_dev);
> > -    if (limit <= addr) {
> > -        /* conventional pci device can be behind pcie-to-pci bridge.
> > -           256 <= addr < 4K has no effects. */
> > -        return ~0x0;
> > -    }
> >      return pci_host_config_read_common(pci_dev, addr, limit, len);
> >  }
> >  
> 
> It looks good but what about pcie_mmcfg_data_write(), which follows
> the same pattern ?

Oops, didn't spot that.  Adjusted 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-4.1 4/5] pci: Simplify pci_bus_is_root()

[Greg Kurz]

On Tue,  2 Apr 2019 16:40:27 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

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

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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            | 12 +++++++++++-
>  5 files changed, 14 insertions(+), 20 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 6d13ef877b..ea5941fb22 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];
> @@ -164,7 +159,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;
>      pbc->allows_extended_config_space = pcibus_allows_extended_config_space;
> @@ -398,6 +392,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);
> @@ -415,11 +410,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);
> -}
> -
>  bool pci_bus_allows_extended_config_space(PCIBus *bus)
>  {
>      return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(bus);
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index cb44e19b67..942173d830 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 0abb06b357..33ccce320c 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);
>  bool pci_bus_allows_extended_config_space(PCIBus *bus);
>  
>  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index f6df834170..aea98d5040 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -15,14 +15,19 @@ typedef struct PCIBusClass {
>      BusClass parent_class;
>      /*< public >*/
>  
> -    bool (*is_root)(PCIBus *bus);
>      int (*bus_num)(PCIBus *bus);
>      uint16_t (*numa_node)(PCIBus *bus);
>      bool (*allows_extended_config_space)(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;
> @@ -47,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 */

Re: [Qemu-devel] [RFC for-4.1 5/5] pcie: Simplify pci_adjust_config_limit()

[Greg Kurz]

On Tue,  2 Apr 2019 16:40:28 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Since c2077e2c "pci: Adjust PCI config limit based on bus topology",
> pci_adjust_config_limit() has been used in the config space read and write
> paths to only permit access to extended config space on buses which permit
> it.  Specifically it prevents access on devices below a vanilla-PCI bus via
> some combination of bridges, even if both the host bridge and the device
> itself are PCI-E.
> 
> It accomplishes this with a somewhat complex call up the chain of bridges
> to see if any of them prohibit extended config space access.  This is
> overly complex, since we can always know if the bus will support such
> access at the point it is constructed.
> 
> This patch simplifies the test by using a flag in the PCIBus instance
> indicating wither extended configuration space is accessible.  It is always
> false for vanilla PCI buses.  For PCI-E buses, it is true for root buses
> and equal to the parent bus's's capability otherwise.
> 
> This should cause no behavioural change.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

LGTM. Some remarks below.

>  hw/pci/pci.c             | 36 ++++++++++++++++++++++--------------
>  hw/pci/pci_host.c        | 13 +++----------
>  hw/ppc/spapr_pci.c       | 24 +++++++++---------------
>  include/hw/pci/pci_bus.h |  3 ++-
>  4 files changed, 36 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index ea5941fb22..420496571e 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

s/supported/support

> +     * 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_allows_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);
> @@ -142,11 +161,6 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
>      return NUMA_NODE_UNASSIGNED;
>  }
>  
> -static bool pcibus_allows_extended_config_space(PCIBus *bus)
> -{
> -    return false;
> -}
> -
>  static void pci_bus_class_init(ObjectClass *klass, void *data)
>  {
>      BusClass *k = BUS_CLASS(klass);
> @@ -161,7 +175,6 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
>  
>      pbc->bus_num = pcibus_num;
>      pbc->numa_node = pcibus_numa_node;
> -    pbc->allows_extended_config_space = pcibus_allows_extended_config_space;
>  }
>  
>  static const TypeInfo pci_bus_info = {
> @@ -182,16 +195,11 @@ static const TypeInfo conventional_pci_interface_info = {
>      .parent        = TYPE_INTERFACE,
>  };
>  
> -static bool pciebus_allows_extended_config_space(PCIBus *bus)
> -{
> -    return true;
> -}
> -
>  static void pcie_bus_class_init(ObjectClass *klass, void *data)
>  {
> -    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
> +    BusClass *k = BUS_CLASS(klass);
>  
> -    pbc->allows_extended_config_space = pciebus_allows_extended_config_space;
> +    k->realize = pcie_bus_realize;
>  }
>  
>  static const TypeInfo pcie_bus_info = {
> @@ -412,7 +420,7 @@ bool pci_bus_is_express(PCIBus *bus)
>  
>  bool pci_bus_allows_extended_config_space(PCIBus *bus)
>  {
> -    return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(bus);
> +    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
>  }
>  

Maybe make this a static inline in pci_bus.h like you already did with
pci_bus_is_root() in the previous patch ?

>  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 9d64b2e12f..5f3497256c 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -53,16 +53,9 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
>  
>  static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
>  {
> -    if (*limit > PCI_CONFIG_SPACE_SIZE) {
> -        if (!pci_bus_allows_extended_config_space(bus)) {
> -            *limit = PCI_CONFIG_SPACE_SIZE;
> -            return;
> -        }
> -
> -        if (!pci_bus_is_root(bus)) {
> -            PCIDevice *bridge = pci_bridge_get_device(bus);
> -            pci_adjust_config_limit(pci_get_bus(bridge), limit);
> -        }
> +    if ((*limit > PCI_CONFIG_SPACE_SIZE) &&

parenthesitis ? ;-)

> +        !pci_bus_allows_extended_config_space(bus)) {
> +        *limit = PCI_CONFIG_SPACE_SIZE;
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2e76d8cbd8..23d70ca6fe 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1638,26 +1638,11 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
>      memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
>  }
>  
> -static bool spapr_phb_allows_extended_config_space(PCIBus *bus)
> -{
> -    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(BUS(bus)->parent);
> -
> -    return sphb->pcie_ecs;
> -}
> -
> -static void spapr_phb_root_bus_class_init(ObjectClass *klass, void *data)
> -{
> -    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
> -
> -    pbc->allows_extended_config_space = spapr_phb_allows_extended_config_space;
> -}
> -
>  #define TYPE_SPAPR_PHB_ROOT_BUS "spapr-pci-host-bridge-root-bus"
>  
>  static const TypeInfo spapr_phb_root_bus_info = {
>      .name = TYPE_SPAPR_PHB_ROOT_BUS,
>      .parent = TYPE_PCI_BUS,
> -    .class_init = spapr_phb_root_bus_class_init,

The type is quite useless now, it should be dropped I guess, ie.
git revert of "spapr_pci: Fix extended config space accesses".

>  };
>  
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
> @@ -1766,6 +1751,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>                                  &sphb->memspace, &sphb->iospace,
>                                  PCI_DEVFN(0, 0), PCI_NUM_PINS,
>                                  TYPE_SPAPR_PHB_ROOT_BUS);
> +
> +    /*
> +     * Despite resembling a vanilla PCI bus in most ways, the PAPR
> +     * para-virtualized PCI bus *does* permit PCI-E extended config
> +     * space access
> +     */
> +    if (sphb->pcie_ecs) {
> +        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> +    }
>      phb->bus = bus;
>      qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb), NULL);
>  
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index aea98d5040..3c0be4c420 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -17,12 +17,13 @@ typedef struct PCIBusClass {
>  
>      int (*bus_num)(PCIBus *bus);
>      uint16_t (*numa_node)(PCIBus *bus);
> -    bool (*allows_extended_config_space)(PCIBus *bus);
>  } 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 {

Re: [Qemu-devel] [RFC for-4.1 5/5] pcie: Simplify pci_adjust_config_limit()

[David Gibson]

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

On Tue, Apr 02, 2019 at 03:53:05PM +0200, Greg Kurz wrote:
> On Tue,  2 Apr 2019 16:40:28 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Since c2077e2c "pci: Adjust PCI config limit based on bus topology",
> > pci_adjust_config_limit() has been used in the config space read and write
> > paths to only permit access to extended config space on buses which permit
> > it.  Specifically it prevents access on devices below a vanilla-PCI bus via
> > some combination of bridges, even if both the host bridge and the device
> > itself are PCI-E.
> > 
> > It accomplishes this with a somewhat complex call up the chain of bridges
> > to see if any of them prohibit extended config space access.  This is
> > overly complex, since we can always know if the bus will support such
> > access at the point it is constructed.
> > 
> > This patch simplifies the test by using a flag in the PCIBus instance
> > indicating wither extended configuration space is accessible.  It is always
> > false for vanilla PCI buses.  For PCI-E buses, it is true for root buses
> > and equal to the parent bus's's capability otherwise.
> > 
> > This should cause no behavioural change.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> LGTM. Some remarks below.
> 
> >  hw/pci/pci.c             | 36 ++++++++++++++++++++++--------------
> >  hw/pci/pci_host.c        | 13 +++----------
> >  hw/ppc/spapr_pci.c       | 24 +++++++++---------------
> >  include/hw/pci/pci_bus.h |  3 ++-
> >  4 files changed, 36 insertions(+), 40 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index ea5941fb22..420496571e 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
> 
> s/supported/support

Corrected, along with the block comment formatting.

> > +     * 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_allows_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);
> > @@ -142,11 +161,6 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
> >      return NUMA_NODE_UNASSIGNED;
> >  }
> >  
> > -static bool pcibus_allows_extended_config_space(PCIBus *bus)
> > -{
> > -    return false;
> > -}
> > -
> >  static void pci_bus_class_init(ObjectClass *klass, void *data)
> >  {
> >      BusClass *k = BUS_CLASS(klass);
> > @@ -161,7 +175,6 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
> >  
> >      pbc->bus_num = pcibus_num;
> >      pbc->numa_node = pcibus_numa_node;
> > -    pbc->allows_extended_config_space = pcibus_allows_extended_config_space;
> >  }
> >  
> >  static const TypeInfo pci_bus_info = {
> > @@ -182,16 +195,11 @@ static const TypeInfo conventional_pci_interface_info = {
> >      .parent        = TYPE_INTERFACE,
> >  };
> >  
> > -static bool pciebus_allows_extended_config_space(PCIBus *bus)
> > -{
> > -    return true;
> > -}
> > -
> >  static void pcie_bus_class_init(ObjectClass *klass, void *data)
> >  {
> > -    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
> > +    BusClass *k = BUS_CLASS(klass);
> >  
> > -    pbc->allows_extended_config_space = pciebus_allows_extended_config_space;
> > +    k->realize = pcie_bus_realize;
> >  }
> >  
> >  static const TypeInfo pcie_bus_info = {
> > @@ -412,7 +420,7 @@ bool pci_bus_is_express(PCIBus *bus)
> >  
> >  bool pci_bus_allows_extended_config_space(PCIBus *bus)
> >  {
> > -    return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(bus);
> > +    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
> >  }
> >  
> 
> Maybe make this a static inline in pci_bus.h like you already did with
> pci_bus_is_root() in the previous patch ?

I thought I'd tried that earlier and run into include hell because of
the target vs. non-target distinction.  But I must have been mistaken
because I tried it again and it worked fine.

> >  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index 9d64b2e12f..5f3497256c 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -53,16 +53,9 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> >  
> >  static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
> >  {
> > -    if (*limit > PCI_CONFIG_SPACE_SIZE) {
> > -        if (!pci_bus_allows_extended_config_space(bus)) {
> > -            *limit = PCI_CONFIG_SPACE_SIZE;
> > -            return;
> > -        }
> > -
> > -        if (!pci_bus_is_root(bus)) {
> > -            PCIDevice *bridge = pci_bridge_get_device(bus);
> > -            pci_adjust_config_limit(pci_get_bus(bridge), limit);
> > -        }
> > +    if ((*limit > PCI_CONFIG_SPACE_SIZE) &&
> 
> parenthesitis ? ;-)
> 
> > +        !pci_bus_allows_extended_config_space(bus)) {
> > +        *limit = PCI_CONFIG_SPACE_SIZE;
> >      }
> >  }
> >  
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 2e76d8cbd8..23d70ca6fe 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1638,26 +1638,11 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
> >      memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
> >  }
> >  
> > -static bool spapr_phb_allows_extended_config_space(PCIBus *bus)
> > -{
> > -    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(BUS(bus)->parent);
> > -
> > -    return sphb->pcie_ecs;
> > -}
> > -
> > -static void spapr_phb_root_bus_class_init(ObjectClass *klass, void *data)
> > -{
> > -    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
> > -
> > -    pbc->allows_extended_config_space = spapr_phb_allows_extended_config_space;
> > -}
> > -
> >  #define TYPE_SPAPR_PHB_ROOT_BUS "spapr-pci-host-bridge-root-bus"
> >  
> >  static const TypeInfo spapr_phb_root_bus_info = {
> >      .name = TYPE_SPAPR_PHB_ROOT_BUS,
> >      .parent = TYPE_PCI_BUS,
> > -    .class_init = spapr_phb_root_bus_class_init,
> 
> The type is quite useless now, it should be dropped I guess, ie.
> git revert of "spapr_pci: Fix extended config space accesses".

Ah, good point.

> >  };
> >  
> >  static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > @@ -1766,6 +1751,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >                                  &sphb->memspace, &sphb->iospace,
> >                                  PCI_DEVFN(0, 0), PCI_NUM_PINS,
> >                                  TYPE_SPAPR_PHB_ROOT_BUS);
> > +
> > +    /*
> > +     * Despite resembling a vanilla PCI bus in most ways, the PAPR
> > +     * para-virtualized PCI bus *does* permit PCI-E extended config
> > +     * space access
> > +     */
> > +    if (sphb->pcie_ecs) {
> > +        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +    }
> >      phb->bus = bus;
> >      qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb), NULL);
> >  
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index aea98d5040..3c0be4c420 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -17,12 +17,13 @@ typedef struct PCIBusClass {
> >  
> >      int (*bus_num)(PCIBus *bus);
> >      uint16_t (*numa_node)(PCIBus *bus);
> > -    bool (*allows_extended_config_space)(PCIBus *bus);
> >  } 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 --]