Re: [kvm-unit-tests PATCH 3/3] s390x: Test effect of storage keys on some more instructions

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

On 5/6/22 18:52, Claudio Imbrenda wrote:
> On Thu,  5 May 2022 14:46:56 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> 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@linux.ibm.com>
>> ---
>>  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");

[...]