Date:   Wed, 27 Feb 2019 11:16:38 +0100
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     Tony Krowiak <akrowiak@...ux.ibm.com>, 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, 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 v4 1/7] s390: ap: kvm: add PQAP interception for AQIC

On 27/02/2019 10:13, Cornelia Huck wrote:
> On Wed, 27 Feb 2019 09:09:09 +0100
> Pierre Morel <pmorel@...ux.ibm.com> wrote:
> 
>> On 26/02/2019 16:47, Tony Krowiak wrote:
>>> On 2/26/19 6:47 AM, Pierre Morel wrote:
>>>> 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@...ux.ibm.com>
>>>>
>>>> ...snip...
>>>>   
>>>>>> @@ -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.
>>>>>   
>>>>>> +    /* 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;
>>>>>
>>>>> You must have missed my suggestion to move this to the
>>>>> vcpu->kvm->arch.crypto.pqap_hook(vcpu) in the following responses:
>>>>
>>>> Please consider what happen if the vfio_ap module is not loaded.
>>>
>>> I have considered it and even verified my expectations empirically. If
>>> the vfio_ap module is not loaded, you will not be able to create an mdev
>>> device.
>>
>> OK, now please consider that another userland tool, not QEMU uses KVM.
>>
>>> If you don't have an mdev device, you will not be able to
>>> start a guest with a vfio-ap device. If you start a guest without a
>>> vfio-ap device, but enable AP instructions for the guest, there will be
>>> no AP devices attached to the guest. Without any AP devices attached,
>>> the PQAP(AQIC) instructions will not ever get executed.
>>
>> This is not right. The instruction will be executed, eventually, after
>> decoding.
> 
> A sane guest will not issue PQAP(AQIC) if it doesn't have ap
> capabilities, but there's nothing that keeps a guest from issuing that
> instruction regardless.
> 
> However, is this instruction always intercepted and never handled by
> the SIE itself, even if the guest was not configured for ap? By which
> criteria do we enable interception?

It is always intercepted what ever ECA.28 is.
We enable the instruction is allowed through facility 65.

> 
>>
>>> Even if for some
>>> unknown reason the PQAP(AQIC) instruction is executed - for some unknown
>>> reason, it will fail with response code 0x01, AP-queue number not valid.
>>
>> No, before accessing the AP-queue the instruction will be decoded and
>> depending on the installed micro-code it will fail with
>> - OPERATION EXCEPTION if the micro-code is not installed
>> - PRIVILEDGE OPERATION if the instruction is issued from userland
>> (programm state)
>> - SPECIFICATION exception if the instruction do not respect the usage
>> specification
> 
> So, all of these happen prior to checking the function code?

Yes, this is the order of checks AFAIK

> 
>>
>> then it will be interpreted by the microcode and access the queue and
>> only then it will fail with RC 0x01, AP queue not valid.
>>
>> In the case of KVM, we intercept the instruction because it is issued by
>> the guest and we set the AQIC facility on to force interception.
> 
> Will we set that facility even if no vfio-ap device is configured?


Yes we do.


> 
>>
>> KVM do for us all the decode steps I mention here above, if there is or
>> not a pqap hook to be call to simulate the QP queue access.
>>
>> That done, the AP queue virtualisation can be called, this is done by
>> calling the hook.
>>
>>>
>>>    
>>>>   
>>>>>
>>>>> Message ID <342ffd56-b73a-b1f4-004d-de2c4aeef729@...ux.ibm.com>
>>>>> Message ID <e04f0c8b-2fd9-1846-334a-faa48e0e051e@...ux.ibm.com>
>>>>>
>>>>> You previously stated:
>>>>>
>>>>>      "QEMU and KVM can both accept PQAP/AQIC even if the vfio_ap
>>>>> driver is
>>>>>       not loaded. However now that the guest officially get the PQAP/AQIC
>>>>>       instruction we need to handle the specification and operation
>>>>>       exceptions inside KVM _before_ testing and even calling the driver
>>>>>       hook.
>>>>>
>>>>>       I will make the changes in the next iteration."
>>>>
>>>> Still seems right to me, and is done is this patch.
>>>> Isn't it?
>>>
>>> I don't think it's a matter of right and wrong, it's a matter of what
>>> makes sense. IMHO, you want to make things easy if other PQAP functions
>>> are intercepted at some time. In my opinion, there should be a switch
>>> statement in the pqap hook code with a case statement for each PQAP
>>> function supported by the hook. To plug in a new PQAP function handler,
>>> it will be a simple matter of writing the handler function and calling
>>> it from the case statement, like this:
>>>
>>> static int handle_pqap(struct kvm_vcpu *vcpu)
>>> {
>>>       int ret;
>>>       uint8_t fc;
>>>
>>>       fc = vcpu->run->s.regs.gprs[0] >> 24;
>>>
>>>       switch (fc) {
>>>       case 0x03:
>>>           ret = handle_pqap_aqic(vcpu);
>>>       default:
>>>           ret = -EOPNOTSUPP;
>>>       }
>>>
>>>       return ret;
>>> }
>>>
>>> That function belongs in the pqap hook. I see no reaason whatsoever to
>>> check the function code here. If there is no hook, then you will fall
>>> through to the instruction below:
>>>
>>> status.response_code = 0x01;
>>
>> See answer above, what you are speaking about is the execution of the
>> instruction, but there can be exceptions during the decode of the
>> instruction.
> 
> If e.g. calling that instruction from userspace always creates a priv
> op exception, that should be checked prior to even looking at the
> function code. Same with other exceptions. From my no-docs point of
> view, it makes sense to have those common checks in handle_pqap() and
> use the switch/case to call handler functions for the individual
> function codes...
> 
>>
>>>    
>>>>   
>>>>>
>>>>> I don't know what any of the above has to do with checking FC=0x03? If
>>>>> that check is moved to the pqap handler hook, it can just as well return
>>>>> -EOPNOTSUPP. In fact, down below you do this:
>>>>>
>>>>>       return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
>>>>>
>>>>> If the RC=0x03 check fails in the hook, it will return -EOPNOTSUPP just
>>>>> like above. None of this is critical, but the parsing of the register
>>>>> values for the PQAP(AQIC) function ought to be done in the code that
>>>>> handles the PQAP instruction IMHO.
>>>>
>>>>
>>>> This interception code must handle the PQAP/AQIC instruction when the
>>>> hook is not used and should not modify the handling for other PQAP
>>>> instructions.
>>>> We can not move anything inside the hook that must be always done.
>>>
>>> What you are saying here makes no sense. If the check for the function
>>> code is moved into the pqap hook and fc != 0x03, the result will be
>>> exactly the same; the hook will return -EOPNOTSUPP.
>>
>> again please consider that the hook may not be initialized.
> 
> I agree.
> 

Thanks for the comments.

Regards,
Pierre


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

Powered by blists - more mailing lists