Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/12/2013 16:08, Igor Mammedov ha scritto:
> On Mon, 09 Dec 2013 15:36:35 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 09/12/2013 15:14, Igor Mammedov ha scritto:
>>>>>
>>>>> Makes sense.  realize() for the "plug" handler, and qdev_unplug for the
>>>>> unplug handler, I guess.
>>> Just to be sure, I've meant not DEVICE.realize() but each device specific
>>> one.
>>
>> If it's each specific device, then why should the hotplug handler link
>> be in DeviceState?
> The reason I've put it there is to eventually replace allow_hotplug field with it,
> it also reduces code duplication (i.e. we wont' have to add it in PCIDevice,
> DimmDevice ...) and potentially allows to use NULL for error in
> property lookup since each bus will have it.

I agree that's the right thing to do.

>> I think it should be in device_set_realized.
> if we dub it nofail then it's fine, otherwise failure path becomes more complicated.
> 
> Calling handler in specific device realize() allows to gracefully abort
> realize() since that device knows what needs to be done to do so:
> For example:
>  @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
>  ...
> +            hdc->hotplug(hotplug_dev, qdev, &local_err);
> +            if (error_is_set(&local_err)) {
> +                int r = pci_unregister_device(&pci_dev->qdev);
> 
> Calling handler in realize will not allow to do it.
> It's just much more flexible to call handler from specific device since it knows
> when it's the best to call handler and how to deal with failure.

We could have separate check/plug methods.  Only check can fail, it must
be idempotent, and it can be invoked while the device is still unrealized.

The reason I liked the interface, is because it removes the need for
each bus to add its own plug/unplug handling.

Paolo

>>> qdev_unplug() might work for now, but I haven't checked all devices that
>>> might use interface and if it would break anything. Ideally it should be
>>> in device's unrealize() complementing realize() part.
>>>
>>> I'd wait till all buses converted to new interface before attempting to
>>> generalize current plug/unplug call pathes though.
>>
>> I agree that adding a default behavior for no link probably requires
>> conversion of all buses.  However, looking for the link in the generic
>> code can be done now.
>>
>> Paolo
>>
>