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

[Halil Pasic]

On Wed, 9 Jan 2019 17:37:49 +0100
David Hildenbrand <address@hidden> wrote:

> 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? 

Maybe a short comment that explains why qdev_simple_device_unplug_cb()
is appropriate as well (i.e. hits that closing the mdev's fd is what
triggers the cleanup of the actual resources)? I personally go log
digging only once I get desperate.

Regards,
Halil