Message-Id: <d56d5c55-2e46-67b3-4351-015218a39810@linux.vnet.ibm.com>
Date:   Sun, 22 Apr 2018 17:11:07 -0400
From:   Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
To:     Pierre Morel <pmorel@...ux.vnet.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     freude@...ibm.com, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, borntraeger@...ibm.com,
        cohuck@...hat.com, kwankhede@...dia.com,
        bjsdjshi@...ux.vnet.ibm.com, pbonzini@...hat.com,
        alex.williamson@...hat.com, alifm@...ux.vnet.ibm.com,
        mjrosato@...ux.vnet.ibm.com, jjherne@...ux.vnet.ibm.com,
        thuth@...hat.com, pasic@...ux.vnet.ibm.com, berrange@...hat.com,
        fiuczy@...ux.vnet.ibm.com, buendgen@...ibm.com
Subject: Re: [PATCH v4 03/15] KVM: s390: refactor crypto initialization

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@...ux.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)
>>>
>>>
>>
>

Powered by blists - more mailing lists