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

Pierre Morel
2019-11-14T17:56:04Z

On 2019-11-14 18:09, Janosch Frank wrote:
> 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?

In fact I do not need it since the CCW instructions are treated
synchronously.

It was for the fact we speak to a real device or if QEMU uses iothreads
for CCW handling.

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

OK

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

OK

I will make the second version more quiet and much better.

Thanks for the comments,

Regards,

Pierre

--
Pierre Morel
IBM Lab Boeblingen