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

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

On 4/12/22 11:32, Thomas Huth wrote:
> On 01/03/2022 10.50, Janis Schoetterl-Glausch 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>
>> ---
> [...]
>>   lib/s390x/asm/arch_def.h |  20 ++---
>>   s390x/skey.c             | 171 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 182 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 40626d72..e443a9cd 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -55,15 +55,17 @@ struct psw {
>>   #define PSW_MASK_BA            0x0000000080000000UL
>>   #define PSW_MASK_64            (PSW_MASK_BA | PSW_MASK_EA)
>>   -#define CTL0_LOW_ADDR_PROT        (63 - 35)
>> -#define CTL0_EDAT            (63 - 40)
>> -#define CTL0_IEP            (63 - 43)
>> -#define CTL0_AFP            (63 - 45)
>> -#define CTL0_VECTOR            (63 - 46)
>> -#define CTL0_EMERGENCY_SIGNAL        (63 - 49)
>> -#define CTL0_EXTERNAL_CALL        (63 - 50)
>> -#define CTL0_CLOCK_COMPARATOR        (63 - 52)
>> -#define CTL0_SERVICE_SIGNAL        (63 - 54)
>> +#define CTL0_LOW_ADDR_PROT            (63 - 35)
>> +#define CTL0_EDAT                (63 - 40)
>> +#define CTL0_FETCH_PROTECTION_OVERRIDE        (63 - 38)
>> +#define CTL0_STORAGE_PROTECTION_OVERRIDE    (63 - 39)
> 
> Just a matter of taste, but IMHO the names are getting a little bit long here ... maybe use "PROT" instead of "PROTECTION" to shorten them a little bit?

It's called CR0_STORAGE_PROTECTION_OVERRIDE in the kernel and I
want it to keep it similar to that.

[...]

>> +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");
> 
> You could also use one report_prefix_push("STORE CPU ADDRESS") prefix for the whole function, so you don't have to repeat that string everywhere again.
> 
>> +    set_storage_key(pagebuf, 0x20, 0);
>> +    *out = 0xbeef;
>> +    asm ("stap %0" : "=Q" (*out));
> 
> I think it might be better to use +Q here ... otherwise the compiler might optimize the "*out = 0xbeef" away, since it sees that the variable is only written twice, but never used in between.

Good catch, I'll use WRITE_ONCE tho, since no exceptions should occur
in the asm and it might be a bit misleading.

[...]
>> +    set_storage_key(pagebuf, 0x00, 0);
> 
> The other tests don't clear the storage key at the end, so why here now?

It's not necessary, but Claudio suggested it for the last version and
I like that it indicates that there is not supposed to be a shared state between the tests.

[...]

>> +    report_prefix_push("SET PREFIX, mismatching key, fetch protection override does not apply");
>> +    out = (uint32_t *)(pagebuf + 2048);
>> +    set_storage_key(pagebuf, 0x28, 0);
>> +    expect_pgm_int();
>> +    install_page(root, virt_to_pte_phys(root, pagebuf), 0);
>> +    WRITE_ONCE(*out, 0xdeadbeef);
> 
> Would it make sense to swap the above two lines, i.e. first the WRITE_ONCE, then the install_page? ... access to *out between the two intall_page() calls requires me to think twice whether that's ok or not ;-)

Yes, that is possible. Alternatively we could do

*(uint32_t *)2048 = 0xdeadbeef;

Which might make it clearer to the reader what's happening.
I'm not sure if it is a good idea of if the compiler would take it as an invitation to do funky things.

[...]

I'll implement your other suggestions, too.
Thanks for the input!
> 
>  Thomas
>