Re: [PATCH v1 4/4] s390x: Testing the Subchannel I/O read

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5300 bytes --]

On 11/14/19 5:38 PM, Pierre Morel wrote:
> 
> On 2019-11-14 10:15, Janosch Frank wrote:
>> On 11/13/19 1:23 PM, Pierre Morel wrote:
>>> This simple test test the I/O reading by the SUB Channel by:
>>> - initializing the Channel SubSystem with predefined CSSID:
>>>    0xfe000000 CSSID for a Virtual CCW
>>>    0x00090000 SSID for CCW-PONG
>>> - initializing the ORB pointing to a single READ CCW
>>> - starts the STSH command with the ORB
>>> - Expect an interrupt
>>> - writes the read data to output
>>>
>>> The test implements lots of traces when DEBUG is on and
>>> tests if memory above the stack is corrupted.
>> What happens if we do not habe the pong device?
> 
> CC error on stsch() which is currently not cached (but will in the next 
> version)
> 
> CC error on msch() and on ssch() which is cached and makes the test to fail.
> 
> 
>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
>> Hmm, what about splitting the patch into css.h/css_dump.c and the actual
>> test in s390x/css.c?
> 
> OK
> 
> 
>>
>>>   s390x/Makefile       |   2 +
>>>   s390x/css.c          | 222 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   s390x/unittests.cfg  |   4 +
>>>   5 files changed, 613 insertions(+)
>>>   create mode 100644 lib/s390x/css.h
>>>   create mode 100644 lib/s390x/css_dump.c
>>>   create mode 100644 s390x/css.c
>>>
>>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>>> new file mode 100644
> 
> OK to all comments...  (I sniped out for clarity)
> 
> ...snip...
> 
> 
>>> +static char buffer[4096];
>>> +
>>> +static void delay(int d)
>>> +{
>>> +	int i, j;
>>> +
>>> +	while (d--)
>>> +		for (i = 1000000; i; i--)
>>> +			for (j = 1000000; j; j--)
>>> +				;
>>> +}
>> You could set a timer.
> 
> 
> Hum, do we really want to do this?

Why exactly do you need it if you can't have an exact time to wait for?

> 
> 
>>
>>> +
>>> +static void set_io_irq_subclass_mask(uint64_t const new_mask)
>>> +{
>>> +	asm volatile (
>>> +		"lctlg %%c6, %%c6, %[source]\n"
>>> +		: /* No outputs */
>>> +		: [source] "R" (new_mask));
>> arch_def.h has lctlg() and ctl_set/clear_bit
> 
> 
> OK, thanks
> 
> 
>>
>>> +}
>>> +
>>> +static void set_system_mask(uint8_t new_mask)
>>> +{
>>> +	asm volatile (
>>> +		"ssm %[source]\n"
>>> +		: /* No outputs */
>>> +		: [source] "R" (new_mask));
>>> +}
>>> +
>>> +static void enable_io_irq(void)
>>> +{
>>> +	set_io_irq_subclass_mask(0x00000000ff000000);
>>> +	set_system_mask(PSW_PRG_MASK >> 56);
>> load_psw_mask(extract_psw_mask() | PSW_PRG_MASK); no need for another
>> inline asm function :)
>>
>> Or add a psw_set/clear_bit function and fixup enter_pstate()
> 
> I look at this.
> 
> 
>>
>>> +}
>>> +
>>> +void handle_io_int(sregs_t *regs)
>>> +{
> ,,,snip...
>>> +
>>> +	delay(1);
>>> +
>>> +	stsch(CSSID_PONG, &schib);
>>> +	dump_schib(&schib);
>> Is all that dumping necessary or just a dev remainder?
> 
> 
> it goes in the logs, so I thought it could be interresting to keep it.

Depends on how much output is produced.
If I have to scroll through your dumps to get to the ouptuts
 of the reports then they are .

See the answer below...

> 
> 
>>
>>> +	DBG("got: %s\n", buffer);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +#define MAX_ERRORS 10
>>> +static int checkmem(phys_addr_t start, phys_addr_t end)
>>> +{
>>> +	phys_addr_t curr;
>>> +	int err = 0;
>>> +
>>> +	for (curr = start; curr != end; curr += PAGE_SIZE)
>>> +		if (memcmp((void *)start, (void *)curr, PAGE_SIZE)) {
>>> +			report("memcmp failed %lx", true, curr);
>> How many errors do you normally run into (hopefully 0)?
> 
> 
> hopefully.
> 
> However I thought it could be interesting to know how many pages have 
> been dirtied.

Honestly, for debugging a failing test we would need to add prints or
attach gdb anyway. So I see no reason to not fail on the first occurrence.

> 
> 
>>
>>> +			if (err++ > MAX_ERRORS)
>>> +				break;
>>> +		}
>>> +	return err;
>>> +}
>>> +
>>> +extern unsigned long bss_end;
>>> +
>>> +int main(int argc, char *argv[])
>>> +{
>>> +	phys_addr_t base, top;
>>> +	int check_mem = 0;
>>> +	int err = 0;
>>> +
>>> +	if (argc == 2 && !strcmp(argv[1], "-i"))
>>> +		check_mem = 1;
>>> +
>>> +	report_prefix_push("css");
>>> +	phys_alloc_get_unused(&base, &top);
>>> +
>>> +	top = 0x08000000; /* 128MB Need to be updated */
>>> +	base = (phys_addr_t)&stacktop;
>>> +
>>> +	if (check_mem)
>>> +		memset((void *)base, 0x00, top - base);
>>> +
>>> +	if (check_mem)
>>> +		err = checkmem(base, top);
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	err = css_run(0);
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	if (check_mem)
>>> +		err = checkmem(base, top);
>>> +
>>> +out:
>>> +	if (err)
>>> +		report("Tested", 0);
>>> +	else
>>> +		report("Tested", 1);
>> Normally we report the sucsess or failure of single actions and a
>> summary will tell us if the whole test ran into errors.
> 
> Right, will be enhanced.
> 
> Thanks for the comments.
> 
> Regards,
> 
> Pierre
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]