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

[Pierre Morel <pmorel@linux.ibm.com>]

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@linux.vnet.ibm.com>
> ---
>   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;
> +
> +	/* 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))


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