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

[Janis Schoetterl-Glausch]

On 1/20/22 16:40, Janosch Frank wrote:
> On 1/18/22 10:52, Janis Schoetterl-Glausch wrote:
>> Test the emulation of TEST PROTECTION in the presence of storage keys.
>> Emulation only occurs under certain conditions, one of which is the host
>> page being protected.
>> Trigger this by protecting the test pages via mprotect.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
>> ---
>>   tools/testing/selftests/kvm/.gitignore    |   1 +
>>   tools/testing/selftests/kvm/Makefile      |   1 +
>>   tools/testing/selftests/kvm/s390x/tprot.c | 184 ++++++++++++++++++++++
>>   3 files changed, 186 insertions(+)
>>   create mode 100644 tools/testing/selftests/kvm/s390x/tprot.c
>>
>> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
>> index 3763105029fb..82c0470b6849 100644
>> --- a/tools/testing/selftests/kvm/.gitignore
>> +++ b/tools/testing/selftests/kvm/.gitignore
>> @@ -7,6 +7,7 @@
>>   /s390x/memop
>>   /s390x/resets
>>   /s390x/sync_regs_test
>> +/s390x/tprot
>>   /x86_64/cr4_cpuid_sync_test
>>   /x86_64/debug_regs
>>   /x86_64/evmcs_test
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index c4e34717826a..df6de8d155e8 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -109,6 +109,7 @@ TEST_GEN_PROGS_aarch64 += kvm_binary_stats_test
>>   TEST_GEN_PROGS_s390x = s390x/memop
>>   TEST_GEN_PROGS_s390x += s390x/resets
>>   TEST_GEN_PROGS_s390x += s390x/sync_regs_test
>> +TEST_GEN_PROGS_s390x += s390x/tprot
>>   TEST_GEN_PROGS_s390x += demand_paging_test
>>   TEST_GEN_PROGS_s390x += dirty_log_test
>>   TEST_GEN_PROGS_s390x += kvm_create_max_vcpus
>> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
>> new file mode 100644
>> index 000000000000..8b52675307f6
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/s390x/tprot.c

[...]

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

[...]

> 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!