On 04/17/2018 07:34 AM, Cornelia Huck wrote:
On Sun, 15 Apr 2018 17:22:12 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> wrote:
Introduces a new function to reset the crypto attributes for all
vcpus whether they are running or not. Each vcpu in KVM will
be removed from SIE prior to resetting the crypto attributes in its
SIE state description. After all vcpus have had their crypto attributes
reset the vcpus will be restored to SIE.
This function will be used in a later patch to set the ECA.28
bit in the SIE state description to enable interpretive execution of
AP instructions. It will also be incorporated into the
kvm_s390_vm_set_crypto(kvm) function to fix an issue whereby the crypto
key wrapping attributes could potentially get out of synch for running
vcpus.
So, this description leads me to think it would make sense to queue
this patch (fixing the key wrapping) independently of this series,
wouldn't it?
I considered that because I figured there might be objections, but
since separating them would create dependency issues I didn't see
any harm in including it here. I can remove this from the explanation
above and the code below and create a separate patch for the key
wrapping if you'd prefer.
Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>
---
arch/s390/kvm/kvm-s390.c | 19 +++++++++++++------
arch/s390/kvm/kvm-s390.h | 14 ++++++++++++++
2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 64c9862..d0c3518 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -791,11 +791,21 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu);
-static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
+void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm)
_reset_all() or _set_all()? Don't really care much, tbh.
Then why bring it up?:) I chose _reset_all because in both places from which
this is called, we are changing a crypto attribute value and are thus
resetting the crypto settings for all the vcpus.
{
- struct kvm_vcpu *vcpu;
int i;
+ struct kvm_vcpu *vcpu;
I'd avoid swapping the order of the declarations.
This was unintentional, I can revert it.
+
+ kvm_s390_vcpu_block_all(kvm);
+
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ kvm_s390_vcpu_crypto_setup(vcpu);
and
+ kvm_s390_vcpu_unblock_all(kvm);
+}
+
+static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
+{
if (!test_kvm_facility(kvm, 76))
return -EINVAL;
@@ -832,10 +842,7 @@ static int kvm_s390_vm_set_crypto(struct kvm *kvm, struct kvm_device_attr *attr)
return -ENXIO;
}
- kvm_for_each_vcpu(i, vcpu, kvm) {
- kvm_s390_vcpu_crypto_setup(vcpu);
- exit_sie(vcpu);
- }
+ kvm_s390_vcpu_crypto_reset_all(kvm);
mutex_unlock(&kvm->lock);
return 0;
}
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 1b5621f..76324b7 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -410,4 +410,18 @@ static inline int kvm_s390_use_sca_entries(void)
}
void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
struct mcck_volatile_info *mcck_info);
+
+/**
+ * kvm_s390_vcpu_crypto_reset_all
+ *
+ * Reset the crypto attributes for each vcpu. This can be done while the vcpus
+ * are running as each vcpu will be removed from SIE before resetting the crypto
+ * attributes and restored to SIE afterward.
+ *
+ * Note: The kvm->lock mutex must be locked prior to calling this function and
+ * unlocked after it returns.
"Must be called with kvm->lock held"?
Yes. The kvm->lock must be held to set the crypto attributes that will be
copied to the vcpus via the kvm_s390_vcpu_crypto_reset_all() function,
so it made sense to hold the lock across the entire operation.
+ *
+ * @kvm: the KVM guest
+ */
+void kvm_s390_vcpu_crypto_reset_all(struct kvm *kvm);
#endif
Other than the nits above, looks good to me.
Great!
