Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification

[Jason J. Herne]

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.

> + * @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 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 :).

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.


--
-- Jason J. Herne (jjherne@xxxxxxxxxxxxx)