Re: [PATCH v4 03/15] KVM: s390: refactor crypto initialization

From: Pierre Morel
Date: Tue Apr 17 2018 - 11:53:09 EST

On 17/04/2018 16:15, Tony Krowiak wrote:
On 04/16/2018 04:56 AM, Pierre Morel wrote:
On 15/04/2018 23:22, Tony Krowiak wrote:
This patch refactors the code that initializes the crypto
configuration for a guest. The crypto configuration is contained in
a crypto control block (CRYCB) which is a satellite control block to
our main hardware virtualization control block. The CRYCB is
attached to the main virtualization control block via a CRYCB
designation (CRYCBD) designation field containing the address of
the CRYCB as well as its format.

Prior to the introduction of AP device virtualization, there was
no need to provide access to or specify the format of the CRYCB for
a guest unless the MSA extension 3 (MSAX3) facility was installed
on the host system. With the introduction of AP device virtualization,
the CRYCB and its format must be made accessible to the guest
regardless of the presence of the MSAX3 facility.

The crypto initialization code is restructured as follows:

* A new compilation unit is introduced to contain all interfaces
ÂÂ and data structures related to configuring a guest's CRYCB for
ÂÂ both the refactoring of crypto initialization as well as all
ÂÂ subsequent patches introducing AP virtualization support.

* Currently, the asm code for querying the AP configuration is
ÂÂ duplicated in the AP bus as well as in KVM. Since the KVM
ÂÂ code was introduced, the AP bus has externalized the interface
ÂÂ for querying the AP configuration. The KVM interface will be
ÂÂ replaced with a call to the AP bus interface. Of course, this
ÂÂ will be moved to the new compilation unit mentioned above.

* An interface to format the CRYCBD field will be provided via
ÂÂ the new compilation unit and called from the KVM vm
ÂÂ initialization.

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>
---
 arch/s390/include/asm/kvm-ap.h | 15 +++++++++
 arch/s390/include/asm/kvm_host.h | 1 +
 arch/s390/kvm/kvm-ap.c | 39 ++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c | 60 ++++----------------------------------
 4 files changed, 61 insertions(+), 54 deletions(-)

diff --git a/arch/s390/include/asm/kvm-ap.h b/arch/s390/include/asm/kvm-ap.h
index 84412a9..736e93e 100644
--- a/arch/s390/include/asm/kvm-ap.h
+++ b/arch/s390/include/asm/kvm-ap.h
@@ -10,6 +10,9 @@
 #ifndef _ASM_KVM_AP
 #define _ASM_KVM_AP

+#include <linux/types.h>
+#include <linux/kvm_host.h>
+
 /**
ÂÂ * kvm_ap_instructions_installed()
ÂÂ *
@@ -20,4 +23,16 @@
ÂÂ */
 int kvm_ap_instructions_installed(void);

+/**
+ * kvm_ap_build_crycbd
+ *
+ * The crypto control block designation (CRYCBD) is a 32-bit field that
+ * designates both the host real address and format of the CRYCB. This function
+ * builds the CRYCBD field for use by the KVM guest.
+ *
+ * @kvm:ÂÂÂ the KVM guest
+ * @crycbd:ÂÂÂ reference to the CRYCBD
+ */
+void kvm_ap_build_crycbd(struct kvm *kvm);
+
 #endif /* _ASM_KVM_AP */
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 81cdb6b..c990a1d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -257,6 +257,7 @@ struct kvm_s390_sie_block {
ÂÂÂÂÂ __u8ÂÂÂ reservedf0[12];ÂÂÂÂÂÂÂ /* 0x00f0 */
 #define CRYCB_FORMAT1 0x00000001
 #define CRYCB_FORMAT2 0x00000003
+#define CRYCB_FORMAT_MASK 0x00000003
ÂÂÂÂÂ __u32ÂÂÂ crycbd;ÂÂÂÂÂÂÂÂÂÂÂ /* 0x00fc */
ÂÂÂÂÂ __u64ÂÂÂ gcr[16];ÂÂÂÂÂÂÂ /* 0x0100 */
ÂÂÂÂÂ __u64ÂÂÂ gbea;ÂÂÂÂÂÂÂÂÂÂÂ /* 0x0180 */
diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
index 1267588..991bae4 100644
--- a/arch/s390/kvm/kvm-ap.c
+++ b/arch/s390/kvm/kvm-ap.c
@@ -10,6 +10,8 @@
 #include <asm/kvm-ap.h>
 #include <asm/ap.h>

+#include "kvm-s390.h"
+
 int kvm_ap_instructions_installed(void)
 {
 #ifdef CONFIG_ZCRYPT
@@ -19,3 +21,40 @@ int kvm_ap_instructions_installed(void)
 #endif
 }
 EXPORT_SYMBOL(kvm_ap_instructions_installed);
+
+static inline int kvm_ap_query_config(struct ap_config_info *config)
+{
+ÂÂÂ memset(config, 0, sizeof(*config));
+
+#ifdef CONFIG_ZCRYPT

I would prefer that you define the interface in an include file
with stubs for the case ZCRYPT is not set.

This is a static function only called internally, but I suppose there is
no harm in defining it as an interface in kvm-ap.h ... it may come
in handy down the road.



+ÂÂÂ if (kvm_ap_instructions_installed())
+ÂÂÂÂÂÂÂ return ap_query_configuration(config);
+#endif
+
+ÂÂÂ return -EOPNOTSUPP;
+}
+
+static int kvm_ap_apxa_installed(void)
+{
+ÂÂÂ struct ap_config_info config;
+
+ÂÂÂ if (kvm_ap_query_config(&config) == 0)
+ÂÂÂÂÂÂÂ return (config.apxa == 1);
+
+ÂÂÂ return 0;
+}
+
+void kvm_ap_build_crycbd(struct kvm *kvm)
+{
+ÂÂÂ kvm->arch.crypto.crycbd = (__u32)(unsigned long) kvm->arch.crypto.crycb;
+ÂÂÂ kvm->arch.crypto.crycbd &= ~(CRYCB_FORMAT_MASK);
+
+ÂÂÂ /* check whether MSAX3 is installed */

It means we do not support AP virtualization without MSA3.
It follows we do not support CRYCB_FORMAT0

If MSAX3 is not installed, that means there is no key wrapping support,
hence CRYCB_FORMAT0. The CRYCB_FORMAT1 and CRYCB_FORMAT2 CRYCBs
both include wrapping key masks. I don't follow your logic here.



It is different from what you explain in the comment.

How is it different? Above, we are setting the CRYCBD value regardless
of whether MSAX3 is installed or not. Previously, the CRYCBD value
was set only if MSAX3 is installed (see comments below)



+ÂÂÂ if (kvm_ap_instructions_installed() && test_kvm_facility(kvm, 76)) {
+ÂÂÂÂÂÂÂ if (kvm_ap_apxa_installed())
+ÂÂÂÂÂÂÂÂÂÂÂ kvm->arch.crypto.crycbd |= CRYCB_FORMAT2;
+ÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂ kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
+ÂÂÂ }

sorry, I was fooled by the test on kvm_instructions_installed() and that CRYCB_FORMAT0 = 0.
since you cleared the format above it is 0 by default.

Since we can not use CRYCB_FORMAT0 if we have no AP instructions, the logic of the test
seems false even the result is right.

I think you can make it more readable if you put all the crycb initialization together
inside the kvm_s390_crypto_init() function instead of exporting part of it inside
kvm_ap_build_crycbd()

Regards,

Pierre

+}
+EXPORT_SYMBOL(kvm_ap_build_crycbd);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d0c3518..b47ff11 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -40,6 +40,7 @@
 #include <asm/sclp.h>
 #include <asm/cpacf.h>
 #include <asm/timex.h>
+#include <asm/kvm-ap.h>
 #include "kvm-s390.h"
 #include "gaccess.h"

@@ -1881,55 +1882,6 @@ long kvm_arch_vm_ioctl(struct file *filp,
ÂÂÂÂÂ return r;
 }

-static int kvm_s390_query_ap_config(u8 *config)
-{
-ÂÂÂ u32 fcn_code = 0x04000000UL;
-ÂÂÂ u32 cc = 0;
-
-ÂÂÂ memset(config, 0, 128);
-ÂÂÂ asm volatile(
-ÂÂÂÂÂÂÂ "lgr 0,%1\n"
-ÂÂÂÂÂÂÂ "lgr 2,%2\n"
-ÂÂÂÂÂÂÂ ".long 0xb2af0000\n"ÂÂÂÂÂÂÂ /* PQAP(QCI) */
-ÂÂÂÂÂÂÂ "0: ipm %0\n"
-ÂÂÂÂÂÂÂ "srl %0,28\n"
-ÂÂÂÂÂÂÂ "1:\n"
-ÂÂÂÂÂÂÂ EX_TABLE(0b, 1b)
-ÂÂÂÂÂÂÂ : "+r" (cc)
-ÂÂÂÂÂÂÂ : "r" (fcn_code), "r" (config)
-ÂÂÂÂÂÂÂ : "cc", "0", "2", "memory"
-ÂÂÂ );
-
-ÂÂÂ return cc;
-}
-
-static int kvm_s390_apxa_installed(void)
-{
-ÂÂÂ u8 config[128];
-ÂÂÂ int cc;
-
-ÂÂÂ if (test_facility(12)) {
-ÂÂÂÂÂÂÂ cc = kvm_s390_query_ap_config(config);
-
-ÂÂÂÂÂÂÂ if (cc)
-ÂÂÂÂÂÂÂÂÂÂÂ pr_err("PQAP(QCI) failed with cc=%d", cc);
-ÂÂÂÂÂÂÂ else
-ÂÂÂÂÂÂÂÂÂÂÂ return config[0] & 0x40;
-ÂÂÂ }
-
-ÂÂÂ return 0;
-}
-
-static void kvm_s390_set_crycb_format(struct kvm *kvm)
-{
-ÂÂÂ kvm->arch.crypto.crycbd = (__u32)(unsigned long) kvm->arch.crypto.crycb;
-
-ÂÂÂ if (kvm_s390_apxa_installed())
-ÂÂÂÂÂÂÂ kvm->arch.crypto.crycbd |= CRYCB_FORMAT2;
-ÂÂÂ else
-ÂÂÂÂÂÂÂ kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
-}
-
 static u64 kvm_s390_get_initial_cpuid(void)
 {
ÂÂÂÂÂ struct cpuid cpuid;
@@ -1941,12 +1893,12 @@ static u64 kvm_s390_get_initial_cpuid(void)

 static void kvm_s390_crypto_init(struct kvm *kvm)
 {
+ÂÂÂ kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
+ÂÂÂ kvm_ap_build_crycbd(kvm);
+

Notice the call to kvm_ap_build_crycbd(kvm) above was added, so
the CRYCBD is being set regardless of the presence of MSAX3.

ÂÂÂÂÂ if (!test_kvm_facility(kvm, 76))
ÂÂÂÂÂÂÂÂÂ return;

-ÂÂÂ kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
-ÂÂÂ kvm_s390_set_crycb_format(kvm);
Notice that this code that was removed to set the CRYCBD is called
only if MSAX3 is not installed - i.e., see the if statement
immediately preceding the two statements above.
-
ÂÂÂÂÂ /* Enable AES/DEA protected key functions by default */
ÂÂÂÂÂ kvm->arch.crypto.aes_kw = 1;
ÂÂÂÂÂ kvm->arch.crypto.dea_kw = 1;
@@ -2475,6 +2427,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)

 static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
 {
+ÂÂÂ vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
+
ÂÂÂÂÂ if (!test_kvm_facility(vcpu->kvm, 76))
ÂÂÂÂÂÂÂÂÂ return;

@@ -2484,8 +2438,6 @@ static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
ÂÂÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 |= ECB3_AES;
ÂÂÂÂÂ if (vcpu->kvm->arch.crypto.dea_kw)
ÂÂÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 |= ECB3_DEA;
-
-ÂÂÂ vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
 }

 void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu)




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