s390x/vfio-ap: Implement hot plug/unplug of vfio-ap device

Commit Message

Anthony Krowiak Jan. 8, 2019, 4:01 p.m. UTC

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 <[e-mail]>
Reviewed-by: Pierre Morel<[e-mail]>
Tested-by: Pierre Morel<[e-mail]>
---
 hw/s390x/ap-bridge.c | 12 +++++++++++-
 hw/vfio/ap.c         |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

David Hildenbrand Jan. 8, 2019, 4:09 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Tested-by: Pierre Morel<[e-mail]>
> ---
>  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?

>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>  }
>  
> @@ -67,6 +73,10 @@ static const TypeInfo ap_bridge_info = {
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = 0,
>      .class_init    = ap_bridge_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  static void ap_register(void)
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 6166ccd47a4a..d8b79ebe53ae 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -169,7 +169,7 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      dc->realize = vfio_ap_realize;
>      dc->unrealize = vfio_ap_unrealize;
> -    dc->hotpluggable = false;
> +    dc->hotpluggable = true;
>      dc->reset = vfio_ap_reset;
>      dc->bus_type = TYPE_AP_BUS;
>  }
>

Anthony Krowiak Jan. 8, 2019, 4:14 p.m. UTC | #2

On 1/8/19 11:01 AM, 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 <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Tested-by: Pierre Morel<[e-mail]>
> ---
>   hw/s390x/ap-bridge.c | 12 +++++++++++-
>   hw/vfio/ap.c         |  2 +-
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 

Please note: During testing of this patch I discovered a bug whereby an
attempt to plug in a vfio-ap device subsequent to an unplug of the
device will result in the following error:

error: Failed to attach device from /root/conf/vfio-ap.xml
error: internal error: unable to execute QEMU command 'device_add': No 
'ap-bus' bus found for device 'vfio-ap'

I wrote a patch for this bug:
Message ID: <[e-mail]>

Anthony Krowiak Jan. 8, 2019, 7:52 p.m. UTC | #3

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 <[e-mail]>
>> Reviewed-by: Pierre Morel<[e-mail]>
>> Tested-by: Pierre Morel<[e-mail]>
>> ---
>>   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. 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.
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.


> 
>>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>   }
>>   
>> @@ -67,6 +73,10 @@ static const TypeInfo ap_bridge_info = {
>>       .parent        = TYPE_SYS_BUS_DEVICE,
>>       .instance_size = 0,
>>       .class_init    = ap_bridge_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_HOTPLUG_HANDLER },
>> +        { }
>> +    }
>>   };
>>   
>>   static void ap_register(void)
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 6166ccd47a4a..d8b79ebe53ae 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -169,7 +169,7 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
>>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>       dc->realize = vfio_ap_realize;
>>       dc->unrealize = vfio_ap_unrealize;
>> -    dc->hotpluggable = false;
>> +    dc->hotpluggable = true;
>>       dc->reset = vfio_ap_reset;
>>       dc->bus_type = TYPE_AP_BUS;
>>   }
>>
> 
>

David Hildenbrand Jan. 8, 2019, 10:13 p.m. UTC | #4

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 <[e-mail]>
>>> Reviewed-by: Pierre Morel<[e-mail]>
>>> Tested-by: Pierre Morel<[e-mail]>
>>> ---
>>>   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")

Cornelia Huck Jan. 9, 2019, 11:30 a.m. UTC | #5

On Tue, 8 Jan 2019 23:13:39 +0100
David Hildenbrand <[e-mail]> 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 <[e-mail]>
> >>> Reviewed-by: Pierre Morel<[e-mail]>
> >>> Tested-by: Pierre Morel<[e-mail]>
> >>> ---
> >>>   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.

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

> 
> > 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")

I hope that severing the connection between QEMU and the host kernel
for AP takes care of cleanup. It's all a bit confusing :(

Anthony Krowiak Jan. 9, 2019, 4:27 p.m. UTC | #6

On 1/9/19 6:30 AM, Cornelia Huck wrote:
> On Tue, 8 Jan 2019 23:13:39 +0100
> David Hildenbrand <[e-mail]> 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 <[e-mail]>
>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>> Tested-by: Pierre Morel<[e-mail]>
>>>>> ---
>>>>>    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 ;)

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

>

David Hildenbrand Jan. 9, 2019, 4:37 p.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]>
>>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>>> Tested-by: Pierre Morel<[e-mail]>
>>>>>> ---
>>>>>>    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 <[e-mail]>

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

Halil Pasic Jan. 9, 2019, 5:05 p.m. UTC | #8

On Tue,  8 Jan 2019 11:01:50 -0500
Tony Krowiak <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Pierre Morel<[e-mail]>
> Tested-by: Pierre Morel<[e-mail]>

Reviewed-by: Halil Pasic <[e-mail]>

Halil Pasic Jan. 9, 2019, 5:13 p.m. UTC | #9

On Wed, 9 Jan 2019 17:37:49 +0100
David Hildenbrand <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >>>>>> Reviewed-by: Pierre Morel<[e-mail]>
> >>>>>> Tested-by: Pierre Morel<[e-mail]>
> >>>>>> ---
> >>>>>>    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

David Hildenbrand Jan. 9, 2019, 5:28 p.m. UTC | #10

On 09.01.19 18:13, Halil Pasic wrote:
> On Wed, 9 Jan 2019 17:37:49 +0100
> David Hildenbrand <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>>>>> Tested-by: Pierre Morel<[e-mail]>
>>>>>>>> ---
>>>>>>>>    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
>

Anthony Krowiak Jan. 10, 2019, 4:22 p.m. UTC | #11

On 1/9/19 12:28 PM, David Hildenbrand wrote:
> On 09.01.19 18:13, Halil Pasic wrote:
>> On Wed, 9 Jan 2019 17:37:49 +0100
>> David Hildenbrand <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>>>> Reviewed-by: Pierre Morel<[e-mail]>
>>>>>>>>> Tested-by: Pierre Morel<[e-mail]>
>>>>>>>>> ---
>>>>>>>>>     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 ;)

Which reminds me, I will also need to update the docs/vfio-ap.txt
document.

> 
>>
>> Regards,
>> Halil
>>
> 
>

Cornelia Huck Jan. 14, 2019, 2:16 p.m. UTC | #12

On Thu, 10 Jan 2019 11:22:48 -0500
Tony Krowiak <[e-mail]> wrote:

> >>> 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 ;)  
> 
> Which reminds me, I will also need to update the docs/vfio-ap.txt
> document.

So, I assume there will be a v2 with some documentation changes?

Anthony Krowiak Jan. 28, 2019, 7:27 p.m. UTC | #13

On 1/14/19 9:16 AM, Cornelia Huck wrote:
> On Thu, 10 Jan 2019 11:22:48 -0500
> Tony Krowiak <[e-mail]> wrote:
> 
>>>>> 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 ;)
>>
>> Which reminds me, I will also need to update the docs/vfio-ap.txt
>> document.
> 
> So, I assume there will be a v2 with some documentation changes?

I'm sorry Connie, I got sidetracked with some other priorities and
missed this response. I will post a v2 with doc changes.

>

Patch

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;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
@@ -67,6 +73,10 @@  static const TypeInfo ap_bridge_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = 0,
     .class_init    = ap_bridge_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void ap_register(void)
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 6166ccd47a4a..d8b79ebe53ae 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -169,7 +169,7 @@  static void vfio_ap_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->realize = vfio_ap_realize;
     dc->unrealize = vfio_ap_unrealize;
-    dc->hotpluggable = false;
+    dc->hotpluggable = true;
     dc->reset = vfio_ap_reset;
     dc->bus_type = TYPE_AP_BUS;
 }