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 <address@hidden>
>> ---
>>   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 <address@hidden>
> 


-- 

Thanks,

David / dhildenb