Re: [PATCH v5 02/13] KVM: s390: refactor crypto initialization

From: Pierre Morel
Date: Wed May 16 2018 - 07:45:26 EST

On 16/05/2018 13:14, Tony Krowiak wrote:
On 05/16/2018 04:51 AM, Pierre Morel wrote:
On 07/05/2018 17:11, 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 as long as the
AP instructions are installed on the host.

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>
---
 arch/s390/include/asm/kvm_host.h | 1 +
 arch/s390/kvm/kvm-s390.c | 64 ++++++++++++++++++++++++++-----------
 2 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 81cdb6b..5393c4d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -255,6 +255,7 @@ struct kvm_s390_sie_block {
ÂÂÂÂÂ __u8ÂÂÂ reservede4[4];ÂÂÂÂÂÂÂ /* 0x00e4 */
ÂÂÂÂÂ __u64ÂÂÂ tecmc;ÂÂÂÂÂÂÂÂÂÂÂ /* 0x00e8 */
ÂÂÂÂÂ __u8ÂÂÂ reservedf0[12];ÂÂÂÂÂÂÂ /* 0x00f0 */
+#define CRYCB_FORMAT_MASK 0x00000003
 #define CRYCB_FORMAT1 0x00000001
 #define CRYCB_FORMAT2 0x00000003
ÂÂÂÂÂ __u32ÂÂÂ crycbd;ÂÂÂÂÂÂÂÂÂÂÂ /* 0x00fc */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 1f50de7..99779a6 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1875,14 +1875,35 @@ long kvm_arch_vm_ioctl(struct file *filp,
ÂÂÂÂÂ return r;
 }

-static void kvm_s390_set_crycb_format(struct kvm *kvm)
+/*
+ * The format of the crypto control block (CRYCB) is specified in the 3 low
+ * order bits of the CRYCB designation (CRYCBD) field as follows:
+ * Format 0: Neither the message security assist extension 3 (MSAX3) nor the
+ *ÂÂÂÂÂÂÂÂÂÂÂÂ AP extended addressing (APXA) facility are installed.
+ * Format 1: The APXA facility is not installed but the MSAX3 facility is.
+ * Format 2: Both the APXA and MSAX3 facilities are installed
+ */
+static void kvm_s390_format_crycb(struct kvm *kvm)
 {
-ÂÂÂ kvm->arch.crypto.crycbd = (__u32)(unsigned long) kvm->arch.crypto.crycb;
+ÂÂÂ /* Clear the CRYCB format bits - i.e., set format 0 by default */
+ÂÂÂ kvm->arch.crypto.crycbd &= ~(CRYCB_FORMAT_MASK);
+
+ÂÂÂ /* Check whether MSAX3 is installed */
+ÂÂÂ if (!test_kvm_facility(kvm, 76))
+ÂÂÂÂÂÂÂ return;

ÂÂÂÂÂ if (kvm_ap_apxa_installed())
ÂÂÂÂÂÂÂÂÂ kvm->arch.crypto.crycbd |= CRYCB_FORMAT2;
ÂÂÂÂÂ else
ÂÂÂÂÂÂÂÂÂ kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
+ one lin
+ÂÂÂ /* Enable AES/DEA protected key functions by default */
+ÂÂÂ kvm->arch.crypto.aes_kw = 1;
+ÂÂÂ kvm->arch.crypto.dea_kw = 1;
+ get_random_bytes(kvm->arch.crypto.crycb->aes_wrapping_key_mask,
+ sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
+ get_random_bytes(kvm->arch.crypto.crycb->dea_wrapping_key_mask,
+ sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
 }

 static u64 kvm_s390_get_initial_cpuid(void)
@@ -1896,19 +1917,17 @@ static u64 kvm_s390_get_initial_cpuid(void)

 static void kvm_s390_crypto_init(struct kvm *kvm)
 {
-ÂÂÂ if (!test_kvm_facility(kvm, 76))
+ÂÂÂ /*
+ÂÂÂÂ * If neither the AP instructions nor the message security assist
+ÂÂÂÂ * extension 3 (MSAX3) are installed, there is no need to initialize a
+ÂÂÂÂ * crypto control block (CRYCB) for the guest.
+ÂÂÂÂ */
+ÂÂÂ if (!kvm_ap_instructions_available() && !test_kvm_facility(kvm, 76))
ÂÂÂÂÂÂÂÂÂ return;

ÂÂÂÂÂ kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
-ÂÂÂ kvm_s390_set_crycb_format(kvm);


For my point of view the all patch can be reduced to putting this
call (kvm_s390_set_crycb_format(kvm);) before testing for facility 76.

(and setting the format correctly in kvm_s390_set_crycb_format(kvm))

I don't see what that buys us; it will just be reshuffling of the logic.
The idea here is that all of the code related to formatting the CRYCB for
use by the guest is contained in the kvm_s390_format_crycb(kvm) function.
We don't need a CRYCB, however, if the AP instructions are not installed
and the MSAX3 facility is not installed, so why even call
kvm_s390_format_crycb(kvm) in that case?

It byes a lot of lines.
I mean that you do exactly the same by only using 3 lines inserted instead of 65 changes.
No logic change.





-
-ÂÂÂ /* Enable AES/DEA protected key functions by default */
-ÂÂÂ kvm->arch.crypto.aes_kw = 1;
-ÂÂÂ kvm->arch.crypto.dea_kw = 1;
- get_random_bytes(kvm->arch.crypto.crycb->aes_wrapping_key_mask,
- sizeof(kvm->arch.crypto.crycb->aes_wrapping_key_mask));
- get_random_bytes(kvm->arch.crypto.crycb->dea_wrapping_key_mask,
- sizeof(kvm->arch.crypto.crycb->dea_wrapping_key_mask));
+ÂÂÂ kvm->arch.crypto.crycbd = (__u32)(unsigned long) kvm->arch.crypto.crycb;
+ÂÂÂ kvm_s390_format_crycb(kvm);
 }

 static void sca_dispose(struct kvm *kvm)
@@ -2430,17 +2449,24 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)

 static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
 {
-ÂÂÂ if (!test_kvm_facility(vcpu->kvm, 76))
+ÂÂÂ /*
+ÂÂÂÂ * If a crypto control block designation (CRYCBD) has not been
+ÂÂÂÂ * initialized
+ÂÂÂÂ */
+ÂÂÂ if (vcpu->kvm->arch.crypto.crycbd == 0)
ÂÂÂÂÂÂÂÂÂ return;

-ÂÂÂ vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);
+ÂÂÂ vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;

-ÂÂÂ if (vcpu->kvm->arch.crypto.aes_kw)
-ÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 |= ECB3_AES;
-ÂÂÂ if (vcpu->kvm->arch.crypto.dea_kw)
-ÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 |= ECB3_DEA;
+ÂÂÂ /* If MSAX3 is installed, set up protected key support */
+ÂÂÂ if (test_kvm_facility(vcpu->kvm, 76)) {
+ÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);

-ÂÂÂ vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
+ÂÂÂÂÂÂÂ if (vcpu->kvm->arch.crypto.aes_kw)
+ÂÂÂÂÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 |= ECB3_AES;
+ÂÂÂÂÂÂÂ if (vcpu->kvm->arch.crypto.dea_kw)
+ÂÂÂÂÂÂÂÂÂÂÂ vcpu->arch.sie_block->ecb3 |= ECB3_DEA;
+ÂÂÂ }
 }

 void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu)




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