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

From: Tony Krowiak
Date: Mon Apr 02 2018 - 14:56:14 EST

On 03/20/2018 06:48 PM, Halil Pasic wrote:

On 03/20/2018 06:58 PM, Tony Krowiak wrote:
I spoke with Christian this morning and he made a suggestion which I think would provide the best solution here.
This is my proposal:
1. Get rid of the KVM_S390_VM_CRYPTO_INTERPRET_AP device attribute and return to setting ECA.28 from the
mdev device open callback.
2. Since there may be vcpus online at the time the mdev device open is called, we must first take all running vcpus out of
SIE and block them. Christian suggested the kvm_s390_vcpu_block_all(struct kvm *kvm) function will do the trick. So I
propose introducing a function like the following to be called during mdev open:
There is one thing you missed, otherwise I'm *very* satisfied with this

What you have missed IMHO is vcpu hottplug. So IMHO you should keep
kvm->arch.crypto.apie, and update it accordingly ...
I agree, I will fix it.

int kvm_ap_set_interpretive_exec(struct kvm *kvm, bool enable)
int i;
struct kvm_vcpu *vcpu;

if (!test_kvm_cpu_feat(kvm, KVM_S390_VM_CPU_FEAT_AP))


... let's say here.

kvm_for_each_vcpu(i, vcpu, kvm) {
And here you can call kvm_s390_vcpu_crypto_setup(vcpu) (the changes to
this function will be required for hotplug) if you like
Sounds good to me.

if (enable)
vcpu->arch.sie_block->eca |= ECA_APIE;
vcpu->arch.sie_block->eca &= ~ECA_APIE;
or keep this stuff, it does not really matter to me.
I'll call the kvm_s390_vcpu_crypto_setup(vcpu) to set ECA_APIE.




return 0;

This interface allows us to set ECA.28 even if vcpus are running
I tend to agree. I will give it a proper review when this gets more
formal (e.g. v4 (preferably) or patches to be fixed up to this series).

Please don't forget to revisit the discussion on kvm_s390_vm_set_crypto:
if the mechanism there isn't right for ECA.28 I think you should tell
us why it's OK for the other attributes if it's OK. If it is not then
I guess you will want to do a stand alone patch for that.
That will no longer be a part of this patch series. We can revisit that as
a separate issue at a future time.