Messages in this thread | | | Date | Fri, 27 May 2022 09:50:50 -0400 | From | "Jason J. Herne" <> | Subject | Re: [PATCH v19 11/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove |
| |
On 4/4/22 18:10, Tony Krowiak wrote: > The callback functions for probing and removing a queue device must take > and release the locks required to perform a dynamic update of a guest's > APCB in the proper order. > > The proper order for taking the locks is: > > matrix_dev->guests_lock => kvm->lock => matrix_dev->mdevs_lock > > The proper order for releasing the locks is: > > matrix_dev->mdevs_lock => kvm->lock => matrix_dev->guests_lock > > A new helper function is introduced to be used by the probe callback to > acquire the required locks. Since the probe callback only has > access to a queue device when it is called, the helper function will find > the ap_matrix_mdev object to which the queue device's APQN is assigned and > return it so the KVM guest to which the mdev is attached can be dynamically > updated. > > Note that in order to find the ap_matrix_mdev (matrix_mdev) object, it is > necessary to search the matrix_dev->mdev_list. This presents a > locking order dilemma because the matrix_dev->mdevs_lock can't be taken to > protect against changes to the list while searching for the matrix_mdev to > which a queue device's APQN is assigned. This is due to the fact that the > proper locking order requires that the matrix_dev->mdevs_lock be taken > after both the matrix_mdev->kvm->lock and the matrix_dev->mdevs_lock. > Consequently, the matrix_dev->guests_lock will be used to protect against > removal of a matrix_mdev object from the list while a queue device is > being probed. This necessitates changes to the mdev probe/remove > callback functions to take the matrix_dev->guests_lock prior to removing > a matrix_mdev object from the list. > > A new macro is also introduced to acquire the locks required to dynamically > update the guest's APCB in the proper order when a queue device is > removed. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_ops.c | 126 +++++++++++++++++++++--------- > 1 file changed, 88 insertions(+), 38 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 2219b1069ceb..080a733f7cd2 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -116,6 +116,74 @@ static const struct vfio_device_ops vfio_ap_matrix_dev_ops; > mutex_unlock(&matrix_dev->guests_lock); \ > }) > > +/** > + * vfio_ap_mdev_get_update_locks_for_apqn: retrieve the matrix mdev to which an > + * APQN is assigned and acquire the > + * locks required to update the APCB of > + * the KVM guest to which the mdev is > + * attached. > + * > + * @apqn: the APQN of a queue device. > + * > + * The proper locking order is: > + * 1. matrix_dev->guests_lock: required to use the KVM pointer to update a KVM > + * guest's APCB. > + * 2. matrix_mdev->kvm->lock: required to update a guest's APCB > + * 3. matrix_dev->mdevs_lock: required to access data stored in a matrix_mdev > + * > + * Note: If @apqn is not assigned to a matrix_mdev, the matrix_mdev->kvm->lock > + * will not be taken. > + * > + * Return: the ap_matrix_mdev object to which @apqn is assigned or NULL if @apqn > + * is not assigned to an ap_matrix_mdev. > + */ > +static struct ap_matrix_mdev *vfio_ap_mdev_get_update_locks_for_apqn(int apqn) > +{ > + struct ap_matrix_mdev *matrix_mdev; > + > + mutex_lock(&matrix_dev->guests_lock); > + > + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) { > + if (test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm) && > + test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm)) { > + if (matrix_mdev->kvm) > + mutex_lock(&matrix_mdev->kvm->lock); > + > + mutex_lock(&matrix_dev->mdevs_lock); > + > + return matrix_mdev; > + } > + } > + > + mutex_lock(&matrix_dev->mdevs_lock); > + > + return NULL; > +} > + > +/** > + * get_update_locks_for_queue: get the locks required to update the APCB of the > + * KVM guest to which the matrix mdev linked to a > + * vfio_ap_queue object is attached. > + * > + * @queue: a pointer to a vfio_ap_queue object. > + * > + * The proper locking order is: > + * 1. matrix_dev->guests_lock: required to use the KVM pointer to update a KVM > + * guest's APCB. > + * 2. queue->matrix_mdev->kvm->lock: required to update a guest's APCB > + * 3. matrix_dev->mdevs_lock: required to access data stored in a matrix_mdev > + * > + * Note: if @queue is not linked to an ap_matrix_mdev object, the KVM lock > + * will not be taken. > + */ > +#define get_update_locks_for_queue(queue) ({ \ > + struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev; \ > + mutex_lock(&matrix_dev->guests_lock); \ > + if (matrix_mdev && matrix_mdev->kvm) \ > + mutex_lock(&matrix_mdev->kvm->lock); \ > + mutex_lock(&matrix_dev->mdevs_lock); \ > +}) > +
One more comment I forgot to include before: This macro is far too similar to existing macro, get_update_locks_for_mdev. And it is only called in one place. Let's remove this and replace the single invocation with:
get_update_locks_for_mdev(q->matrix_mdev);
> /** > * vfio_ap_mdev_get_queue - retrieve a queue with a specific APQN from a > * hash table of queues assigned to a matrix mdev > @@ -615,21 +683,18 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev) > matrix_mdev->pqap_hook = handle_pqap; > vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->shadow_apcb); > hash_init(matrix_mdev->qtable.queues); > - mdev_set_drvdata(mdev, matrix_mdev); > - mutex_lock(&matrix_dev->mdevs_lock); > - list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > - mutex_unlock(&matrix_dev->mdevs_lock); > > ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev); > if (ret) > goto err_list; > + mdev_set_drvdata(mdev, matrix_mdev); > + mutex_lock(&matrix_dev->mdevs_lock); > + list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > + mutex_unlock(&matrix_dev->mdevs_lock); > dev_set_drvdata(&mdev->dev, matrix_mdev); > return 0; > > err_list: > - mutex_lock(&matrix_dev->mdevs_lock); > - list_del(&matrix_mdev->node); > - mutex_unlock(&matrix_dev->mdevs_lock); > vfio_uninit_group_dev(&matrix_mdev->vdev); > kfree(matrix_mdev); > err_dec_available: > @@ -692,11 +757,13 @@ static void vfio_ap_mdev_remove(struct mdev_device *mdev) > > vfio_unregister_group_dev(&matrix_mdev->vdev); > > + mutex_lock(&matrix_dev->guests_lock); > mutex_lock(&matrix_dev->mdevs_lock); > vfio_ap_mdev_reset_queues(matrix_mdev); > vfio_ap_mdev_unlink_fr_queues(matrix_mdev); > list_del(&matrix_mdev->node); > mutex_unlock(&matrix_dev->mdevs_lock); > + mutex_unlock(&matrix_dev->guests_lock); > vfio_uninit_group_dev(&matrix_mdev->vdev); > kfree(matrix_mdev); > atomic_inc(&matrix_dev->available_instances); > @@ -1665,49 +1732,30 @@ void vfio_ap_mdev_unregister(void) > mdev_unregister_driver(&vfio_ap_matrix_driver); > } > > -/* > - * 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; > + struct ap_matrix_mdev *matrix_mdev; > DECLARE_BITMAP(apm_delta, AP_DEVICES); > > q = kzalloc(sizeof(*q), GFP_KERNEL); > if (!q) > return -ENOMEM; > - mutex_lock(&matrix_dev->mdevs_lock); > q->apqn = to_ap_queue(&apdev->device)->qid; > q->saved_isc = VFIO_AP_ISC_INVALID; > - vfio_ap_queue_link_mdev(q); > - if (q->matrix_mdev) { > + > + matrix_mdev = vfio_ap_mdev_get_update_locks_for_apqn(q->apqn); > + > + if (matrix_mdev) { > + vfio_ap_mdev_link_queue(matrix_mdev, q); > memset(apm_delta, 0, sizeof(apm_delta)); > set_bit_inv(AP_QID_CARD(q->apqn), apm_delta); > vfio_ap_mdev_filter_matrix(apm_delta, > - q->matrix_mdev->matrix.aqm, > - q->matrix_mdev); > + matrix_mdev->matrix.aqm, > + matrix_mdev); > } > dev_set_drvdata(&apdev->device, q); > - mutex_unlock(&matrix_dev->mdevs_lock); > + release_update_locks_for_mdev(matrix_mdev); > > return 0; > } > @@ -1716,11 +1764,13 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev) > { > unsigned long apid; > struct vfio_ap_queue *q; > + struct ap_matrix_mdev *matrix_mdev; > > - mutex_lock(&matrix_dev->mdevs_lock); > q = dev_get_drvdata(&apdev->device); > + get_update_locks_for_queue(q); > + matrix_mdev = q->matrix_mdev; > > - if (q->matrix_mdev) { > + if (matrix_mdev) { > vfio_ap_unlink_queue_fr_mdev(q); > > apid = AP_QID_CARD(q->apqn); > @@ -1731,5 +1781,5 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev) > vfio_ap_mdev_reset_queue(q, 1); > dev_set_drvdata(&apdev->device, NULL); > kfree(q); > - mutex_unlock(&matrix_dev->mdevs_lock); > + release_update_locks_for_mdev(matrix_mdev); > }
-- -- Jason J. Herne (jjherne@linux.ibm.com)
| |