Re: [PATCH] s390/vfio-ap: fix sysfs status attribute for AP queue devices

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 11/7/23 03:07, Harald Freudenberger wrote:
> On 2023-11-06 17:03, Tony Krowiak wrote:
>> PING
>> This patch is pretty straight forward, does anyone see a reason why
>> this shouldn't be integrated?
>>
>> On 10/20/23 16:48, Tony Krowiak wrote:
>>> The 'status' attribute for AP queue devices bound to the vfio_ap device
>>> driver displays incorrect status when the mediated device is attached 
>>> to a
>>> guest, but the queue device is not passed through. In the current
>>> implementation, the status displayed is 'in_use' which is not 
>>> correct; it
>>> should be 'assigned'. This can happen if one of the queue devices
>>> associated with a given adapter is not bound to the vfio_ap device 
>>> driver.
>>> For example:
>>>
>>> Queues listed in /sys/bus/ap/drivers/vfio_ap:
>>> 14.0005
>>> 14.0006
>>> 14.000d
>>> 16.0006
>>> 16.000d
>>>
>>> Queues listed in /sys/devices/vfio_ap/matrix/$UUID/matrix
>>> 14.0005
>>> 14.0006
>>> 14.000d
>>> 16.0005
>>> 16.0006
>>> 16.000d
>>>
>>> Queues listed in /sys/devices/vfio_ap/matrix/$UUID/guest_matrix
>>> 14.0005
>>> 14.0006
>>> 14.000d
>>>
>>> The reason no queues for adapter 0x16 are listed in the guest_matrix is
>>> because queue 16.0005 is not bound to the vfio_ap device driver, so no
>>> queue associated with the adapter is passed through to the guest;
>>> therefore, each queue device for adapter 0x16 should display 'assigned'
>>> instead of 'in_use', because those queues are not in use by a guest, but
>>> only assigned to the mediated device.
>>>
>>> Let's check the AP configuration for the guest to determine whether a
>>> queue device is passed through before displaying a status of 'in_use'.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> Fixes: f139862b92cf ("s390/vfio-ap: add status attribute to AP queue 
>>> device's sysfs dir")
>>> Cc: stable@vger.kernel.org
>>> ---
>>>   drivers/s390/crypto/vfio_ap_ops.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 4db538a55192..871c14a6921f 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -1976,6 +1976,7 @@ static ssize_t status_show(struct device *dev,
>>>   {
>>>       ssize_t nchars = 0;
>>>       struct vfio_ap_queue *q;
>>> +    unsigned long apid, apqi;
>>>       struct ap_matrix_mdev *matrix_mdev;
>>>       struct ap_device *apdev = to_ap_dev(dev);
>>>   @@ -1984,7 +1985,11 @@ static ssize_t status_show(struct device *dev,
>>>       matrix_mdev = vfio_ap_mdev_for_queue(q);
>>>         if (matrix_mdev) {
>>> -        if (matrix_mdev->kvm)
>>> +        apid = AP_QID_CARD(q->apqn);
>>> +        apqi = AP_QID_QUEUE(q->apqn);
>>> +        if (matrix_mdev->kvm &&
>>> +            test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>>> +            test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>>>               nchars = scnprintf(buf, PAGE_SIZE, "%s\n",
>>>                          AP_QUEUE_IN_USE);
>>>           else
> 
> I can give you an
> Acked-by: Harald Freudenberger <freude@linux.ibm.com>
> for this. Your explanation sounds sane to me and fixes a wrong
> display. However, I am not familiar with the code so, I can't tell
> if that's correct.
> Just a remark: How can it happen that one queue is not bound to the vfio 
> dd?
> Didn't we actively remove the unbind possibility from the sysfs for devices
> assigned to the vfio dd?

A device bound to the vfio_ap device driver can be manually unbound; 
however, it can not be unbound by the AP bus via a change to the 
apmask/aqmask attributes if it is assigned to a mediated device.

At one point I wanted to remove the unbind sysfs attribute, but as I 
recall I was talked out of it.