`qdev_free` when unplug a pci device

`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(-)

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)
> -- 
> William
> 
>

Isaku Yamahata <yamahata@valinux.co.jp> 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.

Hi Markus,

On Wed, Feb 23, 2011 at 9:30 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 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.

Thanks for the complete explanation. I now understand my mistake and
find out why it wasn't working as expected.
On a Linux virtual machine I forgot to load the `acpiphp` module which
makes the pci device disappear when doing a detach. I thought that,
even without this module the detach should have freed the
corresponding device on qemu side, regardless if the virtual machine
was supporting or not acpi signals.
Please ignore my patch.

Regards,