On 7/21/21 3:37 PM, Jason J. Herne wrote:
On 7/19/21 3:35 PM, Tony Krowiak wrote:
It was pointed out during an unrelated patch review that locks should
not
be open coded - i.e., writing the algorithm of a standard lock in a
function instead of using a lock from the standard library. The
setting and
testing of a busy flag and sleeping on a wait_event is the same thing
a lock does. The open coded locks are invisible to lockdep, so potential
locking problems are not detected.
This patch removes the open coded locks used during
VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag
and wait queue were introduced to resolve a possible circular locking
dependency reported by lockdep when starting a secure execution guest
configured with AP adapters and domains. Reversing the order in which
the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the
issue reported by lockdep, thus enabling the removal of the open coded
locks.
Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
---
arch/s390/kvm/kvm-s390.c | 27 +++++-
drivers/s390/crypto/vfio_ap_ops.c | 132 ++++++++------------------
drivers/s390/crypto/vfio_ap_private.h | 2 -
3 files changed, 63 insertions(+), 98 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index a08f242a9f27..4d2ef3a3286e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct
kvm *kvm)
kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
}
+/*
+ * kvm_arch_crypto_set_masks
+ *
+ * @kvm: a pointer to the object containing the crypto masks
This should probably say "a pointer to the target guest's KVM struct"
or something to that effect. Same comment for the comment above
kvm_arch_crypto_clear_masks.
Will do.
+ * @apm: the mask identifying the accessible AP adapters
+ * @aqm: the mask identifying the accessible AP domains
+ * @adm: the mask identifying the accessible AP control domains
+ *
+ * Set the masks that identify the adapters, domains and control
domains to
+ * which the KVM guest is granted access.
+ *
+ * Note: The kvm->lock mutex must be locked by the caller.
+ */
void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
unsigned long *aqm, unsigned long *adm)
{
struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
- mutex_lock(&kvm->lock);
kvm_s390_vcpu_block_all(kvm);
switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
@@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm
*kvm, unsigned long *apm,
/* recreate the shadow crycb for each vcpu */
kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
kvm_s390_vcpu_unblock_all(kvm);
- mutex_unlock(&kvm->lock);
}
EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
+/*
+ * kvm_arch_crypto_set_masks
Copy/paste error here. Rename to kvm_arch_crypto_CLEAR_masks
I will fix it.
I did not find anything else in my review. However, I'm still very new
to this code, so take that with a grain of salt :).
The grain of salt has been ingested.
Also, I could not apply this to master. If there is a next version do
you mind rebasing? Seeing the patch in full context would be helpful.
This was rebased on the latest master branch at the time then re-tested.
This is something I always
do prior to submitting patches, so is it possible you were using an
older version of master?
