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@xxxxxxxxxxxxx>
---
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); \
+})
+
