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

[Tony Krowiak]

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
proposal.

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))
             return -EOPNOTSUPP;

         mutex_lock(&kvm->lock);

         kvm_s390_vcpu_block_all(kvm);

... let's say here.

Yep

         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;
             else
                 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.

         }

         kvm_s390_vcpu_unblock_all(kvm);

         mutex_unlock(&kvm->lock);

         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.