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

[Tony Krowiak <akrowiak@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > > Fixes: f139862b92cf ("s390/vfio-ap: add status attribute to AP queue 
> > > device's sysfs dir")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > ---
> > >   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@xxxxxxxxxxxxx>
> 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.