| Message ID | 20210121072008.76523-1-pasic@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] s390/vfio-ap: No need to disable IRQ after queue reset | expand |
On Thu, 21 Jan 2021 09:20:44 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 21 Jan 2021 08:20:08 +0100 > Halil Pasic <pasic@linux.ibm.com> wrote: [..] > > --- 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); > > Nit: was moving these two necessary? > No not strictly necessary. I decided that having the data types first and the function prototypes in one place after the former is nicer. > > +int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, > > + unsigned int retry); > > + > > #endif /* _VFIO_AP_PRIVATE_H_ */ > > > > base-commit: 9791581c049c10929e97098374dd1716a81fefcc > > Anyway, if I didn't entangle myself in the various branches, this seems > sane. > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Thank you very much! Regards, Halil
On 21.01.21 08:20, Halil Pasic wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > 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@linux.ibm.com> > [pasic@linux.ibm.com: minor rework before merging] > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > Fixes: ec89b55e3bce ("s390: ap: implement PAPQ AQIC interception in kernel") > Cc: <stable@vger.kernel.org> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> 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 >
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_ */