1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v2 0/6] s390x/pci: hotplug handler fixes and reworks
  4. View patch

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

David Hildenbrand posted 6 patches 6 years, 9 months ago
Maintainers: Collin Walling <walling@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Richard Henderson <rth@twiddle.net>, Christian Borntraeger <borntraeger@de.ibm.com>, Cornelia Huck <cohuck@redhat.com>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[Qemu-devel] [PATCH v2 4/6] s390x/pci: Ignore the unplug call if we already have a release_timer
Posted by David Hildenbrand 6 years, 9 months ago 
... 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
Posted by Collin Walling 6 years, 9 months ago 
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
Posted by Cornelia Huck 6 years, 9 months ago 
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
Posted by David Hildenbrand 6 years, 9 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.