lkml.org 
[lkml]   [2018]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v8 07/22] KVM: s390: refactor crypto initialization
    From
    Date
    On 08/09/2018 01:58 AM, Janosch Frank wrote:
    > On 08.08.2018 16:44, 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>
    >> 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>
    > Acked-by: Janosch Frank <frankja@linux.ibm.com>
    >
    >> ---
    >> arch/s390/include/asm/kvm_host.h | 3 +
    >> arch/s390/include/uapi/asm/kvm.h | 1 +
    >> arch/s390/kvm/kvm-s390.c | 86 ++++++++++++++++++++------------------
    >> 3 files changed, 49 insertions(+), 41 deletions(-)
    >>
    >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
    >> index af39561..0c13f61 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 */
    >> @@ -714,6 +716,7 @@ struct kvm_s390_crypto {
    >> __u32 crycbd;
    >> __u8 aes_kw;
    >> __u8 dea_kw;
    >> + __u8 apie;
    > In the last review I wanted a comment here to know what they do.

    I'm not sure what the 'they' are that you reference here. I couldn't
    find your review comment but I assume you are looking for a comment
    explaining what the 'apie' field is used for. I am removing
    the 'apie' field based on a review comment by David, so there is
    no longer a need for a comment here, assuming that is what you
    are referring to.

    >
    >> 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 the
    >> + * they will not be installed on the guest either.
    > the they

    I'll fix this grammatical error.

    >
    >> + */
    >> + if (ap_instructions_available() && !test_facility(76))
    >> return;
    > I know you're not responsible for that one :) but 0 being the wanted
    > value here is a bit counter-intuitive.

    Based on another review comment by David, I've changed this to:

    if (!test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_AP) &&
    !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;
    >> +
    >> + vcpu->arch.sie_block->eca &= ~ECA_APIE;
    > The scb is zero allocated, are the ECA and the ECB3s set somewhere
    > in-between, or is that your way of making sure the controls are
    > definitely gone for good?

    It is a bit of defensive programming. There is a KVM_SET_DEVICE_ATTR
    ioctl to set crypto attributes (KVM_S390_VM_CRYPTO) that ultimately
    calls the kvm_s390_vm_set_crypto() function. You'll notice that at the
    end of that function, there is a call to kvm_s390_vcpu_crypto_reset_all()
    that calls the kvm_s390_vcpu_crypto_setup() for each vcpu, so it would
    seem that there is the possiblility that there could be a need to
    set ECB3 to a different value. At any rate, I see no good reason to
    remove this.

    >
    >> + if (vcpu->kvm->arch.crypto.apie &&
    >> + test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_AP))
    >> + vcpu->arch.sie_block->eca |= ECA_APIE;
    >>
    >> - 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 on the guest, 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)
    >>
    >

    \
     
     \ /
      Last update: 2018-08-10 18:14    [W:3.850 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site