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

Commit Message

Pierre Morel Feb. 22, 2019, 3:29 p.m. UTC

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 <[e-mail]>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/priv.c             | 52 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

Comments

Anthony Krowiak Feb. 25, 2019, 6:36 p.m. UTC | #1

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 <[e-mail]>
> ---
>   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.

> +	/* 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:

Message ID <[e-mail]>
Message ID <[e-mail]>

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."

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.

> +
> +	/* 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)
> +		return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
> +
> +	/* PQAP/AQIC instructions are authorized but there is no queue */
> +	status.response_code = 0x01;
> +	memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
> +	return 0;
> +}
> +
>   static int handle_stfl(struct kvm_vcpu *vcpu)
>   {
>   	int rc;
> @@ -878,6 +928,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:
>

Pierre Morel Feb. 26, 2019, 11:47 a.m. UTC | #2

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 <[e-mail]>

...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.

> 
> Message ID <[e-mail]>
> Message ID <[e-mail]>
> 
> 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 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.

Regards,
Pierre

Anthony Krowiak Feb. 26, 2019, 3:47 p.m. UTC | #3

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 <[e-mail]>
> 
> ...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. 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. 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.


> 
>>
>> Message ID <[e-mail]>
>> Message ID <[e-mail]>
>>
>> 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;

> 
>>
>> 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.

> 
> Regards,
> Pierre
>

Pierre Morel Feb. 27, 2019, 8:09 a.m. UTC | #4

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 <[e-mail]>
>>
>> ...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.

> 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

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.

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 <[e-mail]>
>>> Message ID <[e-mail]>
>>>
>>> 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.

> 
>>
>>>
>>> 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.

Regards,
Pierre

Cornelia Huck Feb. 27, 2019, 9:13 a.m. UTC | #5

On Wed, 27 Feb 2019 09:09:09 +0100
Pierre Morel <[e-mail]> 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 <[e-mail]>  
> >>
> >> ...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?

> 
> > 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?

> 
> 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?

> 
> 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 <[e-mail]>
> >>> Message ID <[e-mail]>
> >>>
> >>> 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.

Pierre Morel Feb. 27, 2019, 10:16 a.m. UTC | #6

On 27/02/2019 10:13, Cornelia Huck wrote:
> On Wed, 27 Feb 2019 09:09:09 +0100
> Pierre Morel <[e-mail]> 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 <[e-mail]>
>>>>
>>>> ...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 <[e-mail]>
>>>>> Message ID <[e-mail]>
>>>>>
>>>>> 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

Anthony Krowiak Feb. 27, 2019, 6 p.m. UTC | #7

On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>
>>> ...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.

What does that have to do with loading the vfio_ap module? Without the
vfio_ap module, there will be no AP devices for the guest. What are you
suggesting here?

> 
>> 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.

Please explain why the PQAP(AQIC) instruction will be executed on a
guest without any devices? Point me to the code in the AP bus where
PQAP(AQIC) is executed without a queue?

> 
>> 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
> 
> 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.
> 
> 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.

Okay, let's go back to the genesis of this discussion; namely, my
suggestion about moving the fc == 0x03 check into the hook code. If
the vfio_ap module is not loaded, there will be no hook code. In that
case, the check for the hook will fail and ultimately response code
0x01 will be set in the status word (which may not be the right thing
to do?). You have not stated a single good reason for keeping this
check, but I'm done with this silly argument. It certainly doesn't
hurt anything.

> 
>>
>>
>>>
>>>>
>>>> Message ID <[e-mail]>
>>>> Message ID <[e-mail]>
>>>>
>>>> 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.

What are you talking about, "decode of the instruction".

> 
>>
>>>
>>>>
>>>> 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.


So what? Then maybe the code at the end of the function is wrong:

/* PQAP/AQIC instructions are authorized but there is no queue */
status.response_code = 0x01;
memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
return 0;

Why does this make sense? What if the APQN is valid? You don't even know
whether it is or not. The only reason you would even reach this
instruction is if the pqap hook is not initialized. Wouldn't it make
more sense to just return -EOPNOTSUPP here? If there is no hook, then
it is not supported.

> 
> Regards,
> Pierre
>

Christian Borntraeger Feb. 28, 2019, 8:31 a.m. UTC | #8

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 <[e-mail]>
>> ---
>>   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;

Christian Borntraeger Feb. 28, 2019, 9:42 a.m. UTC | #9

On 27.02.2019 19:00, Tony Krowiak wrote:
> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>
>>>> ...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.
> 
> What does that have to do with loading the vfio_ap module? Without the
> vfio_ap module, there will be no AP devices for the guest. What are you
> suggesting here?
> 
>>
>>> 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.
> 
> Please explain why the PQAP(AQIC) instruction will be executed on a
> guest without any devices? Point me to the code in the AP bus where
> PQAP(AQIC) is executed without a queue?

The host must be prepared to handle malicous and broken guests. So if
a guest does PQAP, we must handle that gracefully (e.g. by injecting an
exception)

> 
>>
>>> 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
>>
>> 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.
>>
>> 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.
> 
> Okay, let's go back to the genesis of this discussion; namely, my
> suggestion about moving the fc == 0x03 check into the hook code. If
> the vfio_ap module is not loaded, there will be no hook code. In that
> case, the check for the hook will fail and ultimately response code
> 0x01 will be set in the status word (which may not be the right thing
> to do?). You have not stated a single good reason for keeping this
> check, but I'm done with this silly argument. It certainly doesn't
> hurt anything.

The instruction handler must handle the basic checks for the
instruction itself as outlined above.

Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
The we should pass along everything to QEMU, but this is already done with the
ECA_APIE check, correct?

Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
in QEMU and we have enabled the AP instructions interpretion?
If yes then this has some implication:

1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
right away with a specification exception. I do not want the hook to mess with
the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
hook. As long as we have only fc==3 this does not matter.

Correct?

> 
>>
>>>
>>>
>>>>
>>>>>
>>>>> Message ID <[e-mail]>
>>>>> Message ID <[e-mail]>
>>>>>
>>>>> 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.
> 
> What are you talking about, "decode of the instruction".

I think Pierre is talking about the the KVM instruction decoder.
(see handle_instruction in  intercept.c that will then call handle_b2
and then call handle_pqap).

>>
>>>
>>>>
>>>>>
>>>>> 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.
> 
> 
> So what? Then maybe the code at the end of the function is wrong:
> 
> /* PQAP/AQIC instructions are authorized but there is no queue */
> status.response_code = 0x01;
> memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
> return 0;
> 
> Why does this make sense? What if the APQN is valid? You don't even know
> whether it is or not. The only reason you would even reach this
> instruction is if the pqap hook is not initialized. Wouldn't it make
> more sense to just return -EOPNOTSUPP here? If there is no hook, then
> it is not supported.
> 
>>
>> Regards,
>> Pierre
>>
>

Christian Borntraeger Feb. 28, 2019, 11:03 a.m. UTC | #10

On 28.02.2019 10:42, Christian Borntraeger wrote:
[...]
>> Okay, let's go back to the genesis of this discussion; namely, my
>> suggestion about moving the fc == 0x03 check into the hook code. If
>> the vfio_ap module is not loaded, there will be no hook code. In that
>> case, the check for the hook will fail and ultimately response code
>> 0x01 will be set in the status word (which may not be the right thing
>> to do?). You have not stated a single good reason for keeping this
>> check, but I'm done with this silly argument. It certainly doesn't
>> hurt anything.
> 
> The instruction handler must handle the basic checks for the
> instruction itself as outlined above.
> 
> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
> The we should pass along everything to QEMU, but this is already done with the
> ECA_APIE check, correct?
> 
> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
> in QEMU and we have enabled the AP instructions interpretion?
> If yes then this has some implication:
> 
> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
> right away with a specification exception. I do not want the hook to mess with
> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
> hook. As long as we have only fc==3 this does not matter.
> 
> Correct?

Thinking more about that, I think we should inject a specification exception for all
unknown FCc != 0x3. That would also qualify for keeping it in the instruction handler.

Cornelia Huck Feb. 28, 2019, 11:22 a.m. UTC | #11

On Thu, 28 Feb 2019 12:03:38 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 28.02.2019 10:42, Christian Borntraeger wrote:
> [...]
> >> Okay, let's go back to the genesis of this discussion; namely, my
> >> suggestion about moving the fc == 0x03 check into the hook code. If
> >> the vfio_ap module is not loaded, there will be no hook code. In that
> >> case, the check for the hook will fail and ultimately response code
> >> 0x01 will be set in the status word (which may not be the right thing
> >> to do?). You have not stated a single good reason for keeping this
> >> check, but I'm done with this silly argument. It certainly doesn't
> >> hurt anything.  
> > 
> > The instruction handler must handle the basic checks for the
> > instruction itself as outlined above.
> > 
> > Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
> > The we should pass along everything to QEMU, but this is already done with the
> > ECA_APIE check, correct?
> > 
> > Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
> > in QEMU and we have enabled the AP instructions interpretion?
> > If yes then this has some implication:
> > 
> > 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> > 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
> > right away with a specification exception. I do not want the hook to mess with
> > the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> > 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
> > hook. As long as we have only fc==3 this does not matter.
> > 
> > Correct?  
> 
> Thinking more about that, I think we should inject a specification exception for all
> unknown FCc != 0x3. That would also qualify for keeping it in the instruction handler.
> 

So, to summarize, the function should do:
- Is userspace supposed to emulate everything (!ECA_APIE)? Return
  -EOPNOTSUPP to hand control to it.
- We are now interpreting the instruction in KVM. Do common checks
  (PSTATE etc.) and inject exceptions, if needed.
- Now look at the fc; if there's a handler for it, call that; if not
  (case does not attempt to call a specific handler, or no handler
  registered), inject a specification exception. (Do we want pre-checks
  like for facility 65 here, or in the handler?)

That response code 0x01 thingy probably needs to go into the specific
handler function, if anywhere (don't know the semantics, sorry).

Question: Will the handlers for the individual fcs need to generate
different exceptions on their own? I.e., do they need to do injections
themselves, or should the calling function possibly inject an exception
on error?

(Are there more possible fcs than 0x3 and whatever the other
subfunction was?)

Halil Pasic Feb. 28, 2019, 12:39 p.m. UTC | #12

On Thu, 28 Feb 2019 10:42:23 +0100
Christian Borntraeger <[e-mail]> wrote:

> 
> 
> On 27.02.2019 19:00, Tony Krowiak wrote:
> > On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
> >>>>
> >>>> ...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.
> > 
> > What does that have to do with loading the vfio_ap module? Without the
> > vfio_ap module, there will be no AP devices for the guest. What are you
> > suggesting here?
> > 
> >>
> >>> 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.
> > 
> > Please explain why the PQAP(AQIC) instruction will be executed on a
> > guest without any devices? Point me to the code in the AP bus where
> > PQAP(AQIC) is executed without a queue?
> 
> The host must be prepared to handle malicous and broken guests. So if
> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
> exception)
> 

Nod.

> > 
> >>
> >>> 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
> >>
> >> 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.
> >>
> >> 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.
> > 
> > Okay, let's go back to the genesis of this discussion; namely, my
> > suggestion about moving the fc == 0x03 check into the hook code. If
> > the vfio_ap module is not loaded, there will be no hook code. In that
> > case, the check for the hook will fail and ultimately response code
> > 0x01 will be set in the status word (which may not be the right thing
> > to do?). You have not stated a single good reason for keeping this
> > check, but I'm done with this silly argument. It certainly doesn't
> > hurt anything.
> 
> The instruction handler must handle the basic checks for the
> instruction itself as outlined above.

Nod.

> 
> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
> The we should pass along everything to QEMU, but this is already done with the
> ECA_APIE check, correct?

Nod. 

> 
> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
> in QEMU and we have enabled the AP instructions interpretion?

At least the intention is to not emulate. ECA_APIE is an effective
control though...

> If yes then this has some implication:
> 
> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).

Not necessarily true. TAPQ can be intercepted as well (APFT depends
IC.3). But for now we don't care about that.

> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
> right away with a specification exception. I do not want the hook to mess with
> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))

As far as I can tell he already does test_kvm_facility(vcpu->kvm, 65). I
agree we need a spec exception if guest does not have facility 65, but
does have ap instructions.

> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
> hook. As long as we have only fc==3 this does not matter.
> 

I guess Tony's point is that we may have fc == 0 that is TAPQ in the
APFT flavor. IMHO we don't need to care about that at the moment. 

> Correct?

IMHO mostly.

I also doing the facility checks in kvm is easier, and I think this is
something we can change later if needed without any major trouble.

There are a couple of things I would do differently than Pierre does:
1) Do the PGM_PRIVILEGED_OP before the fc == 3 check.

2) Do the test_kvm_facility(vcpu->kvm, 65) check in the context of fc ==
3. I.e. decide if this hook is about pqap or just about pqap aqic and
make the code convey that decision to its reader.

3) I would most probably test if the queue is available by looking at the
masks in CRYCB here. If not AP_RESPONSE_Q_NOT_AVAIL is what we need.

4) If we have APIE and queues authorized by the CRYCB (i.e. we have a
vfio_ap module loaded an an mdev associated with the kvm) the callback
not set (!(vcpu->kvm->arch.crypto.pqap_hook)) is a BUG! In that case
lying that the queue is not available does not seem right. BTW this is
something Pierre changed since the last version quietly (I can't recall
a mention in the change log or somebody asking for this). If we want to
be very pedantic about this bug scenario our best bet is probably
response code 6.

Regards,
Halil

[..]

Pierre Morel Feb. 28, 2019, 1:10 p.m. UTC | #13

On 28/02/2019 12:03, Christian Borntraeger wrote:
> 
> 
> On 28.02.2019 10:42, Christian Borntraeger wrote:
> [...]
>>> Okay, let's go back to the genesis of this discussion; namely, my
>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>> case, the check for the hook will fail and ultimately response code
>>> 0x01 will be set in the status word (which may not be the right thing
>>> to do?). You have not stated a single good reason for keeping this
>>> check, but I'm done with this silly argument. It certainly doesn't
>>> hurt anything.
>>
>> The instruction handler must handle the basic checks for the
>> instruction itself as outlined above.
>>
>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>> The we should pass along everything to QEMU, but this is already done with the
>> ECA_APIE check, correct?
>>
>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>> in QEMU and we have enabled the AP instructions interpretion?
>> If yes then this has some implication:
>>
>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>> right away with a specification exception. I do not want the hook to mess with
>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
>> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
>> hook. As long as we have only fc==3 this does not matter.
>>
>> Correct?
> 
> Thinking more about that, I think we should inject a specification exception for all
> unknown FCc != 0x3. That would also qualify for keeping it in the instruction handler.
> 

May be return a privileged operation exception if issued from guest's 
program state, but generally I agree with the idea of handling all PQAP 
functions here.

Regards,
Pierre

Pierre Morel Feb. 28, 2019, 1:16 p.m. UTC | #14

On 28/02/2019 12:22, Cornelia Huck wrote:
> On Thu, 28 Feb 2019 12:03:38 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 28.02.2019 10:42, Christian Borntraeger wrote:
>> [...]
>>>> Okay, let's go back to the genesis of this discussion; namely, my
>>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>>> case, the check for the hook will fail and ultimately response code
>>>> 0x01 will be set in the status word (which may not be the right thing
>>>> to do?). You have not stated a single good reason for keeping this
>>>> check, but I'm done with this silly argument. It certainly doesn't
>>>> hurt anything.
>>>
>>> The instruction handler must handle the basic checks for the
>>> instruction itself as outlined above.
>>>
>>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>>> The we should pass along everything to QEMU, but this is already done with the
>>> ECA_APIE check, correct?
>>>
>>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>>> in QEMU and we have enabled the AP instructions interpretion?
>>> If yes then this has some implication:
>>>
>>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>>> right away with a specification exception. I do not want the hook to mess with
>>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
>>> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
>>> hook. As long as we have only fc==3 this does not matter.
>>>
>>> Correct?
>>
>> Thinking more about that, I think we should inject a specification exception for all
>> unknown FCc != 0x3. That would also qualify for keeping it in the instruction handler.
>>
> 
> So, to summarize, the function should do:
> - Is userspace supposed to emulate everything (!ECA_APIE)? Return
>    -EOPNOTSUPP to hand control to it.
> - We are now interpreting the instruction in KVM. Do common checks
>    (PSTATE etc.) and inject exceptions, if needed.
> - Now look at the fc; if there's a handler for it, call that; if not
>    (case does not attempt to call a specific handler, or no handler
>    registered), inject a specification exception. (Do we want pre-checks
>    like for facility 65 here, or in the handler?)
> 
> That response code 0x01 thingy probably needs to go into the specific
> handler function, if anywhere (don't know the semantics, sorry).

What do you mean with specific handler function?

If you mean a switch around the FC with static function's call, I agree, 
if you mean a jump into a hook I do not agree.


> 
> Question: Will the handlers for the individual fcs need to generate
> different exceptions on their own? I.e., do they need to do injections
> themselves, or should the calling function possibly inject an exception
> on error?

There are some specificities.

> 
> (Are there more possible fcs than 0x3 and whatever the other
> subfunction was?)
> 

Yes, at least 5 different FC are implemented in the Linux kernel today 
AFAIK.

Regards,
Pierre

Pierre Morel Feb. 28, 2019, 1:23 p.m. UTC | #15

On 28/02/2019 10:42, Christian Borntraeger wrote:
> 
> 
> On 27.02.2019 19:00, Tony Krowiak wrote:
>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>
>>>>> ...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.
>>
>> What does that have to do with loading the vfio_ap module? Without the
>> vfio_ap module, there will be no AP devices for the guest. What are you
>> suggesting here?
>>
>>>
>>>> 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.
>>
>> Please explain why the PQAP(AQIC) instruction will be executed on a
>> guest without any devices? Point me to the code in the AP bus where
>> PQAP(AQIC) is executed without a queue?
> 
> The host must be prepared to handle malicous and broken guests. So if
> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
> exception)
> 
>>
>>>
>>>> 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
>>>
>>> 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.
>>>
>>> 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.
>>
>> Okay, let's go back to the genesis of this discussion; namely, my
>> suggestion about moving the fc == 0x03 check into the hook code. If
>> the vfio_ap module is not loaded, there will be no hook code. In that
>> case, the check for the hook will fail and ultimately response code
>> 0x01 will be set in the status word (which may not be the right thing
>> to do?). You have not stated a single good reason for keeping this
>> check, but I'm done with this silly argument. It certainly doesn't
>> hurt anything.
> 
> The instruction handler must handle the basic checks for the
> instruction itself as outlined above.
> 
> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
> The we should pass along everything to QEMU, but this is already done with the
> ECA_APIE check, correct?
> 
> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
> in QEMU and we have enabled the AP instructions interpretion?
> If yes then this has some implication:
> 
> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
> right away with a specification exception. I do not want the hook to mess with
> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))


Currently the check test_kvm_facility(vcpu->kvm, 65) is done in the 
instruction handler, what do you mean here?

Regards,
Pierre

Christian Borntraeger Feb. 28, 2019, 1:44 p.m. UTC | #16

On 28.02.2019 14:23, Pierre Morel wrote:
> On 28/02/2019 10:42, Christian Borntraeger wrote:
>>
>>
>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>
>>>>>> ...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.
>>>
>>> What does that have to do with loading the vfio_ap module? Without the
>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>> suggesting here?
>>>
>>>>
>>>>> 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.
>>>
>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>> guest without any devices? Point me to the code in the AP bus where
>>> PQAP(AQIC) is executed without a queue?
>>
>> The host must be prepared to handle malicous and broken guests. So if
>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>> exception)
>>
>>>
>>>>
>>>>> 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
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>
>>> Okay, let's go back to the genesis of this discussion; namely, my
>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>> case, the check for the hook will fail and ultimately response code
>>> 0x01 will be set in the status word (which may not be the right thing
>>> to do?). You have not stated a single good reason for keeping this
>>> check, but I'm done with this silly argument. It certainly doesn't
>>> hurt anything.
>>
>> The instruction handler must handle the basic checks for the
>> instruction itself as outlined above.
>>
>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>> The we should pass along everything to QEMU, but this is already done with the
>> ECA_APIE check, correct?
>>
>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>> in QEMU and we have enabled the AP instructions interpretion?
>> If yes then this has some implication:
>>
>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>> right away with a specification exception. I do not want the hook to mess with
>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> 
> 
> Currently the check test_kvm_facility(vcpu->kvm, 65) is done in the instruction handler, what do you mean here?

Found it. I think we should couple the check for 64 to fc==3. Otherwise both things are somewhat
disconnected when reviewing.

Pierre Morel Feb. 28, 2019, 1:47 p.m. UTC | #17

On 28/02/2019 14:44, Christian Borntraeger wrote:
> 
> 
> On 28.02.2019 14:23, Pierre Morel wrote:
>> On 28/02/2019 10:42, Christian Borntraeger wrote:
>>>
>>>
>>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>>
>>>>>>> ...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.
>>>>
>>>> What does that have to do with loading the vfio_ap module? Without the
>>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>>> suggesting here?
>>>>
>>>>>
>>>>>> 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.
>>>>
>>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>>> guest without any devices? Point me to the code in the AP bus where
>>>> PQAP(AQIC) is executed without a queue?
>>>
>>> The host must be prepared to handle malicous and broken guests. So if
>>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>>> exception)
>>>
>>>>
>>>>>
>>>>>> 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
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> Okay, let's go back to the genesis of this discussion; namely, my
>>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>>> case, the check for the hook will fail and ultimately response code
>>>> 0x01 will be set in the status word (which may not be the right thing
>>>> to do?). You have not stated a single good reason for keeping this
>>>> check, but I'm done with this silly argument. It certainly doesn't
>>>> hurt anything.
>>>
>>> The instruction handler must handle the basic checks for the
>>> instruction itself as outlined above.
>>>
>>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>>> The we should pass along everything to QEMU, but this is already done with the
>>> ECA_APIE check, correct?
>>>
>>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>>> in QEMU and we have enabled the AP instructions interpretion?
>>> If yes then this has some implication:
>>>
>>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>>> right away with a specification exception. I do not want the hook to mess with
>>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
>>
>>
>> Currently the check test_kvm_facility(vcpu->kvm, 65) is done in the instruction handler, what do you mean here?
> 
> Found it. I think we should couple the check for 64 to fc==3. Otherwise both things are somewhat
> disconnected when reviewing.
> 

Right.
In the next version I will go the way you proposed anyway and handle all 
PQAP functions separatly (switch/dedicated functions).
With this, I will have to split the checks to the right place.

Thanks for the comments.

Regards,
Pierre

Cornelia Huck Feb. 28, 2019, 1:52 p.m. UTC | #18

On Thu, 28 Feb 2019 14:16:09 +0100
Pierre Morel <[e-mail]> wrote:

> On 28/02/2019 12:22, Cornelia Huck wrote:

> > So, to summarize, the function should do:
> > - Is userspace supposed to emulate everything (!ECA_APIE)? Return
> >    -EOPNOTSUPP to hand control to it.
> > - We are now interpreting the instruction in KVM. Do common checks
> >    (PSTATE etc.) and inject exceptions, if needed.
> > - Now look at the fc; if there's a handler for it, call that; if not
> >    (case does not attempt to call a specific handler, or no handler
> >    registered), inject a specification exception. (Do we want pre-checks
> >    like for facility 65 here, or in the handler?)
> > 
> > That response code 0x01 thingy probably needs to go into the specific
> > handler function, if anywhere (don't know the semantics, sorry).  
> 
> What do you mean with specific handler function?
> 
> If you mean a switch around the FC with static function's call, I agree, 
> if you mean a jump into a hook I do not agree.

Ah, ok; so each case (that we want to handle) should call into a
subhandler that does
{
	(... check things like facilities ...)
	if (!specific_hook)
		inject_specif_excp_and_return();
	ret = specific_hook();
	if (ret)
		set_resp_code_0x01(); // or in specific_hook()?
}

?
 
> > 
> > Question: Will the handlers for the individual fcs need to generate
> > different exceptions on their own? I.e., do they need to do injections
> > themselves, or should the calling function possibly inject an exception
> > on error?  
> 
> There are some specificities.

Ok, should probably done in the subhandlers?

(I hope I don't muddy the waters too much; but basically, I'm poking
around with a stick in the dark :)

Halil Pasic Feb. 28, 2019, 2:07 p.m. UTC | #19

On Thu, 28 Feb 2019 14:47:35 +0100
Pierre Morel <[e-mail]> wrote:

> On 28/02/2019 14:44, Christian Borntraeger wrote:
> > 
> > 
> > On 28.02.2019 14:23, Pierre Morel wrote:
> >> On 28/02/2019 10:42, Christian Borntraeger wrote:
> >>>
> >>>
> >>> On 27.02.2019 19:00, Tony Krowiak wrote:
> >>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
> >>>>>>>
> >>>>>>> ...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.
> >>>>
> >>>> What does that have to do with loading the vfio_ap module? Without the
> >>>> vfio_ap module, there will be no AP devices for the guest. What are you
> >>>> suggesting here?
> >>>>
> >>>>>
> >>>>>> 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.
> >>>>
> >>>> Please explain why the PQAP(AQIC) instruction will be executed on a
> >>>> guest without any devices? Point me to the code in the AP bus where
> >>>> PQAP(AQIC) is executed without a queue?
> >>>
> >>> The host must be prepared to handle malicous and broken guests. So if
> >>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
> >>> exception)
> >>>
> >>>>
> >>>>>
> >>>>>> 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
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> 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.
> >>>>
> >>>> Okay, let's go back to the genesis of this discussion; namely, my
> >>>> suggestion about moving the fc == 0x03 check into the hook code. If
> >>>> the vfio_ap module is not loaded, there will be no hook code. In that
> >>>> case, the check for the hook will fail and ultimately response code
> >>>> 0x01 will be set in the status word (which may not be the right thing
> >>>> to do?). You have not stated a single good reason for keeping this
> >>>> check, but I'm done with this silly argument. It certainly doesn't
> >>>> hurt anything.
> >>>
> >>> The instruction handler must handle the basic checks for the
> >>> instruction itself as outlined above.
> >>>
> >>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
> >>> The we should pass along everything to QEMU, but this is already done with the
> >>> ECA_APIE check, correct?
> >>>
> >>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
> >>> in QEMU and we have enabled the AP instructions interpretion?
> >>> If yes then this has some implication:
> >>>
> >>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> >>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
> >>> right away with a specification exception. I do not want the hook to mess with
> >>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> >>
> >>
> >> Currently the check test_kvm_facility(vcpu->kvm, 65) is done in the instruction handler, what do you mean here?
> > 
> > Found it. I think we should couple the check for 64 to fc==3. Otherwise both things are somewhat
> > disconnected when reviewing.
> > 
> 
> Right.
> In the next version I will go the way you proposed anyway and handle all 
> PQAP functions separatly (switch/dedicated functions).

Sorry what did Christian propose? I've lost you. Christian's initial
analysis assumed AFAIU that we only have or care for fc == 3.

BTW have you seen my response to Christians analysis and the changes I
proposed?

Regards,
Halil

> With this, I will have to split the checks to the right place.
> 
> Thanks for the comments.
> 
> Regards,
> Pierre
> 
>

Pierre Morel Feb. 28, 2019, 2:12 p.m. UTC | #20

On 28/02/2019 13:39, Halil Pasic wrote:
> On Thu, 28 Feb 2019 10:42:23 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>>
>>
>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>
>>>>>> ...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.
>>>
>>> What does that have to do with loading the vfio_ap module? Without the
>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>> suggesting here?
>>>
>>>>
>>>>> 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.
>>>
>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>> guest without any devices? Point me to the code in the AP bus where
>>> PQAP(AQIC) is executed without a queue?
>>
>> The host must be prepared to handle malicous and broken guests. So if
>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>> exception)
>>
> 
> Nod.
> 
>>>
>>>>
>>>>> 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
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>
>>> Okay, let's go back to the genesis of this discussion; namely, my
>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>> case, the check for the hook will fail and ultimately response code
>>> 0x01 will be set in the status word (which may not be the right thing
>>> to do?). You have not stated a single good reason for keeping this
>>> check, but I'm done with this silly argument. It certainly doesn't
>>> hurt anything.
>>
>> The instruction handler must handle the basic checks for the
>> instruction itself as outlined above.
> 
> Nod.
> 
>>
>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>> The we should pass along everything to QEMU, but this is already done with the
>> ECA_APIE check, correct?
> 
> Nod.
> 
>>
>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>> in QEMU and we have enabled the AP instructions interpretion?
> 
> At least the intention is to not emulate. ECA_APIE is an effective
> control though...
> 
>> If yes then this has some implication:
>>
>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> 
> Not necessarily true. TAPQ can be intercepted as well (APFT depends
> IC.3). But for now we don't care about that.
> 
>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>> right away with a specification exception. I do not want the hook to mess with
>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> 
> As far as I can tell he already does test_kvm_facility(vcpu->kvm, 65). I
> agree we need a spec exception if guest does not have facility 65, but
> does have ap instructions.
> 
>> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
>> hook. As long as we have only fc==3 this does not matter.
>>
> 
> I guess Tony's point is that we may have fc == 0 that is TAPQ in the
> APFT flavor. IMHO we don't need to care about that at the moment.
> 
>> Correct?
> 
> IMHO mostly.
> 
> I also doing the facility checks in kvm is easier, and I think this is
> something we can change later if needed without any major trouble.
> 
> There are a couple of things I would do differently than Pierre does:
> 1) Do the PGM_PRIVILEGED_OP before the fc == 3 check.

Idea was not to modify existing behavior for fc != 3

Also Christian already proposed to handle all FC codes. So in this idea, 
this must be done as you say.

> 
> 2) Do the test_kvm_facility(vcpu->kvm, 65) check in the context of fc ==
> 3. I.e. decide if this hook is about pqap or just about pqap aqic and
> make the code convey that decision to its reader.
> 
> 3) I would most probably test if the queue is available by looking at the
> masks in CRYCB here. If not AP_RESPONSE_Q_NOT_AVAIL is what we need.

This I do not agree with, it is typically the responsibility of the part 
in charge of the virtualization to do this, also the vfio_driver.

> 
> 4) If we have APIE and queues authorized by the CRYCB (i.e. we have a
> vfio_ap module loaded an an mdev associated with the kvm) the callback
> not set (!(vcpu->kvm->arch.crypto.pqap_hook)) is a BUG!

I do not agree with this either, the maintainers ;) will not allow this.

> In that case
> lying that the queue is not available does not seem right. BTW this is
> something Pierre changed since the last version quietly (I can't recall
> a mention in the change log or somebody asking for this). If we want to
> be very pedantic about this bug scenario our best bet is probably
> response code 6.


RC 06 means "Invalid address of AP-queue notification byte"

So you must have think about another code or I do not understand at all 
what you mean.

Regards,
Pierre

Pierre Morel Feb. 28, 2019, 2:13 p.m. UTC | #21

On 28/02/2019 15:07, Halil Pasic wrote:
> On Thu, 28 Feb 2019 14:47:35 +0100
> Pierre Morel <[e-mail]> wrote:
> 
>> On 28/02/2019 14:44, Christian Borntraeger wrote:
>>>
>>>
>>> On 28.02.2019 14:23, Pierre Morel wrote:
>>>> On 28/02/2019 10:42, Christian Borntraeger wrote:
>>>>>
>>>>>
>>>>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>>>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>>>>
>>>>>>>>> ...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.
>>>>>>
>>>>>> What does that have to do with loading the vfio_ap module? Without the
>>>>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>>>>> suggesting here?
>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>>>>> guest without any devices? Point me to the code in the AP bus where
>>>>>> PQAP(AQIC) is executed without a queue?
>>>>>
>>>>> The host must be prepared to handle malicous and broken guests. So if
>>>>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>>>>> exception)
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Okay, let's go back to the genesis of this discussion; namely, my
>>>>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>>>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>>>>> case, the check for the hook will fail and ultimately response code
>>>>>> 0x01 will be set in the status word (which may not be the right thing
>>>>>> to do?). You have not stated a single good reason for keeping this
>>>>>> check, but I'm done with this silly argument. It certainly doesn't
>>>>>> hurt anything.
>>>>>
>>>>> The instruction handler must handle the basic checks for the
>>>>> instruction itself as outlined above.
>>>>>
>>>>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>>>>> The we should pass along everything to QEMU, but this is already done with the
>>>>> ECA_APIE check, correct?
>>>>>
>>>>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>>>>> in QEMU and we have enabled the AP instructions interpretion?
>>>>> If yes then this has some implication:
>>>>>
>>>>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>>>>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>>>>> right away with a specification exception. I do not want the hook to mess with
>>>>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
>>>>
>>>>
>>>> Currently the check test_kvm_facility(vcpu->kvm, 65) is done in the instruction handler, what do you mean here?
>>>
>>> Found it. I think we should couple the check for 64 to fc==3. Otherwise both things are somewhat
>>> disconnected when reviewing.
>>>
>>
>> Right.
>> In the next version I will go the way you proposed anyway and handle all
>> PQAP functions separatly (switch/dedicated functions).
> 
> Sorry what did Christian propose? I've lost you. Christian's initial
> analysis assumed AFAIU that we only have or care for fc == 3.
> 
> BTW have you seen my response to Christians analysis and the changes I
> proposed?

Yes, just pushed the send button. :)

Regards,
Pierre

Pierre Morel Feb. 28, 2019, 2:14 p.m. UTC | #22

On 28/02/2019 14:52, Cornelia Huck wrote:
> On Thu, 28 Feb 2019 14:16:09 +0100
> Pierre Morel <[e-mail]> wrote:
> 
>> On 28/02/2019 12:22, Cornelia Huck wrote:
> 
>>> So, to summarize, the function should do:
>>> - Is userspace supposed to emulate everything (!ECA_APIE)? Return
>>>     -EOPNOTSUPP to hand control to it.
>>> - We are now interpreting the instruction in KVM. Do common checks
>>>     (PSTATE etc.) and inject exceptions, if needed.
>>> - Now look at the fc; if there's a handler for it, call that; if not
>>>     (case does not attempt to call a specific handler, or no handler
>>>     registered), inject a specification exception. (Do we want pre-checks
>>>     like for facility 65 here, or in the handler?)
>>>
>>> That response code 0x01 thingy probably needs to go into the specific
>>> handler function, if anywhere (don't know the semantics, sorry).
>>
>> What do you mean with specific handler function?
>>
>> If you mean a switch around the FC with static function's call, I agree,
>> if you mean a jump into a hook I do not agree.
> 
> Ah, ok; so each case (that we want to handle) should call into a
> subhandler that does
> {
> 	(... check things like facilities ...)
> 	if (!specific_hook)
> 		inject_specif_excp_and_return();
> 	ret = specific_hook();
> 	if (ret)
> 		set_resp_code_0x01(); // or in specific_hook()?
> }
> 
> ?

Yes something in this direction.

>   
>>>
>>> Question: Will the handlers for the individual fcs need to generate
>>> different exceptions on their own? I.e., do they need to do injections
>>> themselves, or should the calling function possibly inject an exception
>>> on error?
>>
>> There are some specificities.
> 
> Ok, should probably done in the subhandlers?
> 
> (I hope I don't muddy the waters too much; but basically, I'm poking
> around with a stick in the dark :)
> 

No problem, it is OK.
My first idea was to make only changes associated with PQAP/AQIC.
We already should have done it for all PQAP functions so it is decided 
that we will do it now as Christian proposed.

Regards,
Pierre

Anthony Krowiak Feb. 28, 2019, 3:35 p.m. UTC | #23

On 2/28/19 4:42 AM, Christian Borntraeger wrote:
> 
> 
> On 27.02.2019 19:00, Tony Krowiak wrote:
>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>
>>>>> ...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.
>>
>> What does that have to do with loading the vfio_ap module? Without the
>> vfio_ap module, there will be no AP devices for the guest. What are you
>> suggesting here?
>>
>>>
>>>> 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.
>>
>> Please explain why the PQAP(AQIC) instruction will be executed on a
>> guest without any devices? Point me to the code in the AP bus where
>> PQAP(AQIC) is executed without a queue?
> 
> The host must be prepared to handle malicous and broken guests. So if
> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
> exception)

I agree, but the context of this discussion is whether it is
more appropriate to check fc == 0x03 in this function or the pqap
hook. If there is no vfio_ap module, which Pierre asked me to consider,
then there will be no hook initialized. Again, nothing Pierre has
stated has convinced me that the fc check belongs here, although there
is no harm in doing so. In fact, a malicious guest can issue PQAP(AQIC)
with fc=0x03, so none of the arguments above makes sense in this
context.

> 
>>
>>>
>>>> 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
>>>
>>> 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.
>>>
>>> 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.
>>
>> Okay, let's go back to the genesis of this discussion; namely, my
>> suggestion about moving the fc == 0x03 check into the hook code. If
>> the vfio_ap module is not loaded, there will be no hook code. In that
>> case, the check for the hook will fail and ultimately response code
>> 0x01 will be set in the status word (which may not be the right thing
>> to do?). You have not stated a single good reason for keeping this
>> check, but I'm done with this silly argument. It certainly doesn't
>> hurt anything.
> 
> The instruction handler must handle the basic checks for the
> instruction itself as outlined above.

The pqap hook IS the instruction handler. Everything up to the point of
calling the hook is validation code. Since the pqap hook is the entity
that actually handles the instruction, it seem to me logical that it
would be the place to parse the sub-function (i.e., fc) of the
PQAP instruction. That is the essence of my argument. I've stated
several sound reasons for asking for the change. Having said that, if
you all feel strongly that it belongs here, no harm done. As the Beatles
once said, let it be.

> 
> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
> The we should pass along everything to QEMU, but this is already done with the
> ECA_APIE check, correct?
> 
> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
> in QEMU and we have enabled the AP instructions interpretion?
> If yes then this has some implication:
> 
> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
> right away with a specification exception. I do not want the hook to mess with
> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
> hook. As long as we have only fc==3 this does not matter.
> 
> Correct?

That all sounds correct assuming we will never do interception of other
AP instructions in KVM.


> 
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Message ID <[e-mail]>
>>>>>> Message ID <[e-mail]>
>>>>>>
>>>>>> 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.
>>
>> What are you talking about, "decode of the instruction".
> 
> I think Pierre is talking about the the KVM instruction decoder.
> (see handle_instruction in  intercept.c that will then call handle_b2
> and then call handle_pqap).

I think this debate has gone on far too long for such a minor
suggestion. If Pierre wants to keep the check for fc here, so be
it. I've wasted waaaaaay to much time on it.

> 
>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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.
>>
>>
>> So what? Then maybe the code at the end of the function is wrong:
>>
>> /* PQAP/AQIC instructions are authorized but there is no queue */
>> status.response_code = 0x01;
>> memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
>> return 0;
>>
>> Why does this make sense? What if the APQN is valid? You don't even know
>> whether it is or not. The only reason you would even reach this
>> instruction is if the pqap hook is not initialized. Wouldn't it make
>> more sense to just return -EOPNOTSUPP here? If there is no hook, then
>> it is not supported.
>>
>>>
>>> Regards,
>>> Pierre
>>>
>>
>

Anthony Krowiak Feb. 28, 2019, 3:36 p.m. UTC | #24

On 2/28/19 6:03 AM, Christian Borntraeger wrote:
> 
> 
> On 28.02.2019 10:42, Christian Borntraeger wrote:
> [...]
>>> Okay, let's go back to the genesis of this discussion; namely, my
>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>> case, the check for the hook will fail and ultimately response code
>>> 0x01 will be set in the status word (which may not be the right thing
>>> to do?). You have not stated a single good reason for keeping this
>>> check, but I'm done with this silly argument. It certainly doesn't
>>> hurt anything.
>>
>> The instruction handler must handle the basic checks for the
>> instruction itself as outlined above.
>>
>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>> The we should pass along everything to QEMU, but this is already done with the
>> ECA_APIE check, correct?
>>
>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>> in QEMU and we have enabled the AP instructions interpretion?
>> If yes then this has some implication:
>>
>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>> right away with a specification exception. I do not want the hook to mess with
>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
>> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
>> hook. As long as we have only fc==3 this does not matter.
>>
>> Correct?
> 
> Thinking more about that, I think we should inject a specification exception for all
> unknown FCc != 0x3. That would also qualify for keeping it in the instruction handler.

Sure, let's do it.

>

Anthony Krowiak Feb. 28, 2019, 3:43 p.m. UTC | #25

On 2/28/19 7:39 AM, Halil Pasic wrote:
> On Thu, 28 Feb 2019 10:42:23 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>>
>>
>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>
>>>>>> ...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.
>>>
>>> What does that have to do with loading the vfio_ap module? Without the
>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>> suggesting here?
>>>
>>>>
>>>>> 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.
>>>
>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>> guest without any devices? Point me to the code in the AP bus where
>>> PQAP(AQIC) is executed without a queue?
>>
>> The host must be prepared to handle malicous and broken guests. So if
>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>> exception)
>>
> 
> Nod.
> 
>>>
>>>>
>>>>> 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
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>
>>> Okay, let's go back to the genesis of this discussion; namely, my
>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>> case, the check for the hook will fail and ultimately response code
>>> 0x01 will be set in the status word (which may not be the right thing
>>> to do?). You have not stated a single good reason for keeping this
>>> check, but I'm done with this silly argument. It certainly doesn't
>>> hurt anything.
>>
>> The instruction handler must handle the basic checks for the
>> instruction itself as outlined above.
> 
> Nod.
> 
>>
>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>> The we should pass along everything to QEMU, but this is already done with the
>> ECA_APIE check, correct?
> 
> Nod.
> 
>>
>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>> in QEMU and we have enabled the AP instructions interpretion?
> 
> At least the intention is to not emulate. ECA_APIE is an effective
> control though...
> 
>> If yes then this has some implication:
>>
>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
> 
> Not necessarily true. TAPQ can be intercepted as well (APFT depends
> IC.3). But for now we don't care about that.
> 
>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>> right away with a specification exception. I do not want the hook to mess with
>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
> 
> As far as I can tell he already does test_kvm_facility(vcpu->kvm, 65). I
> agree we need a spec exception if guest does not have facility 65, but
> does have ap instructions.
> 
>> 3. What shall we do when fc == 0x3? We can certainly do the check here OR in the
>> hook. As long as we have only fc==3 this does not matter.
>>
> 
> I guess Tony's point is that we may have fc == 0 that is TAPQ in the
> APFT flavor. IMHO we don't need to care about that at the moment.
> 
>> Correct?
> 
> IMHO mostly.
> 
> I also doing the facility checks in kvm is easier, and I think this is
> something we can change later if needed without any major trouble.
> 
> There are a couple of things I would do differently than Pierre does:
> 1) Do the PGM_PRIVILEGED_OP before the fc == 3 check.
> 
> 2) Do the test_kvm_facility(vcpu->kvm, 65) check in the context of fc ==
> 3. I.e. decide if this hook is about pqap or just about pqap aqic and
> make the code convey that decision to its reader.
> 
> 3) I would most probably test if the queue is available by looking at the
> masks in CRYCB here. If not AP_RESPONSE_Q_NOT_AVAIL is what we need.
> 
> 4) If we have APIE and queues authorized by the CRYCB (i.e. we have a
> vfio_ap module loaded an an mdev associated with the kvm) the callback
> not set (!(vcpu->kvm->arch.crypto.pqap_hook)) is a BUG! In that case
> lying that the queue is not available does not seem right. BTW this is
> something Pierre changed since the last version quietly (I can't recall
> a mention in the change log or somebody asking for this). If we want to
> be very pedantic about this bug scenario our best bet is probably
> response code 6.
> 

Agreed

> Regards,
> Halil
> 
> [..]
>

Anthony Krowiak Feb. 28, 2019, 3:45 p.m. UTC | #26

On 2/28/19 8:44 AM, Christian Borntraeger wrote:
> 
> 
> On 28.02.2019 14:23, Pierre Morel wrote:
>> On 28/02/2019 10:42, Christian Borntraeger wrote:
>>>
>>>
>>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>>
>>>>>>> ...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.
>>>>
>>>> What does that have to do with loading the vfio_ap module? Without the
>>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>>> suggesting here?
>>>>
>>>>>
>>>>>> 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.
>>>>
>>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>>> guest without any devices? Point me to the code in the AP bus where
>>>> PQAP(AQIC) is executed without a queue?
>>>
>>> The host must be prepared to handle malicous and broken guests. So if
>>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>>> exception)
>>>
>>>>
>>>>>
>>>>>> 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
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> Okay, let's go back to the genesis of this discussion; namely, my
>>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>>> case, the check for the hook will fail and ultimately response code
>>>> 0x01 will be set in the status word (which may not be the right thing
>>>> to do?). You have not stated a single good reason for keeping this
>>>> check, but I'm done with this silly argument. It certainly doesn't
>>>> hurt anything.
>>>
>>> The instruction handler must handle the basic checks for the
>>> instruction itself as outlined above.
>>>
>>> Do we want to allow QEMU to fully emulate everything (the  ECA_APIE case being off)?
>>> The we should pass along everything to QEMU, but this is already done with the
>>> ECA_APIE check, correct?
>>>
>>> Do we agree that when we are beyond the ECA_APIE check, that we do not emulate
>>> in QEMU and we have enabled the AP instructions interpretion?
>>> If yes then this has some implication:
>>>
>>> 1. ECA is on and we should only get PQAP interception for specific FC (namely 3).
>>> 2. What we certainly should check is the facility bit of the guest (65) and reject fc==3
>>> right away with a specification exception. I do not want the hook to mess with
>>> the kvm cpu model. @Pierre would be good to actually check test_kvm_facility(vcpu->kvm, 65))
>>
>>
>> Currently the check test_kvm_facility(vcpu->kvm, 65) is done in the instruction handler, what do you mean here?
> 
> Found it. I think we should couple the check for 64 to fc==3. Otherwise both things are somewhat
> disconnected when reviewing.

I think you meant facility bit 65.

>

Halil Pasic Feb. 28, 2019, 4:51 p.m. UTC | #27

On Thu, 28 Feb 2019 15:12:16 +0100
Pierre Morel <[e-mail]> wrote:

> On 28/02/2019 13:39, Halil Pasic wrote:
> > On Thu, 28 Feb 2019 10:42:23 +0100
> > Christian Borntraeger <[e-mail]> wrote:
[..]
> >> Correct?
> > 
> > IMHO mostly.
> > 
> > I also doing the facility checks in kvm is easier, and I think this is
> > something we can change later if needed without any major trouble.
> > 
> > There are a couple of things I would do differently than Pierre does:
> > 1) Do the PGM_PRIVILEGED_OP before the fc == 3 check.
> 
> Idea was not to modify existing behavior for fc != 3
> 
> Also Christian already proposed to handle all FC codes. So in this idea, 
> this must be done as you say.
> 
> > 
> > 2) Do the test_kvm_facility(vcpu->kvm, 65) check in the context of fc ==
> > 3. I.e. decide if this hook is about pqap or just about pqap aqic and
> > make the code convey that decision to its reader.
> > 
> > 3) I would most probably test if the queue is available by looking at the
> > masks in CRYCB here. If not AP_RESPONSE_Q_NOT_AVAIL is what we need.
> 
> This I do not agree with, it is typically the responsibility of the part 
> in charge of the virtualization to do this, also the vfio_driver.
> 

See at 4) regarding the details. My guess is you disagree with checking
CRYCB explicitly but don't digress with AP_RESPONSE_Q_NOT_AVAIL if APCB
does not authorize the queue. Your idea was to infer APCB all zero from
the fact that pqap_hook is NULL.

If my assumption is right, then yes we can have an implicit coarse check
here and a fine grained check in the client code (vfio_ap).

> > 
> > 4) If we have APIE and queues authorized by the CRYCB (i.e. we have a
> > vfio_ap module loaded an an mdev associated with the kvm) the callback
> > not set (!(vcpu->kvm->arch.crypto.pqap_hook)) is a BUG!
> 
> I do not agree with this either, the maintainers ;) will not allow this.

After an offline discussion we came to the conclusion that I did not
understand your code.

Your train of thought was:

!(vcpu->kvm->arch.crypto.pqap_hook) _implies_ APCB all zero (i.e. the
masks in the CRYCB

This is *why* you respond with AP_RESPONSE_Q_NOT_AVAIL.

However if that is the case I would like that spelled out in a code
comment at least. Furthermore setting pqap_hook and APCB needs to happen
in the right sequence. Means client code (vfio_ap) may only set APCB
after the qpap_hook has been set. Currently we have a race there (as
you first do  kvm_arch_crypto_set_masks and only then
kvm->arch.crypto.pqap_hook. Furthermore I guess
kvm->arch.crypto.pqap_hook needs to be set with the kvm lock held, which
does not seem to be the case.

> 
> > In that case
> > lying that the queue is not available does not seem right. BTW this
> > is something Pierre changed since the last version quietly (I can't
> > recall a mention in the change log or somebody asking for this). If
> > we want to be very pedantic about this bug scenario our best bet is
> > probably response code 6.
> 
> 
> RC 06 means "Invalid address of AP-queue notification byte"
> 
> So you must have think about another code or I do not understand at
> all what you mean.
> 

I did not assume you decided to ignore the possibility of a programming
error (which you at least technically did commit yourself) for what I
described as a BUG.

My train of thought was, if we are very pedantic we can make things work
with degraded functionality in that case. I.e. without AP interrupts.
For that we need to tell the guest something like: yes your queue is
fine and there and all that but AQCI setup interrupts did not work. And
RC 06 is the only RC I see being suitable to convey that.

Detect and handle if the client code does not hold up their end of the
bargain or just ignore the possibility is a design decision. But at least
you should spell out your expectations against the client code.

Regards,
Halil

Christian Borntraeger March 1, 2019, 8:42 a.m. UTC | #28

On 28.02.2019 16:35, Tony Krowiak wrote:
> On 2/28/19 4:42 AM, Christian Borntraeger wrote:
>>
>>
>> On 27.02.2019 19:00, Tony Krowiak wrote:
>>> On 2/27/19 3:09 AM, Pierre Morel 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 <[e-mail]>
>>>>>>
>>>>>> ...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.
>>>
>>> What does that have to do with loading the vfio_ap module? Without the
>>> vfio_ap module, there will be no AP devices for the guest. What are you
>>> suggesting here?
>>>
>>>>
>>>>> 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.
>>>
>>> Please explain why the PQAP(AQIC) instruction will be executed on a
>>> guest without any devices? Point me to the code in the AP bus where
>>> PQAP(AQIC) is executed without a queue?
>>
>> The host must be prepared to handle malicous and broken guests. So if
>> a guest does PQAP, we must handle that gracefully (e.g. by injecting an
>> exception)
> 
> I agree, but the context of this discussion is whether it is
> more appropriate to check fc == 0x03 in this function or the pqap
> hook. If there is no vfio_ap module, which Pierre asked me to consider,
> then there will be no hook initialized. Again, nothing Pierre has
> stated has convinced me that the fc check belongs here, although there
> is no harm in doing so. In fact, a malicious guest can issue PQAP(AQIC)
> with fc=0x03, so none of the arguments above makes sense in this
> context.
> 
>>
>>>
>>>>
>>>>> 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
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>
>>> Okay, let's go back to the genesis of this discussion; namely, my
>>> suggestion about moving the fc == 0x03 check into the hook code. If
>>> the vfio_ap module is not loaded, there will be no hook code. In that
>>> case, the check for the hook will fail and ultimately response code
>>> 0x01 will be set in the status word (which may not be the right thing
>>> to do?). You have not stated a single good reason for keeping this
>>> check, but I'm done with this silly argument. It certainly doesn't
>>> hurt anything.
>>
>> The instruction handler must handle the basic checks for the
>> instruction itself as outlined above.
> 
> The pqap hook IS the instruction handler.

In the kvm sense handle_pqap is the instruction handler.
But can we stop that discussion NOW?
There is things that can be done in both places. As long as the overall code
produces the right result it really does not matter where we do the checks.

This discussion distracts the attention from more important issues, for example
the question about how do we guarantee the de-registration of the interrupt
indicator byte when the kvm guest goes away.

>> I think Pierre is talking about the the KVM instruction decoder.
>> (see handle_instruction in  intercept.c that will then call handle_b2
>> and then call handle_pqap).
> 
> I think this debate has gone on far too long for such a minor
> suggestion. If Pierre wants to keep the check for fc here, so be
> it. I've wasted waaaaaay to much time on it.

Absolutely. Lets stop here and focus on the real things. I think we are
pretty close but we need to tackle some issues.

Pierre Morel March 1, 2019, 12:03 p.m. UTC | #29

On 28/02/2019 15:14, Pierre Morel wrote:
> On 28/02/2019 14:52, Cornelia Huck wrote:
>> On Thu, 28 Feb 2019 14:16:09 +0100
>> Pierre Morel <[e-mail]> wrote:
>>
>>> On 28/02/2019 12:22, Cornelia Huck wrote:
>>
>>>> So, to summarize, the function should do:
>>>> - Is userspace supposed to emulate everything (!ECA_APIE)? Return
>>>>     -EOPNOTSUPP to hand control to it.
>>>> - We are now interpreting the instruction in KVM. Do common checks
>>>>     (PSTATE etc.) and inject exceptions, if needed.
>>>> - Now look at the fc; if there's a handler for it, call that; if not
>>>>     (case does not attempt to call a specific handler, or no handler
>>>>     registered), inject a specification exception. (Do we want 
>>>> pre-checks
>>>>     like for facility 65 here, or in the handler?)
>>>>
>>>> That response code 0x01 thingy probably needs to go into the specific
>>>> handler function, if anywhere (don't know the semantics, sorry).
>>>
>>> What do you mean with specific handler function?
>>>
>>> If you mean a switch around the FC with static function's call, I agree,
>>> if you mean a jump into a hook I do not agree.
>>
>> Ah, ok; so each case (that we want to handle) should call into a
>> subhandler that does
>> {
>>     (... check things like facilities ...)
>>     if (!specific_hook)
>>         inject_specif_excp_and_return();
>>     ret = specific_hook();
>>     if (ret)
>>         set_resp_code_0x01(); // or in specific_hook()?
>> }
>>
>> ?
> 
> Yes something in this direction.

Sorry, after reflection, no, we do not want to change the previous 
behavior so we only handle the AQIC case.

Regards,
Pierre

Christian Borntraeger March 1, 2019, 12:05 p.m. UTC | #30

On 01.03.2019 13:03, Pierre Morel wrote:
> On 28/02/2019 15:14, Pierre Morel wrote:
>> On 28/02/2019 14:52, Cornelia Huck wrote:
>>> On Thu, 28 Feb 2019 14:16:09 +0100
>>> Pierre Morel <[e-mail]> wrote:
>>>
>>>> On 28/02/2019 12:22, Cornelia Huck wrote:
>>>
>>>>> So, to summarize, the function should do:
>>>>> - Is userspace supposed to emulate everything (!ECA_APIE)? Return
>>>>>     -EOPNOTSUPP to hand control to it.
>>>>> - We are now interpreting the instruction in KVM. Do common checks
>>>>>     (PSTATE etc.) and inject exceptions, if needed.
>>>>> - Now look at the fc; if there's a handler for it, call that; if not
>>>>>     (case does not attempt to call a specific handler, or no handler
>>>>>     registered), inject a specification exception. (Do we want pre-checks
>>>>>     like for facility 65 here, or in the handler?)
>>>>>
>>>>> That response code 0x01 thingy probably needs to go into the specific
>>>>> handler function, if anywhere (don't know the semantics, sorry).
>>>>
>>>> What do you mean with specific handler function?
>>>>
>>>> If you mean a switch around the FC with static function's call, I agree,
>>>> if you mean a jump into a hook I do not agree.
>>>
>>> Ah, ok; so each case (that we want to handle) should call into a
>>> subhandler that does
>>> {
>>>     (... check things like facilities ...)
>>>     if (!specific_hook)
>>>         inject_specif_excp_and_return();
>>>     ret = specific_hook();
>>>     if (ret)
>>>         set_resp_code_0x01(); // or in specific_hook()?
>>> }
>>>
>>> ?
>>
>> Yes something in this direction.
> 
> Sorry, after reflection, no, we do not want to change the previous behavior so we only handle the AQIC case.

I think what you wanted to say is the following:
Today (without the patch set) we will answer PQAP with an exception.
With this patch set we want to handle FC==3, but nothing else. So for anything FC!=3 we
will continue to return an exception?

Correct?

Pierre Morel March 1, 2019, 12:10 p.m. UTC | #31

On 28/02/2019 17:51, Halil Pasic wrote:
> On Thu, 28 Feb 2019 15:12:16 +0100
> Pierre Morel <[e-mail]> wrote:
> 
>> On 28/02/2019 13:39, Halil Pasic wrote:
>>> On Thu, 28 Feb 2019 10:42:23 +0100
>>> Christian Borntraeger <[e-mail]> wrote:
> [..]
>>>> Correct?
>>>
>>> IMHO mostly.
>>>
>>> I also doing the facility checks in kvm is easier, and I think this is
>>> something we can change later if needed without any major trouble.
>>>
>>> There are a couple of things I would do differently than Pierre does:
>>> 1) Do the PGM_PRIVILEGED_OP before the fc == 3 check.
>>
>> Idea was not to modify existing behavior for fc != 3
>>
>> Also Christian already proposed to handle all FC codes. So in this idea,
>> this must be done as you say.
>>
>>>
>>> 2) Do the test_kvm_facility(vcpu->kvm, 65) check in the context of fc ==
>>> 3. I.e. decide if this hook is about pqap or just about pqap aqic and
>>> make the code convey that decision to its reader.
>>>
>>> 3) I would most probably test if the queue is available by looking at the
>>> masks in CRYCB here. If not AP_RESPONSE_Q_NOT_AVAIL is what we need.
>>
>> This I do not agree with, it is typically the responsibility of the part
>> in charge of the virtualization to do this, also the vfio_driver.
>>
> 
> See at 4) regarding the details. My guess is you disagree with checking
> CRYCB explicitly but don't digress with AP_RESPONSE_Q_NOT_AVAIL if APCB
> does not authorize the queue. Your idea was to infer APCB all zero from
> the fact that pqap_hook is NULL.
> 
> If my assumption is right, then yes we can have an implicit coarse check
> here and a fine grained check in the client code (vfio_ap).




> 
>>>
>>> 4) If we have APIE and queues authorized by the CRYCB (i.e. we have a
>>> vfio_ap module loaded an an mdev associated with the kvm) the callback
>>> not set (!(vcpu->kvm->arch.crypto.pqap_hook)) is a BUG!
>>
>> I do not agree with this either, the maintainers ;) will not allow this.
> 
> After an offline discussion we came to the conclusion that I did not
> understand your code.
> 
> Your train of thought was:
> 
> !(vcpu->kvm->arch.crypto.pqap_hook) _implies_ APCB all zero (i.e. the
> masks in the CRYCB
> 
> This is *why* you respond with AP_RESPONSE_Q_NOT_AVAIL.
> 
> However if that is the case I would like that spelled out in a code
> comment at least. Furthermore setting pqap_hook and APCB needs to happen
> in the right sequence. Means client code (vfio_ap) may only set APCB
> after the qpap_hook has been set. Currently we have a race there (as
> you first do  kvm_arch_crypto_set_masks and only then
> kvm->arch.crypto.pqap_hook. Furthermore I guess
> kvm->arch.crypto.pqap_hook needs to be set with the kvm lock held, which
> does not seem to be the case.


Yes, that is right.
This part (setting/resetting hook and CRYCB will be modified for the 
reason you mention and also to correctly handle the order of releasing 
KVM and VFIO, as you and Christian mentioned.




> 
>>
>>> In that case
>>> lying that the queue is not available does not seem right. BTW this
>>> is something Pierre changed since the last version quietly (I can't
>>> recall a mention in the change log or somebody asking for this). If
>>> we want to be very pedantic about this bug scenario our best bet is
>>> probably response code 6.
>>
>>
>> RC 06 means "Invalid address of AP-queue notification byte"
>>
>> So you must have think about another code or I do not understand at
>> all what you mean.
>>
> 
> I did not assume you decided to ignore the possibility of a programming
> error (which you at least technically did commit yourself) for what I
> described as a BUG.
> 
> My train of thought was, if we are very pedantic we can make things work
> with degraded functionality in that case. I.e. without AP interrupts.
> For that we need to tell the guest something like: yes your queue is
> fine and there and all that but AQCI setup interrupts did not work. And
> RC 06 is the only RC I see being suitable to convey that.
> 
> Detect and handle if the client code does not hold up their end of the
> bargain or just ignore the possibility is a design decision. But at least
> you should spell out your expectations against the client code.
> 
> Regards,
> Halil
> 

I prefer to comment the obligation for the vfio_driver to register the 
callback instead to add code complexity for which will eventually go 
deeper and deeper.


Thanks,
Pierre

Cornelia Huck March 1, 2019, 12:36 p.m. UTC | #32

On Fri, 1 Mar 2019 13:05:54 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 01.03.2019 13:03, Pierre Morel wrote:
> > On 28/02/2019 15:14, Pierre Morel wrote:  
> >> On 28/02/2019 14:52, Cornelia Huck wrote:  
> >>> On Thu, 28 Feb 2019 14:16:09 +0100
> >>> Pierre Morel <[e-mail]> wrote:
> >>>  
> >>>> On 28/02/2019 12:22, Cornelia Huck wrote:  
> >>>  
> >>>>> So, to summarize, the function should do:
> >>>>> - Is userspace supposed to emulate everything (!ECA_APIE)? Return
> >>>>>     -EOPNOTSUPP to hand control to it.
> >>>>> - We are now interpreting the instruction in KVM. Do common checks
> >>>>>     (PSTATE etc.) and inject exceptions, if needed.
> >>>>> - Now look at the fc; if there's a handler for it, call that; if not
> >>>>>     (case does not attempt to call a specific handler, or no handler
> >>>>>     registered), inject a specification exception. (Do we want pre-checks
> >>>>>     like for facility 65 here, or in the handler?)
> >>>>>
> >>>>> That response code 0x01 thingy probably needs to go into the specific
> >>>>> handler function, if anywhere (don't know the semantics, sorry).  
> >>>>
> >>>> What do you mean with specific handler function?
> >>>>
> >>>> If you mean a switch around the FC with static function's call, I agree,
> >>>> if you mean a jump into a hook I do not agree.  
> >>>
> >>> Ah, ok; so each case (that we want to handle) should call into a
> >>> subhandler that does
> >>> {
> >>>     (... check things like facilities ...)
> >>>     if (!specific_hook)
> >>>         inject_specif_excp_and_return();
> >>>     ret = specific_hook();
> >>>     if (ret)
> >>>         set_resp_code_0x01(); // or in specific_hook()?
> >>> }
> >>>
> >>> ?  
> >>
> >> Yes something in this direction.  
> > 
> > Sorry, after reflection, no, we do not want to change the previous behavior so we only handle the AQIC case.  
> 
> I think what you wanted to say is the following:
> Today (without the patch set) we will answer PQAP with an exception.
> With this patch set we want to handle FC==3, but nothing else. So for anything FC!=3 we
> will continue to return an exception?
> 
> Correct?
> 

That sounds reasonable; but I don't see how this conflicts with my
proposal? Just don't introduce a subfunction for fc != 3...

Pierre Morel March 1, 2019, 3:32 p.m. UTC | #33

On 01/03/2019 13:36, Cornelia Huck wrote:
> On Fri, 1 Mar 2019 13:05:54 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 01.03.2019 13:03, Pierre Morel wrote:
>>> On 28/02/2019 15:14, Pierre Morel wrote:
>>>> On 28/02/2019 14:52, Cornelia Huck wrote:
>>>>> On Thu, 28 Feb 2019 14:16:09 +0100
>>>>> Pierre Morel <[e-mail]> wrote:
>>>>>   
>>>>>> On 28/02/2019 12:22, Cornelia Huck wrote:
>>>>>   
>>>>>>> So, to summarize, the function should do:
>>>>>>> - Is userspace supposed to emulate everything (!ECA_APIE)? Return
>>>>>>>      -EOPNOTSUPP to hand control to it.
>>>>>>> - We are now interpreting the instruction in KVM. Do common checks
>>>>>>>      (PSTATE etc.) and inject exceptions, if needed.
>>>>>>> - Now look at the fc; if there's a handler for it, call that; if not
>>>>>>>      (case does not attempt to call a specific handler, or no handler
>>>>>>>      registered), inject a specification exception. (Do we want pre-checks
>>>>>>>      like for facility 65 here, or in the handler?)
>>>>>>>
>>>>>>> That response code 0x01 thingy probably needs to go into the specific
>>>>>>> handler function, if anywhere (don't know the semantics, sorry).
>>>>>>
>>>>>> What do you mean with specific handler function?
>>>>>>
>>>>>> If you mean a switch around the FC with static function's call, I agree,
>>>>>> if you mean a jump into a hook I do not agree.
>>>>>
>>>>> Ah, ok; so each case (that we want to handle) should call into a
>>>>> subhandler that does
>>>>> {
>>>>>      (... check things like facilities ...)
>>>>>      if (!specific_hook)
>>>>>          inject_specif_excp_and_return();
>>>>>      ret = specific_hook();
>>>>>      if (ret)
>>>>>          set_resp_code_0x01(); // or in specific_hook()?
>>>>> }
>>>>>
>>>>> ?
>>>>
>>>> Yes something in this direction.
>>>
>>> Sorry, after reflection, no, we do not want to change the previous behavior so we only handle the AQIC case.
>>
>> I think what you wanted to say is the following:
>> Today (without the patch set) we will answer PQAP with an exception.
>> With this patch set we want to handle FC==3, but nothing else. So for anything FC!=3 we
>> will continue to return an exception?
>>
>> Correct?

Yes correct.
Thanks for the much preciser explanation.

>>
> 
> That sounds reasonable; but I don't see how this conflicts with my
> proposal? Just don't introduce a subfunction for fc != 3...
> 

Correct too, it does not conflict, as you said it is just not introduce 
subfunctions.

Regards,
Pierre

Patch

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;
+	/* 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;
+
+	/* 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)
+		return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
+
+	/* PQAP/AQIC instructions are authorized but there is no queue */
+	status.response_code = 0x01;
+	memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
+	return 0;
+}
+
 static int handle_stfl(struct kvm_vcpu *vcpu)
 {
 	int rc;
@@ -878,6 +928,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: