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

From: Tony Krowiak
Date: Fri Mar 29 2019 - 09:02:47 EST

On 3/29/19 4:52 AM, Pierre Morel wrote:
On 28/03/2019 17:12, Tony Krowiak wrote:
On 3/22/19 10:43 AM, Pierre Morel wrote:
We prepare the interception of the PQAP/AQIC instruction for
the case the AQIC facility is enabled in the guest.

First of all we do not want to change existing behavior when
intercepting AP instructions without the SIE allowing the guest
to use AP instructions.

In this patch we only handle the AQIC interception allowed by
facility 65 which will be enabled when the complete interception
infrastructure will be present.

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 and only this command.

But we want to be able to return a correct answer to the guest
even there is no VFIO AP driver in the kernel.
Therefor, 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.

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

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
 arch/s390/include/asm/kvm_host.h | 8 ++++
 arch/s390/kvm/priv.c | 90 +++++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h | 2 +
 3 files changed, 100 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index a496276..624460b 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -18,6 +18,7 @@
 #include <linux/kvm_host.h>
 #include <linux/kvm.h>
 #include <linux/seqlock.h>
+#include <linux/module.h>
 #include <asm/debug.h>
 #include <asm/cpu.h>
 #include <asm/fpu/api.h>
@@ -721,8 +722,15 @@ struct kvm_s390_cpu_model {
ÂÂÂÂÂ unsigned short ibc;
 };
+struct kvm_s390_module_hook {
+ÂÂÂ int (*hook)(struct kvm_vcpu *vcpu);
+ÂÂÂ void *data;
+ÂÂÂ struct module *owner;
+};
+
 struct kvm_s390_crypto {
ÂÂÂÂÂ struct kvm_s390_crypto_cb *crycb;
+ÂÂÂ struct kvm_s390_module_hook *pqap_hook;
ÂÂÂÂÂ __u32 crycbd;
ÂÂÂÂÂ __u8 aes_kw;
ÂÂÂÂÂ __u8 dea_kw;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 8679bd7..793e48a 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,93 @@ 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 and TAPQ instructions, 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)
+{
+ÂÂÂ struct ap_queue_status status = {};
+ÂÂÂ unsigned long reg0;
+ÂÂÂ int ret;
+ÂÂÂ uint8_t fc;
+
+ÂÂÂ /* 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;
+ÂÂÂ /*
+ÂÂÂÂ * The only possibly intercepted instructions when AP instructions are
+ÂÂÂÂ * available for the guest are AQIC and TAPQ with the t bit set
+ÂÂÂÂ * since we do not set IC.3 (FIII) we currently will not intercept
+ÂÂÂÂ * TAPQ.
+ÂÂÂÂ * The following code will only treat AQIC function code.
+ÂÂÂÂ */
+ÂÂÂ reg0 = vcpu->run->s.regs.gprs[0];
+ÂÂÂ fc = reg0 >> 24;
+ÂÂÂ if (fc != 0x03) {
+ÂÂÂÂÂÂÂ pr_warn("%s: Unexpected interception code 0x%02x\n",
+ÂÂÂÂÂÂÂÂÂÂÂ __func__, fc);
+ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
+ÂÂÂ }
+ÂÂÂ /* All 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);
+ÂÂÂ /*
+ÂÂÂÂ * Common tests for PQAP instructions to generate a specification
+ÂÂÂÂ * exception
+ÂÂÂÂ */
+ÂÂÂ /* Zero bits overwrite produce a specification exception */
+ÂÂÂ if (reg0 & 0x007f0000UL)
+ÂÂÂÂÂÂÂ goto specification_except;
+ÂÂÂ /* If APXA is not installed APQN is limited */
+ÂÂÂ if (!(vcpu->kvm->arch.crypto.crycbd & 0x02))
+ÂÂÂÂÂÂÂ if (reg0 & 0x000030f0UL)
+ÂÂÂÂÂÂÂÂÂÂÂ goto specification_except;
+ÂÂÂ /* AQIC needs facility 65 */
+ÂÂÂ if (!test_kvm_facility(vcpu->kvm, 65))
+ÂÂÂÂÂÂÂ goto specification_except;
+
+ÂÂÂ /*
+ÂÂÂÂ * Verify that the hook callback is registered, lock the owner
+ÂÂÂÂ * and call the hook.
+ÂÂÂÂ */
+ÂÂÂ if (vcpu->kvm->arch.crypto.pqap_hook) {
+ÂÂÂÂÂÂÂ if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
+ÂÂÂÂÂÂÂÂÂÂÂ return -EOPNOTSUPP;
+ÂÂÂÂÂÂÂ ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
+ÂÂÂÂÂÂÂ module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+ÂÂÂ /*
+ÂÂÂÂ * It is the duty of the vfio_driver to register a hook
+ÂÂÂÂ * If it does not and we get an exception on AQIC we must
+ÂÂÂÂ * guess that there is no vfio_ap_driver at all and no one
+ÂÂÂÂ * to handle the guests's CRYCB and the CRYCB is empty.
+ÂÂÂÂ */
+ÂÂÂ status.response_code = 0x01;
+ÂÂÂ memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
+ÂÂÂ return 0;
+
+specification_except:
+ÂÂÂ return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+}
+
 static int handle_stfl(struct kvm_vcpu *vcpu)
 {
ÂÂÂÂÂ int rc;
@@ -878,6 +966,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:
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 76b7f98..a910be1 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -16,6 +16,7 @@
 #include <linux/mdev.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
+#include <linux/kvm_host.h>
 #include "ap_bus.h"
@@ -81,6 +82,7 @@ struct ap_matrix_mdev {
ÂÂÂÂÂ struct ap_matrix matrix;
ÂÂÂÂÂ struct notifier_block group_notifier;
ÂÂÂÂÂ struct kvm *kvm;
+ÂÂÂ struct kvm_s390_module_hook pqap_hook;

I don't understand the purpose of adding this field. We set up the
the kvm->arch.crypto.pqap_hook in the vfio_ap_mdev_set_kvm which is
also in this same file, why not just use a static struct kvm_s390_module_hook and reuse it when setting up
kvm->arch.crypto.pqap_hook? It saves you from initializing it every
time an ap_matrix_mdev is created.

Having this field embedded in the matrix_mdev allows to easily retrieve the matrix_mdev from the the hook.

The only place you do this is in the handle_pqap hook. The reason you
get the matrix_mdev there is to get the vfio_ap_queue object from the
matrix_mdev qlist. You could just as well have gotten the vfio_ap_queue
using the vfio_ap_find_queue() function.


Thanks,
Pierre