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 <[email protected]> >> --- >> 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 <[email protected]> > -- Thanks, David / dhildenb
