| Message ID | 1521051954-25715-5-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 [..]
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. Pierre
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@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? 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; >> + 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. Assuming the scenario plays out as you described, why would the other vcpus be using the old ECA.28 value if the kvm_s390_vcpu_crypto_setup() function is executed for each of them to set the new value for 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? Unless I am missing something in the scenario you described, it seems that the reason the exit_sie(vcpu) function is called is to ensure that the vcpus that are already running acquire the new attribute values changed by this function when they are restored to SIE. Of course, my assumption is that the kvm_arch_vcpu_setup() function - which calls the kvm_s390_vcpu_crypto_setup() function - is invoked when the vcpu is restored to SIE. > > Regards, > Halil > > [..]
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? > > > Pierre > > >
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. >> >> >> Pierre >> >> >> >
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@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? > 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.
On 03/15/2018 04:23 PM, 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@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? > The mutex is unlocked prior to return at the end of the function. Pierre already pointed out what I mean. >> >> 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. No strong opinion here. >> >>> + 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. > Assuming the scenario plays out as you described, why would the other vcpus > be using the old ECA.28 value if the kvm_s390_vcpu_crypto_setup() function > is executed for each of them to set the new value for ECA.28? I'm puzzled I though I just described that. The threads implementing the vcpus are, or at least may be concurrent to the thread doing the loop and kvm_s390_vcpu_crypto_setup() for each vcpu. Changing the ECA.28 for each vcpu in the configuration ain't likely to be simultaneous (we do the kvm_s390_vcpu_crypto_setup() in the loop), but even if it were simultaneous what would guarantee that the changes is observed as one atomic change (that is: no mix is observed by the guest)? (And please read the documentation.) >> >> 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? > Unless I am missing something in the scenario you described, it seems that > the reason the exit_sie(vcpu) function is called is to ensure that the vcpus > that are already running acquire the new attribute values changed by this > function when they are restored to SIE. Of course, my assumption is that > the kvm_arch_vcpu_setup() function - which calls the kvm_s390_vcpu_crypto_setup() > function - is invoked when the vcpu is restored to SIE. I don't know what are you talking about kvm_s390_vcpu_crypto_setup(vcpu) is invoked in the loop. That changes the State Description. How is it guaranteed that no vCPU is going to work according to the new ECA.28 value before *all* vCPUs are made out of SIE by exit_sie()? Your answers sadly didn't contribute much to my understanding. hope mine will be more successful in contributing to yours. Regards, Halil
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(). 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 >>> >>> >>> >> >
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. > 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 >>>> >>>> >>>> >>> >> >
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@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? >> 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. > > > >
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) > > > >> 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 >>>>> >>>>> >>>>> >>>> >>> >> >
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. >> >> >> >>> 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 >>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
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. By what means do you suggest we do that? > > >>> >>> >>> >>>> 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 >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
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 >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
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 ... > > 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. > > 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 > 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. > } > > 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.
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. >
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 98957c2..bbac5a1 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -699,6 +699,7 @@ struct kvm_s390_crypto { __u32 crycbd; __u8 aes_kw; __u8 dea_kw; + __u8 apie; }; #define APCB0_MASK_SIZE 1 diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h index a580dec..fdcbeb9 100644 --- a/arch/s390/include/uapi/asm/kvm.h +++ b/arch/s390/include/uapi/asm/kvm.h @@ -161,6 +161,7 @@ struct kvm_s390_vm_cpu_subfunc { #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW 1 #define KVM_S390_VM_CRYPTO_DISABLE_AES_KW 2 #define KVM_S390_VM_CRYPTO_DISABLE_DEA_KW 3 +#define KVM_S390_VM_CRYPTO_INTERPRET_AP 4 /* kvm attributes for migration mode */ #define KVM_S390_VM_MIGRATION_STOP 0 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)) + 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; @@ -1453,6 +1466,7 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr) case KVM_S390_VM_CRYPTO_ENABLE_DEA_KW: case KVM_S390_VM_CRYPTO_DISABLE_AES_KW: case KVM_S390_VM_CRYPTO_DISABLE_DEA_KW: + case KVM_S390_VM_CRYPTO_INTERPRET_AP: ret = 0; break; default: @@ -2409,6 +2423,11 @@ static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu) { vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd; + if (vcpu->kvm->arch.crypto.apie) + vcpu->arch.sie_block->eca |= ECA_APIE; + else + vcpu->arch.sie_block->eca &= ~ECA_APIE; + if (!test_kvm_facility(vcpu->kvm, 76)) return;
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> --- arch/s390/include/asm/kvm_host.h | 1 + arch/s390/include/uapi/asm/kvm.h | 1 + arch/s390/kvm/kvm-s390.c | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 0 deletions(-)