Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

From: Tony Krowiak
Date: Tue Feb 19 2019 - 17:51:11 EST


On 2/19/19 2:50 PM, Pierre Morel wrote:
On 18/02/2019 23:42, Cornelia Huck wrote:
On Mon, 18 Feb 2019 19:29:10 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

On 15/02/2019 23:02, Tony Krowiak wrote:
On 2/14/19 8:51 AM, Pierre Morel wrote:

+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *ÂÂ AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+ÂÂÂ uint8_t fc;
+
+ÂÂÂ /* Verify that the hook callback is registered */
+ÂÂÂ if (!vcpu->kvm->arch.crypto.pqap_hook)
+ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
+ÂÂÂ /* 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;

This does not belong here. Function code 3 is one of 7 function codes
that can be sent with the PQAP instruction. This belongs in the PQAP
hook code.

On one hand, effectively I would prefer to put the code in the VFIO
driver code.
On the other hand, doing this would lead to export the code for
test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

Looking at this (and without access to the specification...), I think
the check for problem state makes sense in here (if this applies to all
PQAP functions equally, which seems likely). The check for the facility
makes more sense in the handler. You can probably still inject the
specification exception here if you use a clever return code.


If there is no objection on exporting the KVM functions... I can do this.

I think I understand where you are coming from. In looking back at the
original patch, I see there are checks using the test_kvm_facility and
kvm_s390_inject_program_int functions placed after your check for
fc != 0x03. You clearly misunderstood what I was asking you to do.
I was suggesting that ONLY the check for 'fc != 0x03' be done in the
hook. I was NOT suggesting the instructions following the check for
fc != 0x03 be done in the hook, so there is no need to export any KVM
functions.



Another option: Provide a way to register a callback per function code;
this allows you to still do the check here and extend it later for
other function codes (which will probably be indicated by another
facility).

I like this option even better.

Regards,
Pierre