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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

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

> >   
> >> +};
> >> +
> >> +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);

> 
> [...]
> 
> > It's __really__ hard to understand this since the state is changed both by the guest and host. Please add comments to this and maybe also add some to the test struct explaining why you expect the results for each test.
> >   
> 
> I think I'll concentrate the comments at the tests array so we have one location
> that lays out the complete logic and then one only has to check if the guest
> and host match up with that, respectively, instead of having to model their interaction
> in ones head.
> 
> I'll incorporate your other feedback, too.
> 
> Thanks!