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 <da...@redhat.com> 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 <akrow...@linux.ibm.com> >>>>>> Reviewed-by: Pierre Morel<pmo...@linux.ibm.com> >>>>>> Tested-by: Pierre Morel<pmo...@linux.ibm.com> >>>>>> --- >>>>>> 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 <da...@redhat.com> > >> >>> >>>> 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