On 21.01.21 08:20, Halil Pasic wrote: > From: Tony Krowiak <akrowiak@xxxxxxxxxxxxx> > > The queues assigned to a matrix mediated device are currently reset when: > > * The VFIO_DEVICE_RESET ioctl is invoked > * The mdev fd is closed by userspace (QEMU) > * The mdev is removed from sysfs. > > Immediately after the reset of a queue, a call is made to disable > interrupts for the queue. This is entirely unnecessary because the reset of > a queue disables interrupts, so this will be removed. > > Furthermore, vfio_ap_irq_disable() does an unconditional PQAP/AQIC which > can result in a specification exception (when the corresponding facility > is not available), so this is actually a bugfix. > > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx> > [pasic@xxxxxxxxxxxxx: minor rework before merging] > Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx> > Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxx> > Fixes: ec89b55e3bce ("s390: ap: implement PAPQ AQIC interception in kernel") > Cc: <stable@xxxxxxxxxxxxxxx> Acked-by: Christian Borntraeger <borntraeger@xxxxxxxxxx> Heiko, Vasily, lets carry this via the s390 tree. > > --- > > Since it turned out disabling the interrupts via PQAP/AQIC is not only > unnecesary but also buggy, we decided to put this patch, which > used to be apart of the series https://lkml.org/lkml/2020/12/22/757 on the fast > lane. > > If the backports turn out to be a bother, which I hope won't be the case > not, I am happy to help with those. > > --- > drivers/s390/crypto/vfio_ap_drv.c | 6 +- > drivers/s390/crypto/vfio_ap_ops.c | 100 ++++++++++++++++---------- > drivers/s390/crypto/vfio_ap_private.h | 12 ++-- > 3 files changed, 69 insertions(+), 49 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c > index be2520cc010b..7dc72cb718b0 100644 > --- a/drivers/s390/crypto/vfio_ap_drv.c > +++ b/drivers/s390/crypto/vfio_ap_drv.c > @@ -71,15 +71,11 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev) > static void vfio_ap_queue_dev_remove(struct ap_device *apdev) > { > struct vfio_ap_queue *q; > - int apid, apqi; > > mutex_lock(&matrix_dev->lock); > q = dev_get_drvdata(&apdev->device); > + vfio_ap_mdev_reset_queue(q, 1); > dev_set_drvdata(&apdev->device, NULL); > - apid = AP_QID_CARD(q->apqn); > - apqi = AP_QID_QUEUE(q->apqn); > - vfio_ap_mdev_reset_queue(apid, apqi, 1); > - vfio_ap_irq_disable(q); > kfree(q); > mutex_unlock(&matrix_dev->lock); > } > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index e0bde8518745..7ceb6c433b3b 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -25,6 +25,7 @@ > #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" > > static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev); > +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn); > > static int match_apqn(struct device *dev, const void *data) > { > @@ -49,20 +50,15 @@ static struct vfio_ap_queue *vfio_ap_get_queue( > int apqn) > { > struct vfio_ap_queue *q; > - struct device *dev; > > if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm)) > return NULL; > if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm)) > return NULL; > > - dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, > - &apqn, match_apqn); > - if (!dev) > - return NULL; > - q = dev_get_drvdata(dev); > - q->matrix_mdev = matrix_mdev; > - put_device(dev); > + q = vfio_ap_find_queue(apqn); > + if (q) > + q->matrix_mdev = matrix_mdev; > > return q; > } > @@ -119,13 +115,18 @@ static void vfio_ap_wait_for_irqclear(int apqn) > */ > static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q) > { > - if (q->saved_isc != VFIO_AP_ISC_INVALID && q->matrix_mdev) > + if (!q) > + return; > + if (q->saved_isc != VFIO_AP_ISC_INVALID && > + !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) { > kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc); > - if (q->saved_pfn && q->matrix_mdev) > + q->saved_isc = VFIO_AP_ISC_INVALID; > + } > + if (q->saved_pfn && !WARN_ON(!q->matrix_mdev)) { > vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev), > &q->saved_pfn, 1); > - q->saved_pfn = 0; > - q->saved_isc = VFIO_AP_ISC_INVALID; > + q->saved_pfn = 0; > + } > } > > /** > @@ -144,7 +145,7 @@ static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q) > * Returns if ap_aqic function failed with invalid, deconfigured or > * checkstopped AP. > */ > -struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q) > +static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q) > { > struct ap_qirq_ctrl aqic_gisa = {}; > struct ap_queue_status status; > @@ -1114,48 +1115,70 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, > return NOTIFY_OK; > } > > -static void vfio_ap_irq_disable_apqn(int apqn) > +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn) > { > struct device *dev; > - struct vfio_ap_queue *q; > + struct vfio_ap_queue *q = NULL; > > dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, > &apqn, match_apqn); > if (dev) { > q = dev_get_drvdata(dev); > - vfio_ap_irq_disable(q); > put_device(dev); > } > + > + return q; > } > > -int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, > +int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, > unsigned int retry) > { > struct ap_queue_status status; > + int ret; > int retry2 = 2; > - int apqn = AP_MKQID(apid, apqi); > > - do { > - status = ap_zapq(apqn); > - switch (status.response_code) { > - case AP_RESPONSE_NORMAL: > - while (!status.queue_empty && retry2--) { > - msleep(20); > - status = ap_tapq(apqn, NULL); > - } > - WARN_ON_ONCE(retry2 <= 0); > - return 0; > - case AP_RESPONSE_RESET_IN_PROGRESS: > - case AP_RESPONSE_BUSY: > + if (!q) > + return 0; > + > +retry_zapq: > + status = ap_zapq(q->apqn); > + switch (status.response_code) { > + case AP_RESPONSE_NORMAL: > + ret = 0; > + break; > + case AP_RESPONSE_RESET_IN_PROGRESS: > + if (retry--) { > msleep(20); > - break; > - default: > - /* things are really broken, give up */ > - return -EIO; > + goto retry_zapq; > } > - } while (retry--); > + ret = -EBUSY; > + break; > + case AP_RESPONSE_Q_NOT_AVAIL: > + case AP_RESPONSE_DECONFIGURED: > + case AP_RESPONSE_CHECKSTOPPED: > + WARN_ON_ONCE(status.irq_enabled); > + ret = -EBUSY; > + goto free_resources; > + default: > + /* things are really broken, give up */ > + WARN(true, "PQAP/ZAPQ completed with invalid rc (%x)\n", > + status.response_code); > + return -EIO; > + } > + > + /* wait for the reset to take effect */ > + while (retry2--) { > + if (status.queue_empty && !status.irq_enabled) > + break; > + msleep(20); > + status = ap_tapq(q->apqn, NULL); > + } > + WARN_ON_ONCE(retry2 <= 0); > > - return -EBUSY; > +free_resources: > + vfio_ap_free_aqic_resources(q); > + > + return ret; > } > > static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev) > @@ -1163,13 +1186,15 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev) > int ret; > int rc = 0; > unsigned long apid, apqi; > + struct vfio_ap_queue *q; > struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > > for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, > matrix_mdev->matrix.apm_max + 1) { > for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, > matrix_mdev->matrix.aqm_max + 1) { > - ret = vfio_ap_mdev_reset_queue(apid, apqi, 1); > + q = vfio_ap_find_queue(AP_MKQID(apid, apqi)); > + ret = vfio_ap_mdev_reset_queue(q, 1); > /* > * Regardless whether a queue turns out to be busy, or > * is not operational, we need to continue resetting > @@ -1177,7 +1202,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev) > */ > if (ret) > rc = ret; > - vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi)); > } > } > > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h > index f46dde56b464..28e9d9989768 100644 > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -88,11 +88,6 @@ struct ap_matrix_mdev { > struct mdev_device *mdev; > }; > > -extern int vfio_ap_mdev_register(void); > -extern void vfio_ap_mdev_unregister(void); > -int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, > - unsigned int retry); > - > struct vfio_ap_queue { > struct ap_matrix_mdev *matrix_mdev; > unsigned long saved_pfn; > @@ -100,5 +95,10 @@ struct vfio_ap_queue { > #define VFIO_AP_ISC_INVALID 0xff > unsigned char saved_isc; > }; > -struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q); > + > +int vfio_ap_mdev_register(void); > +void vfio_ap_mdev_unregister(void); > +int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, > + unsigned int retry); > + > #endif /* _VFIO_AP_PRIVATE_H_ */ > > base-commit: 9791581c049c10929e97098374dd1716a81fefcc >