Re: [kvm-unit-tests PATCH 2/3] s390x: pv: Test sie entry intercepts and validities

[Janosch Frank <frankja@linux.ibm.com>]

On 2/15/23 18:06, Claudio Imbrenda wrote:
> On Wed,  1 Feb 2023 08:48:32 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The lowcore is an important part of any s390 cpu so we need to make
>> sure it's always available when we virtualize one. For non-PV guests
>> that would mean ensuring that the lowcore page is read and writable by
>> the guest.
>>
>> For PV guests we additionally need to make sure that the page is owned
>> by the guest as it is only allowed to access them if that's the
>> case. The code 112 SIE intercept tells us if the lowcore pages aren't
>> secure anymore.
>>
>> Let's check if that intercept is reported by SIE if we export the
>> lowcore pages. Additionally check if that's also the case if the guest
>> shares the lowcore which will make it readable to the host but
>> ownership of the page should not change.
>>
>> Also we check for validities in these conditions:
>>       * Manipulated cpu timer
>>       * Double SIE for same vcpu
>>       * Re-use of VCPU handle from another secure configuration
>>       * ASCE re-use
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> looks good, see some questions below
> 
>> ---
[...]
>> +	extern const char SNIPPET_NAME_START(asm, snippet_pv_icpt_vir_timing)[];
>> +	extern const char SNIPPET_NAME_END(asm, snippet_pv_icpt_vir_timing)[];
>> +	extern const char SNIPPET_HDR_START(asm, snippet_pv_icpt_vir_timing)[];
>> +	extern const char SNIPPET_HDR_END(asm, snippet_pv_icpt_vir_timing)[];
>> +	int size_hdr = SNIPPET_HDR_LEN(asm, snippet_pv_icpt_vir_timing);
>> +	int size_gbin = SNIPPET_LEN(asm, snippet_pv_icpt_vir_timing);
>> +
>> +	report_prefix_push("manipulated cpu time");
>> +	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_icpt_vir_timing),
>> +			SNIPPET_HDR_START(asm, snippet_pv_icpt_vir_timing),
>> +			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>> +
>> +	sie(&vm);
>> +	report(vm.sblk->icptcode == ICPT_PV_NOTIFY && vm.sblk->ipa == 0x8302 &&
>> +	       vm.sblk->ipb == 0x50000000 && vm.save_area.guest.grs[5] == 0x44,
>> +	       "stp done");
>> +	vm.sblk->cputm -= 0x280de80000 / 2;
> 
> so you are subtracting half of the value?
> 
> why not vm.sblk->cputm /= 2?
> or just set a fixed (very low) magic value?
> 
> what should happen if the cpu timer is higher instead of lower?

I'll need to do some digging to find out why I used this specific 
procedure. It's been a very long time since I wrote those tests.

[...]

>> +	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_loop),
>> +			SNIPPET_HDR_START(asm, snippet_loop),
>> +			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>> +
>> +	sie_expect_validity(&vm);
>> +	smp_cpu_setup(1, psw);
>> +	smp_cpu_setup(2, psw);
>> +	while (vm.sblk->icptcode != ICPT_VALIDITY) { mb(); }
> 
> maybe put the mb(); in a separate line

Can do

> 
>> +	/* Yes I know this is not reliable as one cpu might overwrite it */
> 
> the wording in this comment could be improved

How about:
This might not be fully reliable but it should be sufficient for our 
current goals.

[...]

>> +	report_prefix_push("shared");
>> +	sie(&vm);
>> +	/* Guest indicates that it has shared the new lowcore */
>> +	report(vm.sblk->icptcode == ICPT_PV_NOTIFY && vm.sblk->ipa == 0x8302 &&
>> +	       vm.sblk->ipb == 0x50000000 && vm.save_area.guest.grs[5] == 0x44,
>> +	       "intercept values");
>> +
>> +	uv_export(vm.sblk->mso + lc_off);
>> +	uv_export(vm.sblk->mso + lc_off + PAGE_SIZE);
> 
> why are you not testing both pages individually here, like you did
> above?

Hmm, I don't think there was a reason behind this. I'll add it.

[...]