Messages in this thread | | | Date | Tue, 31 May 2022 08:02:41 -0400 | Subject | Re: [PATCH v19 11/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove | From | Tony Krowiak <> |
| |
On 5/27/22 9:50 AM, Jason J. Herne wrote: > 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);
Yikes, I see another flaw in this macro! Either the input parameter needs to be renamed to 'q' or the q->matrix_mdev needs to be changed to queue->matrix_mdev. I think I'll go with the former since vfio_ap_queue is referred to as 'q' everywhere else.
> > >> /** >> * 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); >> } > >
| |