| Message ID | 20231020204838.409521-1-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: fix sysfs status attribute for AP queue devices | expand |
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
On Fri, 20 Oct 2023 16:48:35 -0400 Tony Krowiak <akrowiak@linux.ibm.com> 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") Acked-by: Halil Pasic <pasic@linux.ibm.com> I'm not sure if there is documentation. I assume there is no additional documentation except for the code and the commit messages on what those actually mean. So there is no way to cross-check and no need to update it. I personally don't feel like having clarity on these states. In use does not actually mean that the guest is actually using it: the guest can happily ignore the queue. The unassigned is pretty clear. What "assigned" vs "in use" is supposed to express, not so much to me. I don't think this fix qualifies for the stable process, but it has been a while since I've looked at the corresponding process documentation. > Cc: stable@vger.kernel.org
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?
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.
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
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(-)