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