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

[Tony Krowiak <akrowiak@linux.vnet.ibm.com>]

On 03/16/2018 03:51 AM, Pierre Morel wrote:
> On 16/03/2018 00:39, Tony Krowiak wrote:
>> On 03/15/2018 01:56 PM, Pierre Morel wrote:
>>> On 15/03/2018 18:21, Tony Krowiak wrote:
>>>> On 03/15/2018 11:45 AM, Pierre Morel wrote:
>>>>> On 15/03/2018 16:26, Tony Krowiak wrote:
>>>>>> On 03/15/2018 09:00 AM, Pierre Morel wrote:
>>>>>>> On 14/03/2018 22:57, 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@linux.vnet.ibm.com>
>>>>>>>>> ---
>>>>>>>> [..]
>>>>>>>>
>>>>>>>>> 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?
>>>>>>>>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>>> +                return -EOPNOTSUPP;
>>>>>>>>> +            kvm->arch.crypto.apie = 1;
>>>>>>>>> +            VM_EVENT(kvm, 3, "%s",
>>>>>>>>> +                 "ENABLE: AP interpretive execution");
>>>>>>>>> +        } else {
>>>>>>>>> +            kvm->arch.crypto.apie = 0;
>>>>>>>>> +            VM_EVENT(kvm, 3, "%s",
>>>>>>>>> +                 "DISABLE: AP interpretive execution");
>>>>>>>>> +        }
>>>>>>>>> +        break;
>>>>>>>>>       default:
>>>>>>>>>           mutex_unlock(&kvm->lock);
>>>>>>>>>           return -ENXIO;
>>>>>>>> I wonder how the loop after this switch works for 
>>>>>>>> KVM_S390_VM_CRYPTO_INTERPRET_AP:
>>>>>>>>
>>>>>>>>          kvm_for_each_vcpu(i, vcpu, kvm) {
>>>>>>>>                  kvm_s390_vcpu_crypto_setup(vcpu);
>>>>>>>>                  exit_sie(vcpu);
>>>>>>>>          }
>>>>>>>>
>>>>>>>>  From not doing something like for KVM_S390_VM_CRYPTO_INTERPRET_AP
>>>>>>>>
>>>>>>>>          if (kvm->created_vcpus) {
>>>>>>>>                  mutex_unlock(&kvm->lock);
>>>>>>>>                  return -EBUSY;
>>>>>>>> and from the aforementioned loop I guess ECA.28 can be changed
>>>>>>>> for a running guest.
>>>>>>>>
>>>>>>>> If there are running vcpus when KVM_S390_VM_CRYPTO_INTERPRET_AP is
>>>>>>>> changed (set) these will be taken out of SIE by exit_sie(). 
>>>>>>>> Then for the
>>>>>>>> corresponding threads the control probably goes to QEMU (the 
>>>>>>>> emulator in
>>>>>>>> the userspace). And it puts that vcpu back into the SIE, and 
>>>>>>>> then that
>>>>>>>> cpu starts acting according to the new ECA.28 value. While 
>>>>>>>> other vcpus
>>>>>>>> may still work with the old value of ECA.28.
>>>>>>>>
>>>>>>>> I'm not saying what I describe above is necessarily something 
>>>>>>>> broken.
>>>>>>>> But I would like to have it explained, why is it OK -- provided 
>>>>>>>> I did not
>>>>>>>> make any errors in my reasoning (assumptions included).
>>>>>>>>
>>>>>>>> Can you help me understand this code?
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Halil
>>>>>>>>
>>>>>>>> [..]
>>>>>>>>
>>>>>>>
>>>>>>> I have the same concerns as Halil.
>>>>>>>
>>>>>>> We do not need to change the virtulization type
>>>>>>> (hardware/software) on the fly for the current use case.
>>>>>>>
>>>>>>> Couldn't we delay this until we have one and in between only 
>>>>>>> make the vCPU hotplug clean?
>>>>>>>
>>>>>>> We only need to let the door open for the day we have such a use 
>>>>>>> case.
>>>>>> Are you suggesting this code be removed? If so, then where and 
>>>>>> under what conditions would
>>>>>> you suggest setting ECA.28 given you objected to setting it based 
>>>>>> on whether the
>>>>>> AP feature is installed?
>>>>>
>>>>> I would only call kvm_s390_vcpu_crypto_setup() from inside 
>>>>> kvm_arch_vcpu_init()
>>>>> as it is already.
>>>> It is not called from kvm_arch_vcpu_init(), it is called from 
>>>> kvm_arch_vcpu_setup(). 
>>>
>>> hum, sorry for this.
>>> However, the idea pertains, not to call this function from inside an 
>>> ioctl changing crypto parameters, but only during vcpu creation.
>> Unfortunately, the ioctl does not get called until after the vcpus 
>> are created (see my comments below)
>
> That is why I think you should not change the ECA field from the 
> crypto ioctl but only during the vcpu initialization phase.
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:

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

         kvm_for_each_vcpu(i, vcpu, kvm) {
             if (enable)
                 vcpu->arch.sie_block->eca |= ECA_APIE;
             else
                 vcpu->arch.sie_block->eca &= ~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.
>
>
>>>
>>>
>>>
>>>> Also,
>>>> this loop was already here, I did not put it in. Assuming whomever 
>>>> put it there did so
>>>> for a reason, it is not my place to remove it. According to a trace 
>>>> I ran, the calls to this
>>>> function occur after the vcpus are created. Consequently, the 
>>>> kvm_s390_vcpu_crypto_setup()
>>>> function would not be called without the loop and neither the key 
>>>> wrapping support nor the
>>>> ECA_APIE would be configured in the vcpu's SIE descriptor.
>>>>
>>>> If you have a better idea for where/how to set this flag, I'm all
>>>> ears. It would be nice if it could be set before the vcpus are 
>>>> created, but I haven't
>>>> found a good candidate. I suspect that the loop was put in to make 
>>>> sure that all vcpus
>>>> get updated regardless of whether they are running or not, but I 
>>>> don't know what happens
>>>> after a vcpu is kicked out of SIE. I suspect, as Halil surmised, 
>>>> that QEMU
>>>> restores the vcpus to SIE. This would seemingly cause the 
>>>> kvm_arch_vcpu_setup() to get
>>>> called at which time the ECA_APIE value as well as the key wrapping 
>>>> values will get set.
>>>> If somebody has knowledge of the flow here, please feel free to 
>>>> pitch in.
>>>>>
>>>>>
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Pierre
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>