| Message ID | 20201223011606.5265-6-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
On Tue, 22 Dec 2020 20:15:56 -0500 Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > --- > 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? > 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);
On 1/11/21 2:17 PM, Halil Pasic wrote: > On Tue, 22 Dec 2020 20:15:56 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > >> --- >> 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);
On Wed, 13 Jan 2021 16:41:27 -0500 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 1/11/21 2:17 PM, Halil Pasic wrote: > > On Tue, 22 Dec 2020 20:15:56 -0500 > > Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> > > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > > [..] > >> + > >> 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. > > I was asking out of curiosity, not because I want it changed. I was also wondering if somebody could see a partially initialized device: we even first call dev_set_drvdata() and only then finish the initialization. Before 's390/vfio-ap: use new AP bus interface to search for queue devices', which is the previous patch, we had the klist code in between, which uses spinlocks, which I think ensure, that all effects of probe are seen when we get the queue from vfio_ap_find_queue(). But with patch 4 in place that is not the case any more. Or am I wrong? Regards, Halil
On 1/13/21 9:50 PM, Halil Pasic wrote: > On Wed, 13 Jan 2021 16:41:27 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 1/11/21 2:17 PM, Halil Pasic wrote: >>> On Tue, 22 Dec 2020 20:15:56 -0500 >>> Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> >>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> >>> > [..] > >>>> + >>>> 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. >> >> > I was asking out of curiosity, not because I want it changed. I was > also wondering if somebody could see a partially initialized device: > we even first call dev_set_drvdata() and only then finish the > initialization. Before 's390/vfio-ap: use new AP bus interface to search > for queue devices', which is the previous patch, we had the klist code > in between, which uses spinlocks, which I think ensure, that all > effects of probe are seen when we get the queue from > vfio_ap_find_queue(). But with patch 4 in place that is not the case any > more. Or am I wrong? You are correct insofar as patch 4 replaces the driver_find_device() function call with a call to AP bus's ap_get_qdev() function which does not use spinlocks. Without digging deeply into the probe call chain I do not know whether or not the use of spinlocks by the klist code ensures all effects of the probe are seen when we get the queue from vfio_ap_find_queue(). What I'm sure about is that since both vfio_ap_find_queue() and the setting of the drvdata in the probe function are always done under the mdev lock, consistency should be maintained. What I did decide when thinking about your previous review comment is that we should probably initialize the vfio_ap_queue object before setting the drvdata, so I made that change. > > Regards, > Halil
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); + 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);
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@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_ops.c | 140 +++++++++++++++++++++----- drivers/s390/crypto/vfio_ap_private.h | 3 + 2 files changed, 117 insertions(+), 26 deletions(-)