Re: [Qemu-devel] [PATCH] `qdev_free` when unplug a pci device

[Markus Armbruster]

Isaku Yamahata <address@hidden> writes:

> On Tue, Feb 22, 2011 at 06:36:20PM +0100, William Dauchy wrote:
>>  `qdev_free` when unplug a pci device
>>  It resolves a bug when detaching/attaching a network device
>> 
>>  # virsh detach-interface dom0 network --mac 52:54:00:f6:84:ba
>>  Interface detached successfully
>> 
>>  # virsh attach-interface dom0 network default --mac 52:54:00:f6:84:ba
>>  error: Failed to attach interface
>>  error: internal error unable to execute QEMU command 'device_add': 
>> Duplicate ID 'net0' for device
>> 
>> A detached pci device was not freed of the list `qemu_device_opts`
>> ---
>>  hw/pci.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>> 
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 8b76cea..1e9a8f0 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1671,13 +1671,16 @@ static int pci_unplug_device(DeviceState *qdev)
>>  {
>>      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
>>      PCIDeviceInfo *info = container_of(qdev->info, PCIDeviceInfo, qdev);
>> +    int unplug;
>>  
>>      if (info->no_hotplug) {
>>          qerror_report(QERR_DEVICE_NO_HOTPLUG, info->qdev.name);
>>          return -1;
>>      }
>> -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
>> +    unplug = dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
>>                               PCI_HOTPLUG_DISABLED);
>> +    qdev_free(qdev);
>
> Good catch.
> qdev_free() should be called only when unplug had succeeded.
> Although piix4 unplug always successes, pcie unplug may fail.
>
>> +    return unplug;
>>  }
>>  
>>  void pci_qdev_register(PCIDeviceInfo *info)

I don't think this patch is correct.  Let me explain.

Device hot unplug is *not* guaranteed to succeed.

For some buses, such as USB, it always succeeds immediately, i.e. when
the device_del monitor command finishes, the device is gone.  Live is
good.

But for PCI, device_del merely initiates the ACPI unplug rain dance.  It
doesn't wait for the dance to complete.  Why?  The dance can take an
unpredictable amount of time, including forever.

Problem: Subsequent device_add can fail if it reuses the qdev ID or PCI
slot, and the unplug has not yet completed (race condition), or it
failed.  Yes, Virginia, PCI hotplug *can* fail.

When unplug succeeds, the qdev is automatically destroyed.
pciej_write() does that for PIIX4.  Looks like pcie_cap_slot_event()
does it for PCIE.

Your patch adds a *second* qdev_free().  No good.