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

Cornelia Huck
2019-11-13T13:05:51Z

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