[Qemu-devel] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[Qemu-devel] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[David Hildenbrand]

These are all the patches that are not yet upstream (@Conny you might
already picked some, including them for the full picture) and after
a good discussion yesterday, including a patch t get rid of the release
timer. I ran a couple of sanity tests on this series.

#1 and #2 fix hotplugging of PCI bridges.
#3 warns when "zpci=off"
#4 refactors unplugging
#5 get's rid of the release timer
#6 processes all unplug requests on reboot

@Colin, can you review/ack? Especially Patch #4 is needed for qdev
patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
chaining + virtio-pmem")

David Hildenbrand (6):
  s390x/pci: Fix primary bus number for PCI bridges
  s390x/pci: Fix hotplugging of PCI bridges
  s390x/pci: Warn when adding PCI devices without the 'zpci' feature
  s390x/pci: Introduce unplug requests and split unplug handler
  s390x/pci: Drop release timer and replace it with a flag
  s390x/pci: Unplug remaining requested devices on pcihost reset

 hw/s390x/s390-pci-bus.c | 233 ++++++++++++++++++++++++++--------------
 hw/s390x/s390-pci-bus.h |   4 +-
 2 files changed, 152 insertions(+), 85 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/6] s390x/pci: Fix primary bus number for PCI bridges

[David Hildenbrand]

The primary bus number corresponds always to the bus number of the
bus the bridge is attached to.

Right now, if we have two bridges attached to the same bus (e.g. root
bus) this is however not the case. The first bridge will have primary
bus 0, the second bridge primary bus 1, which is wrong. Fix the assignment.

While at it, drop setting the PCI_SUBORDINATE_BUS temporarily to 0xff.
Setting it temporarily to that value (as discussed e.g. in [1]), is
only relevant for a running system that probes the buses. The value is
effectively unused for us just doing a DFS.

Also add a comment why we have to reassign during every reset (which I
found to be surprising.

Please note that hotplugging of bridges is in general still broken, will
be fixed next.

[1] http://www.science.unitn.it/~fiorella/guidelinux/tlk/node76.html

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index f017c1ded0..b7c4613fde 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -862,7 +862,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         qbus_set_hotplug_handler(bus, DEVICE(s), errp);
 
         if (dev->hotplugged) {
-            pci_default_write_config(pdev, PCI_PRIMARY_BUS, s->bus_no, 1);
+            pci_default_write_config(pdev, PCI_PRIMARY_BUS,
+                                     pci_dev_bus_num(pdev), 1);
             s->bus_no += 1;
             pci_default_write_config(pdev, PCI_SECONDARY_BUS, s->bus_no, 1);
             do {
@@ -1016,8 +1017,6 @@ static void s390_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
                                       void *opaque)
 {
     S390pciState *s = opaque;
-    unsigned int primary = s->bus_no;
-    unsigned int subordinate = 0xff;
     PCIBus *sec_bus = NULL;
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
@@ -1026,7 +1025,7 @@ static void s390_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
     }
 
     (s->bus_no)++;
-    pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+    pci_default_write_config(pdev, PCI_PRIMARY_BUS, pci_dev_bus_num(pdev), 1);
     pci_default_write_config(pdev, PCI_SECONDARY_BUS, s->bus_no, 1);
     pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, s->bus_no, 1);
 
@@ -1035,7 +1034,7 @@ static void s390_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
         return;
     }
 
-    pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
+    /* Assign numbers to all child bridges. The last is the highest number. */
     pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
                         s390_pci_enumerate_bridge, s);
     pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, s->bus_no, 1);
@@ -1046,6 +1045,10 @@ static void s390_pcihost_reset(DeviceState *dev)
     S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
     PCIBus *bus = s->parent_obj.bus;
 
+    /*
+     * When resetting a PCI bridge, the assigned numbers are set to 0. So
+     * on every system reset, we also have to reassign numbers.
+     */
     s->bus_no = 0;
     pci_for_each_device(bus, pci_bus_num(bus), s390_pci_enumerate_bridge, s);
 }
-- 
2.17.2

[Qemu-devel] [PATCH v2 2/6] s390x/pci: Fix hotplugging of PCI bridges

[David Hildenbrand]

When hotplugging a PCI bridge right now to the root port, we resolve
pci_get_bus(pdev)->parent_dev, which results in a SEGFAULT. Hotplugging
really only works right now when hotplugging to another bridge.

Instead, we have to properly check if we are already at the root.

Let's cleanup the code while at it a bit and factor out updating the
subordiante bus number into a separate function. The check for
"old_nr < nr" is right now not strictly necessary, but makes it more
obvious what is actually going on.

Most probably fixing up the topology is not our responsibility when
hotplugging. The guest has to sort this out. But let's keep it for now
and only fix current code to not crash.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index b7c4613fde..9b5c5fff60 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -843,6 +843,21 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 }
 
+static void s390_pci_update_subordinate(PCIDevice *dev, uint32_t nr)
+{
+    uint32_t old_nr;
+
+    pci_default_write_config(dev, PCI_SUBORDINATE_BUS, nr, 1);
+    while (!pci_bus_is_root(pci_get_bus(dev))) {
+        dev = pci_get_bus(dev)->parent_dev;
+
+        old_nr = pci_default_read_config(dev, PCI_SUBORDINATE_BUS, 1);
+        if (old_nr < nr) {
+            pci_default_write_config(dev, PCI_SUBORDINATE_BUS, nr, 1);
+        }
+    }
+}
+
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
@@ -851,26 +866,21 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     S390PCIBusDevice *pbdev = NULL;
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
-        BusState *bus;
         PCIBridge *pb = PCI_BRIDGE(dev);
-        PCIDevice *pdev = PCI_DEVICE(dev);
 
+        pdev = PCI_DEVICE(dev);
         pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
         pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
 
-        bus = BUS(&pb->sec_bus);
-        qbus_set_hotplug_handler(bus, DEVICE(s), errp);
+        qbus_set_hotplug_handler(BUS(&pb->sec_bus), DEVICE(s), errp);
 
         if (dev->hotplugged) {
             pci_default_write_config(pdev, PCI_PRIMARY_BUS,
                                      pci_dev_bus_num(pdev), 1);
             s->bus_no += 1;
             pci_default_write_config(pdev, PCI_SECONDARY_BUS, s->bus_no, 1);
-            do {
-                pdev = pci_get_bus(pdev)->parent_dev;
-                pci_default_write_config(pdev, PCI_SUBORDINATE_BUS,
-                                         s->bus_no, 1);
-            } while (pci_get_bus(pdev) && pci_dev_bus_num(pdev));
+
+            s390_pci_update_subordinate(pdev, s->bus_no);
         }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         pdev = PCI_DEVICE(dev);
-- 
2.17.2

[Qemu-devel] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[David Hildenbrand]

We decided to always create the PCI host bridge, even if 'zpci' is not
enabled (due to migration compatibility). This however right now allows
to add zPCI/PCI devices to a VM although the guest will never actually see
them, confusing people that are using a simple CPU model that has no
'zpci' enabled - "Why isn't this working" (David Hildenbrand)

Let's check for 'zpci' and at least print a warning that this will not
work as expected. We could also bail out, however that might break
existing QEMU commandlines.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 9b5c5fff60..2efd9186c2 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 {
     S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
 
+    if (!s390_has_feat(S390_FEAT_ZPCI)) {
+        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
+                    " The guest will not be able to see/use this device");
+    }
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         PCIDevice *pdev = PCI_DEVICE(dev);
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 4/6] s390x/pci: Introduce unplug requests and split unplug handler

[David Hildenbrand]

PCI on s390x is really weird and how it was modeled in QEMU might not have
been the right choice. Anyhow, right now it is the case that:
- Hotplugging a PCI device will silently create a zPCI device
  (if none is provided)
- Hotunplugging a zPCI device will unplug the PCI device (if any)
- Hotunplugging a PCI device will unplug also the zPCI device
As far as I can see, we can no longer change this behavior. But we
should fix it.

Both device types are handled via a single hotplug handler call. This
is problematic for various reasons:
1. Unplugging via the zPCI device allows to unplug devices that are not
   hot removable. (check performed in qdev_unplug()) - bad.
2. Hotplug handler chains are not possible for the unplug case. In the
   future, the machine might want to override hotplug handlers, to
   process device specific stuff and to then branch off to the actual
   hotplug handler. We need separate hotplug handler calls for both the
   PCI and zPCI device to make this work reliably. All other PCI
   implementations are already prepared to handle this correctly, only
   s390x is missing.

Therefore, introduce the unplug_request handler and properly perform
unplug checks by redirecting to the separate unplug_request handlers.
When finally unplugging, perform two separate hotplug_handler_unplug()
calls, first for the PCI device, followed by the zPCI device. This now
nicely splits unplugging paths for both devices.

The redirect part is a little hairy, as the user is allowed to trigger
unplug either via the PCI or the zPCI device. So redirect always to the
PCI unplug request handler first and remember if that check has been
performed in the zPCI device. Redirect then to the zPCI device unplug
request handler to perform the magic. Remembering that we already
checked the PCI device breaks the redirect loop.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 166 +++++++++++++++++++++++++++-------------
 hw/s390x/s390-pci-bus.h |   1 +
 2 files changed, 113 insertions(+), 54 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 2efd9186c2..e84e00d20c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -148,6 +148,22 @@ out:
     psccb->header.response_code = cpu_to_be16(rc);
 }
 
+static void s390_pci_perform_unplug(S390PCIBusDevice *pbdev)
+{
+    HotplugHandler *hotplug_ctrl;
+
+    /* Unplug the PCI device */
+    if (pbdev->pdev) {
+        hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(pbdev->pdev));
+        hotplug_handler_unplug(hotplug_ctrl, DEVICE(pbdev->pdev),
+                               &error_abort);
+    }
+
+    /* Unplug the zPCI device */
+    hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(pbdev));
+    hotplug_handler_unplug(hotplug_ctrl, DEVICE(pbdev), &error_abort);
+}
+
 void s390_pci_sclp_deconfigure(SCCB *sccb)
 {
     IoaCfgSccb *psccb = (IoaCfgSccb *)sccb;
@@ -179,7 +195,7 @@ void s390_pci_sclp_deconfigure(SCCB *sccb)
         rc = SCLP_RC_NORMAL_COMPLETION;
 
         if (pbdev->release_timer) {
-            qdev_unplug(DEVICE(pbdev->pdev), NULL);
+            s390_pci_perform_unplug(pbdev);
         }
     }
 out:
@@ -217,6 +233,24 @@ S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s,
     return NULL;
 }
 
+static S390PCIBusDevice *s390_pci_find_dev_by_pci(S390pciState *s,
+                                                  PCIDevice *pci_dev)
+{
+    S390PCIBusDevice *pbdev;
+
+    if (!pci_dev) {
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(pbdev, &s->zpci_devs, link) {
+        if (pbdev->pdev == pci_dev) {
+            return pbdev;
+        }
+    }
+
+    return NULL;
+}
+
 S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
 {
     return g_hash_table_lookup(s->zpci_table, &idx);
@@ -955,77 +989,100 @@ static void s390_pcihost_timer_cb(void *opaque)
     pbdev->state = ZPCI_FS_STANDBY;
     s390_pci_generate_plug_event(HP_EVENT_CONFIGURED_TO_STBRES,
                                  pbdev->fh, pbdev->fid);
-    qdev_unplug(DEVICE(pbdev), NULL);
+    s390_pci_perform_unplug(pbdev);
 }
 
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
     S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
-    PCIDevice *pci_dev = NULL;
-    PCIBus *bus;
-    int32_t devfn;
     S390PCIBusDevice *pbdev = NULL;
 
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pci_dev = PCI_DEVICE(dev);
+        PCIBus *bus;
+        int32_t devfn;
+
+        pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
+        g_assert(pbdev);
+
+        s390_pci_generate_plug_event(HP_EVENT_STANDBY_TO_RESERVED,
+                                     pbdev->fh, pbdev->fid);
+        bus = pci_get_bus(pci_dev);
+        devfn = pci_dev->devfn;
+        object_unparent(OBJECT(pci_dev));
+
+        s390_pci_msix_free(pbdev);
+        s390_pci_iommu_free(s, bus, devfn);
+        pbdev->pdev = NULL;
+        pbdev->state = ZPCI_FS_RESERVED;
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
+        pbdev = S390_PCI_DEVICE(dev);
+
+        if (pbdev->release_timer) {
+            timer_del(pbdev->release_timer);
+            timer_free(pbdev->release_timer);
+            pbdev->release_timer = NULL;
+        }
+        pbdev->fid = 0;
+        QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
+        g_hash_table_remove(s->zpci_table, &pbdev->idx);
+        object_unparent(OBJECT(pbdev));
+    }
+}
+
+static void s390_pcihost_unplug_request(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev,
+                                        Error **errp)
+{
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
+    S390PCIBusDevice *pbdev;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");
-        return;
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        pci_dev = PCI_DEVICE(dev);
-
-        QTAILQ_FOREACH(pbdev, &s->zpci_devs, link) {
-            if (pbdev->pdev == pci_dev) {
-                break;
-            }
-        }
-        assert(pbdev != NULL);
+        /*
+         * Redirect the unplug request to the zPCI device and remember that
+         * we've checked the PCI device already (to prevent endless recursion).
+         */
+        pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
+        g_assert(pbdev);
+        pbdev->pci_unplug_request_processed = true;
+        qdev_unplug(DEVICE(pbdev), errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
         pbdev = S390_PCI_DEVICE(dev);
-        pci_dev = pbdev->pdev;
-    } else {
-        g_assert_not_reached();
-    }
 
-    switch (pbdev->state) {
-    case ZPCI_FS_RESERVED:
-        goto out;
-    case ZPCI_FS_STANDBY:
-        break;
-    default:
-        if (pbdev->release_timer) {
+        /*
+         * If unplug was initially requested for the zPCI device, we
+         * first have to redirect to the PCI device, which will in return
+         * redirect back to us after performing its checks (if the request
+         * is not blocked, e.g. because it's a PCI bridge).
+         */
+        if (pbdev->pdev && !pbdev->pci_unplug_request_processed) {
+            qdev_unplug(DEVICE(pbdev->pdev), errp);
             return;
         }
-        s390_pci_generate_plug_event(HP_EVENT_DECONFIGURE_REQUEST,
-                                     pbdev->fh, pbdev->fid);
-        pbdev->release_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                                            s390_pcihost_timer_cb,
-                                            pbdev);
-        timer_mod(pbdev->release_timer,
-                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + HOT_UNPLUG_TIMEOUT);
-        return;
-    }
+        pbdev->pci_unplug_request_processed = false;
 
-    if (pbdev->release_timer) {
-        timer_del(pbdev->release_timer);
-        timer_free(pbdev->release_timer);
-        pbdev->release_timer = NULL;
+        switch (pbdev->state) {
+        case ZPCI_FS_STANDBY:
+        case ZPCI_FS_RESERVED:
+            s390_pci_perform_unplug(pbdev);
+            break;
+        default:
+            if (pbdev->release_timer) {
+                return;
+            }
+            s390_pci_generate_plug_event(HP_EVENT_DECONFIGURE_REQUEST,
+                                         pbdev->fh, pbdev->fid);
+            pbdev->release_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                                s390_pcihost_timer_cb, pbdev);
+            timer_mod(pbdev->release_timer,
+                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + HOT_UNPLUG_TIMEOUT);
+        }
+    } else {
+        g_assert_not_reached();
     }
-
-    s390_pci_generate_plug_event(HP_EVENT_STANDBY_TO_RESERVED,
-                                 pbdev->fh, pbdev->fid);
-    bus = pci_get_bus(pci_dev);
-    devfn = pci_dev->devfn;
-    object_unparent(OBJECT(pci_dev));
-    fmb_timer_free(pbdev);
-    s390_pci_msix_free(pbdev);
-    s390_pci_iommu_free(s, bus, devfn);
-    pbdev->pdev = NULL;
-    pbdev->state = ZPCI_FS_RESERVED;
-out:
-    pbdev->fid = 0;
-    QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
-    g_hash_table_remove(s->zpci_table, &pbdev->idx);
-    object_unparent(OBJECT(pbdev));
 }
 
 static void s390_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
@@ -1077,6 +1134,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
     dc->realize = s390_pcihost_realize;
     hc->pre_plug = s390_pcihost_pre_plug;
     hc->plug = s390_pcihost_plug;
+    hc->unplug_request = s390_pcihost_unplug_request;
     hc->unplug = s390_pcihost_unplug;
     msi_nonbroken = true;
 }
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index dadad1f758..b1a6bb8296 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -336,6 +336,7 @@ struct S390PCIBusDevice {
     IndAddr *summary_ind;
     IndAddr *indicator;
     QEMUTimer *release_timer;
+    bool pci_unplug_request_processed;
     QTAILQ_ENTRY(S390PCIBusDevice) link;
 };
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[David Hildenbrand]

Let's handle it similar to x86 ACPI PCI code and don't use a timer.
Instead, remember if an unplug request is pending and keep it pending
for eternity. (a follow up patch will process the request on
reboot).

We expect that a guest that is up and running, will process the unplug
request and trigger the unplug. This is normal operation, no timer needed.

If the guest does not react, this usually means something in the guest
is going wrong. Simply removing the device after 30 seconds does not
really sound like a good idea. It might sometimes be wanted, but I
consider this rather an "opt-in" decision as it might harm a guest not
prepared for it.

If we ever actually want a "forced/surprise removal", we will have to
implement something on top of the existing "device_del" framework. E.g.
also x86 might want to do a forced/surprise removal of PCI devices under
some conditions. "device_del X, forced=true" could be an option and will
require changes to the hotplug handler infrastructure.

This will then move the responsibility on when to do a forced removal
to a higher level. Doing a forced removal right now overcomplicates
things and doesn't really.

Let's allow to send multiple requests.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 38 +++++++-------------------------------
 hw/s390x/s390-pci-bus.h |  3 +--
 2 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e84e00d20c..867801ccf9 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -194,7 +194,7 @@ void s390_pci_sclp_deconfigure(SCCB *sccb)
         pbdev->state = ZPCI_FS_STANDBY;
         rc = SCLP_RC_NORMAL_COMPLETION;
 
-        if (pbdev->release_timer) {
+        if (pbdev->unplug_requested) {
             s390_pci_perform_unplug(pbdev);
         }
     }
@@ -975,23 +975,6 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 }
 
-static void s390_pcihost_timer_cb(void *opaque)
-{
-    S390PCIBusDevice *pbdev = opaque;
-
-    if (pbdev->summary_ind) {
-        pci_dereg_irqs(pbdev);
-    }
-    if (pbdev->iommu->enabled) {
-        pci_dereg_ioat(pbdev->iommu);
-    }
-
-    pbdev->state = ZPCI_FS_STANDBY;
-    s390_pci_generate_plug_event(HP_EVENT_CONFIGURED_TO_STBRES,
-                                 pbdev->fh, pbdev->fid);
-    s390_pci_perform_unplug(pbdev);
-}
-
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
@@ -1018,12 +1001,6 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
         pbdev->state = ZPCI_FS_RESERVED;
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
         pbdev = S390_PCI_DEVICE(dev);
-
-        if (pbdev->release_timer) {
-            timer_del(pbdev->release_timer);
-            timer_free(pbdev->release_timer);
-            pbdev->release_timer = NULL;
-        }
         pbdev->fid = 0;
         QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
         g_hash_table_remove(s->zpci_table, &pbdev->idx);
@@ -1070,15 +1047,14 @@ static void s390_pcihost_unplug_request(HotplugHandler *hotplug_dev,
             s390_pci_perform_unplug(pbdev);
             break;
         default:
-            if (pbdev->release_timer) {
-                return;
-            }
+            /*
+             * Allow to send multiple requests, e.g. if the guest crashed
+             * before releasing the device, we would not be able to send
+             * another request to the same VM (e.g. fresh OS).
+             */
+            pbdev->unplug_requested = true;
             s390_pci_generate_plug_event(HP_EVENT_DECONFIGURE_REQUEST,
                                          pbdev->fh, pbdev->fid);
-            pbdev->release_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                                                s390_pcihost_timer_cb, pbdev);
-            timer_mod(pbdev->release_timer,
-                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + HOT_UNPLUG_TIMEOUT);
         }
     } else {
         g_assert_not_reached();
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index b1a6bb8296..550f3cc5e9 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -35,7 +35,6 @@
 #define ZPCI_MAX_UID 0xffff
 #define UID_UNDEFINED 0
 #define UID_CHECKING_ENABLED 0x01
-#define HOT_UNPLUG_TIMEOUT (NANOSECONDS_PER_SECOND * 60 * 5)
 
 #define S390_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(S390pciState, (obj), TYPE_S390_PCI_HOST_BRIDGE)
@@ -335,8 +334,8 @@ struct S390PCIBusDevice {
     MemoryRegion msix_notify_mr;
     IndAddr *summary_ind;
     IndAddr *indicator;
-    QEMUTimer *release_timer;
     bool pci_unplug_request_processed;
+    bool unplug_requested;
     QTAILQ_ENTRY(S390PCIBusDevice) link;
 };
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 6/6] s390x/pci: Unplug remaining requested devices on pcihost reset

[David Hildenbrand]

When resetting the guest we should unplug and remove all devices that
are still pending.

With this patch, the requested device will be unpluged on reboot
(S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge
via qemu_devices_reset()).

This approach is similar to what's done for acpi PCI hotplug in
acpi_pcihp_reset() -> acpi_pcihp_update() ->
acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot().

s390_pci_generate_plug_event()'s will still be generated, I guess this
is not an issue. The same thing would happen right now when unplugging
a device just before starting the guest.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 867801ccf9..b9b0f44087 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev)
 {
     S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
     PCIBus *bus = s->parent_obj.bus;
+    S390PCIBusDevice *pbdev, *next;
+
+    /* Process all pending unplug requests */
+    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
+        if (pbdev->unplug_requested) {
+            if (pbdev->summary_ind) {
+                pci_dereg_irqs(pbdev);
+            }
+            if (pbdev->iommu->enabled) {
+                pci_dereg_ioat(pbdev->iommu);
+            }
+            pbdev->state = ZPCI_FS_STANDBY;
+            s390_pci_perform_unplug(pbdev);
+        }
+    }
 
     /*
      * When resetting a PCI bridge, the assigned numbers are set to 0. So
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[Cornelia Huck]

On Wed, 30 Jan 2019 16:57:27 +0100
David Hildenbrand <david@redhat.com> wrote:

> These are all the patches that are not yet upstream (@Conny you might
> already picked some, including them for the full picture) and after

I usually keep s390-next up-to-date, so if the patches are not there, I
have not yet queued them.

> a good discussion yesterday, including a patch t get rid of the release
> timer. I ran a couple of sanity tests on this series.
> 
> #1 and #2 fix hotplugging of PCI bridges.
> #3 warns when "zpci=off"
> #4 refactors unplugging
> #5 get's rid of the release timer
> #6 processes all unplug requests on reboot
> 
> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
> chaining + virtio-pmem")
> 
> David Hildenbrand (6):
>   s390x/pci: Fix primary bus number for PCI bridges
>   s390x/pci: Fix hotplugging of PCI bridges
>   s390x/pci: Warn when adding PCI devices without the 'zpci' feature
>   s390x/pci: Introduce unplug requests and split unplug handler
>   s390x/pci: Drop release timer and replace it with a flag
>   s390x/pci: Unplug remaining requested devices on pcihost reset
> 
>  hw/s390x/s390-pci-bus.c | 233 ++++++++++++++++++++++++++--------------
>  hw/s390x/s390-pci-bus.h |   4 +-
>  2 files changed, 152 insertions(+), 85 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 6/6] s390x/pci: Unplug remaining requested devices on pcihost reset

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> When resetting the guest we should unplug and remove all devices that
> are still pending.
> 
> With this patch, the requested device will be unpluged on reboot
> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge
> via qemu_devices_reset()).
> 
> This approach is similar to what's done for acpi PCI hotplug in
> acpi_pcihp_reset() -> acpi_pcihp_update() ->
> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot().
> 
> s390_pci_generate_plug_event()'s will still be generated, I guess this
> is not an issue. The same thing would happen right now when unplugging
> a device just before starting the guest.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 867801ccf9..b9b0f44087 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev)
>   {
>       S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
>       PCIBus *bus = s->parent_obj.bus;
> +    S390PCIBusDevice *pbdev, *next;
> +
> +    /* Process all pending unplug requests */
> +    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
> +        if (pbdev->unplug_requested) {
> +            if (pbdev->summary_ind) {
> +                pci_dereg_irqs(pbdev);
> +            }
> +            if (pbdev->iommu->enabled) {
> +                pci_dereg_ioat(pbdev->iommu);
> +            }
> +            pbdev->state = ZPCI_FS_STANDBY;
> +            s390_pci_perform_unplug(pbdev);
> +        }
> +    }
>   
>       /*
>        * When resetting a PCI bridge, the assigned numbers are set to 0. So
> 

Just "talking out loud" to reaffirm my understanding:

The ..._unplug_request function will have already generated the
deconfigure unplug event, and the unplug_requested flag will be set.
Makes sense -- easy to follow. :)

The callback that this code is essentially replacing used to generate a
CONFIGURED_TO_STBRES plug event, but as I recall from a previous
conversation regarding PCI states, we can skip this event because we're
removing the device from the guest during a reset here (there are a few
reasons why we'd do this, but no sense in listing them here). Also makes
sense to me.

Reviewed-by: Collin Walling <walling@linux.ibm.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> Let's handle it similar to x86 ACPI PCI code and don't use a timer.
> Instead, remember if an unplug request is pending and keep it pending
> for eternity. (a follow up patch will process the request on
> reboot).
> 
> We expect that a guest that is up and running, will process the unplug
> request and trigger the unplug. This is normal operation, no timer needed.
> 
> If the guest does not react, this usually means something in the guest
> is going wrong. Simply removing the device after 30 seconds does not
> really sound like a good idea. It might sometimes be wanted, but I
> consider this rather an "opt-in" decision as it might harm a guest not
> prepared for it.
> 
> If we ever actually want a "forced/surprise removal", we will have to
> implement something on top of the existing "device_del" framework. E.g.
> also x86 might want to do a forced/surprise removal of PCI devices under
> some conditions. "device_del X, forced=true" could be an option and will
> require changes to the hotplug handler infrastructure.
> 
> This will then move the responsibility on when to do a forced removal
> to a higher level. Doing a forced removal right now overcomplicates

nit: "over-complicates" or "over complicates"

> things and doesn't really.

"...and doesn't really." sounds odd to me :)

> 
> Let's allow to send multiple requests.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

Just a quick clean up of the commit message, and all is good.

Reviewed-by: Collin Walling <walling@linux.ibm.com>

Re: [Qemu-devel] [PATCH v2 4/6] s390x/pci: Introduce unplug requests and split unplug handler

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> PCI on s390x is really weird and how it was modeled in QEMU might not have
> been the right choice. Anyhow, right now it is the case that:
> - Hotplugging a PCI device will silently create a zPCI device
>    (if none is provided)
> - Hotunplugging a zPCI device will unplug the PCI device (if any)
> - Hotunplugging a PCI device will unplug also the zPCI device
> As far as I can see, we can no longer change this behavior. But we
> should fix it.
> 
> Both device types are handled via a single hotplug handler call. This
> is problematic for various reasons:
> 1. Unplugging via the zPCI device allows to unplug devices that are not
>     hot removable. (check performed in qdev_unplug()) - bad.
> 2. Hotplug handler chains are not possible for the unplug case. In the
>     future, the machine might want to override hotplug handlers, to
>     process device specific stuff and to then branch off to the actual
>     hotplug handler. We need separate hotplug handler calls for both the
>     PCI and zPCI device to make this work reliably. All other PCI
>     implementations are already prepared to handle this correctly, only
>     s390x is missing.
> 
> Therefore, introduce the unplug_request handler and properly perform
> unplug checks by redirecting to the separate unplug_request handlers.
> When finally unplugging, perform two separate hotplug_handler_unplug()
> calls, first for the PCI device, followed by the zPCI device. This now
> nicely splits unplugging paths for both devices.
> 
> The redirect part is a little hairy, as the user is allowed to trigger
> unplug either via the PCI or the zPCI device. So redirect always to the
> PCI unplug request handler first and remember if that check has been
> performed in the zPCI device. Redirect then to the zPCI device unplug
> request handler to perform the magic. Remembering that we already
> checked the PCI device breaks the redirect loop.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

Thanks for clearing up some of the areas I was having troubles following
during the last post. I think this looks a lot better than what we had
before. Patches 4, 5, and 6 make the code a lot easier to understand
what is going on here.

I assume you were able to squeeze in an easy test? At least making sure
we can still hot unplug a device from a guest successfully?

 From the code perspective:

Reviewed-by: Collin Walling <walling@linux.ibm.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> These are all the patches that are not yet upstream (@Conny you might
> already picked some, including them for the full picture) and after
> a good discussion yesterday, including a patch t get rid of the release
> timer. I ran a couple of sanity tests on this series.
> 
> #1 and #2 fix hotplugging of PCI bridges.
> #3 warns when "zpci=off"
> #4 refactors unplugging
> #5 get's rid of the release timer
> #6 processes all unplug requests on reboot
> 
> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
> chaining + virtio-pmem")

I've given my r-b for 4, 5, and 6. IIRC, the other patches in this
series looked fine from previous versions, but I'd like to look at them
more carefully before ack'ing. Please allow me some time to devote to
them in the near future. Trying my best to balance things :)

> 
> David Hildenbrand (6):
>    s390x/pci: Fix primary bus number for PCI bridges
>    s390x/pci: Fix hotplugging of PCI bridges
>    s390x/pci: Warn when adding PCI devices without the 'zpci' feature
>    s390x/pci: Introduce unplug requests and split unplug handler
>    s390x/pci: Drop release timer and replace it with a flag
>    s390x/pci: Unplug remaining requested devices on pcihost reset
> 
>   hw/s390x/s390-pci-bus.c | 233 ++++++++++++++++++++++++++--------------
>   hw/s390x/s390-pci-bus.h |   4 +-
>   2 files changed, 152 insertions(+), 85 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 4/6] s390x/pci: Introduce unplug requests and split unplug handler

[David Hildenbrand]

On 31.01.19 21:40, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> PCI on s390x is really weird and how it was modeled in QEMU might not have
>> been the right choice. Anyhow, right now it is the case that:
>> - Hotplugging a PCI device will silently create a zPCI device
>>    (if none is provided)
>> - Hotunplugging a zPCI device will unplug the PCI device (if any)
>> - Hotunplugging a PCI device will unplug also the zPCI device
>> As far as I can see, we can no longer change this behavior. But we
>> should fix it.
>>
>> Both device types are handled via a single hotplug handler call. This
>> is problematic for various reasons:
>> 1. Unplugging via the zPCI device allows to unplug devices that are not
>>     hot removable. (check performed in qdev_unplug()) - bad.
>> 2. Hotplug handler chains are not possible for the unplug case. In the
>>     future, the machine might want to override hotplug handlers, to
>>     process device specific stuff and to then branch off to the actual
>>     hotplug handler. We need separate hotplug handler calls for both the
>>     PCI and zPCI device to make this work reliably. All other PCI
>>     implementations are already prepared to handle this correctly, only
>>     s390x is missing.
>>
>> Therefore, introduce the unplug_request handler and properly perform
>> unplug checks by redirecting to the separate unplug_request handlers.
>> When finally unplugging, perform two separate hotplug_handler_unplug()
>> calls, first for the PCI device, followed by the zPCI device. This now
>> nicely splits unplugging paths for both devices.
>>
>> The redirect part is a little hairy, as the user is allowed to trigger
>> unplug either via the PCI or the zPCI device. So redirect always to the
>> PCI unplug request handler first and remember if that check has been
>> performed in the zPCI device. Redirect then to the zPCI device unplug
>> request handler to perform the magic. Remembering that we already
>> checked the PCI device breaks the redirect loop.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
> 
> Thanks for clearing up some of the areas I was having troubles following
> during the last post. I think this looks a lot better than what we had
> before. Patches 4, 5, and 6 make the code a lot easier to understand
> what is going on here.
> 
> I assume you were able to squeeze in an easy test? At least making sure
> we can still hot unplug a device from a guest successfully?

I only tested under TCG, but I did quite some tests
- Plug/unplug a device
- Disable slot in guest, unplug device
- Plug bridges, try to unplug
- Create hierarchy of bridges (statically and dynamically)
- Plug devices to bridge hierarchies ...

Thanks!

> 
>  From the code perspective:
> 
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[David Hildenbrand]

On 31.01.19 21:33, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> Let's handle it similar to x86 ACPI PCI code and don't use a timer.
>> Instead, remember if an unplug request is pending and keep it pending
>> for eternity. (a follow up patch will process the request on
>> reboot).
>>
>> We expect that a guest that is up and running, will process the unplug
>> request and trigger the unplug. This is normal operation, no timer needed.
>>
>> If the guest does not react, this usually means something in the guest
>> is going wrong. Simply removing the device after 30 seconds does not
>> really sound like a good idea. It might sometimes be wanted, but I
>> consider this rather an "opt-in" decision as it might harm a guest not
>> prepared for it.
>>
>> If we ever actually want a "forced/surprise removal", we will have to
>> implement something on top of the existing "device_del" framework. E.g.
>> also x86 might want to do a forced/surprise removal of PCI devices under
>> some conditions. "device_del X, forced=true" could be an option and will
>> require changes to the hotplug handler infrastructure.
>>
>> This will then move the responsibility on when to do a forced removal
>> to a higher level. Doing a forced removal right now overcomplicates
> 
> nit: "over-complicates" or "over complicates"
> 
>> things and doesn't really.
> 
> "...and doesn't really." sounds odd to me :)

Hmm, I guess this was meant to be

"and doesn't really seem to be required." :)

> 
>>
>> Let's allow to send multiple requests.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
> 
> Just a quick clean up of the commit message, and all is good.
> 

Thanks!

> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 6/6] s390x/pci: Unplug remaining requested devices on pcihost reset

[David Hildenbrand]

On 31.01.19 21:26, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> When resetting the guest we should unplug and remove all devices that
>> are still pending.
>>
>> With this patch, the requested device will be unpluged on reboot
>> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge
>> via qemu_devices_reset()).
>>
>> This approach is similar to what's done for acpi PCI hotplug in
>> acpi_pcihp_reset() -> acpi_pcihp_update() ->
>> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot().
>>
>> s390_pci_generate_plug_event()'s will still be generated, I guess this
>> is not an issue. The same thing would happen right now when unplugging
>> a device just before starting the guest.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 867801ccf9..b9b0f44087 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev)
>>   {
>>       S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
>>       PCIBus *bus = s->parent_obj.bus;
>> +    S390PCIBusDevice *pbdev, *next;
>> +
>> +    /* Process all pending unplug requests */
>> +    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
>> +        if (pbdev->unplug_requested) {
>> +            if (pbdev->summary_ind) {
>> +                pci_dereg_irqs(pbdev);
>> +            }
>> +            if (pbdev->iommu->enabled) {
>> +                pci_dereg_ioat(pbdev->iommu);
>> +            }
>> +            pbdev->state = ZPCI_FS_STANDBY;
>> +            s390_pci_perform_unplug(pbdev);
>> +        }
>> +    }
>>   
>>       /*
>>        * When resetting a PCI bridge, the assigned numbers are set to 0. So
>>
> 
> Just "talking out loud" to reaffirm my understanding:
> 
> The ..._unplug_request function will have already generated the
> deconfigure unplug event, and the unplug_requested flag will be set.
> Makes sense -- easy to follow. :)

Yes, that's how it should work :)

> 
> The callback that this code is essentially replacing used to generate a
> CONFIGURED_TO_STBRES plug event, but as I recall from a previous
> conversation regarding PCI states, we can skip this event because we're
> removing the device from the guest during a reset here (there are a few
> reasons why we'd do this, but no sense in listing them here). Also makes
> sense to me.

Yes, it's pretty much to the guest like this device never existed.

Thanks!

> 
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[David Hildenbrand]

On 30.01.19 16:57, David Hildenbrand wrote:
> Let's handle it similar to x86 ACPI PCI code and don't use a timer.
> Instead, remember if an unplug request is pending and keep it pending
> for eternity. (a follow up patch will process the request on
> reboot).
> 
> We expect that a guest that is up and running, will process the unplug
> request and trigger the unplug. This is normal operation, no timer needed.
> 
> If the guest does not react, this usually means something in the guest
> is going wrong. Simply removing the device after 30 seconds does not
> really sound like a good idea. It might sometimes be wanted, but I
> consider this rather an "opt-in" decision as it might harm a guest not
> prepared for it.
> 
> If we ever actually want a "forced/surprise removal", we will have to
> implement something on top of the existing "device_del" framework. E.g.
> also x86 might want to do a forced/surprise removal of PCI devices under
> some conditions. "device_del X, forced=true" could be an option and will
> require changes to the hotplug handler infrastructure.
> 
> This will then move the responsibility on when to do a forced removal
> to a higher level. Doing a forced removal right now overcomplicates
> things and doesn't really.
> 
> Let's allow to send multiple requests.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 38 +++++++-------------------------------
>  hw/s390x/s390-pci-bus.h |  3 +--
>  2 files changed, 8 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e84e00d20c..867801ccf9 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -194,7 +194,7 @@ void s390_pci_sclp_deconfigure(SCCB *sccb)
>          pbdev->state = ZPCI_FS_STANDBY;
>          rc = SCLP_RC_NORMAL_COMPLETION;
>  
> -        if (pbdev->release_timer) {
> +        if (pbdev->unplug_requested) {
>              s390_pci_perform_unplug(pbdev);
>          }
>      }
> @@ -975,23 +975,6 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> -static void s390_pcihost_timer_cb(void *opaque)
> -{
> -    S390PCIBusDevice *pbdev = opaque;
> -
> -    if (pbdev->summary_ind) {
> -        pci_dereg_irqs(pbdev);
> -    }
> -    if (pbdev->iommu->enabled) {
> -        pci_dereg_ioat(pbdev->iommu);
> -    }
> -
> -    pbdev->state = ZPCI_FS_STANDBY;
> -    s390_pci_generate_plug_event(HP_EVENT_CONFIGURED_TO_STBRES,
> -                                 pbdev->fh, pbdev->fid);
> -    s390_pci_perform_unplug(pbdev);
> -}
> -
>  static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                  Error **errp)
>  {
> @@ -1018,12 +1001,6 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          pbdev->state = ZPCI_FS_RESERVED;
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>          pbdev = S390_PCI_DEVICE(dev);
> -
> -        if (pbdev->release_timer) {
> -            timer_del(pbdev->release_timer);
> -            timer_free(pbdev->release_timer);
> -            pbdev->release_timer = NULL;
> -        }
>          pbdev->fid = 0;
>          QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
>          g_hash_table_remove(s->zpci_table, &pbdev->idx);
> @@ -1070,15 +1047,14 @@ static void s390_pcihost_unplug_request(HotplugHandler *hotplug_dev,
>              s390_pci_perform_unplug(pbdev);
>              break;
>          default:
> -            if (pbdev->release_timer) {
> -                return;
> -            }
> +            /*
> +             * Allow to send multiple requests, e.g. if the guest crashed
> +             * before releasing the device, we would not be able to send
> +             * another request to the same VM (e.g. fresh OS).
> +             */
> +            pbdev->unplug_requested = true;
>              s390_pci_generate_plug_event(HP_EVENT_DECONFIGURE_REQUEST,
>                                           pbdev->fh, pbdev->fid);
> -            pbdev->release_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> -                                                s390_pcihost_timer_cb, pbdev);
> -            timer_mod(pbdev->release_timer,
> -                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + HOT_UNPLUG_TIMEOUT);
>          }
>      } else {
>          g_assert_not_reached();
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index b1a6bb8296..550f3cc5e9 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -35,7 +35,6 @@
>  #define ZPCI_MAX_UID 0xffff
>  #define UID_UNDEFINED 0
>  #define UID_CHECKING_ENABLED 0x01
> -#define HOT_UNPLUG_TIMEOUT (NANOSECONDS_PER_SECOND * 60 * 5)
>  
>  #define S390_PCI_HOST_BRIDGE(obj) \
>      OBJECT_CHECK(S390pciState, (obj), TYPE_S390_PCI_HOST_BRIDGE)
> @@ -335,8 +334,8 @@ struct S390PCIBusDevice {
>      MemoryRegion msix_notify_mr;
>      IndAddr *summary_ind;
>      IndAddr *indicator;
> -    QEMUTimer *release_timer;
>      bool pci_unplug_request_processed;
> +    bool unplug_requested;
>      QTAILQ_ENTRY(S390PCIBusDevice) link;
>  };
>  
> 

Thinking out loud:

We should migrate the flag in the future. This is already a problem
right now, as the timer is also not migrated.

If the unplug request is sent and we migrate before the guest can react,
the unplug would not happen.

However, looks like migration of zpci devices is not implemented _at all_.

This does not matter for pci passthrough (main use case). But when
wanting to use e.g. virtio-pci-net, things are shaky after migration.

So this is future work.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[David Hildenbrand]

On 31.01.19 21:43, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> These are all the patches that are not yet upstream (@Conny you might
>> already picked some, including them for the full picture) and after
>> a good discussion yesterday, including a patch t get rid of the release
>> timer. I ran a couple of sanity tests on this series.
>>
>> #1 and #2 fix hotplugging of PCI bridges.
>> #3 warns when "zpci=off"
>> #4 refactors unplugging
>> #5 get's rid of the release timer
>> #6 processes all unplug requests on reboot
>>
>> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
>> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
>> chaining + virtio-pmem")
> 
> I've given my r-b for 4, 5, and 6. IIRC, the other patches in this
> series looked fine from previous versions, but I'd like to look at them
> more carefully before ack'ing. Please allow me some time to devote to
> them in the near future. Trying my best to balance things :)

Thanks, patch 4 was the most important one for me (needed for the qdev
reworks). All the other stuff was fallout from testing and code review.

> 
>>
>> David Hildenbrand (6):
>>    s390x/pci: Fix primary bus number for PCI bridges
>>    s390x/pci: Fix hotplugging of PCI bridges
>>    s390x/pci: Warn when adding PCI devices without the 'zpci' feature
>>    s390x/pci: Introduce unplug requests and split unplug handler
>>    s390x/pci: Drop release timer and replace it with a flag
>>    s390x/pci: Unplug remaining requested devices on pcihost reset
>>
>>   hw/s390x/s390-pci-bus.c | 233 ++++++++++++++++++++++++++--------------
>>   hw/s390x/s390-pci-bus.h |   4 +-
>>   2 files changed, 152 insertions(+), 85 deletions(-)
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[Cornelia Huck]

On Thu, 31 Jan 2019 15:43:16 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> On 1/30/19 10:57 AM, David Hildenbrand wrote:
> > These are all the patches that are not yet upstream (@Conny you might
> > already picked some, including them for the full picture) and after
> > a good discussion yesterday, including a patch t get rid of the release
> > timer. I ran a couple of sanity tests on this series.
> > 
> > #1 and #2 fix hotplugging of PCI bridges.
> > #3 warns when "zpci=off"
> > #4 refactors unplugging
> > #5 get's rid of the release timer
> > #6 processes all unplug requests on reboot
> > 
> > @Colin, can you review/ack? Especially Patch #4 is needed for qdev
> > patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
> > chaining + virtio-pmem")  
> 
> I've given my r-b for 4, 5, and 6. IIRC, the other patches in this
> series looked fine from previous versions, but I'd like to look at them
> more carefully before ack'ing. Please allow me some time to devote to
> them in the near future. Trying my best to balance things :)

Great, thanks for looking. If patches 4-6 can be applied without
patches 1-3 (I think they can?), I'll go ahead and queue them.

(I'll probably send a pull request next week, will be happy to include
the other patches as well.)

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[David Hildenbrand]

On 01.02.19 09:35, Cornelia Huck wrote:
> On Thu, 31 Jan 2019 15:43:16 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>>> These are all the patches that are not yet upstream (@Conny you might
>>> already picked some, including them for the full picture) and after
>>> a good discussion yesterday, including a patch t get rid of the release
>>> timer. I ran a couple of sanity tests on this series.
>>>
>>> #1 and #2 fix hotplugging of PCI bridges.
>>> #3 warns when "zpci=off"
>>> #4 refactors unplugging
>>> #5 get's rid of the release timer
>>> #6 processes all unplug requests on reboot
>>>
>>> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
>>> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
>>> chaining + virtio-pmem")  
>>
>> I've given my r-b for 4, 5, and 6. IIRC, the other patches in this
>> series looked fine from previous versions, but I'd like to look at them
>> more carefully before ack'ing. Please allow me some time to devote to
>> them in the near future. Trying my best to balance things :)
> 
> Great, thanks for looking. If patches 4-6 can be applied without
> patches 1-3 (I think they can?), I'll go ahead and queue them.

Yes, I think they can, patch 1-3 mainly deals with plugging, these ones
with unplugging. Thanks Conny!

> 
> (I'll probably send a pull request next week, will be happy to include
> the other patches as well.)
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[Cornelia Huck]

On Thu, 31 Jan 2019 22:21:01 +0100
David Hildenbrand <david@redhat.com> wrote:

> Thinking out loud:
> 
> We should migrate the flag in the future. This is already a problem
> right now, as the timer is also not migrated.
> 
> If the unplug request is sent and we migrate before the guest can react,
> the unplug would not happen.
> 
> However, looks like migration of zpci devices is not implemented _at all_.

Oh, joy.

> 
> This does not matter for pci passthrough (main use case). But when
> wanting to use e.g. virtio-pci-net, things are shaky after migration.
> 
> So this is future work.
> 

Hopefully folks running s390x guests are rather using virtio-ccw
devices anyway...

I agree that this needs to be taken care of on top of these changes.
Should we mark zpci devices unmigratable, or does it work for some
values of "work"?

Re: [Qemu-devel] [PATCH v2 6/6] s390x/pci: Unplug remaining requested devices on pcihost reset

[Cornelia Huck]

On Wed, 30 Jan 2019 16:57:33 +0100
David Hildenbrand <david@redhat.com> wrote:

> When resetting the guest we should unplug and remove all devices that
> are still pending.
> 
> With this patch, the requested device will be unpluged on reboot
> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge
> via qemu_devices_reset()).
> 
> This approach is similar to what's done for acpi PCI hotplug in
> acpi_pcihp_reset() -> acpi_pcihp_update() ->
> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot().
> 
> s390_pci_generate_plug_event()'s will still be generated, I guess this
> is not an issue. The same thing would happen right now when unplugging
> a device just before starting the guest.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 867801ccf9..b9b0f44087 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev)
>  {
>      S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
>      PCIBus *bus = s->parent_obj.bus;
> +    S390PCIBusDevice *pbdev, *next;
> +
> +    /* Process all pending unplug requests */
> +    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
> +        if (pbdev->unplug_requested) {
> +            if (pbdev->summary_ind) {
> +                pci_dereg_irqs(pbdev);
> +            }
> +            if (pbdev->iommu->enabled) {
> +                pci_dereg_ioat(pbdev->iommu);
> +            }
> +            pbdev->state = ZPCI_FS_STANDBY;
> +            s390_pci_perform_unplug(pbdev);
> +        }
> +    }
>  
>      /*
>       * When resetting a PCI bridge, the assigned numbers are set to 0. So

I'm afraid this one doesn't quite apply without the first patches in
the series, and I'm not sure how to fix it up. I'll either take a
rebase on a codebase without patches 1-3, or this patch together with
patches 1-3 after they get acks (whatever comes first :)

Re: [Qemu-devel] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[David Hildenbrand]

On 01.02.19 11:08, Cornelia Huck wrote:
> On Thu, 31 Jan 2019 22:21:01 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Thinking out loud:
>>
>> We should migrate the flag in the future. This is already a problem
>> right now, as the timer is also not migrated.
>>
>> If the unplug request is sent and we migrate before the guest can react,
>> the unplug would not happen.
>>
>> However, looks like migration of zpci devices is not implemented _at all_.
> 
> Oh, joy.
> 
>>
>> This does not matter for pci passthrough (main use case). But when
>> wanting to use e.g. virtio-pci-net, things are shaky after migration.
>>
>> So this is future work.
>>
> 
> Hopefully folks running s390x guests are rather using virtio-ccw
> devices anyway...
> 
> I agree that this needs to be taken care of on top of these changes.
> Should we mark zpci devices unmigratable, or does it work for some
> values of "work"?
> 

It works if the guest never configures a zpci device I guess ... so I
think this is pretty much broken. E.g. the state of the zpci device is
not even migrated unless I am missing something.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 4/6] s390x/pci: Introduce unplug requests and split unplug handler

[Cornelia Huck]

On Wed, 30 Jan 2019 16:57:31 +0100
David Hildenbrand <david@redhat.com> wrote:

> PCI on s390x is really weird and how it was modeled in QEMU might not have
> been the right choice. Anyhow, right now it is the case that:
> - Hotplugging a PCI device will silently create a zPCI device
>   (if none is provided)
> - Hotunplugging a zPCI device will unplug the PCI device (if any)
> - Hotunplugging a PCI device will unplug also the zPCI device
> As far as I can see, we can no longer change this behavior. But we
> should fix it.
> 
> Both device types are handled via a single hotplug handler call. This
> is problematic for various reasons:
> 1. Unplugging via the zPCI device allows to unplug devices that are not
>    hot removable. (check performed in qdev_unplug()) - bad.
> 2. Hotplug handler chains are not possible for the unplug case. In the
>    future, the machine might want to override hotplug handlers, to
>    process device specific stuff and to then branch off to the actual
>    hotplug handler. We need separate hotplug handler calls for both the
>    PCI and zPCI device to make this work reliably. All other PCI
>    implementations are already prepared to handle this correctly, only
>    s390x is missing.
> 
> Therefore, introduce the unplug_request handler and properly perform
> unplug checks by redirecting to the separate unplug_request handlers.
> When finally unplugging, perform two separate hotplug_handler_unplug()
> calls, first for the PCI device, followed by the zPCI device. This now
> nicely splits unplugging paths for both devices.
> 
> The redirect part is a little hairy, as the user is allowed to trigger
> unplug either via the PCI or the zPCI device. So redirect always to the
> PCI unplug request handler first and remember if that check has been
> performed in the zPCI device. Redirect then to the zPCI device unplug
> request handler to perform the magic. Remembering that we already
> checked the PCI device breaks the redirect loop.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 166 +++++++++++++++++++++++++++-------------
>  hw/s390x/s390-pci-bus.h |   1 +
>  2 files changed, 113 insertions(+), 54 deletions(-)

Thanks, applied.

Re: [Qemu-devel] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[Cornelia Huck]

On Wed, 30 Jan 2019 16:57:32 +0100
David Hildenbrand <david@redhat.com> wrote:

> Let's handle it similar to x86 ACPI PCI code and don't use a timer.
> Instead, remember if an unplug request is pending and keep it pending
> for eternity. (a follow up patch will process the request on
> reboot).
> 
> We expect that a guest that is up and running, will process the unplug
> request and trigger the unplug. This is normal operation, no timer needed.
> 
> If the guest does not react, this usually means something in the guest
> is going wrong. Simply removing the device after 30 seconds does not
> really sound like a good idea. It might sometimes be wanted, but I
> consider this rather an "opt-in" decision as it might harm a guest not
> prepared for it.
> 
> If we ever actually want a "forced/surprise removal", we will have to
> implement something on top of the existing "device_del" framework. E.g.
> also x86 might want to do a forced/surprise removal of PCI devices under
> some conditions. "device_del X, forced=true" could be an option and will
> require changes to the hotplug handler infrastructure.
> 
> This will then move the responsibility on when to do a forced removal
> to a higher level. Doing a forced removal right now overcomplicates
> things and doesn't really.
> 
> Let's allow to send multiple requests.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 38 +++++++-------------------------------
>  hw/s390x/s390-pci-bus.h |  3 +--
>  2 files changed, 8 insertions(+), 33 deletions(-)

Thanks, applied.

Re: [Qemu-devel] [PATCH v2 5/6] s390x/pci: Drop release timer and replace it with a flag

[Cornelia Huck]

On Fri, 1 Feb 2019 11:37:56 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 01.02.19 11:08, Cornelia Huck wrote:
> > On Thu, 31 Jan 2019 22:21:01 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Thinking out loud:
> >>
> >> We should migrate the flag in the future. This is already a problem
> >> right now, as the timer is also not migrated.
> >>
> >> If the unplug request is sent and we migrate before the guest can react,
> >> the unplug would not happen.
> >>
> >> However, looks like migration of zpci devices is not implemented _at all_.  
> > 
> > Oh, joy.
> >   
> >>
> >> This does not matter for pci passthrough (main use case). But when
> >> wanting to use e.g. virtio-pci-net, things are shaky after migration.
> >>
> >> So this is future work.
> >>  
> > 
> > Hopefully folks running s390x guests are rather using virtio-ccw
> > devices anyway...
> > 
> > I agree that this needs to be taken care of on top of these changes.
> > Should we mark zpci devices unmigratable, or does it work for some
> > values of "work"?
> >   
> 
> It works if the guest never configures a zpci device I guess ... so I
> think this is pretty much broken. E.g. the state of the zpci device is
> not even migrated unless I am missing something.

Ok, that seems to call for marking zpci devices unmigratable...

Re: [Qemu-devel] [PATCH v2 6/6] s390x/pci: Unplug remaining requested devices on pcihost reset

[David Hildenbrand]

On 01.02.19 11:19, Cornelia Huck wrote:
> On Wed, 30 Jan 2019 16:57:33 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> When resetting the guest we should unplug and remove all devices that
>> are still pending.
>>
>> With this patch, the requested device will be unpluged on reboot
>> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge
>> via qemu_devices_reset()).
>>
>> This approach is similar to what's done for acpi PCI hotplug in
>> acpi_pcihp_reset() -> acpi_pcihp_update() ->
>> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot().
>>
>> s390_pci_generate_plug_event()'s will still be generated, I guess this
>> is not an issue. The same thing would happen right now when unplugging
>> a device just before starting the guest.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-pci-bus.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 867801ccf9..b9b0f44087 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev)
>>  {
>>      S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
>>      PCIBus *bus = s->parent_obj.bus;
>> +    S390PCIBusDevice *pbdev, *next;
>> +
>> +    /* Process all pending unplug requests */
>> +    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
>> +        if (pbdev->unplug_requested) {
>> +            if (pbdev->summary_ind) {
>> +                pci_dereg_irqs(pbdev);
>> +            }
>> +            if (pbdev->iommu->enabled) {
>> +                pci_dereg_ioat(pbdev->iommu);
>> +            }
>> +            pbdev->state = ZPCI_FS_STANDBY;
>> +            s390_pci_perform_unplug(pbdev);
>> +        }
>> +    }
>>  
>>      /*
>>       * When resetting a PCI bridge, the assigned numbers are set to 0. So
> 
> I'm afraid this one doesn't quite apply without the first patches in
> the series, and I'm not sure how to fix it up. I'll either take a
> rebase on a codebase without patches 1-3, or this patch together with
> patches 1-3 after they get acks (whatever comes first :)
> 

I guess it's only the comment that get's added in patch #1, so no major
conflicts.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[Collin Walling]

On 2/1/19 4:18 AM, David Hildenbrand wrote:
> On 01.02.19 09:35, Cornelia Huck wrote:
>> On Thu, 31 Jan 2019 15:43:16 -0500
>> Collin Walling <walling@linux.ibm.com> wrote:
>>
>>> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>>>> These are all the patches that are not yet upstream (@Conny you might
>>>> already picked some, including them for the full picture) and after
>>>> a good discussion yesterday, including a patch t get rid of the release
>>>> timer. I ran a couple of sanity tests on this series.
>>>>
>>>> #1 and #2 fix hotplugging of PCI bridges.
>>>> #3 warns when "zpci=off"
>>>> #4 refactors unplugging
>>>> #5 get's rid of the release timer
>>>> #6 processes all unplug requests on reboot
>>>>
>>>> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
>>>> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
>>>> chaining + virtio-pmem")
>>>
>>> I've given my r-b for 4, 5, and 6. IIRC, the other patches in this
>>> series looked fine from previous versions, but I'd like to look at them
>>> more carefully before ack'ing. Please allow me some time to devote to
>>> them in the near future. Trying my best to balance things :)
>>
>> Great, thanks for looking. If patches 4-6 can be applied without
>> patches 1-3 (I think they can?), I'll go ahead and queue them.
> 
> Yes, I think they can, patch 1-3 mainly deals with plugging, these ones
> with unplugging. Thanks Conny!
> 
>>
>> (I'll probably send a pull request next week, will be happy to include
>> the other patches as well.)
>>
> 
> 

I noted the conflict you ran into when applying. I'll try to get to
reviewing the other patches either by today or Monday. If for some
reason I don't get to it by then, please ping me.

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> We decided to always create the PCI host bridge, even if 'zpci' is not
> enabled (due to migration compatibility). This however right now allows
> to add zPCI/PCI devices to a VM although the guest will never actually see
> them, confusing people that are using a simple CPU model that has no
> 'zpci' enabled - "Why isn't this working" (David Hildenbrand)
> 
> Let's check for 'zpci' and at least print a warning that this will not
> work as expected. We could also bail out, however that might break
> existing QEMU commandlines.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 9b5c5fff60..2efd9186c2 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>   {
>       S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>   
> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> +        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
> +                    " The guest will not be able to see/use this device");
> +    }
> +
>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>           PCIDevice *pdev = PCI_DEVICE(dev);
>   
> 

I wonder if someone might misconstrue this as "the _PCI device_ needs
the zpci feature." I think "'zpci' CPU feature required to support
PCI/zPCI devices." reads better. The last sentence is fine to me.

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[David Hildenbrand]

On 04.02.19 21:19, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> We decided to always create the PCI host bridge, even if 'zpci' is not
>> enabled (due to migration compatibility). This however right now allows
>> to add zPCI/PCI devices to a VM although the guest will never actually see
>> them, confusing people that are using a simple CPU model that has no
>> 'zpci' enabled - "Why isn't this working" (David Hildenbrand)
>>
>> Let's check for 'zpci' and at least print a warning that this will not
>> work as expected. We could also bail out, however that might break
>> existing QEMU commandlines.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 9b5c5fff60..2efd9186c2 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>   {
>>       S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>>   
>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
>> +        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
>> +                    " The guest will not be able to see/use this device");
>> +    }
>> +
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>           PCIDevice *pdev = PCI_DEVICE(dev);
>>   
>>
> 
> I wonder if someone might misconstrue this as "the _PCI device_ needs
> the zpci feature." I think "'zpci' CPU feature required to support
> PCI/zPCI devices." reads better. The last sentence is fine to me.
> 

Well, the guest needs the 'zpci' feature to see the device. And that's
what that message says in my opinion. Not that a device needs to have a
feature (I added "CPU feature" for this reason).

"required to support" does it not make very clear what we actually want
to say.

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[Collin Walling]

On 2/4/19 4:54 PM, David Hildenbrand wrote:
> On 04.02.19 21:19, Collin Walling wrote:
>> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>>> We decided to always create the PCI host bridge, even if 'zpci' is not
>>> enabled (due to migration compatibility). This however right now allows
>>> to add zPCI/PCI devices to a VM although the guest will never actually see
>>> them, confusing people that are using a simple CPU model that has no
>>> 'zpci' enabled - "Why isn't this working" (David Hildenbrand)
>>>
>>> Let's check for 'zpci' and at least print a warning that this will not
>>> work as expected. We could also bail out, however that might break
>>> existing QEMU commandlines.
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    hw/s390x/s390-pci-bus.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 9b5c5fff60..2efd9186c2 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>    {
>>>        S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>>>    
>>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
>>> +        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
>>> +                    " The guest will not be able to see/use this device");
>>> +    }
>>> +
>>>        if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>>            PCIDevice *pdev = PCI_DEVICE(dev);
>>>    
>>>
>>
>> I wonder if someone might misconstrue this as "the _PCI device_ needs
>> the zpci feature." I think "'zpci' CPU feature required to support
>> PCI/zPCI devices." reads better. The last sentence is fine to me.
>>
> 
> Well, the guest needs the 'zpci' feature to see the device. And that's
> what that message says in my opinion. Not that a device needs to have a
> feature (I added "CPU feature" for this reason).
> 
> "required to support" does it not make very clear what we actually want
> to say.
> 
> Thanks!
> 

I see your point. We can still plug in the device without the CPU
feature, but the device will ultimately be useless to the guest. Thanks
for clearing that up.

Still, the wording reads strangely to me. I read it as the PCI device
itself requires a "zpci CPU feature" which of course does not make sense
(and I fully understand that's not what you mean here).

What do you think about:

"PCI/zPCI device plugging without 'zpci' CPU feature enabled." along
with your second sentence, of course.

Either way you decide, it's still a good idea to have this warning in
here. I'm really just debating syntax and not semantics, so it's not
really important. I won't impede this patch over a differing opinion of
a small rewording. :)

Reviewed-by: Collin Walling <walling@linux.ibm.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[David Hildenbrand]

On 04.02.19 23:42, Collin Walling wrote:
> On 2/4/19 4:54 PM, David Hildenbrand wrote:
>> On 04.02.19 21:19, Collin Walling wrote:
>>> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>>>> We decided to always create the PCI host bridge, even if 'zpci' is not
>>>> enabled (due to migration compatibility). This however right now allows
>>>> to add zPCI/PCI devices to a VM although the guest will never actually see
>>>> them, confusing people that are using a simple CPU model that has no
>>>> 'zpci' enabled - "Why isn't this working" (David Hildenbrand)
>>>>
>>>> Let's check for 'zpci' and at least print a warning that this will not
>>>> work as expected. We could also bail out, however that might break
>>>> existing QEMU commandlines.
>>>>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>    hw/s390x/s390-pci-bus.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>> index 9b5c5fff60..2efd9186c2 100644
>>>> --- a/hw/s390x/s390-pci-bus.c
>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>> @@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>>    {
>>>>        S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>>>>    
>>>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
>>>> +        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
>>>> +                    " The guest will not be able to see/use this device");
>>>> +    }
>>>> +
>>>>        if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>>>            PCIDevice *pdev = PCI_DEVICE(dev);
>>>>    
>>>>
>>>
>>> I wonder if someone might misconstrue this as "the _PCI device_ needs
>>> the zpci feature." I think "'zpci' CPU feature required to support
>>> PCI/zPCI devices." reads better. The last sentence is fine to me.
>>>
>>
>> Well, the guest needs the 'zpci' feature to see the device. And that's
>> what that message says in my opinion. Not that a device needs to have a
>> feature (I added "CPU feature" for this reason).
>>
>> "required to support" does it not make very clear what we actually want
>> to say.
>>
>> Thanks!
>>
> 
> I see your point. We can still plug in the device without the CPU
> feature, but the device will ultimately be useless to the guest. Thanks
> for clearing that up.
> 
> Still, the wording reads strangely to me. I read it as the PCI device
> itself requires a "zpci CPU feature" which of course does not make sense
> (and I fully understand that's not what you mean here).
> 
> What do you think about:
> 
> "PCI/zPCI device plugging without 'zpci' CPU feature enabled." along
> with your second sentence, of course.

"Plugging a PCI/zPCI device without the 'zpci' CPU feature enabled. The
guest will not be able to see/use this device."

would make sense to me!

> 
> Either way you decide, it's still a good idea to have this warning in
> here. I'm really just debating syntax and not semantics, so it's not
> really important. I won't impede this patch over a differing opinion of
> a small rewording. :)
> 
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/6] s390x/pci: Fix hotplugging of PCI bridges

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> When hotplugging a PCI bridge right now to the root port, we resolve
> pci_get_bus(pdev)->parent_dev, which results in a SEGFAULT. Hotplugging
> really only works right now when hotplugging to another bridge.
> 
> Instead, we have to properly check if we are already at the root.
> 
> Let's cleanup the code while at it a bit and factor out updating the
> subordiante bus number into a separate function. The check for

s/subordiante/subordinate

> "old_nr < nr" is right now not strictly necessary, but makes it more
> obvious what is actually going on.
> 
> Most probably fixing up the topology is not our responsibility when
> hotplugging. The guest has to sort this out. But let's keep it for now
> and only fix current code to not crash.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 28 +++++++++++++++++++---------
>   1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index b7c4613fde..9b5c5fff60 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -843,6 +843,21 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       }
>   }
>   
> +static void s390_pci_update_subordinate(PCIDevice *dev, uint32_t nr)
> +{
> +    uint32_t old_nr;
> +
> +    pci_default_write_config(dev, PCI_SUBORDINATE_BUS, nr, 1);
> +    while (!pci_bus_is_root(pci_get_bus(dev))) {
> +        dev = pci_get_bus(dev)->parent_dev;
> +
> +        old_nr = pci_default_read_config(dev, PCI_SUBORDINATE_BUS, 1);
> +        if (old_nr < nr) {
> +            pci_default_write_config(dev, PCI_SUBORDINATE_BUS, nr, 1);
> +        }
> +    }
> +} > +
>   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>   {
> @@ -851,26 +866,21 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       S390PCIBusDevice *pbdev = NULL;
>   
>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> -        BusState *bus;
>           PCIBridge *pb = PCI_BRIDGE(dev);
> -        PCIDevice *pdev = PCI_DEVICE(dev);
>   
> +        pdev = PCI_DEVICE(dev);
>           pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
>           pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
>   
> -        bus = BUS(&pb->sec_bus);
> -        qbus_set_hotplug_handler(bus, DEVICE(s), errp);
> +        qbus_set_hotplug_handler(BUS(&pb->sec_bus), DEVICE(s), errp);
>   
>           if (dev->hotplugged) {
>               pci_default_write_config(pdev, PCI_PRIMARY_BUS,
>                                        pci_dev_bus_num(pdev), 1);
>               s->bus_no += 1;
>               pci_default_write_config(pdev, PCI_SECONDARY_BUS, s->bus_no, 1);
> -            do {
> -                pdev = pci_get_bus(pdev)->parent_dev;
> -                pci_default_write_config(pdev, PCI_SUBORDINATE_BUS,
> -                                         s->bus_no, 1);
> -            } while (pci_get_bus(pdev) && pci_dev_bus_num(pdev));
> +
> +            s390_pci_update_subordinate(pdev, s->bus_no);
>           }
>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>           pdev = PCI_DEVICE(dev);
> 

Looks good to me...

Reviewed-by: Collin Walling <walling@linux.ibm.com>

Side note: unrelated to the changes here -- and if you can clarify for
me -- any idea why we do s->bus_no += 1? This throws me off a bit and
begs me to ask what exactly is the S390pciState object suppose to
represent? (My guess is that it is representative of the entire PCI
topology, and we increment the bus_no to denote the subordinate bus
number?)

(let me know if these kind of discussions are too noisy and deemed
inappropriate for the mailing list, and I'll start pestering you off-
list instead)

Re: [Qemu-devel] [PATCH v2 1/6] s390x/pci: Fix primary bus number for PCI bridges

[Collin Walling]

On 1/30/19 10:57 AM, David Hildenbrand wrote:
> The primary bus number corresponds always to the bus number of the
> bus the bridge is attached to.
> 
> Right now, if we have two bridges attached to the same bus (e.g. root
> bus) this is however not the case. The first bridge will have primary
> bus 0, the second bridge primary bus 1, which is wrong. Fix the assignment.
> 
> While at it, drop setting the PCI_SUBORDINATE_BUS temporarily to 0xff.
> Setting it temporarily to that value (as discussed e.g. in [1]), is
> only relevant for a running system that probes the buses. The value is
> effectively unused for us just doing a DFS.
> 
> Also add a comment why we have to reassign during every reset (which I
> found to be surprising.
> 
> Please note that hotplugging of bridges is in general still broken, will
> be fixed next.
> 
> [1] http://www.science.unitn.it/~fiorella/guidelinux/tlk/node76.html
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

Reviewed-by: Collin Walling <walling@linux.ibm.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/6] s390x/pci: Fix hotplugging of PCI bridges

[David Hildenbrand]

On 04.02.19 23:48, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> When hotplugging a PCI bridge right now to the root port, we resolve
>> pci_get_bus(pdev)->parent_dev, which results in a SEGFAULT. Hotplugging
>> really only works right now when hotplugging to another bridge.
>>
>> Instead, we have to properly check if we are already at the root.
>>
>> Let's cleanup the code while at it a bit and factor out updating the
>> subordiante bus number into a separate function. The check for
> 
> s/subordiante/subordinate
> 
>> "old_nr < nr" is right now not strictly necessary, but makes it more
>> obvious what is actually going on.
>>
>> Most probably fixing up the topology is not our responsibility when
>> hotplugging. The guest has to sort this out. But let's keep it for now
>> and only fix current code to not crash.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 28 +++++++++++++++++++---------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index b7c4613fde..9b5c5fff60 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -843,6 +843,21 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>       }
>>   }
>>   
>> +static void s390_pci_update_subordinate(PCIDevice *dev, uint32_t nr)
>> +{
>> +    uint32_t old_nr;
>> +
>> +    pci_default_write_config(dev, PCI_SUBORDINATE_BUS, nr, 1);
>> +    while (!pci_bus_is_root(pci_get_bus(dev))) {
>> +        dev = pci_get_bus(dev)->parent_dev;
>> +
>> +        old_nr = pci_default_read_config(dev, PCI_SUBORDINATE_BUS, 1);
>> +        if (old_nr < nr) {
>> +            pci_default_write_config(dev, PCI_SUBORDINATE_BUS, nr, 1);
>> +        }
>> +    }
>> +} > +
>>   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>                                 Error **errp)
>>   {
>> @@ -851,26 +866,21 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>       S390PCIBusDevice *pbdev = NULL;
>>   
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
>> -        BusState *bus;
>>           PCIBridge *pb = PCI_BRIDGE(dev);
>> -        PCIDevice *pdev = PCI_DEVICE(dev);
>>   
>> +        pdev = PCI_DEVICE(dev);
>>           pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
>>           pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
>>   
>> -        bus = BUS(&pb->sec_bus);
>> -        qbus_set_hotplug_handler(bus, DEVICE(s), errp);
>> +        qbus_set_hotplug_handler(BUS(&pb->sec_bus), DEVICE(s), errp);
>>   
>>           if (dev->hotplugged) {
>>               pci_default_write_config(pdev, PCI_PRIMARY_BUS,
>>                                        pci_dev_bus_num(pdev), 1);
>>               s->bus_no += 1;
>>               pci_default_write_config(pdev, PCI_SECONDARY_BUS, s->bus_no, 1);
>> -            do {
>> -                pdev = pci_get_bus(pdev)->parent_dev;
>> -                pci_default_write_config(pdev, PCI_SUBORDINATE_BUS,
>> -                                         s->bus_no, 1);
>> -            } while (pci_get_bus(pdev) && pci_dev_bus_num(pdev));
>> +
>> +            s390_pci_update_subordinate(pdev, s->bus_no);
>>           }
>>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>           pdev = PCI_DEVICE(dev);
>>
> 
> Looks good to me...
> 

Thanks for the review!!

> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 
> Side note: unrelated to the changes here -- and if you can clarify for
> me -- any idea why we do s->bus_no += 1? This throws me off a bit and
> begs me to ask what exactly is the S390pciState object suppose to
> represent? (My guess is that it is representative of the entire PCI
> topology, and we increment the bus_no to denote the subordinate bus
> number?)
On x86, the bios builds the topology. On spapr and s390x, the firmware
builds the topology. The topology is constructed in a way that all buses
can be found ("tree traversed") from the root.

In a clean topology, each bridge has it's dedicated number.

primary: The bus the bridge is attached to
secondary: The bus the bridge spans up
subordinate: The highest bus number that can be found from this bridge

So when we add a new bridge, we have to assign a new "global" bus number
for the topology. This is what we do here. So we denote actually the
"seconardy" bus nr here, which we propagate as "subordinate" up to the root.

But this is the interesting point: When hotplugging on x86 and on power,
the _guest_ is responsible for rebuilding the topology. Not the bios,
not the firmware. No numbers are assigned. Code like we have here does
not exist for them.


And most probably this is also broken on s390x: When hotplugging a
bridge, we should not mess with the topology (because as Thomas noted,
we can easily break the topology so the search does no longer work
reliably).

But this is your task to find out :)

Although it sounds like I "speak PCI", I really only have a rough idea
how it all (is supposed to) work(s).

So while I am fixing the current code, we should find out next if we can
drop this "messing with the topology on hotplug" completely. Or e.g.
rework it to have standby numbers we can use when hotplugging ...


> 
> (let me know if these kind of discussions are too noisy and deemed
> inappropriate for the mailing list, and I'll start pestering you off-
> list instead)

Not at all. This allows other people to learn as well and also to jump
in in case I make up things.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/6] s390x/pci: Fix hotplugging of PCI bridges

[Cornelia Huck]

On Tue, 5 Feb 2019 00:43:08 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.02.19 23:48, Collin Walling wrote:

> > Side note: unrelated to the changes here -- and if you can clarify for
> > me -- any idea why we do s->bus_no += 1? This throws me off a bit and
> > begs me to ask what exactly is the S390pciState object suppose to
> > represent? (My guess is that it is representative of the entire PCI
> > topology, and we increment the bus_no to denote the subordinate bus
> > number?)  
> On x86, the bios builds the topology. On spapr and s390x, the firmware
> builds the topology. The topology is constructed in a way that all buses
> can be found ("tree traversed") from the root.
> 
> In a clean topology, each bridge has it's dedicated number.
> 
> primary: The bus the bridge is attached to
> secondary: The bus the bridge spans up
> subordinate: The highest bus number that can be found from this bridge
> 
> So when we add a new bridge, we have to assign a new "global" bus number
> for the topology. This is what we do here. So we denote actually the
> "seconardy" bus nr here, which we propagate as "subordinate" up to the root.
> 
> But this is the interesting point: When hotplugging on x86 and on power,
> the _guest_ is responsible for rebuilding the topology. Not the bios,
> not the firmware. No numbers are assigned. Code like we have here does
> not exist for them.
> 
> 
> And most probably this is also broken on s390x: When hotplugging a
> bridge, we should not mess with the topology (because as Thomas noted,
> we can easily break the topology so the search does no longer work
> reliably).

One thing I'm always wondering about: How does that work on "real"
hardware? The guest basically sees uid/fid, and I'm not sure how much
topology actually shines through here (visible in some generic pci
structures?) In QEMU, we're plugging into the generic pci
infrastructure, so our behaviour may be different than what we see on
an LPAR.

> 
> But this is your task to find out :)
> 
> Although it sounds like I "speak PCI", I really only have a rough idea
> how it all (is supposed to) work(s).
> 
> So while I am fixing the current code, we should find out next if we can
> drop this "messing with the topology on hotplug" completely. Or e.g.
> rework it to have standby numbers we can use when hotplugging ...
> 
> 
> > 
> > (let me know if these kind of discussions are too noisy and deemed
> > inappropriate for the mailing list, and I'll start pestering you off-
> > list instead)  
> 
> Not at all. This allows other people to learn as well and also to jump
> in in case I make up things.
> 

Seconded. Remember that people sometimes only read along and are not
really visible in the discussion :)

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[Cornelia Huck]

On Mon, 4 Feb 2019 23:45:35 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.02.19 23:42, Collin Walling wrote:
> > On 2/4/19 4:54 PM, David Hildenbrand wrote:  
> >> On 04.02.19 21:19, Collin Walling wrote:  
> >>> On 1/30/19 10:57 AM, David Hildenbrand wrote:  
> >>>> We decided to always create the PCI host bridge, even if 'zpci' is not
> >>>> enabled (due to migration compatibility). This however right now allows
> >>>> to add zPCI/PCI devices to a VM although the guest will never actually see
> >>>> them, confusing people that are using a simple CPU model that has no
> >>>> 'zpci' enabled - "Why isn't this working" (David Hildenbrand)
> >>>>
> >>>> Let's check for 'zpci' and at least print a warning that this will not
> >>>> work as expected. We could also bail out, however that might break
> >>>> existing QEMU commandlines.
> >>>>
> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>>    hw/s390x/s390-pci-bus.c | 5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >>>> index 9b5c5fff60..2efd9186c2 100644
> >>>> --- a/hw/s390x/s390-pci-bus.c
> >>>> +++ b/hw/s390x/s390-pci-bus.c
> >>>> @@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>>>    {
> >>>>        S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> >>>>    
> >>>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> >>>> +        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
> >>>> +                    " The guest will not be able to see/use this device");
> >>>> +    }
> >>>> +
> >>>>        if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> >>>>            PCIDevice *pdev = PCI_DEVICE(dev);
> >>>>    
> >>>>  
> >>>
> >>> I wonder if someone might misconstrue this as "the _PCI device_ needs
> >>> the zpci feature." I think "'zpci' CPU feature required to support
> >>> PCI/zPCI devices." reads better. The last sentence is fine to me.
> >>>  
> >>
> >> Well, the guest needs the 'zpci' feature to see the device. And that's
> >> what that message says in my opinion. Not that a device needs to have a
> >> feature (I added "CPU feature" for this reason).
> >>
> >> "required to support" does it not make very clear what we actually want
> >> to say.
> >>
> >> Thanks!
> >>  
> > 
> > I see your point. We can still plug in the device without the CPU
> > feature, but the device will ultimately be useless to the guest. Thanks
> > for clearing that up.
> > 
> > Still, the wording reads strangely to me. I read it as the PCI device
> > itself requires a "zpci CPU feature" which of course does not make sense
> > (and I fully understand that's not what you mean here).
> > 
> > What do you think about:
> > 
> > "PCI/zPCI device plugging without 'zpci' CPU feature enabled." along
> > with your second sentence, of course.  
> 
> "Plugging a PCI/zPCI device without the 'zpci' CPU feature enabled. The
> guest will not be able to see/use this device."
> 
> would make sense to me!

Ok, I have now

        warn_report("Plugging a PCI/zPCI device without the 'zpci' CPU "
                    "feature enabled; the guest will not be able to see/use "
                    "this device");

> 
> > 
> > Either way you decide, it's still a good idea to have this warning in
> > here. I'm really just debating syntax and not semantics, so it's not
> > really important. I won't impede this patch over a differing opinion of
> > a small rewording. :)
> > 
> > Reviewed-by: Collin Walling <walling@linux.ibm.com>
> >   
> 
> Thanks!
> 

Re: [Qemu-devel] [PATCH v2 6/6] s390x/pci: Unplug remaining requested devices on pcihost reset

[Cornelia Huck]

On Fri, 1 Feb 2019 16:06:43 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 01.02.19 11:19, Cornelia Huck wrote:
> > On Wed, 30 Jan 2019 16:57:33 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> When resetting the guest we should unplug and remove all devices that
> >> are still pending.
> >>
> >> With this patch, the requested device will be unpluged on reboot

s/unpluged/unplugged/

> >> (S390_RESET_EXTERNAL and S390_RESET_REIPL, which reset the pcihost bridge
> >> via qemu_devices_reset()).
> >>
> >> This approach is similar to what's done for acpi PCI hotplug in
> >> acpi_pcihp_reset() -> acpi_pcihp_update() ->
> >> acpi_pcihp_update_hotplug_bus() -> acpi_pcihp_eject_slot().
> >>
> >> s390_pci_generate_plug_event()'s will still be generated, I guess this
> >> is not an issue. The same thing would happen right now when unplugging
> >> a device just before starting the guest.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/s390x/s390-pci-bus.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >> index 867801ccf9..b9b0f44087 100644
> >> --- a/hw/s390x/s390-pci-bus.c
> >> +++ b/hw/s390x/s390-pci-bus.c
> >> @@ -1092,6 +1092,21 @@ static void s390_pcihost_reset(DeviceState *dev)
> >>  {
> >>      S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
> >>      PCIBus *bus = s->parent_obj.bus;
> >> +    S390PCIBusDevice *pbdev, *next;
> >> +
> >> +    /* Process all pending unplug requests */
> >> +    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
> >> +        if (pbdev->unplug_requested) {
> >> +            if (pbdev->summary_ind) {
> >> +                pci_dereg_irqs(pbdev);
> >> +            }
> >> +            if (pbdev->iommu->enabled) {
> >> +                pci_dereg_ioat(pbdev->iommu);
> >> +            }
> >> +            pbdev->state = ZPCI_FS_STANDBY;
> >> +            s390_pci_perform_unplug(pbdev);
> >> +        }
> >> +    }
> >>  
> >>      /*
> >>       * When resetting a PCI bridge, the assigned numbers are set to 0. So  
> > 
> > I'm afraid this one doesn't quite apply without the first patches in
> > the series, and I'm not sure how to fix it up. I'll either take a
> > rebase on a codebase without patches 1-3, or this patch together with
> > patches 1-3 after they get acks (whatever comes first :)
> >   
> 
> I guess it's only the comment that get's added in patch #1, so no major
> conflicts.

As I'm now picking 1-3, this obviously fits without conflicts again :)

Re: [Qemu-devel] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[Cornelia Huck]

On Wed, 30 Jan 2019 16:57:27 +0100
David Hildenbrand <david@redhat.com> wrote:

> These are all the patches that are not yet upstream (@Conny you might
> already picked some, including them for the full picture) and after
> a good discussion yesterday, including a patch t get rid of the release
> timer. I ran a couple of sanity tests on this series.
> 
> #1 and #2 fix hotplugging of PCI bridges.
> #3 warns when "zpci=off"
> #4 refactors unplugging
> #5 get's rid of the release timer
> #6 processes all unplug requests on reboot
> 
> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
> chaining + virtio-pmem")
> 
> David Hildenbrand (6):
>   s390x/pci: Fix primary bus number for PCI bridges
>   s390x/pci: Fix hotplugging of PCI bridges
>   s390x/pci: Warn when adding PCI devices without the 'zpci' feature
>   s390x/pci: Introduce unplug requests and split unplug handler
>   s390x/pci: Drop release timer and replace it with a flag
>   s390x/pci: Unplug remaining requested devices on pcihost reset
> 
>  hw/s390x/s390-pci-bus.c | 233 ++++++++++++++++++++++++++--------------
>  hw/s390x/s390-pci-bus.h |   4 +-
>  2 files changed, 152 insertions(+), 85 deletions(-)
> 

Thanks, applied the remaining patches.

Re: [Qemu-devel] [PATCH v2 0/6] s390x/pci: remaining hot/un)plug patches

[David Hildenbrand]

On 05.02.19 10:55, Cornelia Huck wrote:
> On Wed, 30 Jan 2019 16:57:27 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> These are all the patches that are not yet upstream (@Conny you might
>> already picked some, including them for the full picture) and after
>> a good discussion yesterday, including a patch t get rid of the release
>> timer. I ran a couple of sanity tests on this series.
>>
>> #1 and #2 fix hotplugging of PCI bridges.
>> #3 warns when "zpci=off"
>> #4 refactors unplugging
>> #5 get's rid of the release timer
>> #6 processes all unplug requests on reboot
>>
>> @Colin, can you review/ack? Especially Patch #4 is needed for qdev
>> patches already on the list. ("[PATCH RFCv2 0/9] qdev: Hotplug handler
>> chaining + virtio-pmem")
>>
>> David Hildenbrand (6):
>>   s390x/pci: Fix primary bus number for PCI bridges
>>   s390x/pci: Fix hotplugging of PCI bridges
>>   s390x/pci: Warn when adding PCI devices without the 'zpci' feature
>>   s390x/pci: Introduce unplug requests and split unplug handler
>>   s390x/pci: Drop release timer and replace it with a flag
>>   s390x/pci: Unplug remaining requested devices on pcihost reset
>>
>>  hw/s390x/s390-pci-bus.c | 233 ++++++++++++++++++++++++++--------------
>>  hw/s390x/s390-pci-bus.h |   4 +-
>>  2 files changed, 152 insertions(+), 85 deletions(-)
>>
> 
> Thanks, applied the remaining patches.
> 

Thanks a lot Conny!

-- 

Thanks,

David / dhildenb