On 5/6/22 18:52, Claudio Imbrenda wrote:
> On Thu, 5 May 2022 14:46:56 +0200
> Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote:
>
>> Test correctness of some instructions handled by user space instead of
>> KVM with regards to storage keys.
>> Test success and error conditions, including coverage of storage and
>> fetch protection override.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
>> ---
>> s390x/skey.c | 277 ++++++++++++++++++++++++++++++++++++++++++++
>> s390x/unittests.cfg | 1 +
>> 2 files changed, 278 insertions(+)
>>
>> diff --git a/s390x/skey.c b/s390x/skey.c
>> index 56bf5f45..d50470a8 100644
>> --- a/s390x/skey.c
>> +++ b/s390x/skey.c
>> @@ -12,6 +12,7 @@
>> #include <asm/asm-offsets.h>
>> #include <asm/interrupt.h>
>> #include <vmalloc.h>
>> +#include <css.h>
>> #include <asm/page.h>
>> #include <asm/facility.h>
>> #include <asm/mem.h>
>> @@ -284,6 +285,114 @@ static void test_store_cpu_address(void)
>> report_prefix_pop();
>> }
>>
>> +/*
>> + * Perform CHANNEL SUBSYSTEM CALL (CHSC) instruction while temporarily executing
>> + * with access key 1.
>> + */
>> +static unsigned int channel_subsystem_call_key_1(void *communication_block)
>
> this function name is very long (maybe chsc_with_key_1 instead?)
It's because of consistency with set_prefix_key_1 where I spelled out the instruction
name too. Granted the name of chsc is longer.
When in doubt I go for what seems more readable/contains more information.
>
>> +{
>> + uint32_t program_mask;
>> +
>> + asm volatile (
>> + "spka 0x10\n\t"
>> + ".insn rre,0xb25f0000,%[communication_block],0\n\t"
>> + "spka 0\n\t"
>> + "ipm %[program_mask]\n"
>> + : [program_mask] "=d" (program_mask)
>> + : [communication_block] "d" (communication_block)
>> + : "memory"
>> + );
>> + return program_mask >> 28;
>> +}
>> +
>> +static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)
>
> same here (init_comm_block?)
Since we're only performing one kind of operation in this test, that is fine,
but I'll add a comment saying how we initialize the communication block then.
>
>> +{
>> + memset(communication_block, 0, PAGE_SIZE);
>> + communication_block[0] = 0x10;
>> + communication_block[1] = 0x10;
>> + communication_block[9] = 0;
>> +}
>> +
>> +static void test_channel_subsystem_call(void)
>> +{
>> + static const char request_name[] = "Store channel-subsystem-characteristics";
>
> so this "request_name" is for when CHSC succeeds? why not just
> "Success" then?
That's the operation being performed. So maybe I should change it to
msg[] = "Performed store channel-subsystem-characteristics" ?
>
>> + uint16_t *communication_block = (uint16_t *)&pagebuf;
>
> long name (consider comm_block, or even cb)
>
>> + unsigned int cc;
>> +
>> + report_prefix_push("CHANNEL SUBSYSTEM CALL");
>> +
>> + report_prefix_push("zero key");
>> + init_store_channel_subsystem_characteristics(communication_block);
>
> see what I mean when I say that the names are too long? ^
Fits in 80 columns ;-)
>
>> + set_storage_key(communication_block, 0x10, 0);
>> + asm volatile (
>> + ".insn rre,0xb25f0000,%[communication_block],0\n\t"
>> + "ipm %[cc]\n"
>> + : [cc] "=d" (cc)
>> + : [communication_block] "d" (communication_block)
>> + : "memory"
>> + );
>> + cc = cc >> 28;
>> + report(cc == 0 && communication_block[9], request_name);
>> + report_prefix_pop();
>> +
>> + report_prefix_push("matching key");
>> + init_store_channel_subsystem_characteristics(communication_block);
>> + set_storage_key(communication_block, 0x10, 0);
>
> you just set the storage key in the previous test, and you did not set
> it back to 0, why do you need to set it again?
It's not necessary, but I want the tests to be independent from each other,
so you can remove/reorder/add ones without having to think.
>
>> + cc = channel_subsystem_call_key_1(communication_block);
>> + report(cc == 0 && communication_block[9], request_name);
>> + report_prefix_pop();
>> +
[...]
>> +
>> + cc = stsch(test_device_sid, schib);
>> + if (cc) {
>> + report_fail("could not store SCHIB");
>> + return;
>> + }
>> +
>> + report_prefix_push("zero key");
>> + schib->pmcw.intparm = 100;
>> + set_storage_key(schib, 0x28, 0);
>> + cc = msch(test_device_sid, schib);
>> + if (!cc) {
>> + WRITE_ONCE(schib->pmcw.intparm, 0);
>
> why are you using WRITE_ONCE here?
It's a dead store because of the stsch below.
That line is just for good measure so we know stsch really overwrote the value.
>
>> + cc = stsch(test_device_sid, schib);
>> + report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");
[...]
