Message-Id: <fa584953-e0e9-2282-16c3-00a0b6e8db2f@linux.ibm.com>
Date:   Tue, 19 Mar 2019 10:55:50 +0100
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     borntraeger@...ibm.com, alex.williamson@...hat.com,
        linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, frankja@...ux.ibm.com, akrowiak@...ux.ibm.com,
        pasic@...ux.ibm.com, david@...hat.com, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, freude@...ux.ibm.com, mimu@...ux.ibm.com
Subject: Re: [PATCH v5 1/7] s390: ap: kvm: add PQAP interception for AQIC

On 15/03/2019 15:10, Pierre Morel wrote:
> On 15/03/2019 14:26, Pierre Morel wrote:
>> On 15/03/2019 11:20, Cornelia Huck wrote:
>>> On Wed, 13 Mar 2019 17:04:58 +0100
>>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>>
>>>> +/*
>>>> + * 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 = {};
>>>> +    int ret;
>>>> +    /* 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;
>>>> +    /* Verify that the function code is AQIC */
>>>> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
>>>> +    /* We do not want to change the behavior we had before this 
>>>> patch*/
>>>> +    if (fc != 0x03)
>>>> +        return -EOPNOTSUPP;
>>>> +
>>>> +    /* 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);
>>>> +    /* AQIC instruction is allowed only if facility 65 is available */
>>>> +    if (!test_kvm_facility(vcpu->kvm, 65))
>>>> +        return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>>>> +    /* Verify that the hook callback is registered and call it */
>>>> +    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;
>>>
>>> I'm still confused here, sorry. From previous discussions I recall that
>>> this indicates "no crypto device" (please correct me if I'm wrong.)
>>>
>>> Before this patch, we had:
>>> - guest issues PQAP/AQIC -> drop to userspace
>>>
>>> With a correct implementation, we get:
>>> - guest issues PQAP/AQIC -> callback does what needs to be done
>>>
>>> With an incorrect implementation (no callback), we get:
>>> - guest issues PQAP/AQIC -> guest gets response code 0x01
>>>
>>> Why not drop to userspace in that case?
>>
>> This is what I had in the previous patches.
>> Hum, I do not remember which discussion lead me to modify this.
>>
>> Anyway, now that you put the finger on this problem, I think the 
>> problem is worse.
>>
>> The behavior with old / new Linux, vfio driver and qemu is:
>>
>> LINUX    VFIO_AP    QEMU    PGM
>> OLD    x    x    OPERATION
>> NEW    -    OLD    SPECIFICATION
>> NEW    -    NEW/aqic=off    SPECIFICATION
>> NEW    x    NEW/aqic=on    -
>>
>> x = whatever
>> - = absent/none
>>
>> So yes there is a change in behavior for the userland for the case 
>> QEMU do not set the AQIC facility 65, OLD QEMU or NEW QEMU wanting to 
>> behave like an older one.
>>
>> I fear we have the same problem with the privileged operation...
>>
>> For the last case, when the kvm_facility(65) is set, the explication 
>> is the following:
>>
>> This is related to the handling of PQAP AQIC which is now authorized 
>> by this patch series.
>> If we authorize PQAP AQIC, by setting the bit for facility 65, the 
>> guest can use this instruction.
>> If the instruction follows the specifications we must answer something 
>> realistic and since there is nothing in the CRYCB (no driver) we 
>> answer that there is no queue.
>>
>> Conclusion:  we must handle this in userland, it will have the benefit 
>> to keep old behavior when there is no callback.
>> OLD QEMU will not see change as they will not set aqic facility
>> NEW QEMU will handle this correctly.
>>
> 
> Sorry, wrong conclusion, handling this in userland will bring us much 
> too far if we want to answer correctly for the case the hook is not 
> there but QEMU accepted the facility for AQIC.

Sorry, forget it, I was tired.

Pierre

> 
> The alternative is easier, we just continue to respond with the 
> OPERATION exception here and only handle the specification and 
> privileged exception cases in QEMU and in the hook.
> 
> So, I think the discussion will go on until you come back :)
> 
> Regards,
> Pierre
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Powered by blists - more mailing lists