On 2/24/22 15:30, Claudio Imbrenda wrote:
> On Thu, 24 Feb 2022 12:09:50 +0100
> 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>
>> ---
>>
>> *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
>
[...]
