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

[Cornelia Huck <cohuck@redhat.com>]

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?

>   0x00090000 SSID for CCW-PONG

subchannel id, or subchannel set id?

> - initializing the ORB pointing to a single READ CCW

Out of curiosity: Would using a NOP also be an option?

> - starts the STSH command with the ORB

s/STSH/SSCH/ ?

> - 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 :)

> +{
> +	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?

> +{
> +	register unsigned long reg1 asm("1") = chpid;
> +	int ccode;
> +
> +	asm volatile(
> +		"	rchp\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28"
> +		: "=d" (ccode)
> +		: "d" (reg1)
> +		: "cc");
> +	return ccode;
> +}

(...)

> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..6cdaf61
> --- /dev/null
> +++ b/s390x/css.c

(...)

> +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));
> +}
> +
> +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);

So, you always enable all iscs? Maybe 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?

> +	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?)


> +}
> +
> +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 :)

> +	int cc;
> +
> +	if (fake)
> +		return 0;
> +	css_enable();
> +
> +	enable_io_irq();
> +
> +	ccw[0].code = PONG_READ;
> +	ccw[0].flags = CCW_F_PCI;
> +	ccw[0].count = 80;
> +	ccw[0].data = (unsigned int)(unsigned long) &buffer;
> +
> +	p->intparm = 0xcafec0ca;
> +	p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> +	p->cpa = (unsigned int) (unsigned long)&ccw[0];
> +
> +	printf("ORB AT %p\n", orb);
> +	dump_orb(p);
> +	cc = ssch(CSSID_PONG, p);
> +	if (cc) {
> +		DBG("cc: %x\n", cc);
> +		return cc;
> +	}
> +
> +	delay(1);
> +
> +	stsch(CSSID_PONG, &schib);
> +	dump_schib(&schib);
> +	DBG("got: %s\n", buffer);
> +
> +	return 0;
> +}
(...)