Re: [PATCH v3 04/14] KVM: s390: device attribute to set AP interpretive execution

From: Tony Krowiak
Date: Thu Mar 15 2018 - 19:38:07 EST


On 03/15/2018 12:00 PM, Pierre Morel wrote:
On 15/03/2018 16:23, Tony Krowiak wrote:
On 03/14/2018 05:57 PM, Halil Pasic wrote:

On 03/14/2018 07:25 PM, Tony Krowiak wrote:
The VFIO AP device model exploits interpretive execution of AP
instructions (APIE) to provide guests passthrough access to AP
devices. This patch introduces a new device attribute in the
KVM_S390_VM_CRYPTO device attribute group to set APIE from
the VFIO AP device defined on the guest.

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>
---
[..]

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index a60c45b..bc46b67 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -815,6 +815,19 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
VM_EVENT(kvm, 3, "%s", "DISABLE: DEA keywrapping support");
break;
+ case KVM_S390_VM_CRYPTO_INTERPRET_AP:
+ if (attr->addr) {
+ if (!test_kvm_cpu_feat(kvm, KVM_S390_VM_CPU_FEAT_AP))
Unlock mutex before returning?
The mutex is unlocked prior to return at the end of the function.

Maybe flip conditions (don't allow manipulating apie if feature not there).
Clearing the anyways clear apie if feature not there ain't too bad, but
rejecting the operation appears nicer to me.
I think what you're saying is something like this:

if (!test_kvm_cpu_feat(kvm, KVM_S390_VM_CPU_FEAT_AP))
return -EOPNOTSUPP;

kvm->arch.crypto.apie = (attr->addr) ? 1 : 0;

I can make arguments for doing this either way, but since the attribute
is will most likely only be set by an AP device in userspace, I suppose
it makes sense to allow setting of the attribute if the AP feature is
installed. It certainly makes sense for the dedicated implementation.

+ return -EOPNOTSUPP;

Obviously Halil is speaking on this return statement.
Which returns without unlocking the mutex.
Got it.