| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 27 Apr 2020 15:05:23 +0200
From: Harald Freudenberger <freude@...ux.ibm.com>
To: Halil Pasic <pasic@...ux.ibm.com>,
Tony Krowiak <akrowiak@...ux.ibm.com>
Cc: linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, borntraeger@...ibm.com, cohuck@...hat.com,
mjrosato@...ux.ibm.com, pmorel@...ux.ibm.com,
alex.williamson@...hat.com, kwankhede@...dia.com,
jjherne@...ux.ibm.com, fiuczy@...ux.ibm.com
Subject: Re: [PATCH v7 01/15] s390/vfio-ap: store queue struct in hash table
for quick access
On 24.04.20 05:57, Halil Pasic wrote:
> On Tue, 7 Apr 2020 15:20:01 -0400
> Tony Krowiak <akrowiak@...ux.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@...ux.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_ */
Powered by blists - more mailing lists