On 1/21/22 13:28, Claudio Imbrenda wrote:
> On Fri, 21 Jan 2022 12:03:20 +0100
> Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> 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.
[...]
