[PATCH v2 0/5] KVM: s390: vsie: Consolidate CRYCB validation

[PATCH v2 0/5] 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.


Pierre Morel (5):
  KVM: s390: vsie: BUG correction by shadow_crycb
  KVM: s390: vsie: Only accept FORMAT1 CRYCB for guest2
  KVM: s390: vsie: Allow support for a host without AP
  KVM: s390: vsie: Always test the crycbd for NULL
  KVM: s390: vsie: Do the CRYCB validation first

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

-- 
2.7.4

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 v2 1/5] 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>
Reviewed-by: David Hildenbrand <david@redhat.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 v2 2/5] KVM: s390: vsie: Only accept FORMAT1 CRYCB for guest2

[Pierre Morel]

As the comment above the function suggested the shadowing
of the guest2 CRYCB can only accept a format 1 since
AP instructions are not supported in the guest.

Let's modify the check which allowed to accept a format 2 too.

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

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 12b9707..56a9d47 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -156,7 +156,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	u8 ecb3_flags;
 
 	scb_s->crycbd = 0;
-	if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
+	if (!(crycbd_o == CRYCB_FORMAT1))
+		return 0;
+	if (!(vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
 		return 0;
 	/* format-1 is supported with message-security-assist extension 3 */
 	if (!test_kvm_facility(vcpu->kvm, 76))
-- 
2.7.4

[PATCH v2 3/5] KVM: s390: vsie: Allow support for a host without AP

[Pierre Morel]

Currently the CRYCB format used in the host for the
shadowed CRYCB is FORMAT2 while no check is done if
AP instructions are supported in the host.

We better use the format the host calculated for the
guest 1 as the host already tested it against its
facility set.

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

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 56a9d47..0b12916 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -154,6 +154,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;
+	unsigned long g1_fmt;
 
 	scb_s->crycbd = 0;
 	if (!(crycbd_o == CRYCB_FORMAT1))
@@ -180,8 +181,8 @@ 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;
+	g1_fmt = vcpu->arch.sie_block->crycbd & 0x03;
+	scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | g1_fmt;
 
 	/* xor both blocks in one run */
 	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
-- 
2.7.4

[PATCH v2 4/5] KVM: s390: vsie: Always test the crycbd for NULL

[Pierre Morel]

BUG: the crycbd must be tested for null even if
not crossing a page boundary (which will never
occur in this case anyway).

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/kvm/vsie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 0b12916..7ee4329 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -172,7 +172,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 
 	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
 		return set_validity_icpt(scb_s, 0x003CU);
-	else if (!crycb_addr)
+	if (!crycb_addr)
 		return set_validity_icpt(scb_s, 0x0039U);
 
 	/* copy only the wrapping keys */
-- 
2.7.4

[PATCH v2 5/5] 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 7ee4329..fca25aa 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -164,17 +164,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);
 	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

Re: [PATCH v2 1/5] KVM: s390: vsie: BUG correction by shadow_crycb

[David Hildenbrand]

On 22.08.2018 18:51, 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>
> ---
>  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;
> 

Please fixup the subject as requested. (were there more RB-s?)

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 2/5] KVM: s390: vsie: Only accept FORMAT1 CRYCB for guest2

[David Hildenbrand]

On 22.08.2018 18:51, Pierre Morel wrote:
> As the comment above the function suggested the shadowing
> of the guest2 CRYCB can only accept a format 1 since
> AP instructions are not supported in the guest.
> 
> Let's modify the check which allowed to accept a format 2 too.

As the bit is ignored without AP/APXA, it is perfectly valid to accept a
format 2, we just have to interpret it as format 1 (which is what we do)

What am I missing?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 12b9707..56a9d47 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -156,7 +156,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	u8 ecb3_flags;
>  
>  	scb_s->crycbd = 0;
> -	if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
> +	if (!(crycbd_o == CRYCB_FORMAT1))
> +		return 0;

huh, this looks very broken. The address is still in there.

> +	if (!(vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>  		return 0;
>  	/* format-1 is supported with message-security-assist extension 3 */
>  	if (!test_kvm_facility(vcpu->kvm, 76))
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 3/5] KVM: s390: vsie: Allow support for a host without AP

[David Hildenbrand]

On 22.08.2018 18:51, Pierre Morel wrote:
> Currently the CRYCB format used in the host for the
> shadowed CRYCB is FORMAT2 while no check is done if
> AP instructions are supported in the host.
> 
> We better use the format the host calculated for the
> guest 1 as the host already tested it against its
> facility set.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 56a9d47..0b12916 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -154,6 +154,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;
> +	unsigned long g1_fmt;
>  
>  	scb_s->crycbd = 0;
>  	if (!(crycbd_o == CRYCB_FORMAT1))
> @@ -180,8 +181,8 @@ 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;
> +	g1_fmt = vcpu->arch.sie_block->crycbd & 0x03;
> +	scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | g1_fmt;
>  
>  	/* xor both blocks in one run */
>  	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
> 

This is wrong. I remember that with APXA, if FORMAT2 is available, we
should always use FORMAT2. That's why we explicitly convert it here.

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 4/5] KVM: s390: vsie: Always test the crycbd for NULL

[David Hildenbrand]

On 22.08.2018 18:51, Pierre Morel wrote:
> BUG: the crycbd must be tested for null even if
> not crossing a page boundary (which will never
> occur in this case anyway).

I don't see the BUG. Can you elaborate? (maybe it is too late for me)

Either we return or we check for !crycb_addr

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 0b12916..7ee4329 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -172,7 +172,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  
>  	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
>  		return set_validity_icpt(scb_s, 0x003CU);
> -	else if (!crycb_addr)
> +	if (!crycb_addr)
>  		return set_validity_icpt(scb_s, 0x0039U);
>  
>  	/* copy only the wrapping keys */
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 5/5] KVM: s390: vsie: Do the CRYCB validation first

[David Hildenbrand]

On 22.08.2018 18:51, 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 7ee4329..fca25aa 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -164,17 +164,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);
>  	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))
> 

That makes sense, especially if ECB3_AES is used but effectively turned
off by us.

What is the expected behavior if ECB3_AES | ECB3_DEA are not set by g2
for g3?

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 1/5] KVM: s390: vsie: BUG correction by shadow_crycb

[Pierre Morel]

On 22/08/2018 18:53, David Hildenbrand wrote:
> On 22.08.2018 18:51, 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>
>> ---
>>   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;
>>
> 
> Please fixup the subject as requested. (were there more RB-s?)
> 

Yes, sorry, will do.

No no other RB yet.

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

Re: [PATCH v2 3/5] KVM: s390: vsie: Allow support for a host without AP

[Pierre Morel]

On 22/08/2018 19:06, David Hildenbrand wrote:
> On 22.08.2018 18:51, Pierre Morel wrote:
>> Currently the CRYCB format used in the host for the
>> shadowed CRYCB is FORMAT2 while no check is done if
>> AP instructions are supported in the host.
>>
>> We better use the format the host calculated for the
>> guest 1 as the host already tested it against its
>> facility set.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 56a9d47..0b12916 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -154,6 +154,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;
>> +	unsigned long g1_fmt;
>>   
>>   	scb_s->crycbd = 0;
>>   	if (!(crycbd_o == CRYCB_FORMAT1))
>> @@ -180,8 +181,8 @@ 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;
>> +	g1_fmt = vcpu->arch.sie_block->crycbd & 0x03;
>> +	scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | g1_fmt;
>>   
>>   	/* xor both blocks in one run */
>>   	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>>
> 
> This is wrong. I remember that with APXA, if FORMAT2 is available, we
> should always use FORMAT2. That's why we explicitly convert it here.
> 

You are right if FORMAT2 is available we should use FORMAT2
but the intention here is to use what KVM crypto init function did,
assuming it did the right thing.

Eventually we are running on a host without AP and we should use FORMAT1.

Isn't it correct?

Regards,
Pierre


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

Re: [PATCH v2 3/5] KVM: s390: vsie: Allow support for a host without AP

[Pierre Morel]

On 22/08/2018 19:06, David Hildenbrand wrote:
> On 22.08.2018 18:51, Pierre Morel wrote:
>> Currently the CRYCB format used in the host for the
>> shadowed CRYCB is FORMAT2 while no check is done if
>> AP instructions are supported in the host.
>>
>> We better use the format the host calculated for the
>> guest 1 as the host already tested it against its
>> facility set.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 56a9d47..0b12916 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -154,6 +154,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;
>> +	unsigned long g1_fmt;
>>   
>>   	scb_s->crycbd = 0;
>>   	if (!(crycbd_o == CRYCB_FORMAT1))
>> @@ -180,8 +181,8 @@ 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;
>> +	g1_fmt = vcpu->arch.sie_block->crycbd & 0x03;
>> +	scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | g1_fmt;
>>   
>>   	/* xor both blocks in one run */
>>   	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>>
> 
> This is wrong. I remember that with APXA, if FORMAT2 is available, we
> should always use FORMAT2. That's why we explicitly convert it here.
> 

You are right if FORMAT2 is available we should use FORMAT2
but the intention here is to use what KVM crypto init function did,
assuming it did the right thing.

Eventually we are running on a host without AP and we should use FORMAT1.

Isn't it correct?

Regards,
Pierre


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

Re: [PATCH v2 4/5] KVM: s390: vsie: Always test the crycbd for NULL

[Pierre Morel]

On 22/08/2018 19:07, David Hildenbrand wrote:
> On 22.08.2018 18:51, Pierre Morel wrote:
>> BUG: the crycbd must be tested for null even if
>> not crossing a page boundary (which will never
>> occur in this case anyway).
> 
> I don't see the BUG. Can you elaborate? (maybe it is too late for me)

No it was too late for me :)
The BUG was by me, my brain was fooled by the "else"
there is not problem there.
CRCBD is always tested.

Sorry.

> 
> Either we return or we check for !crycb_addr
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 0b12916..7ee4329 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -172,7 +172,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>   
>>   	if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
>>   		return set_validity_icpt(scb_s, 0x003CU);
>> -	else if (!crycb_addr)
>> +	if (!crycb_addr)
>>   		return set_validity_icpt(scb_s, 0x0039U);
>>   
>>   	/* copy only the wrapping keys */
>>
> 
> 


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

Re: [PATCH v2 3/5] KVM: s390: vsie: Allow support for a host without AP

[David Hildenbrand]

On 23.08.2018 08:44, Pierre Morel wrote:
> On 22/08/2018 19:06, David Hildenbrand wrote:
>> On 22.08.2018 18:51, Pierre Morel wrote:
>>> Currently the CRYCB format used in the host for the
>>> shadowed CRYCB is FORMAT2 while no check is done if
>>> AP instructions are supported in the host.
>>>
>>> We better use the format the host calculated for the
>>> guest 1 as the host already tested it against its
>>> facility set.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   arch/s390/kvm/vsie.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index 56a9d47..0b12916 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -154,6 +154,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;
>>> +	unsigned long g1_fmt;
>>>   
>>>   	scb_s->crycbd = 0;
>>>   	if (!(crycbd_o == CRYCB_FORMAT1))
>>> @@ -180,8 +181,8 @@ 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;
>>> +	g1_fmt = vcpu->arch.sie_block->crycbd & 0x03;
>>> +	scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | g1_fmt;
>>>   
>>>   	/* xor both blocks in one run */
>>>   	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>>>
>>
>> This is wrong. I remember that with APXA, if FORMAT2 is available, we
>> should always use FORMAT2. That's why we explicitly convert it here.
>>
> 
> You are right if FORMAT2 is available we should use FORMAT2
> but the intention here is to use what KVM crypto init function did,
> assuming it did the right thing.
> 
> Eventually we are running on a host without AP and we should use FORMAT1.
> 
> Isn't it correct?


Yes and no :)

No APXA -> FORMAT2 bit is ignored (and that is one of the reasons why I
am being so strict about simulating HW behavior correctly in nested code
:) )

This only holds as long as we are not using AP. Because from a MSA3
perspective, FORMAT1==FORMAT2 (apart from the length/alignment, which is
fine for us).

Once we support AP (via ECA.28), we'll properly have to create either a
Format0/Format1/Format2. Then, there is actually a semantically
difference ("different fields used").

> 
> Regards,
> Pierre
> 
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 5/5] KVM: s390: vsie: Do the CRYCB validation first

[Pierre Morel]

On 22/08/2018 19:15, David Hildenbrand wrote:
> On 22.08.2018 18:51, 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 7ee4329..fca25aa 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -164,17 +164,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);
>>   	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))
>>
> 
> That makes sense, especially if ECB3_AES is used but effectively turned
> off by us.
> 
> What is the expected behavior if ECB3_AES | ECB3_DEA are not set by g2
> for g3?
> 

The use of functions PCKMO-Encrypt-DEA/AES induce a specification error.

However other MSA3 function will continue to be usable.

Regards,
Pierre

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

Re: [PATCH v2 1/5] KVM: s390: vsie: BUG correction by shadow_crycb

[Cornelia Huck]

On Wed, 22 Aug 2018 18:53:02 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 22.08.2018 18:51, 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>
> > ---
> >  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;
> >   
> 
> Please fixup the subject as requested.

+1

> (were there more RB-s?)
> 

Yep, mine. FTR:

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2 5/5] KVM: s390: vsie: Do the CRYCB validation first

[David Hildenbrand]

On 23.08.2018 09:17, Pierre Morel wrote:
> On 22/08/2018 19:15, David Hildenbrand wrote:
>> On 22.08.2018 18:51, 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 7ee4329..fca25aa 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -164,17 +164,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);
>>>   	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))
>>>
>>
>> That makes sense, especially if ECB3_AES is used but effectively turned
>> off by us.
>>
>> What is the expected behavior if ECB3_AES | ECB3_DEA are not set by g2
>> for g3?
>>
> 
> The use of functions PCKMO-Encrypt-DEA/AES induce a specification error.
> 
> However other MSA3 function will continue to be usable.

No, I meant which checks should be performed here.

> 
> Regards,
> Pierre
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 2/5] KVM: s390: vsie: Only accept FORMAT1 CRYCB for guest2

[Pierre Morel]

On 22/08/2018 18:55, David Hildenbrand wrote:
> On 22.08.2018 18:51, Pierre Morel wrote:
>> As the comment above the function suggested the shadowing
>> of the guest2 CRYCB can only accept a format 1 since
>> AP instructions are not supported in the guest.
>>
>> Let's modify the check which allowed to accept a format 2 too.
> 
> As the bit is ignored without AP/APXA, it is perfectly valid to accept a
> format 2, we just have to interpret it as format 1 (which is what we do)
> 
> What am I missing?
> 

Nothing.
I was still having AP interpretation in mind.


>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 12b9707..56a9d47 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -156,7 +156,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>   	u8 ecb3_flags;
>>   
>>   	scb_s->crycbd = 0;
>> -	if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>> +	if (!(crycbd_o == CRYCB_FORMAT1))
>> +		return 0;
> 
> huh, this looks very broken. The address is still in there.
completely broken you are right

anyway this broken useless patch disappear.

Thanks,

regards,
Pierre


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

Re: [PATCH v2 3/5] KVM: s390: vsie: Allow support for a host without AP

[Pierre Morel]

On 23/08/2018 09:15, David Hildenbrand wrote:
> On 23.08.2018 08:44, Pierre Morel wrote:
>> On 22/08/2018 19:06, David Hildenbrand wrote:
>>> On 22.08.2018 18:51, Pierre Morel wrote:
>>>> Currently the CRYCB format used in the host for the
>>>> shadowed CRYCB is FORMAT2 while no check is done if
>>>> AP instructions are supported in the host.
>>>>
>>>> We better use the format the host calculated for the
>>>> guest 1 as the host already tested it against its
>>>> facility set.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    arch/s390/kvm/vsie.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index 56a9d47..0b12916 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -154,6 +154,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;
>>>> +	unsigned long g1_fmt;
>>>>    
>>>>    	scb_s->crycbd = 0;
>>>>    	if (!(crycbd_o == CRYCB_FORMAT1))
>>>> @@ -180,8 +181,8 @@ 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;
>>>> +	g1_fmt = vcpu->arch.sie_block->crycbd & 0x03;
>>>> +	scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | g1_fmt;
>>>>    
>>>>    	/* xor both blocks in one run */
>>>>    	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>>>>
>>>
>>> This is wrong. I remember that with APXA, if FORMAT2 is available, we
>>> should always use FORMAT2. That's why we explicitly convert it here.
>>>
>>
>> You are right if FORMAT2 is available we should use FORMAT2
>> but the intention here is to use what KVM crypto init function did,
>> assuming it did the right thing.
>>
>> Eventually we are running on a host without AP and we should use FORMAT1.
>>
>> Isn't it correct?
> 
> 
> Yes and no :)
> 
> No APXA -> FORMAT2 bit is ignored (and that is one of the reasons why I
> am being so strict about simulating HW behavior correctly in nested code
> :) )
> 
> This only holds as long as we are not using AP. Because from a MSA3
> perspective, FORMAT1==FORMAT2 (apart from the length/alignment, which is
> fine for us).
> 
> Once we support AP (via ECA.28), we'll properly have to create either a
> Format0/Format1/Format2. Then, there is actually a semantically
> difference ("different fields used").

OK
I would have expect something more explicit in the documentation
like for firmware versions older than xxx bit 30 is ignored
instead of if APXA is not installed.
Still must learn the IBM language! :)

regards,
Pierre




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

Re: [PATCH v2 5/5] KVM: s390: vsie: Do the CRYCB validation first

[Pierre Morel]

On 23/08/2018 09:31, David Hildenbrand wrote:
> On 23.08.2018 09:17, Pierre Morel wrote:
>> On 22/08/2018 19:15, David Hildenbrand wrote:
>>> On 22.08.2018 18:51, 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 7ee4329..fca25aa 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -164,17 +164,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);
>>>>    	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))
>>>>
>>>
>>> That makes sense, especially if ECB3_AES is used but effectively turned
>>> off by us.
>>>
>>> What is the expected behavior if ECB3_AES | ECB3_DEA are not set by g2
>>> for g3?
>>>
>>
>> The use of functions PCKMO-Encrypt-DEA/AES induce a specification error.
>>
>> However other MSA3 function will continue to be usable.
> 
> No, I meant which checks should be performed here.

The SIE should check the validity of the CRYCB.

However since we do not copy the key masks we do not
expect any access error on crycb_o

So it is more a philosophical problem, should the
hypervizor enforce an error here to act as the firmware?


regards,
Pierre



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

Re: [PATCH v2 1/5] KVM: s390: vsie: BUG correction by shadow_crycb

[Janosch Frank]

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

On 22.08.2018 18:53, David Hildenbrand wrote:
> On 22.08.2018 18:51, 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>
>> ---
>>  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;
>>
> 
> Please fixup the subject as requested. (were there more RB-s?)
> 

Yes, he seems to have ignored mine...


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

Re: [PATCH v2 1/5] KVM: s390: vsie: BUG correction by shadow_crycb

[Pierre Morel]

On 23/08/2018 09:27, Cornelia Huck wrote:
> On Wed, 22 Aug 2018 18:53:02 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 22.08.2018 18:51, 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>
>>> ---
>>>   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;
>>>    
>>
>> Please fixup the subject as requested.
> 
> +1
> 
>> (were there more RB-s?)

Already done in my local branch.

>>
> 
> Yep, mine. FTR:
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,

regards,
Pierre

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

Re: [PATCH v2 1/5] KVM: s390: vsie: BUG correction by shadow_crycb

[Pierre Morel]

On 23/08/2018 10:02, Janosch Frank wrote:
> On 22.08.2018 18:53, David Hildenbrand wrote:
>> On 22.08.2018 18:51, 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>
>>> ---
>>>   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;
>>>
>>
>> Please fixup the subject as requested. (were there more RB-s?)
>>
> 
> Yes, he seems to have ignored mine...
> 
Sorry,
I focused on your comment and the answer from David and forgot it.

thanks Janosch I will add it of course

regards,
Pierre

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

Re: [PATCH v2 5/5] KVM: s390: vsie: Do the CRYCB validation first

[Janosch Frank]

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

On 23.08.2018 10:01, Pierre Morel wrote:
> On 23/08/2018 09:31, David Hildenbrand wrote:
>> On 23.08.2018 09:17, Pierre Morel wrote:
>>> On 22/08/2018 19:15, David Hildenbrand wrote:
>>>> On 22.08.2018 18:51, 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 7ee4329..fca25aa 100644
>>>>> --- a/arch/s390/kvm/vsie.c
>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>> @@ -164,17 +164,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);
>>>>>    	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))
>>>>>
>>>>
>>>> That makes sense, especially if ECB3_AES is used but effectively turned
>>>> off by us.
>>>>
>>>> What is the expected behavior if ECB3_AES | ECB3_DEA are not set by g2
>>>> for g3?
>>>>
>>>
>>> The use of functions PCKMO-Encrypt-DEA/AES induce a specification error.
>>>
>>> However other MSA3 function will continue to be usable.
>>
>> No, I meant which checks should be performed here.
> 
> The SIE should check the validity of the CRYCB.
> 
> However since we do not copy the key masks we do not
> expect any access error on crycb_o
> 
> So it is more a philosophical problem, should the
> hypervizor enforce an error here to act as the firmware?

No it's not philosophical, that's actually regulated in the SIE
documentation for the validity intercepts.

CRYCB is checked if (any of these is true): ECA.28, CRYCB Format is one,
APXA installed and CRYCB Format field is three.

ECB3 AES/DEA bits are handled like the matrix, i.e. they are ANDed over
the different levels.

If that's still not what David meant to ask, then I must apologize for
my caffeine deprived brain.

> 
> 
> regards,
> Pierre
> 
> 
> 



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

Re: [PATCH v2 5/5] KVM: s390: vsie: Do the CRYCB validation first

[David Hildenbrand]

On 23.08.2018 10:34, Janosch Frank wrote:
> On 23.08.2018 10:01, Pierre Morel wrote:
>> On 23/08/2018 09:31, David Hildenbrand wrote:
>>> On 23.08.2018 09:17, Pierre Morel wrote:
>>>> On 22/08/2018 19:15, David Hildenbrand wrote:
>>>>> On 22.08.2018 18:51, 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 7ee4329..fca25aa 100644
>>>>>> --- a/arch/s390/kvm/vsie.c
>>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>>> @@ -164,17 +164,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);
>>>>>>    	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))
>>>>>>
>>>>>
>>>>> That makes sense, especially if ECB3_AES is used but effectively turned
>>>>> off by us.
>>>>>
>>>>> What is the expected behavior if ECB3_AES | ECB3_DEA are not set by g2
>>>>> for g3?
>>>>>
>>>>
>>>> The use of functions PCKMO-Encrypt-DEA/AES induce a specification error.
>>>>
>>>> However other MSA3 function will continue to be usable.
>>>
>>> No, I meant which checks should be performed here.
>>
>> The SIE should check the validity of the CRYCB.
>>
>> However since we do not copy the key masks we do not
>> expect any access error on crycb_o
>>
>> So it is more a philosophical problem, should the
>> hypervizor enforce an error here to act as the firmware?
> 
> No it's not philosophical, that's actually regulated in the SIE
> documentation for the validity intercepts.
> 
> CRYCB is checked if (any of these is true): ECA.28, CRYCB Format is one,
> APXA installed and CRYCB Format field is three.

So independent of setting of ECB3 AES/DEA by g2. That's what I wanted to
know, thanks :)

> 
> ECB3 AES/DEA bits are handled like the matrix, i.e. they are ANDed over
> the different levels.
> 
> If that's still not what David meant to ask, then I must apologize for
> my caffeine deprived brain.


-- 

Thanks,

David / dhildenb