[Qemu-devel] [PATCH v2 0/6] s390x/pci: hotplug handler fixes and reworks

[Qemu-devel] [PATCH v2 0/6] s390x/pci: hotplug handler fixes and reworks

[David Hildenbrand]

A bunch of fixes and reworks for s390x/pci hotplug infrastructure.

Patch 1,2: Reworks already posted (pre_plug handler)
Patch 3,4: Fixes for memory leaks
Patch 5: Rework unplug handler (introduce unplug_request handler) which
         also fixes some unplug scenarios
Patch 6: Handle leftover unplug requests on reset

We might decide to drop 1. 3 and 4 can be picked up independently.

v1 -> v2:
- Some rewordings in patch descriptions
- "s390x/pci: Introduce unplug requests and split unplug handler"
-- Some simplifications regarding s390_pci_perform_unplug()

David Hildenbrand (6):
  s390x/pci: Use hotplug_dev instead of looking up the host bridge
  s390x/pci: Move some hotplug checks to the pre_plug handler
  s390x/pci: Always delete and free the release_timer
  s390x/pci: Ignore the unplug call if we already have a release_timer
  s390x/pci: Introduce unplug requests and split unplug handler
  s390x/pci: Unplug remaining devices on pcihost reset

 hw/s390x/s390-pci-bus.c | 215 +++++++++++++++++++++++++++-------------
 hw/s390x/s390-pci-bus.h |   1 +
 2 files changed, 148 insertions(+), 68 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/6] s390x/pci: Use hotplug_dev instead of looking up the host bridge

[David Hildenbrand]

We directly have it in our hands.

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

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 7f911b216a..86dda831f9 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -826,9 +826,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         BusState *bus;
@@ -935,11 +935,11 @@ static void s390_pcihost_timer_cb(void *opaque)
 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;
-    S390pciState *s = s390_get_phb();
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
         error_setg(errp, "PCI bridge hot unplug currently not supported");
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 1/6] s390x/pci: Use hotplug_dev instead of looking up the host bridge

[Collin Walling]

On 1/14/19 5:31 AM, David Hildenbrand wrote:
> We directly have it in our hands.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 7f911b216a..86dda831f9 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -826,9 +826,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
>   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>   {
> +    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>       PCIDevice *pdev = NULL;
>       S390PCIBusDevice *pbdev = NULL;
> -    S390pciState *s = s390_get_phb();
>   
>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
>           BusState *bus;
> @@ -935,11 +935,11 @@ static void s390_pcihost_timer_cb(void *opaque)
>   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;
> -    S390pciState *s = s390_get_phb();
>   
>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
>           error_setg(errp, "PCI bridge hot unplug currently not supported");
> 

Looks like the macro will do the same thing as the function does? I 
wonder if it makes sense to one day replace all function calls with the 
macro.

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

Re: [Qemu-devel] [PATCH v2 1/6] s390x/pci: Use hotplug_dev instead of looking up the host bridge

[Cornelia Huck]

On Tue, 15 Jan 2019 15:57:38 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> On 1/14/19 5:31 AM, David Hildenbrand wrote:
> > We directly have it in our hands.
> > 
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >   hw/s390x/s390-pci-bus.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 7f911b216a..86dda831f9 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -826,9 +826,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
> >   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                                 Error **errp)
> >   {
> > +    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> >       PCIDevice *pdev = NULL;
> >       S390PCIBusDevice *pbdev = NULL;
> > -    S390pciState *s = s390_get_phb();
> >   
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> >           BusState *bus;
> > @@ -935,11 +935,11 @@ static void s390_pcihost_timer_cb(void *opaque)
> >   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;
> > -    S390pciState *s = s390_get_phb();
> >   
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> >           error_setg(errp, "PCI bridge hot unplug currently not supported");
> >   
> 
> Looks like the macro will do the same thing as the function does? I 
> wonder if it makes sense to one day replace all function calls with the 
> macro.

The idea behind the function was to do some caching. Opinions on that
differ :)

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

If you like this patch, I don't really object to merging it :)

[Qemu-devel] [PATCH v2 2/6] s390x/pci: Move some hotplug checks to the pre_plug handler

[David Hildenbrand]

Let's move most of the checks to the new pre_plug handler. As a PCI
bridge is just a PCI device, we can simplify the code.

Notes: We cannot yet move the MSIX check or device ID creation +
zPCI device creation to the pre_plug handler as both parts are not
fixed before actual device realization (and therefore after pre_plug and
before plug). Once that part is factored out, we can move these parts to
the pre_plug handler, too and therefore remove all possible errors from
the plug handler.

Reviewed-by: Collin Walling <walling@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 86dda831f9..1775388524 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -818,11 +818,31 @@ static bool s390_pci_alloc_idx(S390pciState *s, S390PCIBusDevice *pbdev)
     }
 
     pbdev->idx = idx;
-    s->next_idx = (idx + 1) & FH_MASK_INDEX;
-
     return true;
 }
 
+static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                   Error **errp)
+{
+    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pdev = PCI_DEVICE(dev);
+
+        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
+            error_setg(errp, "multifunction not supported in s390");
+            return;
+        }
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
+        S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
+
+        if (!s390_pci_alloc_idx(s, pbdev)) {
+            error_setg(errp, "no slot for plugging zpci device");
+            return;
+        }
+    }
+}
+
 static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
@@ -835,11 +855,6 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         PCIBridge *pb = PCI_BRIDGE(dev);
         PCIDevice *pdev = PCI_DEVICE(dev);
 
-        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
-            error_setg(errp, "multifunction not supported in s390");
-            return;
-        }
-
         pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
         pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
 
@@ -859,11 +874,6 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         pdev = PCI_DEVICE(dev);
 
-        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
-            error_setg(errp, "multifunction not supported in s390");
-            return;
-        }
-
         if (!dev->id) {
             /* In the case the PCI device does not define an id */
             /* we generate one based on the PCI address         */
@@ -905,10 +915,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
         pbdev = S390_PCI_DEVICE(dev);
 
-        if (!s390_pci_alloc_idx(s, pbdev)) {
-            error_setg(errp, "no slot for plugging zpci device");
-            return;
-        }
+        /* the allocated idx is actually getting used */
+        s->next_idx = (pbdev->idx + 1) & FH_MASK_INDEX;
         pbdev->fh = pbdev->idx;
         QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
         g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
@@ -1041,6 +1049,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
 
     dc->reset = s390_pcihost_reset;
     dc->realize = s390_pcihost_realize;
+    hc->pre_plug = s390_pcihost_pre_plug;
     hc->plug = s390_pcihost_plug;
     hc->unplug = s390_pcihost_unplug;
     msi_nonbroken = true;
-- 
2.17.2

[Qemu-devel] [PATCH v2 3/6] s390x/pci: Always delete and free the release_timer

[David Hildenbrand]

We should always get rid of it. I don't see a reason to keep the timer
alive if the devices are going away. This looks like a memory leak.

(hmp) device_add virtio-mouse-pci,id=test
(hmp) device_del test
-> guest notified, timer pending.
-> guest does not react for some reason (e.g. crash)
-> s390_pcihost_timer_cb(). Timer not pending anymore. qmp_unplug().

-> Device deleted. Timer expired (not pending) but not freed.

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

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 1775388524..59325cae3b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -982,7 +982,7 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    if (pbdev->release_timer && timer_pending(pbdev->release_timer)) {
+    if (pbdev->release_timer) {
         timer_del(pbdev->release_timer);
         timer_free(pbdev->release_timer);
         pbdev->release_timer = NULL;
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 3/6] s390x/pci: Always delete and free the release_timer

[Collin Walling]

On 1/14/19 5:31 AM, David Hildenbrand wrote:
> We should always get rid of it. I don't see a reason to keep the timer
> alive if the devices are going away. This looks like a memory leak.
> 
> (hmp) device_add virtio-mouse-pci,id=test
> (hmp) device_del test
> -> guest notified, timer pending.
> -> guest does not react for some reason (e.g. crash)
> -> s390_pcihost_timer_cb(). Timer not pending anymore. qmp_unplug().
> 
> -> Device deleted. Timer expired (not pending) but not freed.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1775388524..59325cae3b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -982,7 +982,7 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>           return;
>       }
>   
> -    if (pbdev->release_timer && timer_pending(pbdev->release_timer)) {
> +    if (pbdev->release_timer) {
>           timer_del(pbdev->release_timer);
>           timer_free(pbdev->release_timer);
>           pbdev->release_timer = NULL;
> 

Looks like the only time we would hit this is when the device is already 
in standby (i.e. not configured). So this makes sense to me.

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

[Qemu-devel] [PATCH v2 4/6] s390x/pci: Ignore the unplug call if we already have a release_timer

[David Hildenbrand]

... otherwise two successive calls to qdev_unplug() (e.g. by an impatient
user) will effectively overwrite pbdev->release_timer, resulting in a
memory leak. We are already processing the unplug.

If there is already a release_timer, the unplug will be performed after
the timeout.

Can be easily triggered by
(hmp) device_add virtio-mouse-pci,id=test
(hmp) stop
(hmp) device_del test
(hmp) device_del test

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

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 59325cae3b..34a9cb2a80 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -972,6 +972,9 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     case ZPCI_FS_STANDBY:
         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,
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 4/6] s390x/pci: Ignore the unplug call if we already have a release_timer

[Collin Walling]

On 1/14/19 5:31 AM, David Hildenbrand wrote:
> ... otherwise two successive calls to qdev_unplug() (e.g. by an impatient
> user) will effectively overwrite pbdev->release_timer, resulting in a
> memory leak. We are already processing the unplug.
> 

Does QEMU not have a way to detect if a device is already in the process 
of being unplugged? Seems like not having that kind of protection could 
cause many problems.

Perhaps that effort would be arduous.

> If there is already a release_timer, the unplug will be performed after
> the timeout.
> 
> Can be easily triggered by
> (hmp) device_add virtio-mouse-pci,id=test
> (hmp) stop
> (hmp) device_del test
> (hmp) device_del test
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 59325cae3b..34a9cb2a80 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -972,6 +972,9 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       case ZPCI_FS_STANDBY:
>           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,
> 

Looks good to me.

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

Re: [Qemu-devel] [PATCH v2 4/6] s390x/pci: Ignore the unplug call if we already have a release_timer

[Cornelia Huck]

On Tue, 15 Jan 2019 17:53:17 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> On 1/14/19 5:31 AM, David Hildenbrand wrote:
> > ... otherwise two successive calls to qdev_unplug() (e.g. by an impatient
> > user) will effectively overwrite pbdev->release_timer, resulting in a
> > memory leak. We are already processing the unplug.
> >   
> 
> Does QEMU not have a way to detect if a device is already in the process 
> of being unplugged? Seems like not having that kind of protection could 
> cause many problems.

There's also that unplug_request callback, which the next patch will
start to use. That pluggery for s390x pci is really a mess :(

> 
> Perhaps that effort would be arduous.
> 
> > If there is already a release_timer, the unplug will be performed after
> > the timeout.
> > 
> > Can be easily triggered by
> > (hmp) device_add virtio-mouse-pci,id=test
> > (hmp) stop
> > (hmp) device_del test
> > (hmp) device_del test
> > 
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >   hw/s390x/s390-pci-bus.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 59325cae3b..34a9cb2a80 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -972,6 +972,9 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >       case ZPCI_FS_STANDBY:
> >           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,
> >   
> 
> Looks good to me.
> 
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> 

Re: [Qemu-devel] [PATCH v2 4/6] s390x/pci: Ignore the unplug call if we already have a release_timer

[David Hildenbrand]

On 15.01.19 23:53, Collin Walling wrote:
> On 1/14/19 5:31 AM, David Hildenbrand wrote:
>> ... otherwise two successive calls to qdev_unplug() (e.g. by an impatient
>> user) will effectively overwrite pbdev->release_timer, resulting in a
>> memory leak. We are already processing the unplug.
>>
> 
> Does QEMU not have a way to detect if a device is already in the process 
> of being unplugged? Seems like not having that kind of protection could 
> cause many problems.
> 
> Perhaps that effort would be arduous.

No, there is no such thing. E.g. on some architectures it might even be
desirable to send multiple unplug requests to the guest. Say you want to
remove a DIMM. Could be that the guest cannot remove it right now, so it
NACKs the requests. Later you want to retry, so you send another
requests. This means we cannot really track pending unplug requests (and
might not even want to do so).

Regarding hotplug_handler_unplug(), the expectation is that once the
function returns without an error, the device was removed (or is ready
to be removed). So here, tracking a pending removal is not necessary.

See patch #5 where I properly convert what we have on s390x pci to
unplug requests.

> 
>> If there is already a release_timer, the unplug will be performed after
>> the timeout.
>>
>> Can be easily triggered by
>> (hmp) device_add virtio-mouse-pci,id=test
>> (hmp) stop
>> (hmp) device_del test
>> (hmp) device_del test
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 59325cae3b..34a9cb2a80 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -972,6 +972,9 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>       case ZPCI_FS_STANDBY:
>>           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,
>>
> 
> Looks good to me.

Thanks!

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


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v2 5/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 PCI bridges as
   checks are not performed - bad.
2. Unplugging via the zPCI device allows to unplug devices that are not
   hot removable. (check performed in qdev_unplug()) - bad.
3. 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 | 161 +++++++++++++++++++++++++++-------------
 hw/s390x/s390-pci-bus.h |   1 +
 2 files changed, 111 insertions(+), 51 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 34a9cb2a80..bfd7fc1d2d 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);
@@ -937,74 +971,98 @@ 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;
-    }
 
-    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);
+        }
     }
-
-    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;
-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,
@@ -1054,6 +1112,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 f47a0f2da5..6987c31df2 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -307,6 +307,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 6/6] s390x/pci: Unplug remaining devices on pcihost reset

[David Hildenbrand]

When resetting the guest we should unplug and remove all devices that
are still pending. Otherwise the fresh guest will see devices that will
suddenly vanish.

Can be triggered e.g. via
(hmp) device_add virtio-mouse-pci,id=test
(hmp) stop
(hmp) device_del test
(hmp) system_reset
(hmp) c

The device will vanish after roughly 5 minutes. With this patch, the
device will vanish on reboot (S390_RESET_EXTERNAL and S390_RESET_REIPL,
which reset the pcihost bridge via qemu_devices_reset()). If we want
these devices to vanish directly on any reset (S390_RESET_MODIFIED_CLEAR
and S390_RESET_LOAD_NORMAL), we have to modify s390_machine_reset(). But
I have the feeling that this should not be done for all reset types.

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 (same thing could happen right now if the timer expires
just after reset).

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

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index bfd7fc1d2d..f399eeede6 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1098,6 +1098,14 @@ static void s390_pcihost_reset(DeviceState *dev)
 {
     S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
     PCIBus *bus = s->parent_obj.bus;
+    S390PCIBusDevice *pbdev, *next;
+
+    /* Unplug all pending devices that were requested to be released */
+    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
+        if (pbdev->release_timer) {
+            s390_pcihost_timer_cb(pbdev);
+        }
+    }
 
     s->bus_no = 0;
     pci_for_each_device(bus, pci_bus_num(bus), s390_pci_enumerate_bridge, s);
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 0/6] s390x/pci: hotplug handler fixes and reworks

[Cornelia Huck]

On Mon, 14 Jan 2019 11:31:04 +0100
David Hildenbrand <david@redhat.com> wrote:

> A bunch of fixes and reworks for s390x/pci hotplug infrastructure.
> 
> Patch 1,2: Reworks already posted (pre_plug handler)
> Patch 3,4: Fixes for memory leaks
> Patch 5: Rework unplug handler (introduce unplug_request handler) which
>          also fixes some unplug scenarios
> Patch 6: Handle leftover unplug requests on reset
> 
> We might decide to drop 1. 3 and 4 can be picked up independently.

1-4 all have been reviewed, so I went ahead and picked them up.

> 
> v1 -> v2:
> - Some rewordings in patch descriptions
> - "s390x/pci: Introduce unplug requests and split unplug handler"
> -- Some simplifications regarding s390_pci_perform_unplug()
> 
> David Hildenbrand (6):
>   s390x/pci: Use hotplug_dev instead of looking up the host bridge
>   s390x/pci: Move some hotplug checks to the pre_plug handler
>   s390x/pci: Always delete and free the release_timer
>   s390x/pci: Ignore the unplug call if we already have a release_timer
>   s390x/pci: Introduce unplug requests and split unplug handler
>   s390x/pci: Unplug remaining devices on pcihost reset
> 
>  hw/s390x/s390-pci-bus.c | 215 +++++++++++++++++++++++++++-------------
>  hw/s390x/s390-pci-bus.h |   1 +
>  2 files changed, 148 insertions(+), 68 deletions(-)
>