On 4/25/22 13:16, Claudio Imbrenda wrote:
> On Mon, 25 Apr 2022 10:41:28 +0200
> Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
[...]
>> +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.
[...]
