lkml.org 
[lkml]   [2018]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v9 07/22] KVM: s390: refactor crypto initialization
    From
    Date
    On 08/20/2018 12:41 PM, David Hildenbrand wrote:
    > On 13.08.2018 23:48, Tony Krowiak wrote:
    >> From: Tony Krowiak <akrowiak@linux.ibm.com>
    >>
    >> This patch refactors the code that initializes and sets up the
    >> crypto configuration for a guest. The following changes are
    >> implemented via this patch:
    >>
    >> 1. Prior to the introduction of AP device virtualization, it
    >> was not necessary to provide guest access to the CRYCB
    >> unless the MSA extension 3 (MSAX3) facility was installed
    >> on the host system. With the introduction of AP device
    >> virtualization, the CRYCB must be made accessible to the
    >> guest if the AP instructions are installed on the host
    >> and are to be provided to the guest.
    >>
    >> 2. Introduces a flag indicating AP instructions executed on
    >> the guest shall be interpreted by the firmware. It is
    >> initialized to indicate AP instructions are to be
    >> to be interpreted and is used to set the SIE bit for
    >> each vcpu during vcpu setup.
    >>
    >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
    >> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
    >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
    >> Acked-by: Janosch Frank <frankja@linux.ibm.com>
    >> Tested-by: Michael Mueller <mimu@linux.ibm.com>
    >> Tested-by: Farhan Ali <alifm@linux.ibm.com>
    >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
    >> ---
    >> arch/s390/include/asm/kvm_host.h | 2 +
    >> arch/s390/include/uapi/asm/kvm.h | 1 +
    >> arch/s390/kvm/kvm-s390.c | 81 +++++++++++++++++++-------------------
    >> 3 files changed, 44 insertions(+), 40 deletions(-)
    >>
    >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
    >> index af39561..4a739d4 100644
    >> --- a/arch/s390/include/asm/kvm_host.h
    >> +++ b/arch/s390/include/asm/kvm_host.h
    >> @@ -187,6 +187,7 @@ struct kvm_s390_sie_block {
    >> #define ECA_AIV 0x00200000
    >> #define ECA_VX 0x00020000
    >> #define ECA_PROTEXCI 0x00002000
    >> +#define ECA_APIE 0x00000008
    >> #define ECA_SII 0x00000001
    >> __u32 eca; /* 0x004c */
    >> #define ICPT_INST 0x04
    >> @@ -256,6 +257,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/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
    >> index 4cdaa55..a580dec 100644
    >> --- a/arch/s390/include/uapi/asm/kvm.h
    >> +++ b/arch/s390/include/uapi/asm/kvm.h
    >> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
    >> #define KVM_S390_VM_CPU_FEAT_PFMFI 11
    >> #define KVM_S390_VM_CPU_FEAT_SIGPIF 12
    >> #define KVM_S390_VM_CPU_FEAT_KSS 13
    >> +#define KVM_S390_VM_CPU_FEAT_AP 14
    >> struct kvm_s390_vm_cpu_feat {
    >> __u64 feat[16];
    >> };
    >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
    >> index 19f4f44..f7de123 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/ap.h>
    >> #include "kvm-s390.h"
    >> #include "gaccess.h"
    >>
    >> @@ -1881,49 +1882,37 @@ 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;
    >> + struct ap_config_info info;
    >>
    >> - 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;
    >> + if (ap_instructions_available()) {
    >> + if (ap_qci(&info) == 0)
    >> + return info.apxa;
    >> }
    >>
    >> return 0;
    >> }
    >>
    >> +/*
    >> + * 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_set_crycb_format(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_s390_apxa_installed())
    >> kvm->arch.crypto.crycbd |= CRYCB_FORMAT2;
    >> else
    >> @@ -1941,12 +1930,12 @@ static u64 kvm_s390_get_initial_cpuid(void)
    >>
    >> static void kvm_s390_crypto_init(struct kvm *kvm)
    >> {
    >> - if (!test_kvm_facility(kvm, 76))
    >> - return;
    >> -
    >> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
    >> kvm_s390_set_crycb_format(kvm);
    >>
    >> + if (!test_kvm_facility(kvm, 76))
    >> + return;
    >> +
    >> /* Enable AES/DEA protected key functions by default */
    >> kvm->arch.crypto.aes_kw = 1;
    >> kvm->arch.crypto.dea_kw = 1;
    >> @@ -2474,17 +2463,29 @@ 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 neither the AP instructions nor the MSAX3 facility are installed
    >> + * on the host, then there is no need for a CRYCB in SIE because they
    >> + * will not be installed on the guest either.
    >> + */
    >> + if (!test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_AP) &&
    >> + !test_facility(76))
    > I think this can just stay "test_kvm_facility(vcpu->kvm, 76)", and the
    > comment should be changed to
    >
    > "If neither the AP instructions nor the MSAX3 facility are configured
    > for the guest, there is nothing to set up."
    >
    > Or am I missing something important here?

    No, you're not missing anything, it can be test_kvm_facility() and I'm
    more than happy to change the comment.

    >
    > After the CPU has been created, both feature can no longer change. The
    > only thing that might change is kvm->arch.crypto.aes_kw/dea_kw but only
    > with test_kvm_facility(vcpu->kvm, 76).
    >
    >> return;
    >>
    >> + vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
    >> vcpu->arch.sie_block->ecb3 &= ~(ECB3_AES | ECB3_DEA);
    >>
    >> - 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;
    >> + vcpu->arch.sie_block->eca &= ~ECA_APIE;
    > As this feature can never flip, clearing the flag is not necessary.

    Okay.

    >
    >> + if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_AP))
    >> + vcpu->arch.sie_block->eca |= ECA_APIE;
    >>
    >> - vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
    >> + /* If MSAX3 is installed on the guest, set up protected key support */
    >> + if (test_kvm_facility(vcpu->kvm, 76)) {
    >> + 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;
    >> + }
    > As the feature can never change, and aes_kw/dea_kw are only set to 1 in
    > case we have test_kvm_facility(vcpu->kvm, 76), this change is not needed.
    >
    > I think this function can be pretty much left alone. Just add the
    > KVM_S390_VM_CPU_FEAT_AP handling.

    I disagree, what about the case where the KVM_S390_VM_CPU_FEAT_AP is
    configured for the guest but the MSAX3 facility (76) is not?

    >
    >> }
    >>
    >> void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu)
    >>
    >

    \
     
     \ /
      Last update: 2018-08-20 22:34    [W:3.164 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site