Re: [RFC PATCH v1 04/10] KVM: s390: selftests: Test TEST PROTECTION emulation

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

On 1/21/22 13:28, Claudio Imbrenda wrote:
> On Fri, 21 Jan 2022 12:03:20 +0100
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
> [...]
> 
>>>> +
>>>> +static int set_storage_key(void *addr, uint8_t key)
>>>> +{
>>>> +    int not_mapped = 0;
>>>> +  
>>>
>>> Maybe add a short comment:
>>> Check if address is mapped via lra and set the storage key if it is.
>>>   
>>>> +    asm volatile (
>>>> +               "lra    %[addr], 0(0,%[addr])\n"
>>>> +        "    jz    0f\n"
>>>> +        "    llill    %[not_mapped],1\n"
>>>> +        "    j    1f\n"
>>>> +        "0:    sske    %[key], %[addr]\n"
>>>> +        "1:"
>>>> +        : [addr] "+&a" (addr), [not_mapped] "+r" (not_mapped)  
>>>
>>> Shouldn't this be a "=r" instead of a "+r" for not_mapped?  
>>
>> I don't think so. We only write to it on one code path and the compiler mustn't conclude
>> that it can remove the = 0 assignment because the value gets overwritten anyway.
>>
>> Initially I tried to implement the function like this:
>>
>> static int set_storage_key(void *addr, uint8_t key)
>> {
>>         asm goto ("lra  %[addr], 0(0,%[addr])\n\t"
>>                   "jnz  %l[not_mapped]\n\t"
>>                   "sske %[key], %[addr]\n"
>>                 : [addr] "+&a" (addr)
>>                 : [key] "r" (key)
>>                 : "cc", "memory"
>>                 : not_mapped
>>         );
>>         return 0;
>> not_mapped:
>>         return -1;
>> }
>>
>> Which I think is nicer, but the compiler just optimized that completely away.
>> I have no clue why it (thinks it) is allowed to do that.
>>
>>>   
>>>> +        : [key] "r" (key)
>>>> +        : "cc"
>>>> +    );
>>>> +    return -not_mapped;
>>>> +}
>>>> +
>>>> +enum permission {
>>>> +    READ_WRITE = 0,
>>>> +    READ = 1,
>>>> +    NONE = 2,
>>>> +    UNAVAILABLE = 3,  
>>>
>>> TRANSLATION_NA ?
>>> I'm not completely happy with these names but I've yet to come up with a better naming scheme here.  
>>
>> Mentioning translation is a good idea. Don't think there is any harm in using
>> TRANSLATION_NOT_AVAILABLE or TRANSLATION_UNAVAILABLE.
> 
> it's too long, it actually makes the code harder to read when used
> 
> maybe consider something like TRANSL_UNAVAIL as well

Fine with me. I'll rename NONE to RW_PROTECTED. NONE is too nondescript.
> 
>>>   
>>>> +};
>>>> +
>>>> +static enum permission test_protection(void *addr, uint8_t key)
>>>> +{
>>>> +    uint64_t mask;
>>>> +
>>>> +    asm volatile (
>>>> +               "tprot    %[addr], 0(%[key])\n"
>>>> +        "    ipm    %[mask]\n"
>>>> +        : [mask] "=r" (mask)
>>>> +        : [addr] "Q" (*(char *)addr),
>>>> +          [key] "a" (key)
>>>> +        : "cc"
>>>> +    );
>>>> +
>>>> +    return (enum permission)mask >> 28;  
>>>
>>> You could replace the shift with the "srl" that we normally do.  
>>
>> I prefer keeping the asm as small as possible, C is just so much easier to understand.
> 
> we use srl everywhere, but I agree that explicitly using C makes it
> less obscure. and in the end the compiler should generate the same
> instructions anyway.
> 
> my only comment about the above code is that you are casting the
> uint64_t to enum permission _and then_ shifting. _technically_ it
> should still work (enums are just ints), but I think it would
> look cleaner if you write
> 
> 	return (enum permission)(mask >> 28);

That is better indeed.

[...]