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

[Pierre Morel <pmorel@linux.ibm.com>]

On 2019-11-13 14:05, Cornelia Huck wrote:
> On Wed, 13 Nov 2019 13:23:19 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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
> 0 should be fine with recent QEMU versions as well, I guess?
Right


>
>>    0x00090000 SSID for CCW-PONG
> subchannel id, or subchannel set id?

hum, only part of, I had SSID (Subchannel Set ID) 4 (a.k.a m bit) + Bit 
47  =1

But as you said, I can use CSSID 0 and m = 0 which makes:

Subsystem Identification word = 0x00010000


>
>> - initializing the ORB pointing to a single READ CCW
> Out of curiosity: Would using a NOP also be an option?

It will work but will not be handled by this device, css.c intercept it 
in sch_handle_start_func_virtual.

AFAIU If we want to have a really good testing environment, for driver 
testing for exemple, then it would be interesting to add a new 
do_subchannel_work callback like do_subchannel_work_emulation along with 
the _virtual and _paththrough variantes.

Having a dedicated callback for emulation, we can answer to any CSS 
instructions and SSCH commands, including NOP and TIC.

My goal here was to quickly develop a device answering to some basic 
READ/WRITE command to start memory transfers from inside a guest without 
Linux and without implementing VIRTIO in KVM tests.



>
>> - starts the STSH command with the ORB
> s/STSH/SSCH/ ?

:) yes, thanks


>
>> - 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.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 141 +++++++++++++++++++++++++++++
>>   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
>> index 0000000..a7c42fd
>> --- /dev/null
>> +++ b/lib/s390x/css.h
> (...)
>
>> +static inline int rsch(unsigned long schid)
> I don't think anyone has tried rsch with QEMU before; sounds like a
> good idea to test this :)

With an do_subchannel_work_emulation() callback?


>
>> +{
>> +	register unsigned long reg1 asm("1") = schid;
>> +	int ccode;
>> +
>> +	asm volatile(
>> +		"	rsch\n"
>> +		"	ipm	%0\n"
>> +		"	srl	%0,28"
>> +		: "=d" (ccode)
>> +		: "d" (reg1)
>> +		: "cc");
>> +	return ccode;
>> +}
>> +
>> +static inline int rchp(unsigned long chpid)
> Anything useful we can test here?

Not for now.

I certainly can reduce the size of the file by removing the unused CSS 
instructions calls.

...snip...
> +
>> +static void enable_io_irq(void)
>> +{
>> +	set_io_irq_subclass_mask(0x00000000ff000000);
> So, you always enable all iscs? Maybe add a comment?

OK, is just a lazy option to get IRQ for this test.

Right, I add a comment.


>
>> +	set_system_mask(PSW_PRG_MASK >> 56);
>> +}
>> +
>> +void handle_io_int(sregs_t *regs)
>> +{
>> +	int ret = 0;
>> +
>> +	DBG("IO IRQ: subsys_id_word=%08x", lowcore->subsys_id_word);
>> +	DBG("......: io_int_parm   =%08x", lowcore->io_int_param);
>> +	DBG("......: io_int_word   =%08x", lowcore->io_int_word);
>> +	ret = tsch(lowcore->subsys_id_word, &irb);
>> +	dump_irb(&irb);
>> +	if (ret)
>> +		DBG("......: tsch retval %d", ret);
>> +	DBG("IO IRQ: END");
>> +}
>> +
>> +static void set_schib(struct schib *sch)
>> +{
>> +	struct pmcw *p = &sch->pmcw;
>> +
>> +	p->intparm = 0xdeadbeef;
>> +	p->devnum = 0xc0ca;
>> +	p->lpm = 0x80;
>> +	p->flags = 0x3081;
> Use #defines instead of magic numbers?

OK


>
>> +	p->chpid[7] = 0x22;
>> +	p->pim = 0x0f;
>> +	p->pam = 0x0f;
>> +	p->pom = 0x0f;
>> +	p->lpm = 0x0f;
>> +	p->lpum = 0xaa;
>> +	p->pnom = 0xf0;
>> +	p->mbi = 0xaa;
>> +	p->mbi = 0xaaaa;
> Many of these fields are not supposed to be modifiable by the program
> -- do you want to check what you get back after msch?
>
> Also, you set mbi twice ;) (And for it to actually have any effect,
> you'd have to execute SET CHANNEL MONITOR, no?)

Yes, it was for me to check what happens but I should remove most of 
these field initialization.

As you said they are not modifiable by the program.

Will clean this.


>
>
>> +}
>> +
>> +static void css_enable(void)
>> +{
>> +	int ret;
>> +
>> +	ret = stsch(CSSID_PONG, &schib);
>> +	if (ret)
>> +		DBG("stsch: %x\n", ret);
>> +	dump_schib(&schib);
>> +	set_schib(&schib);
>> +	dump_schib(&schib);
>> +	ret = msch(CSSID_PONG, &schib);
>> +	if (ret)
>> +		DBG("msch : %x\n", ret);
>> +}
>> +
>> +/* These two definitions are part of the QEMU PONG interface */
>> +#define PONG_WRITE 0x21
>> +#define PONG_READ  0x22
> Ah, so it's not a plain read/write, but a specialized one? Mention that
> in the patch description?
>
>> +
>> +static int css_run(int fake)
>> +{
>> +	struct orb *p = orb;
> I'd maybe call that variable 'orb' instead; at a glance, I was confused
> what you did with the pmcw below, until I realized that it's the orb :)

OK, is clearly better to use orb.


Thanks,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen