On Tue, 27 Feb 2018 09:27:59 -0500 Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> wrote: > The crypto control block designation (CRYCBD) is a 32-bit > field in the KVM guest's SIE state description. The > contents of bits 1-28 of this field, with three zero bits > appended on the right, designate the host real 31-bit > address of a crypto control block (CRYCB). Bits 30-31 > specify the format of the CRYCB. In the current > implementation, the address of the CRYCB is stored in > the CRYCBD only if the Message-Security-Assist extension > 3 (MSA3) facility is installed. Virtualization of AP > facilities, however, requires that a CRYCB of the > appropriate format be made available to SIE regardless > of whether MSA3 is installed or not. > > This patch introduces a new compilation unit to provide > all interfaces related to configuration of AP facilities. > Let's start by moving the function for setting the CRYCB > format from arch/s390/kvm/kvm-s390 to this new AP > configuration interface. Hm, I would tweak this patch description a bit. First, you talk about what the crycbd is; then, what needs to be done for vfio-ap support; then you simply state that you move some interfaces to a new file. I'd like to see a connection between those parts :) [It sounds a bit like you'd just introduce a new file and move some functions, while you do have more changes in there.] > > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> > --- > MAINTAINERS | 10 ++++++ > arch/s390/include/asm/kvm-ap.h | 16 ++++++++++ > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/Makefile | 2 +- > arch/s390/kvm/kvm-ap.c | 47 ++++++++++++++++++++++++++++ > arch/s390/kvm/kvm-s390.c | 62 +++++--------------------------------- > 6 files changed, 83 insertions(+), 55 deletions(-) > create mode 100644 arch/s390/include/asm/kvm-ap.h > create mode 100644 arch/s390/kvm/kvm-ap.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0ec5881..4acf7c2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11875,6 +11875,16 @@ W: http://www.ibm.com/developerworks/linux/linux390/ > S: Supported > F: drivers/s390/crypto/ > > +S390 VFIO AP DRIVER > +M: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> > +M: Christian BornTraeger <borntraeger@xxxxxxxxxx> Typo. > +M: Martin Schwidefsky <schwidefsky@xxxxxxxxxx> > +L: linux-s390@xxxxxxxxxxxxxxx > +W: http://www.ibm.com/developerworks/linux/linux390/ > +S: Supported > +F: arch/s390/include/asm/kvm/kvm-ap.h > +F: arch/s390/kvm/kvm-ap.c > + > S390 ZFCP DRIVER > M: Steffen Maier <maier@xxxxxxxxxxxxxxxxxx> > M: Benjamin Block <bblock@xxxxxxxxxxxxxxxxxx> (...) > diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c > new file mode 100644 > index 0000000..5305f4c > --- /dev/null > +++ b/arch/s390/kvm/kvm-ap.c > @@ -0,0 +1,47 @@ > +/* > + * Adjunct Processor (AP) configuration management for KVM guests > + * > + * Copyright IBM Corp. 2017 > + * > + * Author(s): Tony Krowiak <akrowia@xxxxxxxxxxxxxxxxxx> > + */ > + > +#include <asm/kvm-ap.h> > +#include <asm/ap.h> > + > +#include "kvm-s390.h" > + > +static int kvm_ap_apxa_installed(void) > +{ > + int ret; > + struct ap_config_info config; > + > + ret = ap_query_configuration(&config); Doesn't that introduce a dependency on CONFIG_ZCRYPT? > + if (ret) > + return 0; > + > + return (config.apxa == 1); > +} > + > +/** > + * kvm_ap_set_crycb_format > + * > + * Set the CRYCB format in the CRYCBD for the KVM guest. Spell out "crypto control block" somewhere? > + * > + * @kvm: the KVM guest > + * @crycbd: the CRYCB descriptor > + */ > +void kvm_ap_set_crycb_format(struct kvm *kvm, __u32 *crycbd) > +{ > + *crycbd = (__u32)(unsigned long) kvm->arch.crypto.crycb; > + > + *crycbd &= ~(CRYCB_FORMAT_MASK); > + > + /* If the MSAX3 is installed */ /* check whether MSAX3 is installed */ ? > + if (test_kvm_facility(kvm, 76)) { > + if (kvm_ap_apxa_installed()) > + *crycbd |= CRYCB_FORMAT2; > + else > + *crycbd |= CRYCB_FORMAT1; > + } > +} > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 5f5a4cb..de1e299 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1913,12 +1866,13 @@ 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->arch.crypto.crycbd = (__u32)(unsigned long) kvm->arch.crypto.crycb; > + kvm_ap_set_crycb_format(kvm, &kvm->arch.crypto.crycbd); Doesn't kvm_ap_set_crycb_format() already initialize its second parameter? Would it make sense to do kvm->arch.crypto.crycbd = kvm_ap_build_crycbd(kvm); or so instead? > + > if (!test_kvm_facility(kvm, 76)) > return; > > - kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb; > - 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; -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
