Re: [Qemu-devel] [PATCH] s390x/vfio-ap: Implement hot plug/unplug of vfio-ap device

[David Hildenbrand]

On 08.01.19 20:52, Tony Krowiak wrote:
> On 1/8/19 11:09 AM, David Hildenbrand wrote:
>> On 08.01.19 17:01, Tony Krowiak wrote:
>>> Introduces hot plug/unplug support for the vfio-ap device. Note that only 
>>> one
>>> vfio-ap device can be attached to the ap-bus, so a vfio-ap device can only 
>>> be
>>> hot plugged if the '-device vfio-ap,sysfsdev=$path_to_mdev' option is not
>>> specified on the QEMU command line.
>>>
>>> Signed-off-by: Tony Krowiak <address@hidden>
>>> Reviewed-by: Pierre Morel<address@hidden>
>>> Tested-by: Pierre Morel<address@hidden>
>>> ---
>>>   hw/s390x/ap-bridge.c | 12 +++++++++++-
>>>   hw/vfio/ap.c         |  2 +-
>>>   2 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>>> index 3795d30dd7c9..25a03412fcb9 100644
>>> --- a/hw/s390x/ap-bridge.c
>>> +++ b/hw/s390x/ap-bridge.c
>>> @@ -39,6 +39,7 @@ static const TypeInfo ap_bus_info = {
>>>   void s390_init_ap(void)
>>>   {
>>>       DeviceState *dev;
>>> +    BusState *bus;
>>>   
>>>       /* If no AP instructions then no need for AP bridge */
>>>       if (!s390_has_feat(S390_FEAT_AP)) {
>>> @@ -52,13 +53,18 @@ void s390_init_ap(void)
>>>       qdev_init_nofail(dev);
>>>   
>>>       /* Create bus on bridge device */
>>> -    qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS);
>>> +    bus = qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS);
>>> +
>>> +    /* Enable hotplugging */
>>> +    qbus_set_hotplug_handler(bus, dev, &error_abort);
>>>    }
>>>   
>>>   static void ap_bridge_class_init(ObjectClass *oc, void *data)
>>>   {
>>>       DeviceClass *dc = DEVICE_CLASS(oc);
>>> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>>>   
>>> +    hc->unplug = qdev_simple_device_unplug_cb;
>>
>> confused, why is there no plug action?
> 
> You will find this is the case for several devices that are hot
> pluggable.

Usually missing hotplug handlers are an indication of legacy code ;)

> The plug callback is invoked after the device is
> attached to the bus and after the device is realized. Not having
> a hot plug callback does not preclude hot plugging of a device.

The hotplug handler is there to

1. Assign resources (e.g. ids etc)
2. Notify the system (e.g. hotplug interrupt)

In legacy code (e.g. PCI) such stuff is usually still located in the
realize function (where it doesn't belong anymore but factoring out is
hard ...)

Looking at hw/vfio/ap.c:realize(), there isn't really anything in there.

So I assume that

1. No resources have to be assigned (for vfio-ap, I guess the IDs and
such are implicit)
2. No notification will happen. How will the guest know that a new AP
adapter is available?

> I presume the purpose of the callback is to provide an opportunity
> to do perform any additional processing that may be required to prepare
> the device for use. In the case of the vfio-ap device, there is nothing
> to do once the device is plugged.

When removing the device, is it really a forced removal? ("simply rip it
out without telling the guest")

-- 

Thanks,

David / dhildenb