| Message ID | 20220404221039.1272245-12-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
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) vfio_ap_mdev_get_update_locks_for_apqn is "crazy long". How about: get_mdev_for_apqn() This function is static and the terms mdev and apqn are specific enough that I don't think it needs to start with vfio_ap. And there is no need to state in the function name that locks are acquired. That point will be obvious to anyone reading the prologue or the code. Aside from that, Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
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); > }
On 5/27/22 9:36 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) > > vfio_ap_mdev_get_update_locks_for_apqn is "crazy long". > How about: > get_mdev_for_apqn() > > This function is static and the terms mdev and apqn are specific > enough that I > don't think it needs to start with vfio_ap. And there is no need to > state in > the function name that locks are acquired. That point will be obvious > to anyone > reading the prologue or the code. The primary purpose of the function is to acquire the locks in the proper order, so I think the name should state that purpose. It may be obvious to someone reading the prologue or this function, but not so obvious in the context of the calling function. Having said that, I will shorten the name to: get_update_locks_for_apqn > > Aside from that, Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com> >
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); We can't do that, but your comment does point out a flaw in this macro; namely, we must take the matrix_dev->guests_lock before attempting to access q->matrix_mdev. An ap_matrix_mdev can be unlinked from a vfio_ap_queue (and vice versa) when the queue is removed, but it also can be unlinked when an adapter or domain is unassigned from an ap_matrix_mdev. In order to ensure that the q->matrix_mdev is not in the process of being nullified (unlinked), we must be holding the guests_lock which is also held when an adapter or domain is unassigned. > > >> /** >> * 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); >> } > >
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); >> } > >
On Tue, 31 May 2022 06:44:46 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > vfio_ap_mdev_get_update_locks_for_apqn is "crazy long". > > How about: > > get_mdev_for_apqn() > > > > This function is static and the terms mdev and apqn are specific > > enough that I > > don't think it needs to start with vfio_ap. And there is no need to > > state in > > the function name that locks are acquired. That point will be obvious > > to anyone > > reading the prologue or the code. > > The primary purpose of the function is to acquire the locks in the > proper order, so > I think the name should state that purpose. It may be obvious to someone > reading > the prologue or this function, but not so obvious in the context of the > calling function. I agree with Tony. To me get_mdev_for_apqn() sounds like getting a reference to a matrix_mdev object (and incrementing its refcount) or something similar. BTW some more bike shedding: I prefer by_apqn instead of for_apqn, because the set of locks we need to take is determined _by_ the apqn parameter, but it ain't semantically the set of locks we need to perform an update operation on the apqn or on the queue associated with the apqn. No strong opinion though -- I'm no native speaker and prepositions are difficult for me. Regards, Halil
On 6/7/22 8:05 AM, Halil Pasic wrote: > On Tue, 31 May 2022 06:44:46 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>> vfio_ap_mdev_get_update_locks_for_apqn is "crazy long". >>> How about: >>> get_mdev_for_apqn() >>> >>> This function is static and the terms mdev and apqn are specific >>> enough that I >>> don't think it needs to start with vfio_ap. And there is no need to >>> state in >>> the function name that locks are acquired. That point will be obvious >>> to anyone >>> reading the prologue or the code. >> The primary purpose of the function is to acquire the locks in the >> proper order, so >> I think the name should state that purpose. It may be obvious to someone >> reading >> the prologue or this function, but not so obvious in the context of the >> calling function. > I agree with Tony. To me get_mdev_for_apqn() sounds like getting a > reference to a matrix_mdev object (and incrementing its refcount) or > something similar. BTW some more bike shedding: I prefer by_apqn instead > of for_apqn, because the set of locks we need to take is determined _by_ > the apqn parameter, but it ain't semantically the set of locks we need > to perform an update operation on the apqn or on the queue associated > with the apqn. No strong opinion though -- I'm no native speaker and > prepositions are difficult for me. I am a native speaker and I had to review prepositions. I learned grammar in elementary school (grades 1-6) and have forgotten much of the terminology as it relates to sentence structure. Anyway, I digress. I'm okay with 'by_apqn'. > > Regards, > Halil
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); \ +}) + /** * 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); }
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(-)