| Message ID | 20220404221039.1272245-11-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 functions backing the matrix mdev's sysfs attribute interfaces to > assign/unassign adapters, domains and control domains 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 > > Two new macros are introduced for this purpose: One to take the locks and > the other to release the locks. These macros will be used by the > assignment/unassignment functions to prepare for dynamic update of > the KVM guest's APCB. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_ops.c | 69 +++++++++++++++++++++++++------ > 1 file changed, 57 insertions(+), 12 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 757bbf449b04..2219b1069ceb 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -71,6 +71,51 @@ static const struct vfio_device_ops vfio_ap_matrix_dev_ops; > mutex_unlock(&matrix_dev->guests_lock); \ > }) > > +/** > + * get_update_locks_for_mdev: Acquire the locks required to dynamically update a > + * KVM guest's APCB in the proper order. > + * > + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP > + * configuration data to use to update a KVM guest's APCB. > + * > + * 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 @matrix_mdev is NULL or is not attached to a KVM guest, the KVM > + * lock will not be taken. > + */ Perhaps the locking order should be documented once at the top of all of the locking functions instead of in each comment. The current method seems needlessly verbose. > +#define get_update_locks_for_mdev(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); \ > +}) It does not make sense to reference matrix_dev on the first line of this macro and then check it for a null value on the next line. If it can be null then the check needs to come before the usage. If it cannot be null, then we can remove the check. Same comment for the release macro. > +/** > + * release_update_locks_for_mdev: Release the locks used to dynamically update a > + * KVM guest's APCB in the proper order. > + * > + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP > + * configuration data to use to update a KVM guest's APCB. > + * > + * The proper unlocking order is: > + * 1. matrix_dev->mdevs_lock > + * 2. matrix_mdev->kvm->lock > + * 3. matrix_dev->guests_lock > + * > + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM > + * lock will not be released. > + */ > +#define release_update_locks_for_mdev(matrix_mdev) ({ \ > + mutex_unlock(&matrix_dev->mdevs_lock); \ > + if (matrix_mdev && matrix_mdev->kvm) \ > + mutex_unlock(&matrix_mdev->kvm->lock); \ > + mutex_unlock(&matrix_dev->guests_lock); \ > +}) > +
On 5/27/22 9:18 AM, Jason J. Herne wrote: > On 4/4/22 18:10, Tony Krowiak wrote: >> The functions backing the matrix mdev's sysfs attribute interfaces to >> assign/unassign adapters, domains and control domains 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 >> >> Two new macros are introduced for this purpose: One to take the locks >> and >> the other to release the locks. These macros will be used by the >> assignment/unassignment functions to prepare for dynamic update of >> the KVM guest's APCB. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 69 +++++++++++++++++++++++++------ >> 1 file changed, 57 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >> b/drivers/s390/crypto/vfio_ap_ops.c >> index 757bbf449b04..2219b1069ceb 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -71,6 +71,51 @@ static const struct vfio_device_ops >> vfio_ap_matrix_dev_ops; >> mutex_unlock(&matrix_dev->guests_lock); \ >> }) >> +/** >> + * get_update_locks_for_mdev: Acquire the locks required to >> dynamically update a >> + * KVM guest's APCB in the proper order. >> + * >> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object >> containing the AP >> + * configuration data to use to update a KVM guest's APCB. >> + * >> + * 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 @matrix_mdev is NULL or is not attached to a KVM guest, >> the KVM >> + * lock will not be taken. >> + */ > > Perhaps the locking order should be documented once at the top of all > of the locking > functions instead of in each comment. The current method seems > needlessly verbose. Perhaps, but I surmise this comment was motivated by the fact you are reviewing the locking macros/functions en masse. On the other hand, someone debugging the code may miss the locking order comments if their debug thread leads them to a locking macro/function that does not have said comments. I think the value of leaving the comments in place outweighs the value of limiting them as you suggested. > >> +#define get_update_locks_for_mdev(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); \ >> +}) > > It does not make sense to reference matrix_dev on the first line of > this macro and > then check it for a null value on the next line. If it can be null > then the check > needs to come before the usage. If it cannot be null, then we can > remove the check. > Same comment for the release macro. You must have misread the code. The second line checks the value of matrix_mdev for NULL, not matrix_dev. There are definitely cases where matrix_mdev can be passed as NULL. > >> +/** >> + * release_update_locks_for_mdev: Release the locks used to >> dynamically update a >> + * KVM guest's APCB in the proper order. >> + * >> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object >> containing the AP >> + * configuration data to use to update a KVM guest's APCB. >> + * >> + * The proper unlocking order is: >> + * 1. matrix_dev->mdevs_lock >> + * 2. matrix_mdev->kvm->lock >> + * 3. matrix_dev->guests_lock >> + * >> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, >> the KVM >> + * lock will not be released. >> + */ >> +#define release_update_locks_for_mdev(matrix_mdev) ({ \ >> + mutex_unlock(&matrix_dev->mdevs_lock); \ >> + if (matrix_mdev && matrix_mdev->kvm) \ >> + mutex_unlock(&matrix_mdev->kvm->lock); \ >> + mutex_unlock(&matrix_dev->guests_lock); \ >> +}) >> +
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 757bbf449b04..2219b1069ceb 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -71,6 +71,51 @@ static const struct vfio_device_ops vfio_ap_matrix_dev_ops; mutex_unlock(&matrix_dev->guests_lock); \ }) +/** + * get_update_locks_for_mdev: Acquire the locks required to dynamically update a + * KVM guest's APCB in the proper order. + * + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP + * configuration data to use to update a KVM guest's APCB. + * + * 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 @matrix_mdev is NULL or is not attached to a KVM guest, the KVM + * lock will not be taken. + */ +#define get_update_locks_for_mdev(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); \ +}) + +/** + * release_update_locks_for_mdev: Release the locks used to dynamically update a + * KVM guest's APCB in the proper order. + * + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP + * configuration data to use to update a KVM guest's APCB. + * + * The proper unlocking order is: + * 1. matrix_dev->mdevs_lock + * 2. matrix_mdev->kvm->lock + * 3. matrix_dev->guests_lock + * + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM + * lock will not be released. + */ +#define release_update_locks_for_mdev(matrix_mdev) ({ \ + mutex_unlock(&matrix_dev->mdevs_lock); \ + if (matrix_mdev && matrix_mdev->kvm) \ + mutex_unlock(&matrix_mdev->kvm->lock); \ + mutex_unlock(&matrix_dev->guests_lock); \ +}) + /** * vfio_ap_mdev_get_queue - retrieve a queue with a specific APQN from a * hash table of queues assigned to a matrix mdev @@ -827,7 +872,7 @@ static ssize_t assign_adapter_store(struct device *dev, struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); - mutex_lock(&matrix_dev->mdevs_lock); + get_update_locks_for_mdev(matrix_mdev); /* If the KVM guest is running, disallow assignment of adapter */ if (matrix_mdev->kvm) { @@ -859,7 +904,7 @@ static ssize_t assign_adapter_store(struct device *dev, matrix_mdev->matrix.aqm, matrix_mdev); ret = count; done: - mutex_unlock(&matrix_dev->mdevs_lock); + release_update_locks_for_mdev(matrix_mdev); return ret; } @@ -902,7 +947,7 @@ static ssize_t unassign_adapter_store(struct device *dev, unsigned long apid; struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); - mutex_lock(&matrix_dev->mdevs_lock); + get_update_locks_for_mdev(matrix_mdev); /* If the KVM guest is running, disallow unassignment of adapter */ if (matrix_mdev->kvm) { @@ -927,7 +972,7 @@ static ssize_t unassign_adapter_store(struct device *dev, ret = count; done: - mutex_unlock(&matrix_dev->mdevs_lock); + release_update_locks_for_mdev(matrix_mdev); return ret; } static DEVICE_ATTR_WO(unassign_adapter); @@ -982,7 +1027,7 @@ static ssize_t assign_domain_store(struct device *dev, struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); unsigned long max_apqi = matrix_mdev->matrix.aqm_max; - mutex_lock(&matrix_dev->mdevs_lock); + get_update_locks_for_mdev(matrix_mdev); /* If the KVM guest is running, disallow assignment of domain */ if (matrix_mdev->kvm) { @@ -1013,7 +1058,7 @@ static ssize_t assign_domain_store(struct device *dev, matrix_mdev); ret = count; done: - mutex_unlock(&matrix_dev->mdevs_lock); + release_update_locks_for_mdev(matrix_mdev); return ret; } @@ -1056,7 +1101,7 @@ static ssize_t unassign_domain_store(struct device *dev, unsigned long apqi; struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); - mutex_lock(&matrix_dev->mdevs_lock); + get_update_locks_for_mdev(matrix_mdev); /* If the KVM guest is running, disallow unassignment of domain */ if (matrix_mdev->kvm) { @@ -1082,7 +1127,7 @@ static ssize_t unassign_domain_store(struct device *dev, ret = count; done: - mutex_unlock(&matrix_dev->mdevs_lock); + release_update_locks_for_mdev(matrix_mdev); return ret; } static DEVICE_ATTR_WO(unassign_domain); @@ -1109,7 +1154,7 @@ static ssize_t assign_control_domain_store(struct device *dev, unsigned long id; struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); - mutex_lock(&matrix_dev->mdevs_lock); + get_update_locks_for_mdev(matrix_mdev); /* If the KVM guest is running, disallow assignment of control domain */ if (matrix_mdev->kvm) { @@ -1135,7 +1180,7 @@ static ssize_t assign_control_domain_store(struct device *dev, vfio_ap_mdev_filter_cdoms(matrix_mdev); ret = count; done: - mutex_unlock(&matrix_dev->mdevs_lock); + release_update_locks_for_mdev(matrix_mdev); return ret; } static DEVICE_ATTR_WO(assign_control_domain); @@ -1163,7 +1208,7 @@ static ssize_t unassign_control_domain_store(struct device *dev, struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev); unsigned long max_domid = matrix_mdev->matrix.adm_max; - mutex_lock(&matrix_dev->mdevs_lock); + get_update_locks_for_mdev(matrix_mdev); /* If a KVM guest is running, disallow unassignment of control domain */ if (matrix_mdev->kvm) { @@ -1186,7 +1231,7 @@ static ssize_t unassign_control_domain_store(struct device *dev, ret = count; done: - mutex_unlock(&matrix_dev->mdevs_lock); + release_update_locks_for_mdev(matrix_mdev); return ret; } static DEVICE_ATTR_WO(unassign_control_domain);
The functions backing the matrix mdev's sysfs attribute interfaces to assign/unassign adapters, domains and control domains 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 Two new macros are introduced for this purpose: One to take the locks and the other to release the locks. These macros will be used by the assignment/unassignment functions to prepare for dynamic update of the KVM guest's APCB. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_ops.c | 69 +++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 12 deletions(-)