Messages in this thread | | | Subject | Re: [PATCH v4 05/15] KVM: s390: enable/disable AP interpretive execution | From | Tony Krowiak <> | Date | Thu, 19 Apr 2018 10:28:50 -0400 |
| |
On 04/18/2018 04:31 AM, Pierre Morel wrote: > On 17/04/2018 20:11, Tony Krowiak wrote: >> On 04/17/2018 12:55 PM, Pierre Morel wrote: >>> On 17/04/2018 18:22, Tony Krowiak wrote: >>>> On 04/17/2018 12:13 PM, Pierre Morel wrote: >>>>> On 17/04/2018 17:02, Tony Krowiak wrote: >>>>>> On 04/16/2018 06:51 AM, Pierre Morel wrote: >>>>>>> On 15/04/2018 23:22, 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 interface to enable and >>>>>>>> disable APIE. >>>>>>>> >>>>>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>>>>>> --- >>>>>>>> arch/s390/include/asm/kvm-ap.h | 16 ++++++++++++++++ >>>>>>>> arch/s390/include/asm/kvm_host.h | 1 + >>>>>>>> arch/s390/kvm/kvm-ap.c | 20 ++++++++++++++++++++ >>>>>>>> arch/s390/kvm/kvm-s390.c | 9 +++++++++ >>>>>>>> 4 files changed, 46 insertions(+), 0 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/s390/include/asm/kvm-ap.h >>>>>>>> b/arch/s390/include/asm/kvm-ap.h >>>>>>>> index 736e93e..a6c092e 100644 >>>>>>>> --- a/arch/s390/include/asm/kvm-ap.h >>>>>>>> +++ b/arch/s390/include/asm/kvm-ap.h >>>>>>>> @@ -35,4 +35,20 @@ >>>>>>>> */ >>>>>>>> void kvm_ap_build_crycbd(struct kvm *kvm); >>>>>>>> >>>>>>>> +/** >>>>>>>> + * kvm_ap_interpret_instructions >>>>>>>> + * >>>>>>>> + * Indicate whether AP instructions shall be interpreted. If >>>>>>>> they are not >>>>>>>> + * interpreted, all AP instructions will be intercepted and >>>>>>>> routed back to >>>>>>>> + * userspace. >>>>>>>> + * >>>>>>>> + * @kvm: the virtual machine attributes >>>>>>>> + * @enable: indicates whether AP instructions are to be >>>>>>>> interpreted (true) or >>>>>>>> + * or not (false). >>>>>>>> + * >>>>>>>> + * Returns 0 if completed successfully; otherwise, returns >>>>>>>> -EOPNOTSUPP >>>>>>>> + * indicating that AP instructions are not installed on the >>>>>>>> guest. >>>>>>>> + */ >>>>>>>> +int kvm_ap_interpret_instructions(struct kvm *kvm, bool enable); >>>>>>>> + >>>>>>>> #endif /* _ASM_KVM_AP */ >>>>>>>> diff --git a/arch/s390/include/asm/kvm_host.h >>>>>>>> b/arch/s390/include/asm/kvm_host.h >>>>>>>> index 3162783..5470685 100644 >>>>>>>> --- a/arch/s390/include/asm/kvm_host.h >>>>>>>> +++ b/arch/s390/include/asm/kvm_host.h >>>>>>>> @@ -715,6 +715,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/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c >>>>>>>> index 991bae4..55d11b5 100644 >>>>>>>> --- a/arch/s390/kvm/kvm-ap.c >>>>>>>> +++ b/arch/s390/kvm/kvm-ap.c >>>>>>>> @@ -58,3 +58,23 @@ void kvm_ap_build_crycbd(struct kvm *kvm) >>>>>>>> } >>>>>>>> } >>>>>>>> EXPORT_SYMBOL(kvm_ap_build_crycbd); >>>>>>>> + >>>>>>>> +int kvm_ap_interpret_instructions(struct kvm *kvm, bool enable) >>>>>>>> +{ >>>>>>>> + int ret = 0; >>>>>>>> + >>>>>>>> + mutex_lock(&kvm->lock); >>>>>>>> + >>>>>>>> + if (!test_kvm_cpu_feat(kvm, KVM_S390_VM_CPU_FEAT_AP)) { >>>>>>> >>>>>>> Do we really need to test CPU_FEAT_AP? >>>>>> >>>>>> Yes we do. >>>>> >>>>> really? why? >>>> >>>> The KVM_S390_VM_CPU_FEAT_AP will not be enabled by KVM if the AP >>>> instructions are not installed on the host. I assume - but have >>>> no way of verifying - that if the AP instructions are not installed >>>> on the host, that interpretation would fail. Do you know what would >>>> happen if AP instructions are interpreted when not installed on >>>> the host? >>> >>> If the host has no AP instructions (his ECA.28=0) but it set ECA.28 >>> for a guest, >>> there will be no AP instructions available in the guest. >> >> Then there's the answer to your question; this is why we to test >> CPU_FEAT_AP. > > We can postpone this discussion when we discuss on VSIE. > For this specific call I just wanted to point out that obviously this > function should not > be called if the guest has no AP instructions.
I disagree, at least as far as the way things are currently designed. Whether AP instructions are interpreted or not is determined by the vfio_ap device driver. The device driver should not be required to have "knowledge" about how a guest is configured in KVM which is why I encapsulated most of the AP guest configuration in kvm-ap.c. Besides, the function above allows setting of AP interpretation only if CPU_FEAT_AP is enabled.
Having said that, I may remove this function since - as you pointed out earlier - AP instructions are interpreted by default if CPU_FEAT_AP is enabled, so there will be no need to set this at this time.
> >> >>> >>> >>>> >>>>> >>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> I understand that KVM_S390_VM_CPU_FEAT_AP means AP instructions >>>>>>> are interpreted. >>>>>>> shouldn't we add this information in the name? >>>>>>> like KVM_S390_VM_CPU_FEAT_APIE >>>>>> >>>>>> KVM_S390_VM_CPU_FEAT_AP does NOT mean AP instructions are >>>>>> interpreted, it means >>>>>> AP instructions are installed. >>>>> >>>>> Right same error I made all along this review. >>>>> But AFAIK it means AP instructions are provided to the guest. >>>>> Then should this function be called if the guest has no AP >>>>> instructions ? >>>>> >>>>> >>>>>> >>>>>>> >>>>>>>> + ret = -EOPNOTSUPP; >>>>>>>> + goto done; >>>>>>>> + } >>>>>>>> + >>>>>>>> + kvm->arch.crypto.apie = enable; >>>>>>>> + kvm_s390_vcpu_crypto_reset_all(kvm); >>>>>>>> + >>>>>>>> +done: >>>>>>>> + mutex_unlock(&kvm->lock); >>>>>>>> + return ret; >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL(kvm_ap_interpret_instructions); >>>>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>>>>> index 55cd897..1dc8566 100644 >>>>>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>>>>> @@ -1901,6 +1901,9 @@ static void kvm_s390_crypto_init(struct >>>>>>>> kvm *kvm) >>>>>>>> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb; >>>>>>>> kvm_ap_build_crycbd(kvm); >>>>>>>> >>>>>>>> + /* Default setting indicating SIE shall interpret AP >>>>>>>> instructions */ >>>>>>>> + kvm->arch.crypto.apie = 1; >>>>>>>> + >>>>>>>> if (!test_kvm_facility(kvm, 76)) >>>>>>>> return; >>>>>>>> >>>>>>>> @@ -2434,6 +2437,12 @@ static void >>>>>>>> kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu) >>>>>>>> { >>>>>>>> vcpu->arch.sie_block->crycbd = >>>>>>>> vcpu->kvm->arch.crypto.crycbd; >>>>>>>> >>>>>>>> + vcpu->arch.sie_block->eca &= ~ECA_APIE; >>>>>>>> + if (vcpu->kvm->arch.crypto.apie && >>>>>>>> + test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_AP)) >>>>>>> >>>>>>> Do we call xxx_crypto_setup() if KVM does not support AP >>>>>>> interpretation? >>>>>> >>>>>> Yes, kvm_s390_vcpu_crypto_setup(vcpu) is called by >>>>>> kvm_arch_vcpu_setup(vcpu) >>>>>> as well as from kvm_s390_vcpu_crypto_reset_all(kvm). Calling it >>>>>> has nothing >>>>>> to do with whether AP interpretation is supported or not as it >>>>>> does much >>>>>> more than that, including setting up of wrapping keys and the >>>>>> CRYCBD. >>>>> >>>>> Sorry, still the same error I made about CPU_FEAT_AP meaning AP >>>>> instructions in the guest >>>>> and not AP interpretation available. >>>>> Could apie be set if AP instruction are not supported? >>>>> >>>>>> >>>>>>> >>>>>>>> + vcpu->arch.sie_block->eca |= ECA_APIE; >>>>>>>> + >>>>>>>> + >>>>>>>> if (!test_kvm_facility(vcpu->kvm, 76)) >>>>>>>> return; >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
| |