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
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 <[email protected]>
---
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
When entering the SIE the CRYCB validation better
be done independently of the instruction's
availability.
Signed-off-by: Pierre Morel <[email protected]>
---
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
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 <[email protected]>
---
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
Copy the key mask to the right offset inside the shadow CRYCB
Signed-off-by: Pierre Morel <[email protected]>
Reviewed-by: David Hildenbrand <[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 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
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 <[email protected]>
---
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
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 <[email protected]>
> Reviewed-by: David Hildenbrand <[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 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> Reviewed-by: David Hildenbrand <[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 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> ---
>> 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
On Wed, 22 Aug 2018 18:53:02 +0200
David Hildenbrand <[email protected]> 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 <[email protected]>
> > Reviewed-by: David Hildenbrand <[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 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 <[email protected]>
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
>> Reviewed-by: David Hildenbrand <[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 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...
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 <[email protected]>
>>>> ---
>>>> 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
On 23/08/2018 09:27, Cornelia Huck wrote:
> On Wed, 22 Aug 2018 18:53:02 +0200
> David Hildenbrand <[email protected]> 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 <[email protected]>
>>> Reviewed-by: David Hildenbrand <[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 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 <[email protected]>
>
Thanks,
regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
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 <[email protected]>
>>>>> ---
>>>>> 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
>
>
>
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 <[email protected]>
>>>>>> ---
>>>>>> 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
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 <[email protected]>
>>> Reviewed-by: David Hildenbrand <[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 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