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

[David Hildenbrand]

On 09.01.19 17:27, Tony Krowiak wrote:
> On 1/9/19 6:30 AM, Cornelia Huck wrote:
>> On Tue, 8 Jan 2019 23:13:39 +0100
>> David Hildenbrand <address@hidden> wrote:
>>
>>> 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)
>>
>> That's my understanding as well. The interesting stuff will be
>> configured on kernel level before the device is even handed to QEMU for
>> consumption.
> 
> The vfio-ap device represents a VFIO mdev device. AP resources - i.e.,
> adapters, domains and control domains - are assigned to the mdev device
> via its sysfs interfaces. This is all handled by the vfio_ap kernel
> driver before a guest can use the mdev device. As part of vfio-ap device
> realization, a file descriptor is opened on the mdev device. When the 
> mdev device's fd is opened, a callback is invoked on the vfio_ap kernel
> device driver. The device driver then updates the guest's AP matrix
> configuration based on the configuration specified via the mdev
> device's sysfs interfaces.
> 
>>
>> (Would be nice to hint at that in the patch description.)
>>
>>> 2. No notification will happen. How will the guest know that a new AP
>>> adapter is available?
>>
>> My understanding is that hotplugging the matrix device will make the
>> guest go from "no adapters/domains are available" to "some
>> adapters/domains are available" (and unplug will do the reverse). I.e.,
>> no hot(un)plugging of individual queues (which would need to be done on
>> the kernel level, and is tricky IIRC.)
>>
>> I'm not sure what the architectured options for notifying the guest are
>> (I dimly recall some kind of "AP configuration has changed event").
>>
>> IIRC, the Linux guest driver scans for new queues periodically. Does it
>> even do that if no queues are available to start with?
> 
> The AP bus - in this case, running in the guest - does a periodic scan
> for AP devices. The bus relies on an AP instruction that queries the
> AP configuration information. When the guest's AP matrix is updated -
> see description of mdev device fd open processing above - the query
> will provide the newly configured AP matrix and the bus will create
> the adapter and queue devices on the guest. Consequently, there is
> nothing to do in a hot plug handler. If you'd like, I'd be more than
> happy to include a hot plug handler that does some logging (or nothing
> at all) so it doesn't look like legacy code ;)

Hehe, no it's fine for me.

Can you extend this patch description a little bit, including what we
discussed here? With that

Reviewed-by: David Hildenbrand <address@hidden>

> 
>>
>>>
>>>> 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")
> 
> When the vfio-ap device is unrealized, the mdev device's file descriptor
> is closed. When the fd is closed, a callback is invoked on the vfio_ap
> kernel device driver. This callback clears the guest's AP matrix
> configuration and resets all of the AP queues affected. When the AP
> device scan is subsequently run by the AP bus on the guest, the AP
> instruction for querying the AP configuration will indicate that there
> are no AP devices configured. The bus will then remove the AP devices
> from the sysfs for the guest.
> 
> 
>>
>> I hope that severing the connection between QEMU and the host kernel
>> for AP takes care of cleanup. It's all a bit confusing :(
> 
> I agree, the AP architecture is complicated and confusing. As I said
> above, when the mdev device fd is closed, the vfio_ap kernel device
> driver will reset all AP queues affected by the de-configuration.
> 
>>
> 


-- 

Thanks,

David / dhildenb