On 09.01.19 18:13, Halil Pasic wrote:
> On Wed, 9 Jan 2019 17:37:49 +0100
> David Hildenbrand <da...@redhat.com> 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 <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?
>
> 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.
I go digging if I can't find a public document on how it works ;)
>
> Regards,
> Halil
>
--
Thanks,
David / dhildenb
