| Message ID | 20190417182925.50744-1-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 17.04.19 20:29, Christian Borntraeger wrote: > Instead of adding a new machine option to disable/enable the keywrapping > options of pckmo (like for AES and DEA) we can now use the CPU model to > decide. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > Reviewed-by: Collin Walling <walling@linux.ibm.com> > --- > v1->v2: - enable vsie > - also check if the host has the pckmo functions > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/kvm-s390.c | 7 +++++++ > arch/s390/kvm/vsie.c | 5 ++++- > 3 files changed, 12 insertions(+), 1 deletion(-) FWIW, I tested this variant successfully with some printk debugging to check if the settings are good. The only question is: does anybody cares about if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) being too long? I find this more readable than if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) Christian
On 18.04.19 09:35, Christian Borntraeger wrote: > On 17.04.19 20:29, Christian Borntraeger wrote: >> Instead of adding a new machine option to disable/enable the keywrapping >> options of pckmo (like for AES and DEA) we can now use the CPU model to >> decide. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Reviewed-by: Collin Walling <walling@linux.ibm.com> >> --- >> v1->v2: - enable vsie >> - also check if the host has the pckmo functions >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/kvm-s390.c | 7 +++++++ >> arch/s390/kvm/vsie.c | 5 ++++- >> 3 files changed, 12 insertions(+), 1 deletion(-) > > FWIW, I tested this variant successfully with some printk debugging to > check if the settings are good. The only question is: does anybody cares > about > if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || > (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) > > being too long? I find this more readable than > > if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & > kvm_s390_available_subfunc.pckmo[4] & 0xe0) || > (vcpu->kvm->arch.model.subfuncs.pckmo[5] & > kvm_s390_available_subfunc.pckmo[5] & 0xc0)) > Can you just factor that out into a function / makro? > Christian >
On 17.04.19 20:29, Christian Borntraeger wrote: > Instead of adding a new machine option to disable/enable the keywrapping > options of pckmo (like for AES and DEA) we can now use the CPU model to > decide. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > Reviewed-by: Collin Walling <walling@linux.ibm.com> > --- > v1->v2: - enable vsie > - also check if the host has the pckmo functions > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/kvm-s390.c | 7 +++++++ > arch/s390/kvm/vsie.c | 5 ++++- > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index c47e22bba87fa..e224246ff93c6 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { > #define ECD_HOSTREGMGMT 0x20000000 > #define ECD_MEF 0x08000000 > #define ECD_ETOKENF 0x02000000 > +#define ECD_ECC 0x00200000 > __u32 ecd; /* 0x01c8 */ > __u8 reserved1cc[18]; /* 0x01cc */ > __u64 pp; /* 0x01de */ > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 0dad61ccde3d6..9869d785677f1 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) > VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", > vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); > } > + /* > + * if any of 32,33,34,40,41 is active in host AND guest, > + * we enable pckmo for ecc > + */ > + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || > + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) Maybe some helper like bool kvm_has_pckmo_subfunc(kvm, nr) { /* magic for one number */ } ... if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33)) ... then you can also get rid of the comment. > + vcpu->arch.sie_block->ecd |= ECD_ECC; > vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) > | SDNXC; > vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c > index d62fa148558b9..c6983d962abfd 100644 > --- a/arch/s390/kvm/vsie.c > +++ b/arch/s390/kvm/vsie.c > @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) > const u32 crycb_addr = crycbd_o & 0x7ffffff8U; > unsigned long *b1, *b2; > u8 ecb3_flags; > + u32 ecd_flags; > int apie_h; > int key_msk = test_kvm_facility(vcpu->kvm, 76); > int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; > @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) > /* we may only allow it if enabled for guest 2 */ > ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & > (ECB3_AES | ECB3_DEA); > - if (!ecb3_flags) > + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; > + if (!ecb3_flags && !ecd_flags) > goto end; Just so I get it right, there are no *new* wrapping keys? Which wrapping keys are used then? > > /* copy only the wrapping keys */ > @@ -329,6 +331,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) > return set_validity_icpt(scb_s, 0x0035U); > > scb_s->ecb3 |= ecb3_flags; > + scb_s->ecd |= ecd_flags; > > /* xor both blocks in one run */ > b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; >
On 18.04.19 09:54, David Hildenbrand wrote: > On 17.04.19 20:29, Christian Borntraeger wrote: >> Instead of adding a new machine option to disable/enable the keywrapping >> options of pckmo (like for AES and DEA) we can now use the CPU model to >> decide. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Reviewed-by: Collin Walling <walling@linux.ibm.com> >> --- >> v1->v2: - enable vsie >> - also check if the host has the pckmo functions >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/kvm-s390.c | 7 +++++++ >> arch/s390/kvm/vsie.c | 5 ++++- >> 3 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index c47e22bba87fa..e224246ff93c6 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { >> #define ECD_HOSTREGMGMT 0x20000000 >> #define ECD_MEF 0x08000000 >> #define ECD_ETOKENF 0x02000000 >> +#define ECD_ECC 0x00200000 >> __u32 ecd; /* 0x01c8 */ >> __u8 reserved1cc[18]; /* 0x01cc */ >> __u64 pp; /* 0x01de */ >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 0dad61ccde3d6..9869d785677f1 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) >> VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", >> vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); >> } >> + /* >> + * if any of 32,33,34,40,41 is active in host AND guest, >> + * we enable pckmo for ecc >> + */ >> + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || >> + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) > > Maybe some helper like > > bool kvm_has_pckmo_subfunc(kvm, nr) > { > /* magic for one number */ > } > ... > > if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33)) > ... > > then you can also get rid of the comment. Will give it a try. > >> + vcpu->arch.sie_block->ecd |= ECD_ECC; >> vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) >> | SDNXC; >> vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; >> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >> index d62fa148558b9..c6983d962abfd 100644 >> --- a/arch/s390/kvm/vsie.c >> +++ b/arch/s390/kvm/vsie.c >> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >> const u32 crycb_addr = crycbd_o & 0x7ffffff8U; >> unsigned long *b1, *b2; >> u8 ecb3_flags; >> + u32 ecd_flags; >> int apie_h; >> int key_msk = test_kvm_facility(vcpu->kvm, 76); >> int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; >> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >> /* we may only allow it if enabled for guest 2 */ >> ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & >> (ECB3_AES | ECB3_DEA); >> - if (!ecb3_flags) >> + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; >> + if (!ecb3_flags && !ecd_flags) >> goto end; > > Just so I get it right, there are no *new* wrapping keys? Which wrapping > keys are used then? Yes, AES.
On 18.04.19 10:58, Christian Borntraeger wrote: > > > On 18.04.19 09:54, David Hildenbrand wrote: >> On 17.04.19 20:29, Christian Borntraeger wrote: >>> Instead of adding a new machine option to disable/enable the keywrapping >>> options of pckmo (like for AES and DEA) we can now use the CPU model to >>> decide. >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> Reviewed-by: Collin Walling <walling@linux.ibm.com> >>> --- >>> v1->v2: - enable vsie >>> - also check if the host has the pckmo functions >>> arch/s390/include/asm/kvm_host.h | 1 + >>> arch/s390/kvm/kvm-s390.c | 7 +++++++ >>> arch/s390/kvm/vsie.c | 5 ++++- >>> 3 files changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index c47e22bba87fa..e224246ff93c6 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { >>> #define ECD_HOSTREGMGMT 0x20000000 >>> #define ECD_MEF 0x08000000 >>> #define ECD_ETOKENF 0x02000000 >>> +#define ECD_ECC 0x00200000 >>> __u32 ecd; /* 0x01c8 */ >>> __u8 reserved1cc[18]; /* 0x01cc */ >>> __u64 pp; /* 0x01de */ >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index 0dad61ccde3d6..9869d785677f1 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) >>> VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", >>> vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); >>> } >>> + /* >>> + * if any of 32,33,34,40,41 is active in host AND guest, >>> + * we enable pckmo for ecc >>> + */ >>> + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || >>> + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) >> >> Maybe some helper like >> >> bool kvm_has_pckmo_subfunc(kvm, nr) >> { >> /* magic for one number */ >> } >> ... >> >> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33)) >> ... >> >> then you can also get rid of the comment. > > Will give it a try. >> >>> + vcpu->arch.sie_block->ecd |= ECD_ECC; >>> vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) >>> | SDNXC; >>> vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; >>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>> index d62fa148558b9..c6983d962abfd 100644 >>> --- a/arch/s390/kvm/vsie.c >>> +++ b/arch/s390/kvm/vsie.c >>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>> const u32 crycb_addr = crycbd_o & 0x7ffffff8U; >>> unsigned long *b1, *b2; >>> u8 ecb3_flags; >>> + u32 ecd_flags; >>> int apie_h; >>> int key_msk = test_kvm_facility(vcpu->kvm, 76); >>> int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; >>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>> /* we may only allow it if enabled for guest 2 */ >>> ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & >>> (ECB3_AES | ECB3_DEA); >>> - if (!ecb3_flags) >>> + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; >>> + if (!ecb3_flags && !ecd_flags) >>> goto end; >> >> Just so I get it right, there are no *new* wrapping keys? Which wrapping >> keys are used then? > > Yes, AES. > Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW, the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()), but will be used. I guess you should also check against kvm->arch.crypto.aes_kw before turning the ecd bit on, just to be sure.
On 18.04.19 11:13, David Hildenbrand wrote: > On 18.04.19 10:58, Christian Borntraeger wrote: >> >> >> On 18.04.19 09:54, David Hildenbrand wrote: >>> On 17.04.19 20:29, Christian Borntraeger wrote: >>>> Instead of adding a new machine option to disable/enable the keywrapping >>>> options of pckmo (like for AES and DEA) we can now use the CPU model to >>>> decide. >>>> >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> Reviewed-by: Collin Walling <walling@linux.ibm.com> >>>> --- >>>> v1->v2: - enable vsie >>>> - also check if the host has the pckmo functions >>>> arch/s390/include/asm/kvm_host.h | 1 + >>>> arch/s390/kvm/kvm-s390.c | 7 +++++++ >>>> arch/s390/kvm/vsie.c | 5 ++++- >>>> 3 files changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>>> index c47e22bba87fa..e224246ff93c6 100644 >>>> --- a/arch/s390/include/asm/kvm_host.h >>>> +++ b/arch/s390/include/asm/kvm_host.h >>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { >>>> #define ECD_HOSTREGMGMT 0x20000000 >>>> #define ECD_MEF 0x08000000 >>>> #define ECD_ETOKENF 0x02000000 >>>> +#define ECD_ECC 0x00200000 >>>> __u32 ecd; /* 0x01c8 */ >>>> __u8 reserved1cc[18]; /* 0x01cc */ >>>> __u64 pp; /* 0x01de */ >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index 0dad61ccde3d6..9869d785677f1 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) >>>> VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", >>>> vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); >>>> } >>>> + /* >>>> + * if any of 32,33,34,40,41 is active in host AND guest, >>>> + * we enable pckmo for ecc >>>> + */ >>>> + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || >>>> + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) >>> >>> Maybe some helper like >>> >>> bool kvm_has_pckmo_subfunc(kvm, nr) >>> { >>> /* magic for one number */ >>> } >>> ... >>> >>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33)) >>> ... >>> >>> then you can also get rid of the comment. >> >> Will give it a try. >>> >>>> + vcpu->arch.sie_block->ecd |= ECD_ECC; >>>> vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) >>>> | SDNXC; >>>> vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; >>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>>> index d62fa148558b9..c6983d962abfd 100644 >>>> --- a/arch/s390/kvm/vsie.c >>>> +++ b/arch/s390/kvm/vsie.c >>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>> const u32 crycb_addr = crycbd_o & 0x7ffffff8U; >>>> unsigned long *b1, *b2; >>>> u8 ecb3_flags; >>>> + u32 ecd_flags; >>>> int apie_h; >>>> int key_msk = test_kvm_facility(vcpu->kvm, 76); >>>> int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; >>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>> /* we may only allow it if enabled for guest 2 */ >>>> ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & >>>> (ECB3_AES | ECB3_DEA); >>>> - if (!ecb3_flags) >>>> + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; >>>> + if (!ecb3_flags && !ecd_flags) >>>> goto end; >>> >>> Just so I get it right, there are no *new* wrapping keys? Which wrapping >>> keys are used then? >> >> Yes, AES. >> > > Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW, > the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()), > but will be used. > > I guess you should also check against kvm->arch.crypto.aes_kw before > turning the ecd bit on, just to be sure. We should rather initialize the aes value when ecc wrapping is enabled.
On 18.04.19 12:17, Christian Borntraeger wrote: > > > On 18.04.19 11:13, David Hildenbrand wrote: >> On 18.04.19 10:58, Christian Borntraeger wrote: >>> >>> >>> On 18.04.19 09:54, David Hildenbrand wrote: >>>> On 17.04.19 20:29, Christian Borntraeger wrote: >>>>> Instead of adding a new machine option to disable/enable the keywrapping >>>>> options of pckmo (like for AES and DEA) we can now use the CPU model to >>>>> decide. >>>>> >>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>> Reviewed-by: Collin Walling <walling@linux.ibm.com> >>>>> --- >>>>> v1->v2: - enable vsie >>>>> - also check if the host has the pckmo functions >>>>> arch/s390/include/asm/kvm_host.h | 1 + >>>>> arch/s390/kvm/kvm-s390.c | 7 +++++++ >>>>> arch/s390/kvm/vsie.c | 5 ++++- >>>>> 3 files changed, 12 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>>>> index c47e22bba87fa..e224246ff93c6 100644 >>>>> --- a/arch/s390/include/asm/kvm_host.h >>>>> +++ b/arch/s390/include/asm/kvm_host.h >>>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { >>>>> #define ECD_HOSTREGMGMT 0x20000000 >>>>> #define ECD_MEF 0x08000000 >>>>> #define ECD_ETOKENF 0x02000000 >>>>> +#define ECD_ECC 0x00200000 >>>>> __u32 ecd; /* 0x01c8 */ >>>>> __u8 reserved1cc[18]; /* 0x01cc */ >>>>> __u64 pp; /* 0x01de */ >>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>> index 0dad61ccde3d6..9869d785677f1 100644 >>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) >>>>> VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", >>>>> vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); >>>>> } >>>>> + /* >>>>> + * if any of 32,33,34,40,41 is active in host AND guest, >>>>> + * we enable pckmo for ecc >>>>> + */ >>>>> + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || >>>>> + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) >>>> >>>> Maybe some helper like >>>> >>>> bool kvm_has_pckmo_subfunc(kvm, nr) >>>> { >>>> /* magic for one number */ >>>> } >>>> ... >>>> >>>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33)) >>>> ... >>>> >>>> then you can also get rid of the comment. >>> >>> Will give it a try. >>>> >>>>> + vcpu->arch.sie_block->ecd |= ECD_ECC; >>>>> vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) >>>>> | SDNXC; >>>>> vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; >>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>>>> index d62fa148558b9..c6983d962abfd 100644 >>>>> --- a/arch/s390/kvm/vsie.c >>>>> +++ b/arch/s390/kvm/vsie.c >>>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>>> const u32 crycb_addr = crycbd_o & 0x7ffffff8U; >>>>> unsigned long *b1, *b2; >>>>> u8 ecb3_flags; >>>>> + u32 ecd_flags; >>>>> int apie_h; >>>>> int key_msk = test_kvm_facility(vcpu->kvm, 76); >>>>> int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; >>>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>>> /* we may only allow it if enabled for guest 2 */ >>>>> ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & >>>>> (ECB3_AES | ECB3_DEA); >>>>> - if (!ecb3_flags) >>>>> + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; >>>>> + if (!ecb3_flags && !ecd_flags) >>>>> goto end; >>>> >>>> Just so I get it right, there are no *new* wrapping keys? Which wrapping >>>> keys are used then? >>> >>> Yes, AES. >>> >> >> Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW, >> the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()), >> but will be used. >> >> I guess you should also check against kvm->arch.crypto.aes_kw before >> turning the ecd bit on, just to be sure. > > We should rather initialize the aes value when ecc wrapping is enabled. > If you don't glue this to KVM_S390_VM_CRYPTO_ENABLE_AES_KW, you will have to find another way to reset AES keys during resets.
On 18.04.19 12:31, David Hildenbrand wrote: > On 18.04.19 12:17, Christian Borntraeger wrote: >> >> >> On 18.04.19 11:13, David Hildenbrand wrote: >>> On 18.04.19 10:58, Christian Borntraeger wrote: >>>> >>>> >>>> On 18.04.19 09:54, David Hildenbrand wrote: >>>>> On 17.04.19 20:29, Christian Borntraeger wrote: >>>>>> Instead of adding a new machine option to disable/enable the keywrapping >>>>>> options of pckmo (like for AES and DEA) we can now use the CPU model to >>>>>> decide. >>>>>> >>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>>> Reviewed-by: Collin Walling <walling@linux.ibm.com> >>>>>> --- >>>>>> v1->v2: - enable vsie >>>>>> - also check if the host has the pckmo functions >>>>>> arch/s390/include/asm/kvm_host.h | 1 + >>>>>> arch/s390/kvm/kvm-s390.c | 7 +++++++ >>>>>> arch/s390/kvm/vsie.c | 5 ++++- >>>>>> 3 files changed, 12 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>>>>> index c47e22bba87fa..e224246ff93c6 100644 >>>>>> --- a/arch/s390/include/asm/kvm_host.h >>>>>> +++ b/arch/s390/include/asm/kvm_host.h >>>>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { >>>>>> #define ECD_HOSTREGMGMT 0x20000000 >>>>>> #define ECD_MEF 0x08000000 >>>>>> #define ECD_ETOKENF 0x02000000 >>>>>> +#define ECD_ECC 0x00200000 >>>>>> __u32 ecd; /* 0x01c8 */ >>>>>> __u8 reserved1cc[18]; /* 0x01cc */ >>>>>> __u64 pp; /* 0x01de */ >>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>>> index 0dad61ccde3d6..9869d785677f1 100644 >>>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) >>>>>> VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", >>>>>> vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); >>>>>> } >>>>>> + /* >>>>>> + * if any of 32,33,34,40,41 is active in host AND guest, >>>>>> + * we enable pckmo for ecc >>>>>> + */ >>>>>> + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || >>>>>> + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) >>>>> >>>>> Maybe some helper like >>>>> >>>>> bool kvm_has_pckmo_subfunc(kvm, nr) >>>>> { >>>>> /* magic for one number */ >>>>> } >>>>> ... >>>>> >>>>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33)) >>>>> ... >>>>> >>>>> then you can also get rid of the comment. >>>> >>>> Will give it a try. >>>>> >>>>>> + vcpu->arch.sie_block->ecd |= ECD_ECC; >>>>>> vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) >>>>>> | SDNXC; >>>>>> vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; >>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>>>>> index d62fa148558b9..c6983d962abfd 100644 >>>>>> --- a/arch/s390/kvm/vsie.c >>>>>> +++ b/arch/s390/kvm/vsie.c >>>>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>>>> const u32 crycb_addr = crycbd_o & 0x7ffffff8U; >>>>>> unsigned long *b1, *b2; >>>>>> u8 ecb3_flags; >>>>>> + u32 ecd_flags; >>>>>> int apie_h; >>>>>> int key_msk = test_kvm_facility(vcpu->kvm, 76); >>>>>> int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; >>>>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>>>> /* we may only allow it if enabled for guest 2 */ >>>>>> ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & >>>>>> (ECB3_AES | ECB3_DEA); >>>>>> - if (!ecb3_flags) >>>>>> + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; >>>>>> + if (!ecb3_flags && !ecd_flags) >>>>>> goto end; >>>>> >>>>> Just so I get it right, there are no *new* wrapping keys? Which wrapping >>>>> keys are used then? >>>> >>>> Yes, AES. >>>> >>> >>> Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW, >>> the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()), >>> but will be used. >>> >>> I guess you should also check against kvm->arch.crypto.aes_kw before >>> turning the ecd bit on, just to be sure. >> >> We should rather initialize the aes value when ecc wrapping is enabled. >> > > If you don't glue this to KVM_S390_VM_CRYPTO_ENABLE_AES_KW, you will > have to find another way to reset AES keys during resets. I think it is better to add 2 new attributes as we can have pckmo for ecc but not for aes (e.g. ECB3_AES == 0 but ECD_ECC ==1).
On 18.04.19 12:40, Christian Borntraeger wrote: >> >> If you don't glue this to KVM_S390_VM_CRYPTO_ENABLE_AES_KW, you will >> have to find another way to reset AES keys during resets. > > I think it is better to add 2 new attributes as we can have pckmo for ecc but > not for aes (e.g. ECB3_AES == 0 but ECD_ECC ==1). On the other hand: MSA9 requires MSA3 and MSA4 to be present. So simply checking for kvm->arch.crypto.aes_kw ==1 as you suggested is very likely good enough.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index c47e22bba87fa..e224246ff93c6 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -278,6 +278,7 @@ struct kvm_s390_sie_block { #define ECD_HOSTREGMGMT 0x20000000 #define ECD_MEF 0x08000000 #define ECD_ETOKENF 0x02000000 +#define ECD_ECC 0x00200000 __u32 ecd; /* 0x01c8 */ __u8 reserved1cc[18]; /* 0x01cc */ __u64 pp; /* 0x01de */ diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 0dad61ccde3d6..9869d785677f1 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u", vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id); } + /* + * if any of 32,33,34,40,41 is active in host AND guest, + * we enable pckmo for ecc + */ + if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) || + (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0)) + vcpu->arch.sie_block->ecd |= ECD_ECC; vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx) | SDNXC; vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb; diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c index d62fa148558b9..c6983d962abfd 100644 --- a/arch/s390/kvm/vsie.c +++ b/arch/s390/kvm/vsie.c @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) const u32 crycb_addr = crycbd_o & 0x7ffffff8U; unsigned long *b1, *b2; u8 ecb3_flags; + u32 ecd_flags; int apie_h; int key_msk = test_kvm_facility(vcpu->kvm, 76); int fmt_o = crycbd_o & CRYCB_FORMAT_MASK; @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) /* we may only allow it if enabled for guest 2 */ ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 & (ECB3_AES | ECB3_DEA); - if (!ecb3_flags) + ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC; + if (!ecb3_flags && !ecd_flags) goto end; /* copy only the wrapping keys */ @@ -329,6 +331,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) return set_validity_icpt(scb_s, 0x0035U); scb_s->ecb3 |= ecb3_flags; + scb_s->ecd |= ecd_flags; /* xor both blocks in one run */ b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;