Re: [PATCH v13 05/15] s390/vfio-ap: manage link between queue struct and matrix mdev

[Tony Krowiak <akrowiak@xxxxxxxxxxxxx>]

On 1/11/21 2:17 PM, Halil Pasic wrote:
> On Tue, 22 Dec 2020 20:15:56 -0500
> Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
>
> > Let's create links between each queue device bound to the vfio_ap device
> > driver and the matrix mdev to which the queue's APQN is assigned. The idea
> > is to facilitate efficient retrieval of the objects representing the queue
> > devices and matrix mdevs as well as to verify that a queue assigned to
> > a matrix mdev is bound to the driver.
> >
> > The links will be created as follows:
> >
> >     * When the queue device is probed, if its APQN is assigned to a matrix
> >       mdev, the structures representing the queue device and the matrix mdev
> >       will be linked.
> >
> >     * When an adapter or domain is assigned to a matrix mdev, for each new
> >       APQN assigned that references a queue device bound to the vfio_ap
> >       device driver, the structures representing the queue device and the
> >       matrix mdev will be linked.
> >
> > The links will be removed as follows:
> >
> >     * When the queue device is removed, if its APQN is assigned to a matrix
> >       mdev, the structures representing the queue device and the matrix mdev
> >       will be unlinked.
> >
> >     * When an adapter or domain is unassigned from a matrix mdev, for each
> >       APQN unassigned that references a queue device bound to the vfio_ap
> >       device driver, the structures representing the queue device and the
> >       matrix mdev will be unlinked.
> >
> > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
>
> > ---
> >   drivers/s390/crypto/vfio_ap_ops.c     | 140 +++++++++++++++++++++-----
> >   drivers/s390/crypto/vfio_ap_private.h |   3 +
> >   2 files changed, 117 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > index 835c963ae16d..cdcc6378b4a5 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -27,33 +27,17 @@
> >   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> >   static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
> >   
> > -/**
> > - * 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)
> > +static struct vfio_ap_queue *
> > +vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
> >   {
> > -	struct vfio_ap_queue *q = NULL;
> > -
> > -	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;
> > +	struct vfio_ap_queue *q;
> >   
> > -	q = vfio_ap_find_queue(apqn);
> > -	if (q)
> > -		q->matrix_mdev = matrix_mdev;
> > +	hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
> > +		if (q && (q->apqn == apqn))
> > +			return q;
> > +	}
> >   
> > -	return q;
> > +	return NULL;
> >   }
> >   
> >   /**
> > @@ -166,7 +150,6 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
> >   		  status.response_code);
> >   end_free:
> >   	vfio_ap_free_aqic_resources(q);
> > -	q->matrix_mdev = NULL;
> >   	return status;
> >   }
> >   
> > @@ -282,7 +265,7 @@ 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_mdev_get_queue(matrix_mdev, apqn);
> >   	if (!q)
> >   		goto out_unlock;
> >   
> > @@ -325,6 +308,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
> >   
> >   	matrix_mdev->mdev = mdev;
> >   	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
> > +	hash_init(matrix_mdev->qtable);
> >   	mdev_set_drvdata(mdev, matrix_mdev);
> >   	matrix_mdev->pqap_hook.hook = handle_pqap;
> >   	matrix_mdev->pqap_hook.owner = THIS_MODULE;
> > @@ -553,6 +537,50 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
> >   	return 0;
> >   }
> >   
> > +static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
> > +				    struct vfio_ap_queue *q)
> > +{
> > +	if (q) {
> > +		q->matrix_mdev = matrix_mdev;
> > +		hash_add(matrix_mdev->qtable,
> > +			 &q->mdev_qnode, q->apqn);
> > +	}
> > +}
> > +
> > +static void vfio_ap_mdev_link_apqn(struct ap_matrix_mdev *matrix_mdev, int apqn)
> > +{
> > +	struct vfio_ap_queue *q;
> > +
> > +	q = vfio_ap_find_queue(apqn);
> > +	vfio_ap_mdev_link_queue(matrix_mdev, q);
> > +}
> > +
> > +static void vfio_ap_mdev_unlink_queue(struct vfio_ap_queue *q)
> > +{
> > +	if (q) {
> > +		q->matrix_mdev = NULL;
> > +		hash_del(&q->mdev_qnode);
> > +	}
> > +}
> > +
> > +static void vfio_ap_mdev_unlink_apqn(int apqn)
> > +{
> > +	struct vfio_ap_queue *q;
> > +
> > +	q = vfio_ap_find_queue(apqn);
> > +	vfio_ap_mdev_unlink_queue(q);
> > +}
> > +
> > +static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
> > +				      unsigned long apid)
> > +{
> > +	unsigned long apqi;
> > +
> > +	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
> > +		vfio_ap_mdev_link_apqn(matrix_mdev,
> > +				       AP_MKQID(apid, apqi));
> > +}
> > +
> >   /**
> >    * assign_adapter_store
> >    *
> > @@ -622,6 +650,7 @@ static ssize_t assign_adapter_store(struct device *dev,
> >   	if (ret)
> >   		goto share_err;
> >   
> > +	vfio_ap_mdev_link_adapter(matrix_mdev, apid);
> >   	ret = count;
> >   	goto done;
> >   
> > @@ -634,6 +663,15 @@ static ssize_t assign_adapter_store(struct device *dev,
> >   }
> >   static DEVICE_ATTR_WO(assign_adapter);
> >   
> > +static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
> > +					unsigned long apid)
> > +{
> > +	unsigned long apqi;
> > +
> > +	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
> > +		vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
> > +}
> > +
> >   /**
> >    * unassign_adapter_store
> >    *
> > @@ -673,6 +711,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
> >   
> >   	mutex_lock(&matrix_dev->lock);
> >   	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
> > +	vfio_ap_mdev_unlink_adapter(matrix_mdev, apid);
> >   	mutex_unlock(&matrix_dev->lock);
> >   
> >   	return count;
> > @@ -699,6 +738,15 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
> >   	return 0;
> >   }
> >   
> > +static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
> > +				     unsigned long apqi)
> > +{
> > +	unsigned long apid;
> > +
> > +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
> > +		vfio_ap_mdev_link_apqn(matrix_mdev, AP_MKQID(apid, apqi));
> > +}
> > +
> >   /**
> >    * assign_domain_store
> >    *
> > @@ -763,6 +811,7 @@ static ssize_t assign_domain_store(struct device *dev,
> >   	if (ret)
> >   		goto share_err;
> >   
> > +	vfio_ap_mdev_link_domain(matrix_mdev, apqi);
> >   	ret = count;
> >   	goto done;
> >   
> > @@ -775,6 +824,14 @@ static ssize_t assign_domain_store(struct device *dev,
> >   }
> >   static DEVICE_ATTR_WO(assign_domain);
> >   
> > +static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
> > +				       unsigned long apqi)
> > +{
> > +	unsigned long apid;
> > +
> > +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
> > +		vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
> > +}
> >   
> >   /**
> >    * unassign_domain_store
> > @@ -815,6 +872,7 @@ static ssize_t unassign_domain_store(struct device *dev,
> >   
> >   	mutex_lock(&matrix_dev->lock);
> >   	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
> > +	vfio_ap_mdev_unlink_domain(matrix_mdev, apqi);
> >   	mutex_unlock(&matrix_dev->lock);
> >   
> >   	return count;
> > @@ -1317,6 +1375,28 @@ void vfio_ap_mdev_unregister(void)
> >   	mdev_unregister_device(&matrix_dev->device);
> >   }
> >   
> > +/*
> > + * vfio_ap_queue_link_mdev
> > + *
> > + * @q: The queue to link with the matrix mdev.
> > + *
> > + * Links @q with the matrix mdev to which the queue's APQN is assigned.
> > + */
> > +static void vfio_ap_queue_link_mdev(struct vfio_ap_queue *q)
> > +{
> > +	unsigned long apid = AP_QID_CARD(q->apqn);
> > +	unsigned long apqi = AP_QID_QUEUE(q->apqn);
> > +	struct ap_matrix_mdev *matrix_mdev;
> > +
> > +	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> > +		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
> > +		    test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
> > +			vfio_ap_mdev_link_queue(matrix_mdev, q);
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> >   int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
> >   {
> >   	struct vfio_ap_queue *q;
> > @@ -1324,9 +1404,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
> >   	q = kzalloc(sizeof(*q), GFP_KERNEL);
> >   	if (!q)
> >   		return -ENOMEM;
> > +	mutex_lock(&matrix_dev->lock);
> >   	dev_set_drvdata(&apdev->device, q);
> >   	q->apqn = to_ap_queue(&apdev->device)->qid;
> >   	q->saved_isc = VFIO_AP_ISC_INVALID;
> > +	vfio_ap_queue_link_mdev(q);
> > +	mutex_unlock(&matrix_dev->lock);
> > +
> Does the critical section have to include more than just
> vfio_ap_queue_link_mdev()? Did we need the critical section
> before this patch?

We did not need the critical section before this patch because
the only function that retrieved the vfio_ap_queue via the queue
device's drvdata was the remove callback. I included the initialization
of the vfio_ap_queue object under lock because the
vfio_ap_find_queue() function retrieves the vfio_ap_queue object from
the queue device's drvdata so it might be advantageous to initialize
it under the mdev lock. On the other hand, I can't come up with a good
argument to change this.


> >   	return 0;
> >   }
> >   
> > @@ -1341,6 +1425,10 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
> >   	apid = AP_QID_CARD(q->apqn);
> >   	apqi = AP_QID_QUEUE(q->apqn);
> >   	vfio_ap_mdev_reset_queue(apid, apqi, 1);
> > +
> > +	if (q->matrix_mdev)
> > +		vfio_ap_mdev_unlink_queue(q);
> > +
> >   	kfree(q);
> >   	mutex_unlock(&matrix_dev->lock);
> >   }
> > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> > index d9003de4fbad..4e5cc72fc0db 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"
> >   
> > @@ -86,6 +87,7 @@ struct ap_matrix_mdev {
> >   	struct kvm *kvm;
> >   	struct kvm_s390_module_hook pqap_hook;
> >   	struct mdev_device *mdev;
> > +	DECLARE_HASHTABLE(qtable, 8);
> >   };
> >   
> >   extern int vfio_ap_mdev_register(void);
> > @@ -97,6 +99,7 @@ struct vfio_ap_queue {
> >   	int	apqn;
> >   #define VFIO_AP_ISC_INVALID 0xff
> >   	unsigned char saved_isc;
> > +	struct hlist_node mdev_qnode;
> >   };
> >   
> >   int vfio_ap_mdev_probe_queue(struct ap_device *queue);