[PATCH v3 0/3] KVM: s390: vsie: Consolidate CRYCB validation

[PATCH v3 0/3] KVM: s390: vsie: Consolidate CRYCB validation

[Pierre Morel]

Before adapting the CRYCB shadowing for a guest supporting
the AP instructions we want to clean the CRYCB shadowing code.

First patch seem obvious.

Second patch introduce a change in the behavior of
the virtual machine in that the CRYCB is validated
whenever we use or not the key masks to mimic the
real machine.

Patch 3 does not correct the compiled code but make
more clear what is done concerning the formating of
the CRYCB in the guest original CRYCB and in the
shadow CRYCB.

Pierre Morel (3):
  KVM: s390: vsie: copy wrapping keys to right place
  KVM: s390: vsie: Do the CRYCB validation first
  KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

 arch/s390/kvm/vsie.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

-- 
2.7.4

Changelog from V2:
- Suppress two useless/broken patches
- Change a patch on CRYCB format to a modification of the comment
- modified the first patch title as expected
- added R-Bs

Changelog from v1
- split into severall small patches
- insert the first cleaning patch ahead
- add a new patch to allow a host not having AP
  instructions

[PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[Pierre Morel]

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Janosch Frank <frankja@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 9175518..12b9707 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

[PATCH v3 2/3] KVM: s390: vsie: Do the CRYCB validation first

[Pierre Morel]

When entering the SIE the CRYCB validation better
be done independently of the instruction's
availability.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/kvm/vsie.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 12b9707..38ea5da 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	/* format-1 is supported with message-security-assist extension 3 */
 	if (!test_kvm_facility(vcpu->kvm, 76))
 		return 0;
-	/* 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)
-		return 0;
 
 	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
 		return set_validity_icpt(scb_s, 0x003CU);
 	else if (!crycb_addr)
 		return set_validity_icpt(scb_s, 0x0039U);
 
+	/* 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)
+		return 0;
+
 	/* copy only the wrapping keys */
 	if (read_guest_real(vcpu, crycb_addr + 72,
 			    vsie_page->crycb.dea_wrapping_key_mask, 56))
-- 
2.7.4

[PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Pierre Morel]

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(-)

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;
-- 
2.7.4

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Janosch Frank]

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

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;
> 



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

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[Christian Borntraeger]

On 08/23/2018 12:25 PM, Pierre Morel wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

This should be cc stable I guess?


> ---
>  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 9175518..12b9707 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;
> 

Re: [PATCH v3 2/3] KVM: s390: vsie: Do the CRYCB validation first

[Christian Borntraeger]

On 08/23/2018 12:25 PM, Pierre Morel wrote:
> When entering the SIE the CRYCB validation better
> be done independently of the instruction's
> availability.

Maybe something like

We need to handle the validity checks for the crycb, no matter what the
settings for the keywrappings are. So lets move the keywrapping checks
after we have done the validy checks.

?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 12b9707..38ea5da 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	/* format-1 is supported with message-security-assist extension 3 */
>  	if (!test_kvm_facility(vcpu->kvm, 76))
>  		return 0;
> -	/* 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)
> -		return 0;
> 
>  	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
>  		return set_validity_icpt(scb_s, 0x003CU);
>  	else if (!crycb_addr)
>  		return set_validity_icpt(scb_s, 0x0039U);
> 
> +	/* 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)
> +		return 0;
> +
>  	/* copy only the wrapping keys */
>  	if (read_guest_real(vcpu, crycb_addr + 72,
>  			    vsie_page->crycb.dea_wrapping_key_mask, 56))
> 

Re: [PATCH v3 2/3] KVM: s390: vsie: Do the CRYCB validation first

[David Hildenbrand]

On 23.08.2018 13:17, Christian Borntraeger wrote:
> 
> 
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> When entering the SIE the CRYCB validation better
>> be done independently of the instruction's
>> availability.
> 
> Maybe something like
> 
> We need to handle the validity checks for the crycb, no matter what the
> settings for the keywrappings are. So lets move the keywrapping checks
> after we have done the validy checks.
> 
> ?

With that

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[David Hildenbrand]

On 23.08.2018 13:07, Christian Borntraeger wrote:
> 
> 
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
> This should be cc stable I guess?
> 

Yes, g3 guests will be run with same wrapping keys.

-- 

Thanks,

David / dhildenb

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[David Hildenbrand]

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,

David / dhildenb

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Janosch Frank]

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

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>
> 



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

Re: [PATCH v3 2/3] KVM: s390: vsie: Do the CRYCB validation first

[Pierre Morel]

On 23/08/2018 13:17, Christian Borntraeger wrote:
> 
> 
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> When entering the SIE the CRYCB validation better
>> be done independently of the instruction's
>> availability.
> 
> Maybe something like
> 
> We need to handle the validity checks for the crycb, no matter what the
> settings for the keywrappings are. So lets move the keywrapping checks
> after we have done the validy checks.
> 
> ?

OK thanks, I use this.

regards,
Pierre


> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 12b9707..38ea5da 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>   	/* format-1 is supported with message-security-assist extension 3 */
>>   	if (!test_kvm_facility(vcpu->kvm, 76))
>>   		return 0;
>> -	/* 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)
>> -		return 0;
>>
>>   	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
>>   		return set_validity_icpt(scb_s, 0x003CU);
>>   	else if (!crycb_addr)
>>   		return set_validity_icpt(scb_s, 0x0039U);
>>
>> +	/* 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)
>> +		return 0;
>> +
>>   	/* copy only the wrapping keys */
>>   	if (read_guest_real(vcpu, crycb_addr + 72,
>>   			    vsie_page->crycb.dea_wrapping_key_mask, 56))
>>


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Pierre Morel]

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


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[Pierre Morel]

On 23/08/2018 13:19, David Hildenbrand wrote:
> On 23.08.2018 13:07, Christian Borntraeger wrote:
>>
>>
>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>> Copy the key mask to the right offset inside the shadow CRYCB
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>
>> This should be cc stable I guess?
>>
> 
> Yes, g3 guests will be run with same wrapping keys.
> 

OK I do send it separated from the other patches.

regard,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v3 2/3] KVM: s390: vsie: Do the CRYCB validation first

[Pierre Morel]

On 23/08/2018 13:19, David Hildenbrand wrote:
> On 23.08.2018 13:17, Christian Borntraeger wrote:
>>
>>
>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>> When entering the SIE the CRYCB validation better
>>> be done independently of the instruction's
>>> availability.
>>
>> Maybe something like
>>
>> We need to handle the validity checks for the crycb, no matter what the
>> settings for the keywrappings are. So lets move the keywrapping checks
>> after we have done the validy checks.
>>
>> ?
> 
> With that
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 

Thanks, I use the description proposed by Christian.

regards,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Pierre Morel]

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>
>>
> 
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Janosch Frank]

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

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>
>>>
>>
>>
> 
> 



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

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[David Hildenbrand]

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

-- 

Thanks,

David / dhildenb

Re: [PATCH v3 3/3] KVM: s390: vsie: Make use of CRYCB FORMAT2 clear

[Janosch Frank]

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

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.


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

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[Christian Borntraeger]

On 08/23/2018 01:41 PM, Pierre Morel wrote:
> On 23/08/2018 13:19, David Hildenbrand wrote:
>> On 23.08.2018 13:07, Christian Borntraeger wrote:
>>>
>>>
>>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>>> Copy the key mask to the right offset inside the shadow CRYCB
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>
>>> This should be cc stable I guess?
>>>
>>
>> Yes, g3 guests will be run with same wrapping keys.
>>
> 
> OK I do send it separated from the other patches.

No need to do that. I can add that when picking up.

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[Pierre Morel]

On 23/08/2018 14:43, Christian Borntraeger wrote:
> 
> 
> On 08/23/2018 01:41 PM, Pierre Morel wrote:
>> On 23/08/2018 13:19, David Hildenbrand wrote:
>>> On 23.08.2018 13:07, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>>>> Copy the key mask to the right offset inside the shadow CRYCB
>>>>>
>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>>
>>>> This should be cc stable I guess?
>>>>
>>>
>>> Yes, g3 guests will be run with same wrapping keys.
>>>
>>
>> OK I do send it separated from the other patches.
> 
> No need to do that. I can add that when picking up.
> 

OK
Thank

Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[Christian Borntraeger]

On 08/23/2018 12:25 PM, Pierre Morel wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Janosch Frank <frankja@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 9175518..12b9707 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))

When we fix that code.., dont  we have to XOR the guest3 wrapping keys with
the guest2 ones? 


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

Re: [PATCH v3 1/3] KVM: s390: vsie: copy wrapping keys to right place

[David Hildenbrand]

On 23.08.2018 15:12, Christian Borntraeger wrote:
> 
> 
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@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 9175518..12b9707 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))
> 
> When we fix that code.., dont  we have to XOR the guest3 wrapping keys with
> the guest2 ones? 

That's done further down in that function.

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


-- 

Thanks,

David / dhildenb

Re: [PATCH v3 0/3] KVM: s390: vsie: Consolidate CRYCB validation

[Janosch Frank]

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

On 8/23/18 12:25 PM, Pierre Morel wrote:
> Before adapting the CRYCB shadowing for a guest supporting
> the AP instructions we want to clean the CRYCB shadowing code.
> 
> First patch seem obvious.
> 
> Second patch introduce a change in the behavior of
> the virtual machine in that the CRYCB is validated
> whenever we use or not the key masks to mimic the
> real machine.
> 
> Patch 3 does not correct the compiled code but make
> more clear what is done concerning the formating of
> the CRYCB in the guest original CRYCB and in the
> shadow CRYCB.
> 

Thanks, applied.


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

Re: [PATCH v3 2/3] KVM: s390: vsie: Do the CRYCB validation first

[Janosch Frank]

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

On 8/23/18 12:25 PM, Pierre Morel wrote:
> When entering the SIE the CRYCB validation better
> be done independently of the instruction's
> availability.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 12b9707..38ea5da 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	/* format-1 is supported with message-security-assist extension 3 */
>  	if (!test_kvm_facility(vcpu->kvm, 76))
>  		return 0;
> -	/* 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)
> -		return 0;
>  
>  	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
>  		return set_validity_icpt(scb_s, 0x003CU);
>  	else if (!crycb_addr)
>  		return set_validity_icpt(scb_s, 0x0039U);
>  
> +	/* 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)
> +		return 0;
> +
>  	/* copy only the wrapping keys */
>  	if (read_guest_real(vcpu, crycb_addr + 72,
>  			    vsie_page->crycb.dea_wrapping_key_mask, 56))
> 

I seemed to have forgotten to add this while being preoccupied with the
search for the validity discussion in #3.

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>


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