Message ID | 20200605214004.14270-3-akrowiak@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | s390/vfio-ap: dynamic configuration support | expand |
Hi Tony, I love your patch! Perhaps something to improve: [auto build test WARNING on kvms390/next] [also build test WARNING on linus/master v5.7] [cannot apply to s390/features linux/master next-20200605] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Tony-Krowiak/s390-vfio-ap-dynamic-configuration-support/20200606-054350 base: https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git next config: s390-allyesconfig (attached as .config) compiler: s390-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>, old ones prefixed by <<): >> drivers/s390/crypto/vfio_ap_ops.c:130:24: warning: no previous prototype for 'vfio_ap_irq_disable' [-Wmissing-prototypes] 130 | struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q) | ^~~~~~~~~~~~~~~~~~~ >> drivers/s390/crypto/vfio_ap_ops.c:1110:5: warning: no previous prototype for 'vfio_ap_mdev_reset_queue' [-Wmissing-prototypes] 1110 | int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, | ^~~~~~~~~~~~~~~~~~~~~~~~ vim +/vfio_ap_irq_disable +130 drivers/s390/crypto/vfio_ap_ops.c ec89b55e3bce7c Pierre Morel 2019-05-21 113 ec89b55e3bce7c Pierre Morel 2019-05-21 114 /** ec89b55e3bce7c Pierre Morel 2019-05-21 115 * vfio_ap_irq_disable ec89b55e3bce7c Pierre Morel 2019-05-21 116 * @q: The vfio_ap_queue ec89b55e3bce7c Pierre Morel 2019-05-21 117 * ec89b55e3bce7c Pierre Morel 2019-05-21 118 * Uses ap_aqic to disable the interruption and in case of success, reset ec89b55e3bce7c Pierre Morel 2019-05-21 119 * in progress or IRQ disable command already proceeded: calls ec89b55e3bce7c Pierre Morel 2019-05-21 120 * vfio_ap_wait_for_irqclear() to check for the IRQ bit to be clear ec89b55e3bce7c Pierre Morel 2019-05-21 121 * and calls vfio_ap_free_aqic_resources() to free the resources associated ec89b55e3bce7c Pierre Morel 2019-05-21 122 * with the AP interrupt handling. ec89b55e3bce7c Pierre Morel 2019-05-21 123 * ec89b55e3bce7c Pierre Morel 2019-05-21 124 * In the case the AP is busy, or a reset is in progress, ec89b55e3bce7c Pierre Morel 2019-05-21 125 * retries after 20ms, up to 5 times. ec89b55e3bce7c Pierre Morel 2019-05-21 126 * ec89b55e3bce7c Pierre Morel 2019-05-21 127 * Returns if ap_aqic function failed with invalid, deconfigured or ec89b55e3bce7c Pierre Morel 2019-05-21 128 * checkstopped AP. ec89b55e3bce7c Pierre Morel 2019-05-21 129 */ ec89b55e3bce7c Pierre Morel 2019-05-21 @130 struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q) ec89b55e3bce7c Pierre Morel 2019-05-21 131 { ec89b55e3bce7c Pierre Morel 2019-05-21 132 struct ap_qirq_ctrl aqic_gisa = {}; ec89b55e3bce7c Pierre Morel 2019-05-21 133 struct ap_queue_status status; ec89b55e3bce7c Pierre Morel 2019-05-21 134 int retries = 5; ec89b55e3bce7c Pierre Morel 2019-05-21 135 ec89b55e3bce7c Pierre Morel 2019-05-21 136 do { ec89b55e3bce7c Pierre Morel 2019-05-21 137 status = ap_aqic(q->apqn, aqic_gisa, NULL); ec89b55e3bce7c Pierre Morel 2019-05-21 138 switch (status.response_code) { ec89b55e3bce7c Pierre Morel 2019-05-21 139 case AP_RESPONSE_OTHERWISE_CHANGED: ec89b55e3bce7c Pierre Morel 2019-05-21 140 case AP_RESPONSE_NORMAL: ec89b55e3bce7c Pierre Morel 2019-05-21 141 vfio_ap_wait_for_irqclear(q->apqn); ec89b55e3bce7c Pierre Morel 2019-05-21 142 goto end_free; ec89b55e3bce7c Pierre Morel 2019-05-21 143 case AP_RESPONSE_RESET_IN_PROGRESS: ec89b55e3bce7c Pierre Morel 2019-05-21 144 case AP_RESPONSE_BUSY: ec89b55e3bce7c Pierre Morel 2019-05-21 145 msleep(20); ec89b55e3bce7c Pierre Morel 2019-05-21 146 break; ec89b55e3bce7c Pierre Morel 2019-05-21 147 case AP_RESPONSE_Q_NOT_AVAIL: ec89b55e3bce7c Pierre Morel 2019-05-21 148 case AP_RESPONSE_DECONFIGURED: ec89b55e3bce7c Pierre Morel 2019-05-21 149 case AP_RESPONSE_CHECKSTOPPED: ec89b55e3bce7c Pierre Morel 2019-05-21 150 case AP_RESPONSE_INVALID_ADDRESS: ec89b55e3bce7c Pierre Morel 2019-05-21 151 default: ec89b55e3bce7c Pierre Morel 2019-05-21 152 /* All cases in default means AP not operational */ ec89b55e3bce7c Pierre Morel 2019-05-21 153 WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__, ec89b55e3bce7c Pierre Morel 2019-05-21 154 status.response_code); ec89b55e3bce7c Pierre Morel 2019-05-21 155 goto end_free; ec89b55e3bce7c Pierre Morel 2019-05-21 156 } ec89b55e3bce7c Pierre Morel 2019-05-21 157 } while (retries--); ec89b55e3bce7c Pierre Morel 2019-05-21 158 ec89b55e3bce7c Pierre Morel 2019-05-21 159 WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__, ec89b55e3bce7c Pierre Morel 2019-05-21 160 status.response_code); ec89b55e3bce7c Pierre Morel 2019-05-21 161 end_free: ec89b55e3bce7c Pierre Morel 2019-05-21 162 vfio_ap_free_aqic_resources(q); 5c4c2126fb6981 Christian Borntraeger 2019-07-05 163 q->matrix_mdev = NULL; ec89b55e3bce7c Pierre Morel 2019-05-21 164 return status; ec89b55e3bce7c Pierre Morel 2019-05-21 165 } ec89b55e3bce7c Pierre Morel 2019-05-21 166 :::::: The code at line 130 was first introduced by commit :::::: ec89b55e3bce7c8a4bc6b1203280e81342d6745c s390: ap: implement PAPQ AQIC interception in kernel :::::: TO: Pierre Morel <pmorel@linux.ibm.com> :::::: CC: Vasily Gorbik <gor@linux.ibm.com> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 05.06.20 23:39, Tony Krowiak wrote: [..] > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -18,6 +18,7 @@ > #include <linux/delay.h> > #include <linux/mutex.h> > #include <linux/kvm_host.h> > +#include <linux/hashtable.h> > > #include "ap_bus.h" > > @@ -90,8 +91,6 @@ struct ap_matrix_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; > @@ -100,5 +99,8 @@ 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); This looks wrong and unrelated. > + > +int vfio_ap_mdev_probe_queue(struct ap_queue *queue); > +void vfio_ap_mdev_remove_queue(struct ap_queue *queue); > + > #endif /* _VFIO_AP_PRIVATE_H_ */ >
On 05.06.20 23:39, Tony Krowiak wrote: > This patch refactor's the vfio_ap device driver to use the AP bus's > ap_get_qdev() function to retrieve the vfio_ap_queue struct containing > information about a queue that is bound to the vfio_ap device driver. > The bus's ap_get_qdev() function retrieves the queue device from a > hashtable keyed by APQN. This is much more efficient than looping over > the list of devices attached to the AP bus by several orders of > magnitude. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_drv.c | 27 ++------- > drivers/s390/crypto/vfio_ap_ops.c | 82 +++++++++++++++------------ > drivers/s390/crypto/vfio_ap_private.h | 8 ++- > 3 files changed, 58 insertions(+), 59 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c > index be2520cc010b..59233cf7419d 100644 > --- a/drivers/s390/crypto/vfio_ap_drv.c > +++ b/drivers/s390/crypto/vfio_ap_drv.c > @@ -51,15 +51,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids); > */ > static int vfio_ap_queue_dev_probe(struct ap_device *apdev) > { > - struct vfio_ap_queue *q; > - > - q = kzalloc(sizeof(*q), GFP_KERNEL); > - if (!q) > - return -ENOMEM; > - dev_set_drvdata(&apdev->device, q); > - q->apqn = to_ap_queue(&apdev->device)->qid; > - q->saved_isc = VFIO_AP_ISC_INVALID; > - return 0; > + struct ap_queue *queue = to_ap_queue(&apdev->device); > + > + return vfio_ap_mdev_probe_queue(queue); > } Here we did not hold a mutex in the old code [...] > +int vfio_ap_mdev_probe_queue(struct ap_queue *queue) > +{ > + struct vfio_ap_queue *q; > + > + q = kzalloc(sizeof(*q), GFP_KERNEL); > + if (!q) > + return -ENOMEM; > + > + mutex_lock(&matrix_dev->lock); > + dev_set_drvdata(&queue->ap_dev.device, q); > + q->apqn = queue->qid; > + q->saved_isc = VFIO_AP_ISC_INVALID; > + mutex_unlock(&matrix_dev->lock); > + here we do. Why do we need the matrix_dev->lock here?
On 6/16/20 11:45 AM, Christian Borntraeger wrote: > > On 05.06.20 23:39, Tony Krowiak wrote: >> This patch refactor's the vfio_ap device driver to use the AP bus's >> ap_get_qdev() function to retrieve the vfio_ap_queue struct containing >> information about a queue that is bound to the vfio_ap device driver. >> The bus's ap_get_qdev() function retrieves the queue device from a >> hashtable keyed by APQN. This is much more efficient than looping over >> the list of devices attached to the AP bus by several orders of >> magnitude. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_drv.c | 27 ++------- >> drivers/s390/crypto/vfio_ap_ops.c | 82 +++++++++++++++------------ >> drivers/s390/crypto/vfio_ap_private.h | 8 ++- >> 3 files changed, 58 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c >> index be2520cc010b..59233cf7419d 100644 >> --- a/drivers/s390/crypto/vfio_ap_drv.c >> +++ b/drivers/s390/crypto/vfio_ap_drv.c >> @@ -51,15 +51,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids); >> */ >> static int vfio_ap_queue_dev_probe(struct ap_device *apdev) >> { >> - struct vfio_ap_queue *q; >> - >> - q = kzalloc(sizeof(*q), GFP_KERNEL); >> - if (!q) >> - return -ENOMEM; >> - dev_set_drvdata(&apdev->device, q); >> - q->apqn = to_ap_queue(&apdev->device)->qid; >> - q->saved_isc = VFIO_AP_ISC_INVALID; >> - return 0; >> + struct ap_queue *queue = to_ap_queue(&apdev->device); >> + >> + return vfio_ap_mdev_probe_queue(queue); >> } > Here we did not hold a mutex in the old code > [...] > >> +int vfio_ap_mdev_probe_queue(struct ap_queue *queue) >> +{ >> + struct vfio_ap_queue *q; >> + >> + q = kzalloc(sizeof(*q), GFP_KERNEL); >> + if (!q) >> + return -ENOMEM; >> + >> + mutex_lock(&matrix_dev->lock); >> + dev_set_drvdata(&queue->ap_dev.device, q); >> + q->apqn = queue->qid; >> + q->saved_isc = VFIO_AP_ISC_INVALID; >> + mutex_unlock(&matrix_dev->lock); >> + > here we do. Why do we need the matrix_dev->lock here? You are correct, we don't need it here; but, we will need it in a subsequent patch where we introduce linking the q to the matrix mdev to which it is assigned. Perhaps the locking should be introduced in that patch. >
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c index be2520cc010b..59233cf7419d 100644 --- a/drivers/s390/crypto/vfio_ap_drv.c +++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -51,15 +51,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids); */ static int vfio_ap_queue_dev_probe(struct ap_device *apdev) { - struct vfio_ap_queue *q; - - q = kzalloc(sizeof(*q), GFP_KERNEL); - if (!q) - return -ENOMEM; - dev_set_drvdata(&apdev->device, q); - q->apqn = to_ap_queue(&apdev->device)->qid; - q->saved_isc = VFIO_AP_ISC_INVALID; - return 0; + struct ap_queue *queue = to_ap_queue(&apdev->device); + + return vfio_ap_mdev_probe_queue(queue); } /** @@ -70,18 +64,9 @@ 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); - 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); + struct ap_queue *queue = to_ap_queue(&apdev->device); + + vfio_ap_mdev_remove_queue(queue); } static void vfio_ap_matrix_dev_release(struct device *dev) diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index e0bde8518745..7c96b6fd9f70 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -26,43 +26,26 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev); -static int match_apqn(struct device *dev, const void *data) -{ - struct vfio_ap_queue *q = dev_get_drvdata(dev); - - return (q->apqn == *(int *)(data)) ? 1 : 0; -} - /** - * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list - * @matrix_mdev: the associated mediated matrix + * vfio_ap_get_queue: Retrieve a queue with a specific APQN. * @apqn: The queue APQN * - * Retrieve a queue with a specific APQN from the list of the - * devices of the vfio_ap_drv. - * Verify that the APID and the APQI are set in the matrix. + * Retrieve a queue with a specific APQN from the AP queue devices attached to + * the AP bus. * - * Returns the pointer to the associated vfio_ap_queue + * Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL. */ -static struct vfio_ap_queue *vfio_ap_get_queue( - struct ap_matrix_mdev *matrix_mdev, - int apqn) +static struct vfio_ap_queue *vfio_ap_get_queue(unsigned long apqn) { + struct ap_queue *queue; 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)) + queue = ap_get_qdev(apqn); + if (!queue) 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 = dev_get_drvdata(&queue->ap_dev.device); + put_device(&queue->ap_dev.device); return q; } @@ -293,10 +276,11 @@ static int handle_pqap(struct kvm_vcpu *vcpu) matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook, struct ap_matrix_mdev, pqap_hook); - q = vfio_ap_get_queue(matrix_mdev, apqn); + q = vfio_ap_get_queue(apqn); if (!q) goto out_unlock; + q->matrix_mdev = matrix_mdev; status = vcpu->run->s.regs.gprs[1]; /* If IR bit(16) is set we enable the interrupt */ @@ -1116,16 +1100,11 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, static void vfio_ap_irq_disable_apqn(int apqn) { - struct device *dev; struct vfio_ap_queue *q; - dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, - &apqn, match_apqn); - if (dev) { - q = dev_get_drvdata(dev); + q = vfio_ap_get_queue(apqn); + if (q) vfio_ap_irq_disable(q); - put_device(dev); - } } int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, @@ -1302,3 +1281,36 @@ void vfio_ap_mdev_unregister(void) { mdev_unregister_device(&matrix_dev->device); } + +int vfio_ap_mdev_probe_queue(struct ap_queue *queue) +{ + struct vfio_ap_queue *q; + + q = kzalloc(sizeof(*q), GFP_KERNEL); + if (!q) + return -ENOMEM; + + mutex_lock(&matrix_dev->lock); + dev_set_drvdata(&queue->ap_dev.device, q); + q->apqn = queue->qid; + q->saved_isc = VFIO_AP_ISC_INVALID; + mutex_unlock(&matrix_dev->lock); + + return 0; +} + +void vfio_ap_mdev_remove_queue(struct ap_queue *queue) +{ + struct vfio_ap_queue *q; + int apid, apqi; + + mutex_lock(&matrix_dev->lock); + q = dev_get_drvdata(&queue->ap_dev.device); + dev_set_drvdata(&queue->ap_dev.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_private.h b/drivers/s390/crypto/vfio_ap_private.h index f46dde56b464..a2aa05bec718 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -18,6 +18,7 @@ #include <linux/delay.h> #include <linux/mutex.h> #include <linux/kvm_host.h> +#include <linux/hashtable.h> #include "ap_bus.h" @@ -90,8 +91,6 @@ struct ap_matrix_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; @@ -100,5 +99,8 @@ 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_probe_queue(struct ap_queue *queue); +void vfio_ap_mdev_remove_queue(struct ap_queue *queue); + #endif /* _VFIO_AP_PRIVATE_H_ */
This patch refactor's the vfio_ap device driver to use the AP bus's ap_get_qdev() function to retrieve the vfio_ap_queue struct containing information about a queue that is bound to the vfio_ap device driver. The bus's ap_get_qdev() function retrieves the queue device from a hashtable keyed by APQN. This is much more efficient than looping over the list of devices attached to the AP bus by several orders of magnitude. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_drv.c | 27 ++------- drivers/s390/crypto/vfio_ap_ops.c | 82 +++++++++++++++------------ drivers/s390/crypto/vfio_ap_private.h | 8 ++- 3 files changed, 58 insertions(+), 59 deletions(-)