| Message ID | 1535019956-23539-4-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: vsie: Consolidate CRYCB validation | expand |
On 8/23/18 12:25 PM, Pierre Morel wrote: > The comment preceding the shadow_crycb function is > misleading, we effectively accept FORMAT2 CRYCB in the > guest. I beg to differ: if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) return 0; > > When using FORMAT2 in the host we do not need to or with > FORMAT1. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > arch/s390/kvm/vsie.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c > index 38ea5da..e0e6fbf 100644 > --- a/arch/s390/kvm/vsie.c > +++ b/arch/s390/kvm/vsie.c > @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) > * Create a shadow copy of the crycb block and setup key wrapping, if > * requested for guest 3 and enabled for guest 2. > * > - * We only accept format-1 (no AP in g2), but convert it into format-2 > + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), > + * and we convert it into format-2 in the shadow CRYCB. > * There is nothing to do for format-0. > * > * Returns: - 0 if shadowed or nothing to do > @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | > - CRYCB_FORMAT2; > + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; That's purely cosmetic but valid. > > /* xor both blocks in one run */ > b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; >
On 23.08.2018 13:05, Janosch Frank wrote: > On 8/23/18 12:25 PM, Pierre Morel wrote: >> The comment preceding the shadow_crycb function is >> misleading, we effectively accept FORMAT2 CRYCB in the >> guest. > > I beg to differ: > > if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) > return 0; FORMAT2 includes bit FORMAT1 (backwards compatible) > >> >> When using FORMAT2 in the host we do not need to or with >> FORMAT1. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> arch/s390/kvm/vsie.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >> index 38ea5da..e0e6fbf 100644 >> --- a/arch/s390/kvm/vsie.c >> +++ b/arch/s390/kvm/vsie.c >> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >> * Create a shadow copy of the crycb block and setup key wrapping, if >> * requested for guest 3 and enabled for guest 2. >> * >> - * We only accept format-1 (no AP in g2), but convert it into format-2 >> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), >> + * and we convert it into format-2 in the shadow CRYCB. >> * There is nothing to do for format-0. >> * >> * Returns: - 0 if shadowed or nothing to do >> @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | >> - CRYCB_FORMAT2; >> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; > > That's purely cosmetic but valid. > >> >> /* xor both blocks in one run */ >> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; Reviewed-by: David Hildenbrand <david@redhat.com>
On 8/23/18 1:21 PM, David Hildenbrand wrote: > On 23.08.2018 13:05, Janosch Frank wrote: >> On 8/23/18 12:25 PM, Pierre Morel wrote: >>> The comment preceding the shadow_crycb function is >>> misleading, we effectively accept FORMAT2 CRYCB in the >>> guest. >> >> I beg to differ: >> >> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >> return 0; > > FORMAT2 includes bit FORMAT1 (backwards compatible) Right, this check is very misleading because of the constant, we effectively test against Format 0 and Format 2. Can we make this clearer by explicitly ANDing 0x01 or adding a comment? Code makes sense: Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > >> >>> >>> When using FORMAT2 in the host we do not need to or with >>> FORMAT1. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> arch/s390/kvm/vsie.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>> index 38ea5da..e0e6fbf 100644 >>> --- a/arch/s390/kvm/vsie.c >>> +++ b/arch/s390/kvm/vsie.c >>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>> * Create a shadow copy of the crycb block and setup key wrapping, if >>> * requested for guest 3 and enabled for guest 2. >>> * >>> - * We only accept format-1 (no AP in g2), but convert it into format-2 >>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), >>> + * and we convert it into format-2 in the shadow CRYCB. >>> * There is nothing to do for format-0. >>> * >>> * Returns: - 0 if shadowed or nothing to do >>> @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | >>> - CRYCB_FORMAT2; >>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; >> >> That's purely cosmetic but valid. >> >>> >>> /* xor both blocks in one run */ >>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; > > Reviewed-by: David Hildenbrand <david@redhat.com> >
On 23/08/2018 13:21, David Hildenbrand wrote: > On 23.08.2018 13:05, Janosch Frank wrote: >> On 8/23/18 12:25 PM, Pierre Morel wrote: >>> The comment preceding the shadow_crycb function is >>> misleading, we effectively accept FORMAT2 CRYCB in the >>> guest. >> >> I beg to differ: >> >> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >> return 0; > > FORMAT2 includes bit FORMAT1 (backwards compatible) > >> >>> >>> When using FORMAT2 in the host we do not need to or with >>> FORMAT1. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> arch/s390/kvm/vsie.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>> index 38ea5da..e0e6fbf 100644 >>> --- a/arch/s390/kvm/vsie.c >>> +++ b/arch/s390/kvm/vsie.c >>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>> * Create a shadow copy of the crycb block and setup key wrapping, if >>> * requested for guest 3 and enabled for guest 2. >>> * >>> - * We only accept format-1 (no AP in g2), but convert it into format-2 >>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), >>> + * and we convert it into format-2 in the shadow CRYCB. >>> * There is nothing to do for format-0. >>> * >>> * Returns: - 0 if shadowed or nothing to do >>> @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | >>> - CRYCB_FORMAT2; >>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; >> >> That's purely cosmetic but valid. >> >>> >>> /* xor both blocks in one run */ >>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; > > Reviewed-by: David Hildenbrand <david@redhat.com> > Thanks, regards, Pierre
On 23/08/2018 13:33, Janosch Frank wrote: > On 8/23/18 1:21 PM, David Hildenbrand wrote: >> On 23.08.2018 13:05, Janosch Frank wrote: >>> On 8/23/18 12:25 PM, Pierre Morel wrote: >>>> The comment preceding the shadow_crycb function is >>>> misleading, we effectively accept FORMAT2 CRYCB in the >>>> guest. >>> >>> I beg to differ: >>> >>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >>> return 0; >> >> FORMAT2 includes bit FORMAT1 (backwards compatible) > > Right, this check is very misleading because of the constant, we > effectively test against Format 0 and Format 2. > > Can we make this clearer by explicitly ANDing 0x01 or adding a comment? yes, done, I modified the comment in front of the function. > > Code makes sense: > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> Thanks, regards, Pierre > >> >>> >>>> >>>> When using FORMAT2 in the host we do not need to or with >>>> FORMAT1. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> arch/s390/kvm/vsie.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>>> index 38ea5da..e0e6fbf 100644 >>>> --- a/arch/s390/kvm/vsie.c >>>> +++ b/arch/s390/kvm/vsie.c >>>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>> * Create a shadow copy of the crycb block and setup key wrapping, if >>>> * requested for guest 3 and enabled for guest 2. >>>> * >>>> - * We only accept format-1 (no AP in g2), but convert it into format-2 >>>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), >>>> + * and we convert it into format-2 in the shadow CRYCB. >>>> * There is nothing to do for format-0. >>>> * >>>> * Returns: - 0 if shadowed or nothing to do >>>> @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | >>>> - CRYCB_FORMAT2; >>>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; >>> >>> That's purely cosmetic but valid. >>> >>>> >>>> /* xor both blocks in one run */ >>>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; >> >> Reviewed-by: David Hildenbrand <david@redhat.com> >> > >
On 8/23/18 1:47 PM, Pierre Morel wrote: > On 23/08/2018 13:33, Janosch Frank wrote: >> On 8/23/18 1:21 PM, David Hildenbrand wrote: >>> On 23.08.2018 13:05, Janosch Frank wrote: >>>> On 8/23/18 12:25 PM, Pierre Morel wrote: >>>>> The comment preceding the shadow_crycb function is >>>>> misleading, we effectively accept FORMAT2 CRYCB in the >>>>> guest. >>>> >>>> I beg to differ: >>>> >>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >>>> return 0; >>> >>> FORMAT2 includes bit FORMAT1 (backwards compatible) >> >> Right, this check is very misleading because of the constant, we >> effectively test against Format 0 and Format 2. >> >> Can we make this clearer by explicitly ANDing 0x01 or adding a comment? > > yes, done, I modified the comment in front of the function. Which is not what I want, what I want is: /* CRYCB_FORMAT2 includes the bit for CRYCB_FORMAT1, so we allow both formats here */ if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) return 0; > >> >> Code makes sense: >> Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > > Thanks, > > regards, > Pierre > >> >>> >>>> >>>>> >>>>> When using FORMAT2 in the host we do not need to or with >>>>> FORMAT1. >>>>> >>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>> --- >>>>> arch/s390/kvm/vsie.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >>>>> index 38ea5da..e0e6fbf 100644 >>>>> --- a/arch/s390/kvm/vsie.c >>>>> +++ b/arch/s390/kvm/vsie.c >>>>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >>>>> * Create a shadow copy of the crycb block and setup key wrapping, if >>>>> * requested for guest 3 and enabled for guest 2. >>>>> * >>>>> - * We only accept format-1 (no AP in g2), but convert it into format-2 >>>>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), >>>>> + * and we convert it into format-2 in the shadow CRYCB. >>>>> * There is nothing to do for format-0. >>>>> * >>>>> * Returns: - 0 if shadowed or nothing to do >>>>> @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | >>>>> - CRYCB_FORMAT2; >>>>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; >>>> >>>> That's purely cosmetic but valid. >>>> >>>>> >>>>> /* xor both blocks in one run */ >>>>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask; >>> >>> Reviewed-by: David Hildenbrand <david@redhat.com> >>> >> >> > >
On 23.08.2018 13:53, Janosch Frank wrote: > On 8/23/18 1:47 PM, Pierre Morel wrote: >> On 23/08/2018 13:33, Janosch Frank wrote: >>> On 8/23/18 1:21 PM, David Hildenbrand wrote: >>>> On 23.08.2018 13:05, Janosch Frank wrote: >>>>> On 8/23/18 12:25 PM, Pierre Morel wrote: >>>>>> The comment preceding the shadow_crycb function is >>>>>> misleading, we effectively accept FORMAT2 CRYCB in the >>>>>> guest. >>>>> >>>>> I beg to differ: >>>>> >>>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >>>>> return 0; >>>> >>>> FORMAT2 includes bit FORMAT1 (backwards compatible) >>> >>> Right, this check is very misleading because of the constant, we >>> effectively test against Format 0 and Format 2. >>> >>> Can we make this clearer by explicitly ANDing 0x01 or adding a comment? >> >> yes, done, I modified the comment in front of the function. > > Which is not what I want, what I want is: > > /* CRYCB_FORMAT2 includes the bit for CRYCB_FORMAT1, so we allow both > formats here */ > if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) > return 0; While it's not wrong, it is also not required. And it might soon be obsolete again (with APXA, as you said, there we always have to check). But I'll leave that to you
On 8/23/18 2:03 PM, David Hildenbrand wrote: > On 23.08.2018 13:53, Janosch Frank wrote: >> On 8/23/18 1:47 PM, Pierre Morel wrote: >>> On 23/08/2018 13:33, Janosch Frank wrote: >>>> On 8/23/18 1:21 PM, David Hildenbrand wrote: >>>>> On 23.08.2018 13:05, Janosch Frank wrote: >>>>>> On 8/23/18 12:25 PM, Pierre Morel wrote: >>>>>>> The comment preceding the shadow_crycb function is >>>>>>> misleading, we effectively accept FORMAT2 CRYCB in the >>>>>>> guest. >>>>>> >>>>>> I beg to differ: >>>>>> >>>>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >>>>>> return 0; >>>>> >>>>> FORMAT2 includes bit FORMAT1 (backwards compatible) >>>> >>>> Right, this check is very misleading because of the constant, we >>>> effectively test against Format 0 and Format 2. >>>> >>>> Can we make this clearer by explicitly ANDing 0x01 or adding a comment? >>> >>> yes, done, I modified the comment in front of the function. >> >> Which is not what I want, what I want is: >> >> /* CRYCB_FORMAT2 includes the bit for CRYCB_FORMAT1, so we allow both >> formats here */ >> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1)) >> return 0; > > While it's not wrong, it is also not required. And it might soon be > obsolete again (with APXA, as you said, there we always have to check). > > But I'll leave that to you > I have not checked the vfio-ap patches, Pierre just told me that it goes away in a few weeks anyway, so let's leave it out.
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c index 38ea5da..e0e6fbf 100644 --- a/arch/s390/kvm/vsie.c +++ b/arch/s390/kvm/vsie.c @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) * Create a shadow copy of the crycb block and setup key wrapping, if * requested for guest 3 and enabled for guest 2. * - * We only accept format-1 (no AP in g2), but convert it into format-2 + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2), + * and we convert it into format-2 in the shadow CRYCB. * There is nothing to do for format-0. * * Returns: - 0 if shadowed or nothing to do @@ -179,8 +180,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->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 | - CRYCB_FORMAT2; + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2; /* xor both blocks in one run */ b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
The comment preceding the shadow_crycb function is misleading, we effectively accept FORMAT2 CRYCB in the guest. When using FORMAT2 in the host we do not need to or with FORMAT1. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- arch/s390/kvm/vsie.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)