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

Tony Krowiak
2018-04-22T21:11:07Z

On 04/17/2018 11:52 AM, Pierre Morel wrote:
> 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@linux.vnet.ibm.com>
>>>> ---
>>>> 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.

The logic needs to be changed:

If neither the AP instructions nor MSAX3 is installed
return
Set the address of the CRYCB into the CRYCBD
Clear the format mask in the CRYCBD - i.e., set to format 0 by default
If MSAX3 is installed
If APXA is installed
Set format 2
Else
Set format 1
Set up protected key support (i.e., key wrapping)

>
>
> 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()

I agree, I am going to do that.

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