s390x: Testing the Channel Subsystem I/O

If we use multiple source of interrupts, for example, using SCLP console to print information while using I/O interrupts, we need to have a re-entrant register saving interruption handling.

Instead of saving at a static memory address, let's save the base registers and the floating point registers on the stack.

Note that we keep the static register saving to recover from the RESET tests.

Signed-off-by Pierre Morel <[email protected]>

---
 s390x/cstart64.S | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

s390x/cstart64.S +0 -0

index 9af6bb3..ba2e67c 100644

s390x/cstart64.S +5 -0

+
+/* Restore the register in reverse order */
+	.macro RESTORE_IRQ_REGS
+	/* Restore fp register from stack: SP still where it was left */
+	/* and offset to SP is a multile of reg number */

s390x/cstart64.S +3 -2

+
 .section .text
 /*
  * load_reset calling convention:

@@ -182,9 +212,9 @@ mcck_int:
 	lpswe	GEN_LC_MCCK_OLD_PSW
 
 io_int:
-	SAVE_REGS
+	SAVE_IRQ_REGS
 	brasl	%r14, handle_io_int
-	RESTORE_REGS
+	RESTORE_IRQ_REGS
 	lpswe	GEN_LC_IO_OLD_PSW
 
 svc_int:

Quoted 3 lines · Pierre Morel

If we use multiple source of interrupts, for example, using SCLP console to print information while using I/O interrupts, we need to have a re-entrant register saving interruption handling.

So the primary reason is to print during I/O interrupts (which we already do, but usually never trigger - handle_io_int())

Quoted 3 lines

Instead of saving at a static memory address, let's save the base registers and the floating point registers on the stack.

".. in case of I/O interrupts".

Quoted 4 lines

Note that we keep the static register saving to recover from the RESET tests.

and for all other types of interrupts.

Quoted 15 lines

Signed-off-by Pierre Morel <[email protected]>

---
  s390x/cstart64.S | 34 ++++++++++++++++++++++++++++++++--
  1 file changed, 32 insertions(+), 2 deletions(-)

s390x/cstart64.S +0 -0

index 9af6bb3..ba2e67c 100644
@@ -118,6 +118,36 @@ memsetxc:
  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
  	.endm
 

s390x/cstart64.S memsetxc: · line 122

Quoted 2 lines

s390x/cstart64.S +2 -0

+/* Save registers on the stack (r15), so we can have stacked interrupts. */
+	.macro SAVE_IRQ_REGS

s/SAVE_IRQ_REGS/SAVE_REGS_STACK/ ?

Same for the other macro.

s390x/cstart64.S memsetxc: · line 123

Quoted 2 lines

s390x/cstart64.S +1 -0

+	/* Allocate a stack frame for 15 integer registers */

not integers. sizeof(int) == 4.

"doublewords" should be the right s390x speak.

s390x/cstart64.S memsetxc: · line 134

Quoted 12 lines

s390x/cstart64.S +11 -0

+	slgfi   %r15, 15 * 8
+	/* Store all registers from r0 to r14 on the stack */
+	stmg    %r0, %r14, 0(%r15)
+	/* Allocate a stack frame for 16 floating point registers */
+	/* The size of a FP register is the size of an integer */
+	slgfi   %r15, 16 * 8
+	/* Save fp register on stack: offset to SP is multiple of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8(%r15)
+	.endr
+	.endm

What about the FPC?

This patch defines the PSW bits EA/BA used to initialize the PSW masks for exceptions.

Since some PSW mask definitions exist already in arch_def.h we add these definitions there.
We move all PSW definitions together and protect assembler code against C syntax.

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/asm/arch_def.h | 16 ++++++++++++----
 s390x/cstart64.S         | 15 ++++++++-------
 2 files changed, 20 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +12 -4

index 15a4d49..c54409a 100644
@@ -10,15 +10,22 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_EXT			0x0100000000000000UL
+#define PSW_MASK_DAT			0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_PSTATE			0x0001000000000000UL
+#define PSW_MASK_BA			0x0000000080000000UL
+#define PSW_MASK_EA			0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
+#define PSW_RESET_MASK		(PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW)
+
+#ifndef __ASSEMBLER__
 struct psw {
 	uint64_t	mask;
 	uint64_t	addr;
 };
 
-#define PSW_MASK_EXT			0x0100000000000000UL
-#define PSW_MASK_DAT			0x0400000000000000UL
-#define PSW_MASK_PSTATE			0x0001000000000000UL
-
 #define CR0_EXTM_SCLP			0X0000000000000200UL
 #define CR0_EXTM_EXTC			0X0000000000002000UL
 #define CR0_EXTM_EMGC			0X0000000000004000UL

@@ -297,4 +304,5 @@ static inline uint32_t get_prefix(void)
 	return current_prefix;
 }
 
+#endif /* __ASSEMBLER */
 #endif

s390x/cstart64.S +1 -0

index ba2e67c..e394b3a 100644
@@ -12,6 +12,7 @@
  */
 #include <asm/asm-offsets.h>
 #include <asm/sigp.h>
+#include <asm/arch_def.h>
 
 .section .init
 

Quoted 64 lines · Pierre Morel

This patch defines the PSW bits EA/BA used to initialize the PSW masks for exceptions.

Since some PSW mask definitions exist already in arch_def.h we add these definitions there.
We move all PSW definitions together and protect assembler code against C syntax.

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/asm/arch_def.h | 16 ++++++++++++----
  s390x/cstart64.S         | 15 ++++++++-------
  2 files changed, 20 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +12 -4

index 15a4d49..c54409a 100644
@@ -10,15 +10,22 @@
  #ifndef _ASM_S390X_ARCH_DEF_H_
  #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_EXT			0x0100000000000000UL
+#define PSW_MASK_DAT			0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_PSTATE			0x0001000000000000UL
+#define PSW_MASK_BA			0x0000000080000000UL
+#define PSW_MASK_EA			0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
+#define PSW_RESET_MASK		(PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW)
+
+#ifndef __ASSEMBLER__
  struct psw {
  	uint64_t	mask;
  	uint64_t	addr;
  };
 
-#define PSW_MASK_EXT			0x0100000000000000UL
-#define PSW_MASK_DAT			0x0400000000000000UL
-#define PSW_MASK_PSTATE			0x0001000000000000UL
-
  #define CR0_EXTM_SCLP			0X0000000000000200UL
  #define CR0_EXTM_EXTC			0X0000000000002000UL
  #define CR0_EXTM_EMGC			0X0000000000004000UL

@@ -297,4 +304,5 @@ static inline uint32_t get_prefix(void)
  	return current_prefix;
  }
 
+#endif /* __ASSEMBLER */
  #endif

s390x/cstart64.S +1 -0

index ba2e67c..e394b3a 100644
@@ -12,6 +12,7 @@
   */
  #include <asm/asm-offsets.h>
  #include <asm/sigp.h>
+#include <asm/arch_def.h>
 
  .section .init
 

s390x/cstart64.S svc_int: · line 229

Quoted 5 lines

s390x/cstart64.S +1 -1

@@ -225,19 +226,19 @@ svc_int:
 
  	.align	8
  reset_psw:
-	.quad	0x0008000180000000
+	.quad	PSW_RESET_MASK

I'd really prefer

.quad PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW

here instead and drop PSW_RESET_MASK. Makes it clearer that we are talking about a special short psw here.

Apart from that, looks good to me.

Let's make it possible to add and remove a custom io interrupt handler, that can be used instead of the normal one.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
 lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
 lib/s390x/interrupt.h |  8 ++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 lib/s390x/interrupt.h

Quoted 7 lines · On Fri, 24 Apr 2020 12:45:46 +0200 Pierre Morel <[email protected]> wrote:

Let's make it possible to add and remove a custom io interrupt handler, that can be used instead of the normal one.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

Quoted 5 lines

---
  lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
  lib/s390x/interrupt.h |  8 ++++++++
  2 files changed, 30 insertions(+), 1 deletion(-)

create mode 100644 lib/s390x/interrupt.h

As the "normal one" means "no handler, just abort", is there any reason not simply to always provide one? What is the use case for multiple I/O interrupt handlers?

Quoted 16 lines · Cornelia Huck

Quoted 12 lines · On Fri, 24 Apr 2020 12:45:46 +0200 Pierre Morel <[email protected]> wrote:

Let's make it possible to add and remove a custom io interrupt handler, that can be used instead of the normal one.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
  lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
  lib/s390x/interrupt.h |  8 ++++++++
  2 files changed, 30 insertions(+), 1 deletion(-)

create mode 100644 lib/s390x/interrupt.h

Quoted 4 lines

As the "normal one" means "no handler, just abort", is there any reason not simply to always provide one? What is the use case for multiple I/O interrupt handlers?

I can only agree, I proposed this initially.
David asked for a registration.

These are the include and library utilities for the css tests patch series.

Debug function can be activated by defining DEBUG_CSS before the inclusion of the css.h header file.

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
 2 files changed, 413 insertions(+)
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c

lib/s390x/css_dump.c new file +6 -0

new file mode 100644
index 0000000..2f33fab
@@ -0,0 +1,157 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2020 IBM Corp.
+ *
+ * Authors:

Quoted 2 lines · On Fri, 24 Apr 2020 12:45:47 +0200 Pierre Morel <[email protected]> wrote:

These are the include and library utilities for the css tests patch series.

"Provide some definitions and library routines that can be used by tests targeting the channel subsystem."

?

Quoted 12 lines

Debug function can be activated by defining DEBUG_CSS before the inclusion of the css.h header file.

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
  lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
  2 files changed, 413 insertions(+)

create mode 100644 lib/s390x/css.h
create mode 100644 lib/s390x/css_dump.c

(...)

Quoted 16 lines

lib/s390x/css_dump.c new file +10 -0

new file mode 100644
index 0000000..2f33fab
@@ -0,0 +1,157 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2020 IBM Corp.
+ *
+ * Authors:
+ *  Pierre Morel <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.

lib/s390x/css_dump.c line 16

Quoted 6 lines

lib/s390x/css_dump.c new file +6 -0

+ *
+ * Description:
+ * Provides the dumping functions for various structures used by subchannels:
+ * - ORB  : Operation request block, describes the I/O operation and points to
+ *          a CCW chain
+ * - CCW  : Channel Command Word, describes the data and flow control

"describes the command, data, and flow control" ?

lib/s390x/css_dump.c line 17

Quoted 2 lines

lib/s390x/css_dump.c new file +1 -0

+ * - IRB  : Interuption response Block, describes the result of an operation

s/operation/operation;/

Quoted 3 lines

lib/s390x/css_dump.c new file +2 -0

+ *          holds a SCSW and model-dependent data.
+ * - SCHIB: SubCHannel Information Block composed of:

lib/s390x/css_dump.c line 23

Quoted 4 lines

lib/s390x/css_dump.c new file +4 -0

+ *   - SCSW: SubChannel Status Word, status of the channel.
+ *   - PMCW: Path Management Control Word
+ * You need the QEMU ccw-pong device in QEMU to answer the I/O transfers.
+ */

(...)

Otherwise, looks good.

Quoted 6 lines · Cornelia Huck

Quoted 1 line · On Fri, 24 Apr 2020 12:45:47 +0200 Pierre Morel <[email protected]> wrote:

+ * - IRB : Interuption response Block, describes the result of an operation

s/operation/operation;/

? I do not understand, do you want a ";" at the end of "operation"

Isn't it a typo error?

Quoted 2 lines

Quoted 2 lines

+ * holds a SCSW and model-dependent data.

Quoted 3 lines · On Fri, 15 May 2020 09:02:37 +0200 Pierre Morel <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Fri, 24 Apr 2020 12:45:47 +0200
Pierre Morel <[email protected]> wrote:

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ * - IRB : Interuption response Block, describes the result of an operation

s/operation/operation;/

? I do not understand, do you want a ";" at the end of "operation"

Yes, I think that makes the description read more like a proper sentence (describe what its purpose is; describe its contents).

First step for testing the channel subsystem is to enumerate the css and retrieve the css devices.

This tests the success of STSCH I/O instruction, we do not test the reaction of the VM for an instruction with wrong parameters.

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/css.h     |  1 +
 s390x/Makefile      |  2 +
 s390x/css.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 ++
 4 files changed, 99 insertions(+)
 create mode 100644 s390x/css.c

lib/s390x/css.h +0 -0

index bab0dd5..9417541 100644

lib/s390x/css.h +0 -0

 struct schib {
 	struct pmcw pmcw;

s390x/Makefile +1 -0

index ddb4b48..baebf18 100644
@@ -17,6 +17,7 @@ tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary

s390x/Makefile +0 -0

 OBJDIRS += lib/s390x
 

s390x/css.c new file +12 -0

new file mode 100644
index 0000000..cc97e79
@@ -0,0 +1,92 @@
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2020 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+

s390x/css.c new file +24 -0

+
+static struct schib schib;
+static int test_device_sid;
+
+static void test_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+	int scn;
+	int scn_found = 0;
+	int dev_found = 0;
+
+	for (scn = 0; scn < 0xffff; scn++) {
+		cc = stsch(scn|SID_ONE, &schib);
+		switch (cc) {
+		case 0:		/* 0 means SCHIB stored */
+			break;
+		case 3:		/* 3 means no more channels */
+			goto out;
+		default:	/* 1 or 2 should never happened for STSCH */
+			report(0, "Unexpected cc=%d on subchannel number 0x%x",
+			       cc, scn);
+			return;
+		}

s390x/css.c new file +33 -0

+	if (!dev_found) {
+		report(0,
+		       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+		       scn, scn_found, dev_found);
+		return;
+	}
+	report(1,
+	       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+	       scn, scn_found, dev_found);
+}
+
+static struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "enumerate (stsch)", test_enumerate },
+	{ NULL, NULL }
+};
+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("Channel Subsystem");
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		tests[i].func();
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	return report_summary();
+}

s390x/unittests.cfg +4 -0

index 07013b2..a436ec0 100644
@@ -83,3 +83,7 @@ extra_params = -m 1G
 [sclp-3g]
 file = sclp.elf
 extra_params = -m 3G
+
+[css]
+file = css.elf
+extra_params =-device ccw-pong

Quoted 3 lines · Pierre Morel

First step for testing the channel subsystem is to enumerate the css and retrieve the css devices.

lib/s390x/css.h struct pmcw { · line 85

Quoted 9 lines

lib/s390x/css.h +1 -0

index bab0dd5..9417541 100644
@@ -82,6 +82,7 @@ struct pmcw {
  	uint8_t  chpid[8];
  	uint32_t flags2;
  };
+#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)

Why isn't this in the library patch?

lib/s390x/css.h cflatobjs += lib/s390x/sclp-console.o · line 55

Quoted 5 lines

lib/s390x/css.h +1 -0

@@ -51,6 +52,7 @@ cflatobjs += lib/s390x/sclp-console.o
  cflatobjs += lib/s390x/interrupt.o
  cflatobjs += lib/s390x/mmu.o
  cflatobjs += lib/s390x/smp.o
+cflatobjs += lib/s390x/css_dump.o

Why isn't this in the library patch?

Quoted 1 line

+#define SID_ONE 0x00010000

Why isn't this in the library patch?

lib/s390x/css.h

Quoted 2 lines

lib/s390x/css.h +2 -0

+		if (!test_device_sid)
+			test_device_sid = scn|SID_ONE;

Give the pipe some space :)

lib/s390x/css.h

Quoted 1 line

lib/s390x/css.h +1 -0

+		dev_found++;

Newlines would make this more readable.

lib/s390x/css.h

Quoted 3 lines

lib/s390x/css.h +2 -0

+	}
+out:

We can report dev_found instead of 0/1 and a if/else

report(dev_found,

	"Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
                     scn, scn_found, dev_found);	

First step for testing the channel subsystem is to enumerate the css and retrieve the css devices.

This tests the success of STSCH I/O instruction, we do not test the

reaction of the VM for an instruction with wrong parameters.

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/css.h     |  1 +
 s390x/Makefile      |  2 +
 s390x/css.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 ++
 4 files changed, 99 insertions(+)
 create mode 100644 s390x/css.c

s390x/css.c new file +6 -0

+static void test_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+	int scn;
+	int scn_found = 0;

s390x/css.c new file +4 -0

+			report(0, "Unexpected cc=%d on subchannel number 0x%x",
+			       cc, scn);
+			return;
+		}

Quoted 4 lines · On Fri, 24 Apr 2020 12:45:48 +0200 Pierre Morel <[email protected]> wrote:

First step for testing the channel subsystem is to enumerate the css and retrieve the css devices.

This tests the success of STSCH I/O instruction, we do not test the

s/of/of the/

Quoted 12 lines

reaction of the VM for an instruction with wrong parameters.

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/css.h     |  1 +
  s390x/Makefile      |  2 +
  s390x/css.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++
  s390x/unittests.cfg |  4 ++
  4 files changed, 99 insertions(+)

create mode 100644 s390x/css.c

(...)

Quoted 3 lines

s390x/css.c new file +3 -0

+static void test_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;

s390x/css.c line 42

Quoted 1 line

s390x/css.c new file +1 -0

+		default:	/* 1 or 2 should never happened for STSCH */

s/happened/happen/

s390x/css.c line 46

Quoted 5 lines

s390x/css.c new file +4 -0

+			report(0, "Unexpected cc=%d on subchannel number 0x%x",
+			       cc, scn);
+			return;
+		}

A second step when testing the channel subsystem is to prepare a channel for use.
This includes:
- Get the current SubCHannel Information Block (SCHIB) using STSCH
- Update it in memory to set the ENABLE bit
- Tell the CSS that the SCHIB has been modified using MSCH
- Get the SCHIB from the CSS again to verify that the subchannel is enabled.

This tests the MSCH instruction to enable a channel succesfuly.

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Signed-off-by Pierre Morel <[email protected]>

---
 s390x/css.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

s390x/css.c +0 -0

index cc97e79..fa068bf 100644
@@ -68,11 +68,59 @@ out:
 	       scn, scn_found, dev_found);
 }

s390x/css.c +26 -0

+	/* Read the SCHIB for this subchannel */
+	cc = stsch(test_device_sid, &schib);
+	if (cc) {
+		report(0, "stsch cc=%d", cc);
+		return;
+	}
+
+	/* Update the SCHIB to enable the channel */
+	pmcw->flags |= PMCW_ENABLE;
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(test_device_sid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report(0, "msch cc=%d", cc);
+		return;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(test_device_sid, &schib);

s390x/css.c +3 -0

+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
+	{ "enable (msch)", test_enable },
 	{ NULL, NULL }
 };
 

Quoted 9 lines · Pierre Morel

A second step when testing the channel subsystem is to prepare a channel for use.
This includes:
- Get the current SubCHannel Information Block (SCHIB) using STSCH
- Update it in memory to set the ENABLE bit
- Tell the CSS that the SCHIB has been modified using MSCH
- Get the SCHIB from the CSS again to verify that the subchannel is enabled.

Quoted 1 line

This tests the MSCH instruction to enable a channel succesfuly.

successfully

Quoted 3 lines

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Would we expect needing retries for the pong device?

Quoted 2 lines

s390x/css.c out: · line 79

Quoted 5 lines

s390x/css.c +5 -0

+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}

If these tests are layered on top of each other and need a device to work, we should abort or skip and exit the test if the enumeration doesn't bring up devices

s390x/css.c out: · line 115

Quoted 1 line

s390x/css.c +1 -0

+	report(1, "Tested");

s/Tested/Enabled/

Quoted 6 lines · Janosch Frank

Quoted 2 lines · Pierre Morel

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Would we expect needing retries for the pong device?

Yes it can be that we need to retry some instructions if we want them to succeed.
This is the case for example if we develop a driver for an operating system. When working with firmware, sometime, things do not work at the first time. Mostly due to races in silicium, firmware or hypervisor or between them all.

Since our purpose is to detect such problems we do not retry instructions but report the error.

If we detect such problem we may in the future enhance the tests.

Quoted 15 lines · On Tue, 28 Apr 2020 10:27:36 +0200 Pierre Morel <[email protected]> wrote:

Quoted 6 lines · Janosch Frank

Quoted 2 lines · Pierre Morel

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Would we expect needing retries for the pong device?

Yes it can be that we need to retry some instructions if we want them to succeed.
This is the case for example if we develop a driver for an operating system. When working with firmware, sometime, things do not work at the first time. Mostly due to races in silicium, firmware or hypervisor or between them all.

Quoted 4 lines

Since our purpose is to detect such problems we do not retry instructions but report the error.

If we detect such problem we may in the future enhance the tests.

I think I've seen retries needed on z/VM in the past; do you know if that still happens?

Quoted 23 lines · Cornelia Huck

Quoted 19 lines · On Tue, 28 Apr 2020 10:27:36 +0200 Pierre Morel <[email protected]> wrote:

Quoted 6 lines · Janosch Frank

Quoted 2 lines · Pierre Morel

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Would we expect needing retries for the pong device?

Yes it can be that we need to retry some instructions if we want them to succeed.
This is the case for example if we develop a driver for an operating system. When working with firmware, sometime, things do not work at the first time. Mostly due to races in silicium, firmware or hypervisor or between them all.

Since our purpose is to detect such problems we do not retry instructions but report the error.

If we detect such problem we may in the future enhance the tests.

Quoted 3 lines

I think I've seen retries needed on z/VM in the past; do you know if that still happens?

I did not try the tests under z/VM, nor direct on an LPAR, only under QEMU/KVM.
Under QEMU/KVM, I did not encounter any need for retry, 100% of the enabled succeeded on first try.

Quoted 28 lines · On Fri, 15 May 2020 09:11:52 +0200 Pierre Morel <[email protected]> wrote:

Quoted 26 lines · Cornelia Huck

Quoted 15 lines · On Tue, 28 Apr 2020 10:27:36 +0200 Pierre Morel <[email protected]> wrote:

Quoted 6 lines · Janosch Frank

Quoted 2 lines · Pierre Morel

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Would we expect needing retries for the pong device?

Yes it can be that we need to retry some instructions if we want them to succeed.
This is the case for example if we develop a driver for an operating system. When working with firmware, sometime, things do not work at the first time. Mostly due to races in silicium, firmware or hypervisor or between them all.

Quoted 4 lines

Since our purpose is to detect such problems we do not retry instructions but report the error.

If we detect such problem we may in the future enhance the tests.

I think I've seen retries needed on z/VM in the past; do you know if that still happens?

Quoted 4 lines

I did not try the tests under z/VM, nor direct on an LPAR, only under QEMU/KVM.
Under QEMU/KVM, I did not encounter any need for retry, 100% of the enabled succeeded on first try.

Yep, QEMU/KVM should be fine. Do you plan to run this on anything else?

Quoted 36 lines · Cornelia Huck

Quoted 32 lines · On Fri, 15 May 2020 09:11:52 +0200 Pierre Morel <[email protected]> wrote:

Quoted 18 lines · Cornelia Huck

Quoted 15 lines · On Tue, 28 Apr 2020 10:27:36 +0200 Pierre Morel <[email protected]> wrote:

Quoted 6 lines · Janosch Frank

Quoted 2 lines · Pierre Morel

This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made.

Would we expect needing retries for the pong device?

Yes it can be that we need to retry some instructions if we want them to succeed.
This is the case for example if we develop a driver for an operating system. When working with firmware, sometime, things do not work at the first time. Mostly due to races in silicium, firmware or hypervisor or between them all.

Quoted 8 lines

Quoted 4 lines

Since our purpose is to detect such problems we do not retry instructions but report the error.

If we detect such problem we may in the future enhance the tests.

I think I've seen retries needed on z/VM in the past; do you know if that still happens?

I did not try the tests under z/VM, nor direct on an LPAR, only under QEMU/KVM.
Under QEMU/KVM, I did not encounter any need for retry, 100% of the enabled succeeded on first try.

Quoted 2 lines

Yep, QEMU/KVM should be fine. Do you plan to run this on anything else?

I'd like to have it compatible with z/VM / LPAR as well if it isn't too much work. You never know when you need it and having tests for all hypervisors has been quite a help in the past.

wfi(irq_mask) allows the programm to wait for an interrupt.

The interrupt handler is in charge to remove the WAIT bit when it finished handling interrupt.

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/asm/arch_def.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

lib/s390x/asm/arch_def.h +1 -0

index a0d2362..e04866c 100644
@@ -13,6 +13,7 @@
 #define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_WAIT			0x0002000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
 #define PSW_MASK_BA			0x0000000080000000UL
 #define PSW_MASK_EA			0x0000000100000000UL

lib/s390x/asm/arch_def.h +4 -0

+	load_psw_mask(psw_mask);
+}
+
+
 static inline void enter_pstate(void)
 {
 	uint64_t mask;

Quoted 1 line · Pierre Morel

wfi(irq_mask) allows the programm to wait for an interrupt.

s/programm/program/

Quoted 3 lines

The interrupt handler is in charge to remove the WAIT bit when it finished handling interrupt.

...finished handling the interrupt.

Quoted 5 lines

  }

+static inline void wfi(uint64_t irq_mask)

enabled_wait()

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 260

Quoted 3 lines

lib/s390x/asm/arch_def.h +2 -0

+{
+	uint64_t psw_mask;

You can directly initialize this variable.

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 263

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+
+	psw_mask = extract_psw_mask();
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);

Maybe add a comment here:

/*
* After being woken and having processed the interrupt, let's restore
the PSW mask.
*/

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 266

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+	load_psw_mask(psw_mask);
+}
+

Quoted 4 lines · Janosch Frank

Quoted 1 line · Pierre Morel

wfi(irq_mask) allows the programm to wait for an interrupt.

s/programm/program/

Thx,

Quoted 5 lines

Quoted 3 lines

The interrupt handler is in charge to remove the WAIT bit when it finished handling interrupt.

...finished handling the interrupt.

OK, thx

Quoted 6 lines

Quoted 5 lines

  }

+static inline void wfi(uint64_t irq_mask)

Quoted 1 line

enabled_wait()

I do not like enabled_wait(), we do not know what is enabled and we do not know what we are waiting for.

What about wait_for_interrupt()

Quoted 18 lines

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 260

Quoted 4 lines

lib/s390x/asm/arch_def.h +2 -0

+{
+	uint64_t psw_mask;

You can directly initialize this variable.

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 263

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+
+	psw_mask = extract_psw_mask();
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);

Maybe add a comment here:

/*
* After being woken and having processed the interrupt, let's restore
the PSW mask.
*/

Quoted 3 lines

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 266

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+	load_psw_mask(psw_mask);
+}
+

I can do this, but wasn't it obvious?

Regards,
Pierre

Quoted 24 lines · Pierre Morel

Quoted 4 lines · Janosch Frank

Quoted 1 line · Pierre Morel

wfi(irq_mask) allows the programm to wait for an interrupt.

s/programm/program/

Thx,

Quoted 5 lines

Quoted 3 lines

The interrupt handler is in charge to remove the WAIT bit when it finished handling interrupt.

...finished handling the interrupt.

OK, thx

Quoted 6 lines

Quoted 5 lines

   }

+static inline void wfi(uint64_t irq_mask)

Quoted 7 lines

Quoted 1 line

enabled_wait()

I do not like enabled_wait(), we do not know what is enabled and we do not know what we are waiting for.

What about wait_for_interrupt()

As long as it's not called wfi...

Quoted 18 lines

Quoted 18 lines

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 260

Quoted 4 lines

lib/s390x/asm/arch_def.h +2 -0

+{
+	uint64_t psw_mask;

You can directly initialize this variable.

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 263

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+
+	psw_mask = extract_psw_mask();
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);

Maybe add a comment here:

/*
* After being woken and having processed the interrupt, let's restore
the PSW mask.
*/

Quoted 5 lines

Quoted 3 lines

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 266

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+	load_psw_mask(psw_mask);
+}
+

I can do this, but wasn't it obvious?

It took me a minute, so it will take even longer for developers that are not yet familiar with s390 kernel development.

Quoted 5 lines

Regards,
Pierre

We add a new css_lib file to contain the I/O function we may

share with different tests.
First function is the subchannel_enable() function.

When a channel is enabled we can start a SENSE_ID command using the SSCH instruction to recognize the control unit and device.

This tests the success of SSCH, the I/O interruption and the TSCH instructions.

The test expects a device with a control unit type of 0xC0CA as the first subchannel of the CSS.

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/asm/arch_def.h |   1 +
 lib/s390x/css.h          |  20 ++++++
 lib/s390x/css_lib.c      |  55 +++++++++++++++
 s390x/Makefile           |   1 +
 s390x/css.c              | 149 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 226 insertions(+)
 create mode 100644 lib/s390x/css_lib.c

s390x/css.c +15 -0

+static void irq_io(void)
+{
+	int ret = 0;
+	char *flags;
+	int sid;
+
+	report_prefix_push("Interrupt");
+	/* Lowlevel set the SID as interrupt parameter. */
+	if (lowcore->io_int_param != test_device_sid) {
+		report(0,
+		       "Bad io_int_param: %x expected %x",
+		       lowcore->io_int_param, test_device_sid);
+		goto pop;
+	}
+	report_prefix_pop();

s390x/css.c +9 -0

+		break;
+	case 0:
+		/* Stay humble on success */
+		break;
+	}
+pop:
+	report_prefix_pop();
+	lowcore->io_old_psw.mask &= ~PSW_MASK_WAIT;
+}

s390x/css.c +23 -0

+/*
+ * test_sense
+ * Pre-requisits:
+ * - We need the QEMU PONG device as the first recognized
+ *   device by the enumeration.
+ * - ./s390x-run s390x/css.elf -device ccw-pong,cu_type=0xc0ca
+ */
+static void test_sense(void)
+{
+	int ret;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	ret = enable_subchannel(test_device_sid);
+	if (ret < 0) {
+		report(0,
+		       "Could not enable the subchannel: %08x",
+		       test_device_sid);
+		return;
+	}

s390x/css.c +18 -0

+		goto unreg_cb;
+	}
+
+	report_info("reserved %02x cu_type %04x cu_model %02x dev_type %04x dev_model %02x",
+		    senseid.reserved, senseid.cu_type, senseid.cu_model,
+		    senseid.dev_type, senseid.dev_model);
+
+	if (senseid.cu_type == PONG_CU)
+		report(1, "cu_type: expect 0x%04x got 0x%04x",
+		       PONG_CU_TYPE, senseid.cu_type);
+	else
+		report(0, "cu_type: expect 0x%04x got 0x%04x",
+		       PONG_CU_TYPE, senseid.cu_type);
+
+unreg_cb:
+	unregister_io_int_func(irq_io);
+}
+

Quoted 1 line · On Fri, 24 Apr 2020 12:45:51 +0200 Pierre Morel <[email protected]> wrote:

We add a new css_lib file to contain the I/O function we may

s/function/functions/

s390x/css.c static void test_enable(void) · line 165

Quoted 9 lines

s390x/css.c +9 -0

+	report_prefix_push("tsch");
+	sid = lowcore->subsys_id_word;
+	ret = tsch(sid, &irb);
+	switch (ret) {
+	case 1:
+		dump_irb(&irb);
+		flags = dump_scsw_flags(irb.scsw.ctrl);
+		report(0,
+		       "I/O interrupt, but sch not status pending: %s", flags);

"...but tsch reporting sch as not status pending" ?

A buggy implementation might give the wrong cc for tsch, but still indicate status pending in the control block.

s390x/css.c static void test_enable(void) · line 168

Quoted 4 lines

s390x/css.c +3 -0

+		break;
+	case 2:
+		report(0, "TSCH returns unexpected CC 2");

s390x/css.c static void test_enable(void) · line 171

Quoted 3 lines

s390x/css.c +3 -0

+		break;
+	case 3:
+		report(0, "Subchannel %08x not operational", sid);

"tsch reporting subchannel %08x as not operational" ?

Quoted 10 lines

s390x/css.c +9 -0

+		break;
+	case 0:
+		/* Stay humble on success */
+		break;
+	}
+pop:
+	report_prefix_pop();
+	lowcore->io_old_psw.mask &= ~PSW_MASK_WAIT;
+}

s390x/css.c static void test_enable(void) · line 235

Quoted 3 lines

s390x/css.c +3 -0

+	lowcore->io_int_param = 0;
+
+	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid));

You're always send the full (extended) sense id block. What if the the machine you're running on doesn't support extended sense id? Would the SLI ccw flag help?

s390x/css.c static void test_enable(void) · line 237

Quoted 3 lines

s390x/css.c +2 -0

+	if (!ret) {
+		report(0, "start_senseid failed");

"ssch failed for SENSE ID on sch <sch>" ?

s390x/css.c static void test_enable(void) · line 246

Quoted 10 lines

s390x/css.c +9 -0

+		goto unreg_cb;
+	}
+
+	wfi(PSW_MASK_IO);
+
+	if (lowcore->io_int_param != test_device_sid) {
+		report(0,
+		       "No interrupts. io_int_param: expect 0x%08x, got 0x%08x",
+		       test_device_sid, lowcore->io_int_param);

Doesn't irq_io() already moan here?

To test a write command with the SSCH instruction we need a QEMU device, with control unit type 0xC0CA. The PONG device is such a device.

This type of device responds to PONG_WRITE requests by incrementing an integer, stored as a string at offset 0 of the CCW data.

Signed-off-by Pierre Morel <[email protected]>

---
 s390x/css.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

s390x/css.c +6 -0

index b9dbf01..7cd8731 100644
@@ -23,6 +23,12 @@
 #define PSW_PRG_MASK (PSW_MASK_EA | PSW_MASK_BA)
 
 #define PONG_CU_TYPE		0xc0ca
+/* Channel Commands for PONG device */
+#define PONG_WRITE	0x21 /* Write */
+#define PONG_READ	0x22 /* Read buffer */
+
+#define BUFSZ	9
+static char buffer[BUFSZ];
 
 struct lowcore *lowcore = (void *)0x0;
 

s390x/css.c +33 -0

+
+	while (cnt++ < max) {
+		snprintf(buffer, BUFSZ, "%08x\n", cnt);
+		success = start_subchannel(PONG_WRITE, buffer, BUFSZ);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+
+		wfi(PSW_MASK_IO);
+
+		success = start_subchannel(PONG_READ, buffer, BUFSZ);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+
+		wfi(PSW_MASK_IO);
+
+		result = atol(buffer);
+		if (result != (cnt + 1)) {
+			report(0, "Bad answer from pong: %08x - %08x",
+			       cnt, result);
+			goto unreg_cb;
+		}
+	}
+	report(1, "ping-pong count 0x%08x", cnt);
+
+unreg_cb:
+	unregister_io_int_func(irq_io);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);

@@ -269,6 +322,7 @@ static struct {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "sense (ssch/tsch)", test_sense },
+	{ "ping-pong (ssch/tsch)", test_ping },
 	{ NULL, NULL }
 };
 

Quoted 7 lines · Pierre Morel

To test a write command with the SSCH instruction we need a QEMU device, with control unit type 0xC0CA. The PONG device is such a device.

This type of device responds to PONG_WRITE requests by incrementing an integer, stored as a string at offset 0 of the CCW data.

Signed-off-by Pierre Morel <[email protected]>

s390x/css.c unreg_cb: · line 277

Quoted 7 lines

s390x/css.c +7 -0

+static void test_ping(void)
+{
+	int success, result;
+	int cnt = 0, max = 4;
+
+	if (senseid.cu_type != PONG_CU) {
+		report_skip("No PONG, no ping-pong");

"Device is not a pong device."

s390x/css.c unreg_cb: · line 285

Quoted 9 lines

s390x/css.c +8 -0

+		return;
+	}
+
+	result = register_io_int_func(irq_io);
+	if (result) {
+		report(0, "Could not register IRQ handler");
+		return;
+	}

I'm not sure if it's worth checking the return values or even having a check in register_io_int_func() in the first place.