From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752900AbeDSO3E (ORCPT ); Thu, 19 Apr 2018 10:29:04 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:38300 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752629AbeDSO26 (ORCPT ); Thu, 19 Apr 2018 10:28:58 -0400 Subject: Re: [PATCH v4 05/15] KVM: s390: enable/disable AP interpretive execution To: Pierre Morel , linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, buendgen@de.ibm.com Cc: freude@de.ibm.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com, kwankhede@nvidia.com, bjsdjshi@linux.vnet.ibm.com, pbonzini@redhat.com, alex.williamson@redhat.com, alifm@linux.vnet.ibm.com, mjrosato@linux.vnet.ibm.com, jjherne@linux.vnet.ibm.com, thuth@redhat.com, pasic@linux.vnet.ibm.com, berrange@redhat.com, fiuczy@linux.vnet.ibm.com, akrowiak@linux.vnet.ibm.com References: <1523827345-11600-1-git-send-email-akrowiak@linux.vnet.ibm.com> <1523827345-11600-6-git-send-email-akrowiak@linux.vnet.ibm.com> <15b60572-40d6-0f03-11f9-c50cb7eb00e8@linux.vnet.ibm.com> <6cba35df-b0de-8000-bb39-c4cec8622c57@linux.vnet.ibm.com> <2b053349-071e-17ed-6ebd-a37bcfd2f330@linux.vnet.ibm.com> <0d18aeef-dfe7-89b6-f91e-022a71fb5ed9@linux.vnet.ibm.com> From: Tony Krowiak Date: Thu, 19 Apr 2018 10:28:50 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 18041914-0048-0000-0000-0000025FFD92 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00008883; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000257; SDB=6.01020207; UDB=6.00520534; IPR=6.00799444; MB=3.00020667; MTD=3.00000008; XFM=3.00000015; UTC=2018-04-19 14:28:55 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18041914-0049-0000-0000-000044D61390 Message-Id: <8c917b82-15cc-ad95-4195-f70cd67ddd6d@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-04-19_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1804190128 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>>>> --- >>>>>>>> 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; >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >