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@xxxxxxxxxxxxx>
>> ---
> [...]
>> 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
>
