Copy the key mask to the right offset inside the shadow CRYCB
Signed-off-by: Pierre Morel <[email protected]>
---
arch/s390/kvm/vsie.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 63844b9..a2b28cd 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
return set_validity_icpt(scb_s, 0x0039U);
/* copy only the wrapping keys */
- if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
+ if (read_guest_real(vcpu, crycb_addr + 72,
+ vsie_page->crycb.dea_wrapping_key_mask, 56))
return set_validity_icpt(scb_s, 0x0035U);
scb_s->ecb3 |= ecb3_flags;
--
2.7.4
On 21.08.2018 16:19, Pierre Morel wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
"KVM: s390: vsie: copy wrapping keys to right place"
Indeed, we're writing it into the apcb0 mask (which is ignored).
Luckily, the xor-ing with the g2 keys seems to work.
Reviewed-by: David Hildenbrand <[email protected]>
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 63844b9..a2b28cd 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> return set_validity_icpt(scb_s, 0x0039U);
>
> /* copy only the wrapping keys */
> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
> + if (read_guest_real(vcpu, crycb_addr + 72,
> + vsie_page->crycb.dea_wrapping_key_mask, 56))
> return set_validity_icpt(scb_s, 0x0035U);
>
> scb_s->ecb3 |= ecb3_flags;
>
--
Thanks,
David / dhildenb
On 21.08.2018 16:33, Cornelia Huck wrote:
> On Tue, 21 Aug 2018 16:19:38 +0200
> Pierre Morel <[email protected]> wrote:
>
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kvm/vsie.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 63844b9..a2b28cd 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> return set_validity_icpt(scb_s, 0x0039U);
>>
>> /* copy only the wrapping keys */
>> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
>> + if (read_guest_real(vcpu, crycb_addr + 72,
>> + vsie_page->crycb.dea_wrapping_key_mask, 56))
>
> Don't you need the address here?
As it is defined as an array, the address would result in the same value.
(&vsie_page->crycb.dea_wrapping_key_mask[0])
>
>> return set_validity_icpt(scb_s, 0x0035U);
>>
>> scb_s->ecb3 |= ecb3_flags;
>
--
Thanks,
David / dhildenb
On Tue, 21 Aug 2018 16:36:19 +0200
David Hildenbrand <[email protected]> wrote:
> On 21.08.2018 16:33, Cornelia Huck wrote:
> > On Tue, 21 Aug 2018 16:19:38 +0200
> > Pierre Morel <[email protected]> wrote:
> >
> >> Copy the key mask to the right offset inside the shadow CRYCB
> >>
> >> Signed-off-by: Pierre Morel <[email protected]>
> >> ---
> >> arch/s390/kvm/vsie.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> >> index 63844b9..a2b28cd 100644
> >> --- a/arch/s390/kvm/vsie.c
> >> +++ b/arch/s390/kvm/vsie.c
> >> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> >> return set_validity_icpt(scb_s, 0x0039U);
> >>
> >> /* copy only the wrapping keys */
> >> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
> >> + if (read_guest_real(vcpu, crycb_addr + 72,
> >> + vsie_page->crycb.dea_wrapping_key_mask, 56))
> >
> > Don't you need the address here?
>
> As it is defined as an array, the address would result in the same value.
>
> (&vsie_page->crycb.dea_wrapping_key_mask[0])
/me looks...
Indeed, it just looks a bit odd.
With the changed subject
Reviewed-by: Cornelia Huck <[email protected]>
>
> >
> >> return set_validity_icpt(scb_s, 0x0035U);
> >>
> >> scb_s->ecb3 |= ecb3_flags;
> >
>
>
On 21.08.2018 16:19, Pierre Morel wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 63844b9..a2b28cd 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> return set_validity_icpt(scb_s, 0x0039U);
>
> /* copy only the wrapping keys */
> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
> + if (read_guest_real(vcpu, crycb_addr + 72,
> + vsie_page->crycb.dea_wrapping_key_mask, 56))
> return set_validity_icpt(scb_s, 0x0035U);
>
> scb_s->ecb3 |= ecb3_flags;
>
Are we able to use offsetof and sizeof here? I'd rather have a few more
characters than magic offsets.
What about CC stable?
Reviewed-by: Janosch Frank <[email protected]>
On 21.08.2018 16:43, Janosch Frank wrote:
> On 21.08.2018 16:19, Pierre Morel wrote:
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kvm/vsie.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 63844b9..a2b28cd 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> return set_validity_icpt(scb_s, 0x0039U);
>>
>> /* copy only the wrapping keys */
>> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
>> + if (read_guest_real(vcpu, crycb_addr + 72,
>> + vsie_page->crycb.dea_wrapping_key_mask, 56))
>> return set_validity_icpt(scb_s, 0x0035U);
>>
>> scb_s->ecb3 |= ecb3_flags;
>>
>
> Are we able to use offsetof and sizeof here? I'd rather have a few more
> characters than magic offsets.
> What about CC stable?
Thought about both things, too.
1. offsetof and sizeof will most likely make sense (although will most
likely make this very ugly due to the long names involved)
2. I am not sure about wrapping keys. We never had migration problems,
so I assume this does not break migration (maybe it will break once we
fix it on one side :) ). As we xor with the g2 keys, we will never match
the g2 keys. HW will xor with our keys either way, so it cannot match
our keys.
But two g3 guests will now have the same wrapping keys, I assume that's bad?
I guess we'll have to wait for Christian, I remember he was one of the
people that understood what wrapping keys are actually good for (and the
real key used in HW can simply, silently change, e.g. when migrating to
another system - due to the xoring).
Not having understood/looked into the details, I guess this should be
stable material.
>
>
> Reviewed-by: Janosch Frank <[email protected]>
>
--
Thanks,
David / dhildenb
On Tue, 21 Aug 2018 16:19:38 +0200
Pierre Morel <[email protected]> wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 63844b9..a2b28cd 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> return set_validity_icpt(scb_s, 0x0039U);
>
> /* copy only the wrapping keys */
> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
> + if (read_guest_real(vcpu, crycb_addr + 72,
> + vsie_page->crycb.dea_wrapping_key_mask, 56))
Don't you need the address here?
> return set_validity_icpt(scb_s, 0x0035U);
>
> scb_s->ecb3 |= ecb3_flags;