| Message ID | 20200407192015.19887-2-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > Rather than looping over potentially 65535 objects, let's store the > structures for caching information about queue devices bound to the > vfio_ap device driver in a hash table keyed by APQN. This also looks like a nice code simplification. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ > drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- > drivers/s390/crypto/vfio_ap_private.h | 10 ++- > 3 files changed, 60 insertions(+), 68 deletions(-) > (...) > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 5c0f53c6dde7..134860934fe7 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -26,45 +26,16 @@ > > 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 > - * @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. > - * > - * Returns the pointer to the associated vfio_ap_queue Any reason you're killing this comment, instead of adapting it? The function is even no longer static... > - */ > -static struct vfio_ap_queue *vfio_ap_get_queue( > - struct ap_matrix_mdev *matrix_mdev, > - int apqn) > +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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; These were just optimizations and therefore can be dropped now? > - > - 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); > > - return q; > + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { > + if (q && (apqn == q->apqn)) > + return q; > + } Do we need any serialization here? Previously, the driver core made sure we could get a reference only if the device was still registered; not sure if we need any further guarantees now. > + > + return NULL; > } > > /** (...)
On 4/8/20 6:48 AM, Cornelia Huck wrote: > On Tue, 7 Apr 2020 15:20:01 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> Rather than looping over potentially 65535 objects, let's store the >> structures for caching information about queue devices bound to the >> vfio_ap device driver in a hash table keyed by APQN. > This also looks like a nice code simplification. > >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ >> drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- >> drivers/s390/crypto/vfio_ap_private.h | 10 ++- >> 3 files changed, 60 insertions(+), 68 deletions(-) >> > (...) > >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >> index 5c0f53c6dde7..134860934fe7 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -26,45 +26,16 @@ >> >> 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 >> - * @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. >> - * >> - * Returns the pointer to the associated vfio_ap_queue > Any reason you're killing this comment, instead of adapting it? The > function is even no longer static... I can update and restore the comment and the function should be static. > >> - */ >> -static struct vfio_ap_queue *vfio_ap_get_queue( >> - struct ap_matrix_mdev *matrix_mdev, >> - int apqn) >> +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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; > These were just optimizations and therefore can be dropped now? The purpose of this function has changed from its previous incarnation. This function was originally called from the handle_pqap() function and served two purposes: It retrieved the struct vfio_ap_queue as driver data and linked the matrix_mdev to the vfio_ap_queue. The linking of the matrix_mdev and the vfio_ap_queue are now done when queue devices are probed and when adapters and domains are assigned; so now, the handle_pqap() function calls this function to retrieve both the vfio_ap_queue as well as the matrix_mdev to which it is linked. Consequently, the above code is no longer needed. > >> - >> - 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); >> >> - return q; >> + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { >> + if (q && (apqn == q->apqn)) >> + return q; >> + } > Do we need any serialization here? Previously, the driver core made > sure we could get a reference only if the device was still registered; > not sure if we need any further guarantees now. The vfio_ap_queue structs are created when the queue device is probed and removed when the queue device is removed. > >> + >> + return NULL; >> } >> >> /** > (...) >
On Wed, 8 Apr 2020 11:38:07 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 4/8/20 6:48 AM, Cornelia Huck wrote: > > On Tue, 7 Apr 2020 15:20:01 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > >> Rather than looping over potentially 65535 objects, let's store the > >> structures for caching information about queue devices bound to the > >> vfio_ap device driver in a hash table keyed by APQN. > > This also looks like a nice code simplification. > > > >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > >> --- > >> drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ > >> drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- > >> drivers/s390/crypto/vfio_ap_private.h | 10 ++- > >> 3 files changed, 60 insertions(+), 68 deletions(-) > >> > > (...) > >> - */ > >> -static struct vfio_ap_queue *vfio_ap_get_queue( > >> - struct ap_matrix_mdev *matrix_mdev, > >> - int apqn) > >> +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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; > > These were just optimizations and therefore can be dropped now? > > The purpose of this function has changed from its previous incarnation. > This function was originally called from the handle_pqap() function and > served two purposes: It retrieved the struct vfio_ap_queue as driver data > and linked the matrix_mdev to the vfio_ap_queue. The linking of the > matrix_mdev and the vfio_ap_queue are now done when queue devices > are probed and when adapters and domains are assigned; so now, the > handle_pqap() function calls this function to retrieve both the > vfio_ap_queue as well as the matrix_mdev to which it is linked. > Consequently, > the above code is no longer needed. Thanks for the explanation, that makes sense. > > > > >> - > >> - 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); > >> > >> - return q; > >> + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { > >> + if (q && (apqn == q->apqn)) > >> + return q; > >> + } > > Do we need any serialization here? Previously, the driver core made > > sure we could get a reference only if the device was still registered; > > not sure if we need any further guarantees now. > > The vfio_ap_queue structs are created when the queue device is > probed and removed when the queue device is removed. Ok, so anything further is not needed. > > > > >> + > >> + return NULL; > >> } > >> > >> /** > > (...) > > > Looks good to me, then. With vfio_ap_get_queue made static and the kerneldoc restored/updated: Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 4/8/20 12:27 PM, Cornelia Huck wrote: > On Wed, 8 Apr 2020 11:38:07 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 4/8/20 6:48 AM, Cornelia Huck wrote: >>> On Tue, 7 Apr 2020 15:20:01 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> Rather than looping over potentially 65535 objects, let's store the >>>> structures for caching information about queue devices bound to the >>>> vfio_ap device driver in a hash table keyed by APQN. >>> This also looks like a nice code simplification. >>> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>>> --- >>>> drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ >>>> drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- >>>> drivers/s390/crypto/vfio_ap_private.h | 10 ++- >>>> 3 files changed, 60 insertions(+), 68 deletions(-) >>>> >>> (...) >>>> - */ >>>> -static struct vfio_ap_queue *vfio_ap_get_queue( >>>> - struct ap_matrix_mdev *matrix_mdev, >>>> - int apqn) >>>> +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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; >>> These were just optimizations and therefore can be dropped now? >> The purpose of this function has changed from its previous incarnation. >> This function was originally called from the handle_pqap() function and >> served two purposes: It retrieved the struct vfio_ap_queue as driver data >> and linked the matrix_mdev to the vfio_ap_queue. The linking of the >> matrix_mdev and the vfio_ap_queue are now done when queue devices >> are probed and when adapters and domains are assigned; so now, the >> handle_pqap() function calls this function to retrieve both the >> vfio_ap_queue as well as the matrix_mdev to which it is linked. >> Consequently, >> the above code is no longer needed. > Thanks for the explanation, that makes sense. > >>> >>>> - >>>> - 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); >>>> >>>> - return q; >>>> + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { >>>> + if (q && (apqn == q->apqn)) >>>> + return q; >>>> + } >>> Do we need any serialization here? Previously, the driver core made >>> sure we could get a reference only if the device was still registered; >>> not sure if we need any further guarantees now. >> The vfio_ap_queue structs are created when the queue device is >> probed and removed when the queue device is removed. > Ok, so anything further is not needed. > >>> >>>> + >>>> + return NULL; >>>> } >>>> >>>> /** >>> (...) >>> > Looks good to me, then. With vfio_ap_get_queue made static and the > kerneldoc restored/updated: Already done, thanks for the review. > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> >
On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > Rather than looping over potentially 65535 objects, let's store the > structures for caching information about queue devices bound to the > vfio_ap device driver in a hash table keyed by APQN. @Harald: Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own? If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well. For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h). Regards, Halil > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ > drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- > drivers/s390/crypto/vfio_ap_private.h | 10 ++- > 3 files changed, 60 insertions(+), 68 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c > index be2520cc010b..e9c226c0730e 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) > @@ -135,6 +120,7 @@ static int vfio_ap_matrix_dev_create(void) > > mutex_init(&matrix_dev->lock); > INIT_LIST_HEAD(&matrix_dev->mdev_list); > + hash_init(matrix_dev->qtable); > > dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); > matrix_dev->device.parent = root_device; > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 5c0f53c6dde7..134860934fe7 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -26,45 +26,16 @@ > > 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 > - * @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. > - * > - * Returns the pointer to the associated vfio_ap_queue > - */ > -static struct vfio_ap_queue *vfio_ap_get_queue( > - struct ap_matrix_mdev *matrix_mdev, > - int apqn) > +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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); > > - return q; > + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { > + if (q && (apqn == q->apqn)) > + return q; > + } > + > + return NULL; > } > > /** > @@ -293,10 +264,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 +1088,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 +1269,38 @@ 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; > + hash_add(matrix_dev->qtable, &q->qnode, q->apqn); > + 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); > + hash_del(&q->qnode); > + 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..e1f8c82cc55d 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" > > @@ -43,6 +44,7 @@ struct ap_matrix_dev { > struct list_head mdev_list; > struct mutex lock; > struct ap_driver *vfio_ap_drv; > + DECLARE_HASHTABLE(qtable, 8); > }; > > extern struct ap_matrix_dev *matrix_dev; > @@ -90,8 +92,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; > @@ -99,6 +99,10 @@ struct vfio_ap_queue { > int apqn; > #define VFIO_AP_ISC_INVALID 0xff > unsigned char saved_isc; > + struct hlist_node qnode; > }; > -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_ */
On 24.04.20 05:57, Halil Pasic wrote: > On Tue, 7 Apr 2020 15:20:01 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> Rather than looping over potentially 65535 objects, let's store the >> structures for caching information about queue devices bound to the >> vfio_ap device driver in a hash table keyed by APQN. > @Harald: > Would it make sense to make the efficient lookup of an apqueue base > on its APQN core AP functionality instead of each driver figuring it out > on it's own? > > If I'm not wrong the zcrypt device/driver(s) must the problem of > looking up a queue based on its APQN as well. > > For instance struct ep11_cprb has a target_id filed > (arch/s390/include/uapi/asm/zcrypt.h). > > Regards, > Halil Hi Halil no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device. However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues. So if Tony want's I could provide a function like struct ap_queue *ap_get_ap_queue_dev(ap_qid_t qid); Which returns a ptr to an ap_queue device or an error ptr. It would also do a get_device() on the device within the ap_queue struct (and would need to run a put_device() after the caller has finished using the ap_queue). Tony just tell me, if I should go forward and provide this to you. regards Harald > >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ >> drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- >> drivers/s390/crypto/vfio_ap_private.h | 10 ++- >> 3 files changed, 60 insertions(+), 68 deletions(-) >> >> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c >> index be2520cc010b..e9c226c0730e 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) >> @@ -135,6 +120,7 @@ static int vfio_ap_matrix_dev_create(void) >> >> mutex_init(&matrix_dev->lock); >> INIT_LIST_HEAD(&matrix_dev->mdev_list); >> + hash_init(matrix_dev->qtable); >> >> dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); >> matrix_dev->device.parent = root_device; >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >> index 5c0f53c6dde7..134860934fe7 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -26,45 +26,16 @@ >> >> 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 >> - * @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. >> - * >> - * Returns the pointer to the associated vfio_ap_queue >> - */ >> -static struct vfio_ap_queue *vfio_ap_get_queue( >> - struct ap_matrix_mdev *matrix_mdev, >> - int apqn) >> +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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); >> >> - return q; >> + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { >> + if (q && (apqn == q->apqn)) >> + return q; >> + } >> + >> + return NULL; >> } >> >> /** >> @@ -293,10 +264,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 +1088,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 +1269,38 @@ 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; >> + hash_add(matrix_dev->qtable, &q->qnode, q->apqn); >> + 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); >> + hash_del(&q->qnode); >> + 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..e1f8c82cc55d 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" >> >> @@ -43,6 +44,7 @@ struct ap_matrix_dev { >> struct list_head mdev_list; >> struct mutex lock; >> struct ap_driver *vfio_ap_drv; >> + DECLARE_HASHTABLE(qtable, 8); >> }; >> >> extern struct ap_matrix_dev *matrix_dev; >> @@ -90,8 +92,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; >> @@ -99,6 +99,10 @@ struct vfio_ap_queue { >> int apqn; >> #define VFIO_AP_ISC_INVALID 0xff >> unsigned char saved_isc; >> + struct hlist_node qnode; >> }; >> -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_ */
On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <freude@linux.ibm.com> wrote: > On 24.04.20 05:57, Halil Pasic wrote: > > On Tue, 7 Apr 2020 15:20:01 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > >> Rather than looping over potentially 65535 objects, let's store the > >> structures for caching information about queue devices bound to the > >> vfio_ap device driver in a hash table keyed by APQN. > > @Harald: > > Would it make sense to make the efficient lookup of an apqueue base > > on its APQN core AP functionality instead of each driver figuring it out > > on it's own? > > > > If I'm not wrong the zcrypt device/driver(s) must the problem of > > looking up a queue based on its APQN as well. > > > > For instance struct ep11_cprb has a target_id filed > > (arch/s390/include/uapi/asm/zcrypt.h). > > > > Regards, > > Halil > > Hi Halil > > no, the zcrypt drivers don't have this problem. They build up their own device object which > includes a pointer to the base ap device. I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt? If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific. Regards, Halil > > However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each > ap_card object there exists a list of ap_queues.
On 4/27/20 11:17 AM, Halil Pasic wrote: > On Mon, 27 Apr 2020 15:05:23 +0200 > Harald Freudenberger <freude@linux.ibm.com> wrote: > >> On 24.04.20 05:57, Halil Pasic wrote: >>> On Tue, 7 Apr 2020 15:20:01 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> Rather than looping over potentially 65535 objects, let's store the >>>> structures for caching information about queue devices bound to the >>>> vfio_ap device driver in a hash table keyed by APQN. >>> @Harald: >>> Would it make sense to make the efficient lookup of an apqueue base >>> on its APQN core AP functionality instead of each driver figuring it out >>> on it's own? >>> >>> If I'm not wrong the zcrypt device/driver(s) must the problem of >>> looking up a queue based on its APQN as well. >>> >>> For instance struct ep11_cprb has a target_id filed >>> (arch/s390/include/uapi/asm/zcrypt.h). >>> >>> Regards, >>> Halil >> Hi Halil >> >> no, the zcrypt drivers don't have this problem. They build up their own device object which >> includes a pointer to the base ap device. > I'm a bit confused. Doesn't your code loop first trough the ap_card > objects to find the APID portion of the APQN, and then loop the queue > list of the matching card to find the right ap_queue object? Or did I > miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you > point me to the code that avoids the lookup (by apqn) for zcrypt? The code you reference, _zcrypt_send_ep11_cprb(), does loop through each queue associated with each card, but it doesn't appear to be looking for a queue with a particular APQN. It appears to be looking for a queue meeting a specific set of conditions. At least that's my take after taking a very brief look at the code, so I'm not sure that applies here. > > > If you look at the new function of vfio_ap_get_queue(unsigned long apqn) > it basically about finding the queue based on the apqn, with the > difference that it is vfio specific. > > Regards, > Halil > >> However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each >> ap_card object there exists a list of ap_queues. > > >
On Mon, 27 Apr 2020 17:48:58 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > On 4/27/20 11:17 AM, Halil Pasic wrote: > > On Mon, 27 Apr 2020 15:05:23 +0200 > > Harald Freudenberger <freude@linux.ibm.com> wrote: > > > >> On 24.04.20 05:57, Halil Pasic wrote: > >>> On Tue, 7 Apr 2020 15:20:01 -0400 > >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>> > >>>> Rather than looping over potentially 65535 objects, let's store the > >>>> structures for caching information about queue devices bound to the > >>>> vfio_ap device driver in a hash table keyed by APQN. > >>> @Harald: > >>> Would it make sense to make the efficient lookup of an apqueue base > >>> on its APQN core AP functionality instead of each driver figuring it out > >>> on it's own? > >>> > >>> If I'm not wrong the zcrypt device/driver(s) must the problem of > >>> looking up a queue based on its APQN as well. > >>> > >>> For instance struct ep11_cprb has a target_id filed > >>> (arch/s390/include/uapi/asm/zcrypt.h). > >>> > >>> Regards, > >>> Halil > >> Hi Halil > >> > >> no, the zcrypt drivers don't have this problem. They build up their own device object which > >> includes a pointer to the base ap device. > > I'm a bit confused. Doesn't your code loop first trough the ap_card > > objects to find the APID portion of the APQN, and then loop the queue > > list of the matching card to find the right ap_queue object? Or did I > > miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you > > point me to the code that avoids the lookup (by apqn) for zcrypt? > > The code you reference, _zcrypt_send_ep11_cprb(), does loop through > each queue associated with each card, but it doesn't appear to be > looking for > a queue with a particular APQN. It appears to be looking for a queue > meeting a specific set of conditions. At least that's my take after > taking a very > brief look at the code, so I'm not sure that applies here. > One of the possible conditions is that the APQN is in the targets array. Please have another look at the code below, is_desired_ep11_queue() and is_desired_ep11_card() do APQI and APID part of the check respectively: for_each_zcrypt_card(zc) { /* Check for online EP11 cards */ if (!zc->online || !(zc->card->functions & 0x04000000)) continue; /* Check for user selected EP11 card */ if (targets && !is_desired_ep11_card(zc->card->id, target_num, targets)) continue; /* check if device node has admission for this card */ if (!zcrypt_check_card(perms, zc->card->id)) continue; /* get weight index of the card device */ weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY]; if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight)) continue; for_each_zcrypt_queue(zq, zc) { /* check if device is online and eligible */ if (!zq->online || !zq->ops->send_ep11_cprb || (targets && !is_desired_ep11_queue(zq->queue->qid, target_num, targets))) Yes the size of targets may or may not be 1 (example for size == 1 is the invocation form ep11_cryptsingle()) and the respective costs depend on the usual size of the array. Since the goal of the whole exercise seems to be to pick a single queue, and we settle with the first suitable (first not in the input array, but in our lists) that is suitable, I assumed we wouldn't need many hashtable lookups. Regards, Halil > > > > > > If you look at the new function of vfio_ap_get_queue(unsigned long apqn) > > it basically about finding the queue based on the apqn, with the > > difference that it is vfio specific. > > > > Regards, > > Halil > > > >> However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each > >> ap_card object there exists a list of ap_queues. > > > > > > >
On 27.04.20 17:17, Halil Pasic wrote: > On Mon, 27 Apr 2020 15:05:23 +0200 > Harald Freudenberger <freude@linux.ibm.com> wrote: > >> On 24.04.20 05:57, Halil Pasic wrote: >>> On Tue, 7 Apr 2020 15:20:01 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> Rather than looping over potentially 65535 objects, let's store the >>>> structures for caching information about queue devices bound to the >>>> vfio_ap device driver in a hash table keyed by APQN. >>> @Harald: >>> Would it make sense to make the efficient lookup of an apqueue base >>> on its APQN core AP functionality instead of each driver figuring it out >>> on it's own? >>> >>> If I'm not wrong the zcrypt device/driver(s) must the problem of >>> looking up a queue based on its APQN as well. >>> >>> For instance struct ep11_cprb has a target_id filed >>> (arch/s390/include/uapi/asm/zcrypt.h). >>> >>> Regards, >>> Halil >> Hi Halil >> >> no, the zcrypt drivers don't have this problem. They build up their own device object which >> includes a pointer to the base ap device. > I'm a bit confused. Doesn't your code loop first trough the ap_card > objects to find the APID portion of the APQN, and then loop the queue > list of the matching card to find the right ap_queue object? Or did I > miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you > point me to the code that avoids the lookup (by apqn) for zcrypt? No. The code is looping through zcrypt_card and zcrypt_queue objects which are build up and held by the zcrypt api and the zcrypt driver(s). It does not deal with ap_card and ap_queue devices. > > > If you look at the new function of vfio_ap_get_queue(unsigned long apqn) > it basically about finding the queue based on the apqn, with the > difference that it is vfio specific. > > Regards, > Halil > >> However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each >> ap_card object there exists a list of ap_queues. > > >
On 28.04.20 12:07, Halil Pasic wrote: > On Mon, 27 Apr 2020 17:48:58 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> >> On 4/27/20 11:17 AM, Halil Pasic wrote: >>> On Mon, 27 Apr 2020 15:05:23 +0200 >>> Harald Freudenberger <freude@linux.ibm.com> wrote: >>> >>>> On 24.04.20 05:57, Halil Pasic wrote: >>>>> On Tue, 7 Apr 2020 15:20:01 -0400 >>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>>>> >>>>>> Rather than looping over potentially 65535 objects, let's store the >>>>>> structures for caching information about queue devices bound to the >>>>>> vfio_ap device driver in a hash table keyed by APQN. >>>>> @Harald: >>>>> Would it make sense to make the efficient lookup of an apqueue base >>>>> on its APQN core AP functionality instead of each driver figuring it out >>>>> on it's own? >>>>> >>>>> If I'm not wrong the zcrypt device/driver(s) must the problem of >>>>> looking up a queue based on its APQN as well. >>>>> >>>>> For instance struct ep11_cprb has a target_id filed >>>>> (arch/s390/include/uapi/asm/zcrypt.h). >>>>> >>>>> Regards, >>>>> Halil >>>> Hi Halil >>>> >>>> no, the zcrypt drivers don't have this problem. They build up their own device object which >>>> includes a pointer to the base ap device. >>> I'm a bit confused. Doesn't your code loop first trough the ap_card >>> objects to find the APID portion of the APQN, and then loop the queue >>> list of the matching card to find the right ap_queue object? Or did I >>> miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you >>> point me to the code that avoids the lookup (by apqn) for zcrypt? >> The code you reference, _zcrypt_send_ep11_cprb(), does loop through >> each queue associated with each card, but it doesn't appear to be >> looking for >> a queue with a particular APQN. It appears to be looking for a queue >> meeting a specific set of conditions. At least that's my take after >> taking a very >> brief look at the code, so I'm not sure that applies here. >> > One of the possible conditions is that the APQN is in the targets array. > Please have another look at the code below, is_desired_ep11_queue() > and is_desired_ep11_card() do APQI and APID part of the check > respectively: > > for_each_zcrypt_card(zc) { > /* Check for online EP11 cards */ > if (!zc->online || !(zc->card->functions & 0x04000000)) > continue; > /* Check for user selected EP11 card */ > if (targets && > !is_desired_ep11_card(zc->card->id, target_num, targets)) > continue; > /* check if device node has admission for this card */ > if (!zcrypt_check_card(perms, zc->card->id)) > continue; > /* get weight index of the card device */ > weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY]; > if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight)) > continue; > for_each_zcrypt_queue(zq, zc) { > /* check if device is online and eligible */ > if (!zq->online || > !zq->ops->send_ep11_cprb || > (targets && > !is_desired_ep11_queue(zq->queue->qid, > target_num, targets))) > > > Yes the size of targets may or may not be 1 (example for size == 1 is > the invocation form ep11_cryptsingle()) and the respective costs > depend on the usual size of the array. Since the goal of the whole > exercise seems to be to pick a single queue, and we settle with the first > suitable (first not in the input array, but in our lists) that is > suitable, I assumed we wouldn't need many hashtable lookups. > > Regards, > Halil again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards. If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that for you. One note for the improvement via hash list with the argument about the max 65535 objects. Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve the linear search through an array or list but can you measure the performance gain and then compare this to the complexity. ... just some thoughts about beautifying code ... >>> >>> If you look at the new function of vfio_ap_get_queue(unsigned long apqn) >>> it basically about finding the queue based on the apqn, with the >>> difference that it is vfio specific. >>> >>> Regards, >>> Halil >>> >>>> However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each >>>> ap_card object there exists a list of ap_queues. >>> >>>
On 4/28/20 6:57 AM, Harald Freudenberger wrote: > On 28.04.20 12:07, Halil Pasic wrote: >> On Mon, 27 Apr 2020 17:48:58 -0400 >> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >> >>> On 4/27/20 11:17 AM, Halil Pasic wrote: >>>> On Mon, 27 Apr 2020 15:05:23 +0200 >>>> Harald Freudenberger <freude@linux.ibm.com> wrote: >>>> >>>>> On 24.04.20 05:57, Halil Pasic wrote: >>>>>> On Tue, 7 Apr 2020 15:20:01 -0400 >>>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>>>>> >>>>>>> Rather than looping over potentially 65535 objects, let's store the >>>>>>> structures for caching information about queue devices bound to the >>>>>>> vfio_ap device driver in a hash table keyed by APQN. >>>>>> @Harald: >>>>>> Would it make sense to make the efficient lookup of an apqueue base >>>>>> on its APQN core AP functionality instead of each driver figuring it out >>>>>> on it's own? >>>>>> >>>>>> If I'm not wrong the zcrypt device/driver(s) must the problem of >>>>>> looking up a queue based on its APQN as well. >>>>>> >>>>>> For instance struct ep11_cprb has a target_id filed >>>>>> (arch/s390/include/uapi/asm/zcrypt.h). >>>>>> >>>>>> Regards, >>>>>> Halil >>>>> Hi Halil >>>>> >>>>> no, the zcrypt drivers don't have this problem. They build up their own device object which >>>>> includes a pointer to the base ap device. >>>> I'm a bit confused. Doesn't your code loop first trough the ap_card >>>> objects to find the APID portion of the APQN, and then loop the queue >>>> list of the matching card to find the right ap_queue object? Or did I >>>> miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you >>>> point me to the code that avoids the lookup (by apqn) for zcrypt? >>> The code you reference, _zcrypt_send_ep11_cprb(), does loop through >>> each queue associated with each card, but it doesn't appear to be >>> looking for >>> a queue with a particular APQN. It appears to be looking for a queue >>> meeting a specific set of conditions. At least that's my take after >>> taking a very >>> brief look at the code, so I'm not sure that applies here. >>> >> One of the possible conditions is that the APQN is in the targets array. >> Please have another look at the code below, is_desired_ep11_queue() >> and is_desired_ep11_card() do APQI and APID part of the check >> respectively: >> >> for_each_zcrypt_card(zc) { >> /* Check for online EP11 cards */ >> if (!zc->online || !(zc->card->functions & 0x04000000)) >> continue; >> /* Check for user selected EP11 card */ >> if (targets && >> !is_desired_ep11_card(zc->card->id, target_num, targets)) >> continue; >> /* check if device node has admission for this card */ >> if (!zcrypt_check_card(perms, zc->card->id)) >> continue; >> /* get weight index of the card device */ >> weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY]; >> if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight)) >> continue; >> for_each_zcrypt_queue(zq, zc) { >> /* check if device is online and eligible */ >> if (!zq->online || >> !zq->ops->send_ep11_cprb || >> (targets && >> !is_desired_ep11_queue(zq->queue->qid, >> target_num, targets))) >> >> >> Yes the size of targets may or may not be 1 (example for size == 1 is >> the invocation form ep11_cryptsingle()) and the respective costs >> depend on the usual size of the array. Since the goal of the whole >> exercise seems to be to pick a single queue, and we settle with the first >> suitable (first not in the input array, but in our lists) that is >> suitable, I assumed we wouldn't need many hashtable lookups. >> >> Regards, >> Halil > again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards. > If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c > There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. > And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that > for you. > One note for the improvement via hash list with the argument about the max 65535 objects. > Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are > plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources > are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve > the linear search through an array or list but can you measure the performance gain and then compare this > to the complexity. ... just some thoughts about beautifying code ... I set up a test case to compare searching using a hashtable verses using a list. I created both a hashtable and a list of 5100 objects. Each structure had a single APQN field. I then randomly searched both the hashtable and the list for each APQN. The following table contains the result of 5 test runs. The elapsed times are in nanoseconds. Test: List Search Hashtable Search ------ ----------- ---------------- Avg. Per APQN: 11651 81 Total per 5500 APQNs: 60164268 1085368 Avg. Per APQN: 10925 78 Total per 5500 APQNs: 56482780 1084590 Avg. Per APQN: 10190 80 Total per 5500 APQNs: 52714920 1123205 Avg. Per APQN: 8431 76 Total per 5500 APQNs: 43748838 1061414 Avg. Per APQN: 9678 75 Total per 5500 APQNs: 50103437 1044427 ----------------------------------------------- Per APQN Search Avg: 10175 78 Hashtable is 130 times faster Total Search 5500 Avg: 52642848 1079800 Hashtable is 49 times faster Note that the list search was just a straight search of an object in a list, not a device attached to a bus. I don't know if that would add time, but it seems that the savings using a hashtable are significant. So I have two questions: 1. Would it make more sense to provide AP bus interfaces to search for queue devices by APQN? 2. If so, shall we store the queue devices in a hashtable to make the searches more efficient? >>>> If you look at the new function of vfio_ap_get_queue(unsigned long apqn) >>>> it basically about finding the queue based on the apqn, with the >>>> difference that it is vfio specific. >>>> >>>> Regards, >>>> Halil >>>> >>>>> However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each >>>>> ap_card object there exists a list of ap_queues. >>>>
On 29.04.20 00:30, Tony Krowiak wrote: > > > On 4/28/20 6:57 AM, Harald Freudenberger wrote: >> On 28.04.20 12:07, Halil Pasic wrote: >>> On Mon, 27 Apr 2020 17:48:58 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> On 4/27/20 11:17 AM, Halil Pasic wrote: >>>>> On Mon, 27 Apr 2020 15:05:23 +0200 >>>>> Harald Freudenberger <freude@linux.ibm.com> wrote: >>>>> >>>>>> On 24.04.20 05:57, Halil Pasic wrote: >>>>>>> On Tue, 7 Apr 2020 15:20:01 -0400 >>>>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>>>>>> >>>>>>>> Rather than looping over potentially 65535 objects, let's store the >>>>>>>> structures for caching information about queue devices bound to the >>>>>>>> vfio_ap device driver in a hash table keyed by APQN. >>>>>>> @Harald: >>>>>>> Would it make sense to make the efficient lookup of an apqueue base >>>>>>> on its APQN core AP functionality instead of each driver figuring it out >>>>>>> on it's own? >>>>>>> >>>>>>> If I'm not wrong the zcrypt device/driver(s) must the problem of >>>>>>> looking up a queue based on its APQN as well. >>>>>>> >>>>>>> For instance struct ep11_cprb has a target_id filed >>>>>>> (arch/s390/include/uapi/asm/zcrypt.h). >>>>>>> >>>>>>> Regards, >>>>>>> Halil >>>>>> Hi Halil >>>>>> >>>>>> no, the zcrypt drivers don't have this problem. They build up their own device object which >>>>>> includes a pointer to the base ap device. >>>>> I'm a bit confused. Doesn't your code loop first trough the ap_card >>>>> objects to find the APID portion of the APQN, and then loop the queue >>>>> list of the matching card to find the right ap_queue object? Or did I >>>>> miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you >>>>> point me to the code that avoids the lookup (by apqn) for zcrypt? >>>> The code you reference, _zcrypt_send_ep11_cprb(), does loop through >>>> each queue associated with each card, but it doesn't appear to be >>>> looking for >>>> a queue with a particular APQN. It appears to be looking for a queue >>>> meeting a specific set of conditions. At least that's my take after >>>> taking a very >>>> brief look at the code, so I'm not sure that applies here. >>>> >>> One of the possible conditions is that the APQN is in the targets array. >>> Please have another look at the code below, is_desired_ep11_queue() >>> and is_desired_ep11_card() do APQI and APID part of the check >>> respectively: >>> >>> for_each_zcrypt_card(zc) { >>> /* Check for online EP11 cards */ >>> if (!zc->online || !(zc->card->functions & 0x04000000)) >>> continue; >>> /* Check for user selected EP11 card */ >>> if (targets && >>> !is_desired_ep11_card(zc->card->id, target_num, targets)) >>> continue; >>> /* check if device node has admission for this card */ >>> if (!zcrypt_check_card(perms, zc->card->id)) >>> continue; >>> /* get weight index of the card device */ >>> weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY]; >>> if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight)) >>> continue; >>> for_each_zcrypt_queue(zq, zc) { >>> /* check if device is online and eligible */ >>> if (!zq->online || >>> !zq->ops->send_ep11_cprb || >>> (targets && >>> !is_desired_ep11_queue(zq->queue->qid, >>> target_num, targets))) >>> >>> >>> Yes the size of targets may or may not be 1 (example for size == 1 is >>> the invocation form ep11_cryptsingle()) and the respective costs >>> depend on the usual size of the array. Since the goal of the whole >>> exercise seems to be to pick a single queue, and we settle with the first >>> suitable (first not in the input array, but in our lists) that is >>> suitable, I assumed we wouldn't need many hashtable lookups. >>> >>> Regards, >>> Halil >> again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards. >> If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c >> There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. >> And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that >> for you. >> One note for the improvement via hash list with the argument about the max 65535 objects. >> Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are >> plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources >> are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve >> the linear search through an array or list but can you measure the performance gain and then compare this >> to the complexity. ... just some thoughts about beautifying code ... > > I set up a test case to compare searching using a hashtable verses using a list. > I created both a hashtable and a list of 5100 objects. Each structure had a single > APQN field. I then randomly searched both the hashtable and the list for > each APQN. The following table contains the result of 5 test runs. The elapsed > times are in nanoseconds. > > Test: List Search Hashtable Search > ------ ----------- ---------------- > Avg. Per APQN: 11651 81 > Total per 5500 APQNs: 60164268 1085368 > > Avg. Per APQN: 10925 78 > Total per 5500 APQNs: 56482780 1084590 > > Avg. Per APQN: 10190 80 > Total per 5500 APQNs: 52714920 1123205 > > Avg. Per APQN: 8431 76 > Total per 5500 APQNs: 43748838 1061414 > > Avg. Per APQN: 9678 75 > Total per 5500 APQNs: 50103437 1044427 > ----------------------------------------------- > Per APQN Search Avg: 10175 78 Hashtable is 130 times faster > Total Search 5500 Avg: 52642848 1079800 Hashtable is 49 times faster > > Note that the list search was just a straight search of an object in a list, not > a device attached to a bus. I don't know if that would add time, but it seems > that the savings using a hashtable are significant. Halil, I did not say that a hashtable is not faster than a linear list. The only thing I wanted to express is that we are adding complexity and performance improving code which is not even integrated somewhere. We are beautifying here. > > So I have two questions: > > 1. Would it make more sense to provide AP bus interfaces to search for > queue devices by APQN? > > 2. If so, shall we store the queue devices in a hashtable to make the > searches more efficient? If there is a decision to implement this as a feature function within the AP bus base code, I will use the bus functions provided by the kernel common code. So here this will be a bus_for_each_device() together with a filter function. I don't know how this is implemented within the common bus code. > > >>>>> If you look at the new function of vfio_ap_get_queue(unsigned long apqn) >>>>> it basically about finding the queue based on the apqn, with the >>>>> difference that it is vfio specific. >>>>> >>>>> Regards, >>>>> Halil >>>>> >>>>>> However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each >>>>>> ap_card object there exists a list of ap_queues. >>>>> >
On Tue, 28 Apr 2020 12:57:12 +0200 Harald Freudenberger <freude@linux.ibm.com> wrote: > On 28.04.20 12:07, Halil Pasic wrote: > > On Mon, 27 Apr 2020 17:48:58 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > >> > >> On 4/27/20 11:17 AM, Halil Pasic wrote: > >>> On Mon, 27 Apr 2020 15:05:23 +0200 > >>> Harald Freudenberger <freude@linux.ibm.com> wrote: > >>> > >>>> On 24.04.20 05:57, Halil Pasic wrote: > >>>>> On Tue, 7 Apr 2020 15:20:01 -0400 > >>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>>>> > >>>>>> Rather than looping over potentially 65535 objects, let's store the > >>>>>> structures for caching information about queue devices bound to the > >>>>>> vfio_ap device driver in a hash table keyed by APQN. > >>>>> @Harald: > >>>>> Would it make sense to make the efficient lookup of an apqueue base > >>>>> on its APQN core AP functionality instead of each driver figuring it out > >>>>> on it's own? > >>>>> > >>>>> If I'm not wrong the zcrypt device/driver(s) must the problem of > >>>>> looking up a queue based on its APQN as well. > >>>>> > >>>>> For instance struct ep11_cprb has a target_id filed > >>>>> (arch/s390/include/uapi/asm/zcrypt.h). > >>>>> > >>>>> Regards, > >>>>> Halil > >>>> Hi Halil > >>>> > >>>> no, the zcrypt drivers don't have this problem. They build up their own device object which > >>>> includes a pointer to the base ap device. > >>> I'm a bit confused. Doesn't your code loop first trough the ap_card > >>> objects to find the APID portion of the APQN, and then loop the queue > >>> list of the matching card to find the right ap_queue object? Or did I > >>> miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you > >>> point me to the code that avoids the lookup (by apqn) for zcrypt? > >> The code you reference, _zcrypt_send_ep11_cprb(), does loop through > >> each queue associated with each card, but it doesn't appear to be > >> looking for > >> a queue with a particular APQN. It appears to be looking for a queue > >> meeting a specific set of conditions. At least that's my take after > >> taking a very > >> brief look at the code, so I'm not sure that applies here. > >> > > One of the possible conditions is that the APQN is in the targets array. > > Please have another look at the code below, is_desired_ep11_queue() > > and is_desired_ep11_card() do APQI and APID part of the check > > respectively: > > > > for_each_zcrypt_card(zc) { > > /* Check for online EP11 cards */ > > if (!zc->online || !(zc->card->functions & 0x04000000)) > > continue; > > /* Check for user selected EP11 card */ > > if (targets && > > !is_desired_ep11_card(zc->card->id, target_num, targets)) > > continue; > > /* check if device node has admission for this card */ > > if (!zcrypt_check_card(perms, zc->card->id)) > > continue; > > /* get weight index of the card device */ > > weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY]; > > if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight)) > > continue; > > for_each_zcrypt_queue(zq, zc) { > > /* check if device is online and eligible */ > > if (!zq->online || > > !zq->ops->send_ep11_cprb || > > (targets && > > !is_desired_ep11_queue(zq->queue->qid, > > target_num, targets))) > > > > > > Yes the size of targets may or may not be 1 (example for size == 1 is > > the invocation form ep11_cryptsingle()) and the respective costs > > depend on the usual size of the array. Since the goal of the whole > > exercise seems to be to pick a single queue, and we settle with the first > > suitable (first not in the input array, but in our lists) that is > > suitable, I assumed we wouldn't need many hashtable lookups. > > > > Regards, > > Halil > again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards. Well, if you look at "struct vfio_ap_queue* vfio_ap_get_queue(unsigned long apqn)" it also works with vfio_ap_queue and "has nothing directly to do with ap queue". But ap_queue->private points to zcrypt_queue and vfio_ap_queue when the queue is driven by a zcrypt and a vfio_ap driver respectively. > If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c > There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. > And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that > for you. > One note for the improvement via hash list with the argument about the max 65535 objects. > Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are > plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources > are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve > the linear search through an array or list but can you measure the performance gain and then compare this > to the complexity. ... just some thoughts about beautifying code ... My train of thought is that looking up a queue by its APQN is a functionality potentially common to several drivers. I was hoping for a simplification, not for a ton of added complexity. Also I was thinking about the 256 buckets. I mean "DECLARE_HASHTABLE(qtable, 8);". It would be much easier to reason about the table size at a bus level. Regards, Halil
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c index be2520cc010b..e9c226c0730e 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) @@ -135,6 +120,7 @@ static int vfio_ap_matrix_dev_create(void) mutex_init(&matrix_dev->lock); INIT_LIST_HEAD(&matrix_dev->mdev_list); + hash_init(matrix_dev->qtable); dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 5c0f53c6dde7..134860934fe7 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -26,45 +26,16 @@ 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 - * @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. - * - * Returns the pointer to the associated vfio_ap_queue - */ -static struct vfio_ap_queue *vfio_ap_get_queue( - struct ap_matrix_mdev *matrix_mdev, - int apqn) +struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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); - return q; + hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) { + if (q && (apqn == q->apqn)) + return q; + } + + return NULL; } /** @@ -293,10 +264,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 +1088,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 +1269,38 @@ 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; + hash_add(matrix_dev->qtable, &q->qnode, q->apqn); + 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); + hash_del(&q->qnode); + 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..e1f8c82cc55d 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" @@ -43,6 +44,7 @@ struct ap_matrix_dev { struct list_head mdev_list; struct mutex lock; struct ap_driver *vfio_ap_drv; + DECLARE_HASHTABLE(qtable, 8); }; extern struct ap_matrix_dev *matrix_dev; @@ -90,8 +92,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; @@ -99,6 +99,10 @@ struct vfio_ap_queue { int apqn; #define VFIO_AP_ISC_INVALID 0xff unsigned char saved_isc; + struct hlist_node qnode; }; -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_ */
Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_drv.c | 28 +++------ drivers/s390/crypto/vfio_ap_ops.c | 90 ++++++++++++++------------- drivers/s390/crypto/vfio_ap_private.h | 10 ++- 3 files changed, 60 insertions(+), 68 deletions(-)