Re: [kvm-unit-tests PATCH v4] s390x: Test effect of storage keys on some instructions

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

On 4/25/22 13:16, Claudio Imbrenda wrote:
> On Mon, 25 Apr 2022 10:41:28 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Some instructions are emulated by KVM. Test that KVM correctly emulates
>> storage key checking for two of those instructions (STORE CPU ADDRESS,
>> SET PREFIX).
>> Test success and error conditions, including coverage of storage and
>> fetch protection override.
>> Also add test for TEST PROTECTION, even if that instruction will not be
>> emulated by KVM under normal conditions.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---

[...]

>> +static void test_set_prefix(void)
>> +{
>> +	char lowcore_tmp[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> 
> perhaps it's cleaner to put this as a global (static) variable
> 
> also, please define LC_SIZE (2*PAGE_SIZE) and use that

I'll call that PREFIX_AREA_SIZE, otherwise it is confusing that that is
not the same as sizeof(struct lowcore).
> 
>> +	uint32_t *prefix_ptr = (uint32_t *)pagebuf;
>> +	uint32_t old_prefix;
>> +	pgd_t *root;
>> +
>> +	report_prefix_push("SET PREFIX");
>> +	root = (pgd_t *)(stctg(1) & PAGE_MASK);
>> +	old_prefix = get_prefix();
>> +	memcpy(lowcore_tmp, 0, PAGE_SIZE * 2);
>> +	assert(((uint64_t)&lowcore_tmp >> 31) == 0);
>> +	*prefix_ptr = (uint32_t)(uint64_t)&lowcore_tmp;
>> +
>> +	report_prefix_push("zero key");
>> +	set_prefix(old_prefix);
>> +	set_storage_key(prefix_ptr, 0x20, 0);
>> +	set_prefix(*prefix_ptr);
>> +	report(get_prefix() == *prefix_ptr, "set prefix");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("matching key");
>> +	set_prefix(old_prefix);
>> +	set_storage_key(pagebuf, 0x10, 0);
>> +	set_prefix_key_1(prefix_ptr);
>> +	report(get_prefix() == *prefix_ptr, "set prefix");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("mismatching key");
>> +
>> +	report_prefix_push("no fetch protection");
>> +	set_prefix(old_prefix);
>> +	set_storage_key(pagebuf, 0x20, 0);
>> +	set_prefix_key_1(prefix_ptr);
>> +	report(get_prefix() == *prefix_ptr, "set prefix");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("fetch protection");
>> +	set_prefix(old_prefix);
>> +	set_storage_key(pagebuf, 0x28, 0);
>> +	expect_pgm_int();
>> +	set_prefix_key_1(prefix_ptr);
>> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>> +	report(get_prefix() != *prefix_ptr, "did not set prefix");
> 
> why don't you check == old_prefix instead? that way you know noting has
> changed (also for all the other tests below where you do the same)

Yeah, that's better.

[...]