[PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[Pierre Morel]

Copy the key mask to the right offset inside the shadow CRYCB

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 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

Re: [PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[Cornelia Huck]

On Tue, 21 Aug 2018 16:19:38 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Copy the key mask to the right offset inside the shadow CRYCB
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  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;

Re: [PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[David Hildenbrand]

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 <david@redhat.com>

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  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

Re: [PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[David Hildenbrand]

On 21.08.2018 16:33, Cornelia Huck wrote:
> On Tue, 21 Aug 2018 16:19:38 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  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

Re: [PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[Cornelia Huck]

On Tue, 21 Aug 2018 16:36:19 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 21.08.2018 16:33, Cornelia Huck wrote:
> > On Tue, 21 Aug 2018 16:19:38 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> Copy the key mask to the right offset inside the shadow CRYCB
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >>  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 <cohuck@redhat.com>

> 
> >   
> >>  		return set_validity_icpt(scb_s, 0x0035U);
> >>  
> >>  	scb_s->ecb3 |= ecb3_flags;  
> >   
> 
> 

Re: [PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1057 bytes --]

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 <pmorel@linux.ibm.com>
> ---
>  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 <frankja@linux.ibm.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] KVM: s390: vsie: BUG correction by shadow_crycb

[David Hildenbrand]

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 <pmorel@linux.ibm.com>
>> ---
>>  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 <frankja@linux.ibm.com>
> 


-- 

Thanks,

David / dhildenb