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

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

On 2/24/22 15:30, Claudio Imbrenda wrote:
> On Thu, 24 Feb 2022 12:09:50 +0100
> 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>
>> ---
>>
>>  	*entry_0_p = entry_pagebuf;
>>
>> I'm wondering if we need a barrier here, or would if set_prefix_key_1
>> wasn't made up of an asm volatile. But the mmu code seems to not have a
>> barrier in the equivalent code, so maybe it's never needed.
>>
>>  	set_prefix_key_1(0);
>>
>>  lib/s390x/asm/arch_def.h |  20 ++---
>>  s390x/skey.c             | 169 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 180 insertions(+), 9 deletions(-)
>>

[...]

>> diff --git a/s390x/skey.c b/s390x/skey.c
>> index 58a55436..6ae2d026 100644
>> --- a/s390x/skey.c
>> +++ b/s390x/skey.c
>> @@ -10,7 +10,10 @@
>>  #include <libcflat.h>
>>  #include <asm/asm-offsets.h>
>>  #include <asm/interrupt.h>
>> +#include <vmalloc.h>
>> +#include <mmu.h>
>>  #include <asm/page.h>
>> +#include <asm/pgtable.h>
>>  #include <asm/facility.h>
>>  #include <asm/mem.h>
>>  
>> @@ -147,6 +150,167 @@ static void test_invalid_address(void)
>>  	report_prefix_pop();
>>  }
>>  
>> +static void test_test_protection(void)
>> +{
>> +	unsigned long addr = (unsigned long)pagebuf;
>> +
>> +	report_prefix_push("TPROT");
>> +	set_storage_key(pagebuf, 0x10, 0);
>> +	report(tprot(addr, 0) == 0, "access key 0 -> no protection");
>> +	report(tprot(addr, 1) == 0, "access key matches -> no protection");
>> +	report(tprot(addr, 2) == 1, "access key mismatches, no fetch protection -> store protection");
>> +	set_storage_key(pagebuf, 0x18, 0);
>> +	report(tprot(addr, 2) == 2, "access key mismatches, fetch protection -> fetch & store protection");
>> +	report_prefix_pop();
> 
> is there a reason why you don't set the storage key back to 0 once
> you're done?

None, other than it not being necessary, but I like the idea.
> 
>> +}
>> +
>> +static void store_cpu_address_key_1(uint16_t *out)
>> +{
>> +	asm volatile (
>> +		"spka 0x10(0)\n\t"
>> +		"stap %0\n\t"
>> +		"spka 0(0)\n"
>> +	     : "=Q" (*out)
>> +	);
>> +}
>> +
>> +static void test_store_cpu_address(void)
>> +{
>> +	uint16_t *out = (uint16_t *)pagebuf;
>> +	uint16_t cpu_addr;
>> +
>> +	asm ("stap %0" : "=Q" (cpu_addr));
>> +
>> +	report_prefix_push("STORE CPU ADDRESS, zero key");
>> +	set_storage_key(pagebuf, 0x20, 0);
>> +	*out = 0xbeef;
>> +	asm ("stap %0" : "=Q" (*out));
>> +	report(*out == cpu_addr, "store occurred");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("STORE CPU ADDRESS, matching key");
>> +	set_storage_key(pagebuf, 0x10, 0);
>> +	*out = 0xbeef;
>> +	store_cpu_address_key_1(out);
>> +	report(*out == cpu_addr, "store occurred");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("STORE CPU ADDRESS, mismatching key");
>> +	set_storage_key(pagebuf, 0x20, 0);
>> +	expect_pgm_int();
>> +	store_cpu_address_key_1(out);
>> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> 
> for completeness, maybe also check that nothing gets stored?

Can do.
> 
>> +	report_prefix_pop();
>> +
>> +	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
>> +
>> +	report_prefix_push("STORE CPU ADDRESS, storage-protection override, invalid key");
>> +	set_storage_key(pagebuf, 0x20, 0);
>> +	expect_pgm_int();
>> +	store_cpu_address_key_1(out);
>> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> 
> same here
> 

[...]