Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On 5/13/22 14:33, Claudio Imbrenda wrote:
> On Fri, 13 May 2022 13:04:34 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> On 5/12/22 16:01, Nico Boehr wrote:
>>> Upon migration, we expect storage keys being set by the guest to be preserved,
>>> so add a test for it.
>>>
>>> We keep 128 pages and set predictable storage keys. Then, we migrate and check
>>> they can be read back and the respective access restrictions are in place when
>>> the access key in the PSW doesn't match.
>>>
>>> TCG currently doesn't implement key-controlled protection, see
>>> target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
>>> tests as xfails.
>>>
>>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>>> ---
>>>  s390x/Makefile         |  1 +
>>>  s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>>>  s390x/unittests.cfg    |  4 ++
>>>  3 files changed, 103 insertions(+)
>>>  create mode 100644 s390x/migration-skey.c
>>>

[...]

>>> +	for (i = 0; i < NUM_PAGES; i++) {
>>> +		report_prefix_pushf("page %d", i);
>>> +
>>> +		page = &pagebuf[i][0];
>>> +		actual_key.val = get_storage_key(page);
>>> +		expected_key.val = i * 2;
>>> +
>>> +		/* ignore reference bit */
>>> +		actual_key.str.rf = 0;
>>> +		expected_key.str.rf = 0;
>>> +
>>> +		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
>>> +
>>> +		/* ensure access key doesn't match storage key and is never zero */
>>> +		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;
>>> +		*page = 0xff;
>>> +
>>> +		expect_pgm_int();
>>> +		asm volatile (
>>> +			/* set access key */
>>> +			"spka 0(%[mismatching_key])\n"
>>> +			/* try to write page */
>>> +			"mvi 0(%[page]), 42\n"
>>> +			/* reset access key */
>>> +			"spka 0\n"
>>> +			:
>>> +			: [mismatching_key] "a"(mismatching_key.val),
>>> +			  [page] "a"(page)
>>> +			: "memory"
>>> +		);
>>> +		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);
>>> +		report_xfail(host_is_tcg(), *page == 0xff, "no store occured");  
>>
>> What are you testing with this bit? If storage keys are really effective after the migration?
>> I'm wondering if using tprot would not be better, it should simplify the code a lot.
>> Plus you'd easily test for fetch protection, too.
> 
> on the other hand you could have tprot successful, but then not honour
> the protection it indicates (I don't know how TPROT is implemented in
> TCG)

Not at all with regards to skeys. But neither is checking the keys on access.
And for kvm, both TPROT and checking is handled by SIE.
> 
> to be fair, this test is only about checking that storage keys are
> correctly migrated, maybe the check for actual protection is out of
> scope
> 

Having more tests does no harm and might uncover things nobody thought of,
but I'd also be fine with keeping it short and sweet.
[...]