From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tony Krowiak Subject: Re: [PATCH v6 1/7] s390: ap: kvm: add PQAP interception for AQIC Date: Fri, 29 Mar 2019 09:02:29 -0400 Message-ID: <496e7e46-d47d-2b20-8e73-772f5d8d2143@linux.ibm.com> References: <1553265828-27823-1-git-send-email-pmorel@linux.ibm.com> <1553265828-27823-2-git-send-email-pmorel@linux.ibm.com> <32716ed4-559f-0c36-cdf3-4c6662e02f4b@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: alex.williamson@redhat.com, cohuck@redhat.com, linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org, kvm@vger.kernel.org, frankja@linux.ibm.com, pasic@linux.ibm.com, david@redhat.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, freude@linux.ibm.com, mimu@linux.ibm.com To: pmorel@linux.ibm.com, borntraeger@de.ibm.com Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 3/29/19 4:52 AM, Pierre Morel wrote: > On 28/03/2019 17:12, Tony Krowiak wrote: >> On 3/22/19 10:43 AM, Pierre Morel wrote: >>> We prepare the interception of the PQAP/AQIC instruction for >>> the case the AQIC facility is enabled in the guest. >>> >>> First of all we do not want to change existing behavior when >>> intercepting AP instructions without the SIE allowing the guest >>> to use AP instructions. >>> >>> In this patch we only handle the AQIC interception allowed by >>> facility 65 which will be enabled when the complete interception >>> infrastructure will be present. >>> >>> We add a callback inside the KVM arch structure for s390 for >>> a VFIO driver to handle a specific response to the PQAP >>> instruction with the AQIC command and only this command. >>> >>> But we want to be able to return a correct answer to the guest >>> even there is no VFIO AP driver in the kernel. >>> Therefor, we inject the correct exceptions from inside KVM for the >>> case the callback is not initialized, which happens when the vfio_ap >>> driver is not loaded. >>> >>> We do consider the responsability of the driver to always initialize >>> the PQAP callback if it defines queues by initializing the CRYCB for >>> a guest. >>> If the callback has been setup we call it. >>> If not we setup an answer considering that no queue is available >>> for the guest when no callback has been setup. >>> >>> Signed-off-by: Pierre Morel >>> --- >>>   arch/s390/include/asm/kvm_host.h      |  8 ++++ >>>   arch/s390/kvm/priv.c                  | 90 >>> +++++++++++++++++++++++++++++++++++ >>>   drivers/s390/crypto/vfio_ap_private.h |  2 + >>>   3 files changed, 100 insertions(+) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h >>> b/arch/s390/include/asm/kvm_host.h >>> index a496276..624460b 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -18,6 +18,7 @@ >>>   #include >>>   #include >>>   #include >>> +#include >>>   #include >>>   #include >>>   #include >>> @@ -721,8 +722,15 @@ struct kvm_s390_cpu_model { >>>       unsigned short ibc; >>>   }; >>> +struct kvm_s390_module_hook { >>> +    int (*hook)(struct kvm_vcpu *vcpu); >>> +    void *data; >>> +    struct module *owner; >>> +}; >>> + >>>   struct kvm_s390_crypto { >>>       struct kvm_s390_crypto_cb *crycb; >>> +    struct kvm_s390_module_hook *pqap_hook; >>>       __u32 crycbd; >>>       __u8 aes_kw; >>>       __u8 dea_kw; >>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >>> index 8679bd7..793e48a 100644 >>> --- a/arch/s390/kvm/priv.c >>> +++ b/arch/s390/kvm/priv.c >>> @@ -27,6 +27,7 @@ >>>   #include >>>   #include >>>   #include >>> +#include >>>   #include "gaccess.h" >>>   #include "kvm-s390.h" >>>   #include "trace.h" >>> @@ -592,6 +593,93 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) >>>       } >>>   } >>> +/* >>> + * handle_pqap: Handling pqap interception >>> + * @vcpu: the vcpu having issue the pqap instruction >>> + * >>> + * We now support PQAP/AQIC instructions and we need to correctly >>> + * answer the guest even if no dedicated driver's hook is available. >>> + * >>> + * The intercepting code calls a dedicated callback for this >>> instruction >>> + * if a driver did register one in the CRYPTO satellite of the >>> + * SIE block. >>> + * >>> + * For PQAP AQIC and TAPQ instructions, verify privilege and >>> specifications. >>> + * >>> + * If no callback available, the queues are not available, return >>> this to >>> + * the caller. >>> + * Else return the value returned by the callback. >>> + */ >>> +static int handle_pqap(struct kvm_vcpu *vcpu) >>> +{ >>> +    struct ap_queue_status status = {}; >>> +    unsigned long reg0; >>> +    int ret; >>> +    uint8_t fc; >>> + >>> +    /* Verify that the AP instruction are available */ >>> +    if (!ap_instructions_available()) >>> +        return -EOPNOTSUPP; >>> +    /* Verify that the guest is allowed to use AP instructions */ >>> +    if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >>> +        return -EOPNOTSUPP; >>> +    /* >>> +     * The only possibly intercepted instructions when AP >>> instructions are >>> +     * available for the guest are AQIC and TAPQ with the t bit set >>> +     * since we do not set IC.3 (FIII) we currently will not intercept >>> +     * TAPQ. >>> +     * The following code will only treat AQIC function code. >>> +     */ >>> +    reg0 = vcpu->run->s.regs.gprs[0]; >>> +    fc = reg0 >> 24; >>> +    if (fc != 0x03) { >>> +        pr_warn("%s: Unexpected interception code 0x%02x\n", >>> +            __func__, fc); >>> +        return -EOPNOTSUPP; >>> +    } >>> +    /* All PQAP instructions are allowed for guest kernel only */ >>> +    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) >>> +        return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); >>> +    /* >>> +     * Common tests for PQAP instructions to generate a specification >>> +     * exception >>> +     */ >>> +    /* Zero bits overwrite produce a specification exception */ >>> +    if (reg0 & 0x007f0000UL) >>> +        goto specification_except; >>> +    /* If APXA is not installed APQN is limited */ >>> +    if (!(vcpu->kvm->arch.crypto.crycbd & 0x02)) >>> +        if (reg0 & 0x000030f0UL) >>> +            goto specification_except; >>> +    /* AQIC needs facility 65 */ >>> +    if (!test_kvm_facility(vcpu->kvm, 65)) >>> +        goto specification_except; >>> + >>> +    /* >>> +     * Verify that the hook callback is registered, lock the owner >>> +     * and call the hook. >>> +     */ >>> +    if (vcpu->kvm->arch.crypto.pqap_hook) { >>> +        if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) >>> +            return -EOPNOTSUPP; >>> +        ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); >>> +        module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); >>> +        return ret; >>> +    } >>> +    /* >>> +     * It is the duty of the vfio_driver to register a hook >>> +     * If it does not and we get an exception on AQIC we must >>> +     * guess that there is no vfio_ap_driver at all and no one >>> +     * to handle the guests's CRYCB and the CRYCB is empty. >>> +     */ >>> +    status.response_code = 0x01; >>> +    memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status)); >>> +    return 0; >>> + >>> +specification_except: >>> +    return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); >>> +} >>> + >>>   static int handle_stfl(struct kvm_vcpu *vcpu) >>>   { >>>       int rc; >>> @@ -878,6 +966,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) >>>           return handle_sthyi(vcpu); >>>       case 0x7d: >>>           return handle_stsi(vcpu); >>> +    case 0xaf: >>> +        return handle_pqap(vcpu); >>>       case 0xb1: >>>           return handle_stfl(vcpu); >>>       case 0xb2: >>> diff --git a/drivers/s390/crypto/vfio_ap_private.h >>> b/drivers/s390/crypto/vfio_ap_private.h >>> index 76b7f98..a910be1 100644 >>> --- a/drivers/s390/crypto/vfio_ap_private.h >>> +++ b/drivers/s390/crypto/vfio_ap_private.h >>> @@ -16,6 +16,7 @@ >>>   #include >>>   #include >>>   #include >>> +#include >>>   #include "ap_bus.h" >>> @@ -81,6 +82,7 @@ struct ap_matrix_mdev { >>>       struct ap_matrix matrix; >>>       struct notifier_block group_notifier; >>>       struct kvm *kvm; >>> +    struct kvm_s390_module_hook pqap_hook; >> >> I don't understand the purpose of adding this field. We set up the >> the kvm->arch.crypto.pqap_hook in the vfio_ap_mdev_set_kvm which is >> also in this same file, why not just use a static struct >> kvm_s390_module_hook and reuse it when setting up >> kvm->arch.crypto.pqap_hook? It saves you from initializing it every >> time an ap_matrix_mdev is created. > > Having this field embedded in the matrix_mdev allows to easily retrieve > the matrix_mdev from the the hook. The only place you do this is in the handle_pqap hook. The reason you get the matrix_mdev there is to get the vfio_ap_queue object from the matrix_mdev qlist. You could just as well have gotten the vfio_ap_queue using the vfio_ap_find_queue() function. > > Thanks, > Pierre > > > >