[v7,01/15] s390/vfio-ap: store queue struct in hash table for quick access
diff mbox series

Message ID 20200407192015.19887-2-akrowiak@linux.ibm.com
State New
Headers show
Series
  • s390/vfio-ap: dynamic configuration support
Related show

Commit Message

Tony Krowiak April 7, 2020, 7:20 p.m. UTC
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(-)

Comments

Cornelia Huck April 8, 2020, 10:48 a.m. UTC | #1
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;
>  }
>  
>  /**

(...)
Tony Krowiak April 8, 2020, 3:38 p.m. UTC | #2
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;
>>   }
>>   
>>   /**
> (...)
>
Cornelia Huck April 8, 2020, 4:27 p.m. UTC | #3
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>
Tony Krowiak April 8, 2020, 4:34 p.m. UTC | #4
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>
>
Halil Pasic April 24, 2020, 3:57 a.m. UTC | #5
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_ */
Harald Freudenberger April 27, 2020, 1:05 p.m. UTC | #6
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_ */
Halil Pasic April 27, 2020, 3:17 p.m. UTC | #7
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.
Tony Krowiak April 27, 2020, 9:48 p.m. UTC | #8
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.
>
>
>
Halil Pasic April 28, 2020, 10:07 a.m. UTC | #9
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.
> >
> >
> >
>
Harald Freudenberger April 28, 2020, 10:46 a.m. UTC | #10
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.
>
>
>
Harald Freudenberger April 28, 2020, 10:57 a.m. UTC | #11
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.
>>>
>>>
Tony Krowiak April 28, 2020, 10:30 p.m. UTC | #12
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.
>>>>
Harald Freudenberger April 29, 2020, 7:56 a.m. UTC | #13
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.
>>>>>
>
Halil Pasic April 29, 2020, 11:30 a.m. UTC | #14
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

Patch
diff mbox series

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_ */