[Qemu-devel] [PATCH for-4.0 0/2] spapr: Fix extended config space accesses

[Qemu-devel] [PATCH for-4.0 0/2] spapr: Fix extended config space accesses

[Greg Kurz]

Recent commit c2077e2ca0da7 added stricter checks that now prevent
a guest to access the extended config space of a PCIe device connected
attached to a PHB on a pseries machine.

PAPR compatible PHBs act like legacy PCI busses, but they do allow access
to the full 4k config space of PCIe devices. As discussed several times on
the list ([1] and [2]), we cannot really change PAPR PHB to have a true
PCIe root bus since it would call for massive and unwanted changes in
libvirt.

This series tries to address the issue with a new PCI bus class method
that tells if the PCI bus supports extended config space accesses,
instead of relying on pci_bus_is_express() which wants a PCIe root bus.
A new legacy PCI bus type is added to implement the PAPR behaviour.

Note that this fixes a potential 4.0 regression, hence the for-4.0 tag.

[1] https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07377.html
    https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02839.html

[2] https://lists.gnu.org/archive/html/qemu-ppc/2017-01/msg00034.html

--
Greg

---

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


 hw/pci/pci.c             |   24 ++++++++++++++++++++++++
 hw/pci/pci_host.c        |    2 +-
 hw/ppc/spapr_pci.c       |   26 +++++++++++++++++++++++++-
 include/hw/pci/pci.h     |    2 ++
 include/hw/pci/pci_bus.h |    1 +
 5 files changed, 53 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH for-4.0 1/2] pci: Allow PCI bus subtypes to support extended config space accesses

[Greg Kurz]

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>
---
 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 35451c1e9987..6d13ef877b42 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 5f5345dbac67..9d64b2e12f2c 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 d87f5f93e9cd..0abb06b35747 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 dfb75752cbc1..f6df834170c5 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 {

[Qemu-devel] [PATCH for-4.0 2/2] spapr_pci: Fix extended config space accesses

[Greg Kurz]

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>
---
 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 bba3a86dda6c..68cc15d2f534 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1642,6 +1642,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
@@ -1746,7 +1768,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);
 
@@ -2344,6 +2367,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)

Re: [Qemu-devel] [PATCH for-4.0 0/2] spapr: Fix extended config space accesses

[Alex Williamson]

On Mon, 01 Apr 2019 19:54:57 +0200
Greg Kurz <groug@kaod.org> wrote:

> Recent commit c2077e2ca0da7 added stricter checks that now prevent
> a guest to access the extended config space of a PCIe device connected
> attached to a PHB on a pseries machine.
> 
> PAPR compatible PHBs act like legacy PCI busses, but they do allow access
> to the full 4k config space of PCIe devices. As discussed several times on
> the list ([1] and [2]), we cannot really change PAPR PHB to have a true
> PCIe root bus since it would call for massive and unwanted changes in
> libvirt.
> 
> This series tries to address the issue with a new PCI bus class method
> that tells if the PCI bus supports extended config space accesses,
> instead of relying on pci_bus_is_express() which wants a PCIe root bus.
> A new legacy PCI bus type is added to implement the PAPR behaviour.
> 
> Note that this fixes a potential 4.0 regression, hence the for-4.0 tag.

Post-4.0, please also evaluate:

https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07164.html

Seems that PAPR only has one IOMMU AddressSpace per bus to return
through the iommu_fn so there should be no harm in aliasing the guest
view of devices, but if anything this series warns never to assume that
PAPR behaves normally.

FWIW, this series might make it easier to create a PCI-X Mode 2 bus,
which would otherwise be similar to conventional PCI for
inter-connectivity, but does support extended config space.  Though a
new bus type would be the more obvious way to do it, which clearly has
been a problem here.  Thanks,

Alex

Re: [Qemu-devel] [PATCH for-4.0 0/2] spapr: Fix extended config space accesses

[David Gibson]

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

On Mon, Apr 01, 2019 at 07:54:57PM +0200, Greg Kurz wrote:
> Recent commit c2077e2ca0da7 added stricter checks that now prevent
> a guest to access the extended config space of a PCIe device connected
> attached to a PHB on a pseries machine.
> 
> PAPR compatible PHBs act like legacy PCI busses, but they do allow access
> to the full 4k config space of PCIe devices. As discussed several times on
> the list ([1] and [2]), we cannot really change PAPR PHB to have a true
> PCIe root bus since it would call for massive and unwanted changes in
> libvirt.
> 
> This series tries to address the issue with a new PCI bus class method
> that tells if the PCI bus supports extended config space accesses,
> instead of relying on pci_bus_is_express() which wants a PCIe root bus.
> A new legacy PCI bus type is added to implement the PAPR behaviour.
> 
> Note that this fixes a potential 4.0 regression, hence the for-4.0 tag.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07377.html
>     https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02839.html
> 
> [2] https://lists.gnu.org/archive/html/qemu-ppc/2017-01/msg00034.html

So, I actually had some patches I'd been working on that address both
the c2077e2ca0da7 issue and the PAPR fixup in what I think is a
cleaner manner.  Can't remember now if I posted and they got lost, or
I didn't get around to posting.

Nonetheless, at this point we're fixing a real regression on PAPR.
I'll look at cleaning this up post 4.0.

Reviewed-by: David Gibson <david@gibson.dropbe

I can take this through my tree if people are ok with that.

-- 
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] [Qemu-ppc] [PATCH for-4.0 0/2] spapr: Fix extended config space accesses

[David Gibson]

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

On Tue, Apr 02, 2019 at 09:52:14AM +1100, David Gibson wrote:
> On Mon, Apr 01, 2019 at 07:54:57PM +0200, Greg Kurz wrote:
> > Recent commit c2077e2ca0da7 added stricter checks that now prevent
> > a guest to access the extended config space of a PCIe device connected
> > attached to a PHB on a pseries machine.
> > 
> > PAPR compatible PHBs act like legacy PCI busses, but they do allow access
> > to the full 4k config space of PCIe devices. As discussed several times on
> > the list ([1] and [2]), we cannot really change PAPR PHB to have a true
> > PCIe root bus since it would call for massive and unwanted changes in
> > libvirt.
> > 
> > This series tries to address the issue with a new PCI bus class method
> > that tells if the PCI bus supports extended config space accesses,
> > instead of relying on pci_bus_is_express() which wants a PCIe root bus.
> > A new legacy PCI bus type is added to implement the PAPR behaviour.
> > 
> > Note that this fixes a potential 4.0 regression, hence the for-4.0 tag.
> > 
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07377.html
> >     https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02839.html
> > 
> > [2] https://lists.gnu.org/archive/html/qemu-ppc/2017-01/msg00034.html
> 
> So, I actually had some patches I'd been working on that address both
> the c2077e2ca0da7 issue and the PAPR fixup in what I think is a
> cleaner manner.  Can't remember now if I posted and they got lost, or
> I didn't get around to posting.
> 
> Nonetheless, at this point we're fixing a real regression on PAPR.
> I'll look at cleaning this up post 4.0.
> 
> Reviewed-by: David Gibson <david@gibson.dropbe
> 
> I can take this through my tree if people are ok with that.

Never got a reply on this, but I see these have not yet been merged
via another path.  These fix a real regression for the pseries machine
so I've put them into my tree with the intention to send a pull
request tomorrow, even though they do largely touch generic code.

-- 
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] [Qemu-ppc] [PATCH for-4.0 0/2] spapr: Fix extended config space accesses

[David Gibson]

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

On Tue, Apr 02, 2019 at 09:52:14AM +1100, David Gibson wrote:
> On Mon, Apr 01, 2019 at 07:54:57PM +0200, Greg Kurz wrote:
> > Recent commit c2077e2ca0da7 added stricter checks that now prevent
> > a guest to access the extended config space of a PCIe device connected
> > attached to a PHB on a pseries machine.
> > 
> > PAPR compatible PHBs act like legacy PCI busses, but they do allow access
> > to the full 4k config space of PCIe devices. As discussed several times on
> > the list ([1] and [2]), we cannot really change PAPR PHB to have a true
> > PCIe root bus since it would call for massive and unwanted changes in
> > libvirt.
> > 
> > This series tries to address the issue with a new PCI bus class method
> > that tells if the PCI bus supports extended config space accesses,
> > instead of relying on pci_bus_is_express() which wants a PCIe root bus.
> > A new legacy PCI bus type is added to implement the PAPR behaviour.
> > 
> > Note that this fixes a potential 4.0 regression, hence the for-4.0 tag.
> > 
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07377.html
> >     https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02839.html
> > 
> > [2] https://lists.gnu.org/archive/html/qemu-ppc/2017-01/msg00034.html
> 
> So, I actually had some patches I'd been working on that address both
> the c2077e2ca0da7 issue and the PAPR fixup in what I think is a
> cleaner manner.  Can't remember now if I posted and they got lost, or
> I didn't get around to posting.
> 
> Nonetheless, at this point we're fixing a real regression on PAPR.
> I'll look at cleaning this up post 4.0.
> 
> Reviewed-by: David Gibson <david@gibson.dropbe
> 
> I can take this through my tree if people are ok with that.

Never got a reply on this, but I see these have not yet been merged
via another path.  These fix a real regression for the pseries machine
so I've put them into my tree with the intention to send a pull
request tomorrow, even though they do largely touch generic code.

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