Re: [PATCH v4 1/7] s390: ap: kvm: add PQAP interception for AQIC

[Christian Borntraeger]

On 25.02.2019 19:36, Tony Krowiak wrote:
> On 2/22/19 10:29 AM, Pierre Morel wrote:
>> We prepare the interception of the PQAP/AQIC instruction for
>> the case the AQIC facility is enabled in the guest.
>>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
>> ---
>> Â arch/s390/include/asm/kvm_host.h |Â 1 +
>> Â arch/s390/kvm/priv.cÂÂÂÂÂÂÂÂÂÂÂÂ | 52 ++++++++++++++++++++++++++++++++++++++++
>> Â 2 files changed, 53 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index c5f5156..49cc8b0 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
>> Â Â struct kvm_s390_crypto {
>> ÂÂÂÂÂ struct kvm_s390_crypto_cb *crycb;
>> +ÂÂÂ int (*pqap_hook)(struct kvm_vcpu *vcpu);
>> ÂÂÂÂÂ __u32 crycbd;
>> ÂÂÂÂÂ __u8 aes_kw;
>> ÂÂÂÂÂ __u8 dea_kw;
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 8679bd7..3448abd 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -27,6 +27,7 @@
>> Â #include <asm/io.h>
>> Â #include <asm/ptrace.h>
>> Â #include <asm/sclp.h>
>> +#include <asm/ap.h>
>> Â #include "gaccess.h"
>> Â #include "kvm-s390.h"
>> Â #include "trace.h"
>> @@ -592,6 +593,55 @@ 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 instructions only, 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)
>> +{
>> +ÂÂÂ uint8_t fc;
>> +ÂÂÂ struct ap_queue_status status = {};
>> +
>> +ÂÂÂ /* Verify that the AP instruction are available */
>> +ÂÂÂ if (!ap_instructions_available())
>> +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
> 
> How can the guest even execute an AP instruction if the AP instructions
> are not available? If the AP instructions are not available on the host,
> they will not be available on the guest (i.e., CPU model feature
> S390_FEAT_AP will not be set). I suppose it doesn't hurt to check this
> here given QEMU may not be the only client.

The guest can always issue that instruction, even without the facility bit
and we very likely get an instruction intercept. 
I think the checks below would also catch this, but it certainly does not
hurt?
> 
>> +ÂÂÂ /* Verify that the guest is allowed to use AP instructions */
>> +ÂÂÂ if (!(vcpu->arch.sie_block->eca & ECA_APIE))
>> +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
>> +ÂÂÂ /* Verify that the function code is AQIC */
>> +ÂÂÂ fc = vcpu->run->s.regs.gprs[0] >> 24;
>> +ÂÂÂ if (fc != 0x03)
>> +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;