s390x: Testing the Channel Subsystem I/O

If we use multiple source of interrupts, for exemple, 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 | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

s390x/cstart64.S +0 -0

index 86dd4c4..ff05f9b 100644
@@ -118,6 +118,25 @@ memsetxc:
 	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	.endm

s390x/cstart64.S memsetxc: · line 121

Quoted 1 line

s390x/cstart64.S +1 -0

+	.macro SAVE_IRQ_REGS

s390x/cstart64.S +11 -0

+	.endm
+
+	.macro RESTORE_IRQ_REGS
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8(%r15)
+	.endr
+	algfi   %r15, 16 * 8
+	lmg     %r0, %r14, 0(%r15)
+	algfi   %r15, 15 * 8
+	.endm
+
 .section .text
 /*
  * load_reset calling convention:

s390x/cstart64.S +2 -2

 .globl smp_cpu_setup_state
 smp_cpu_setup_state:
 	xgr	%r1, %r1

@@ -180,9 +201,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 1 line · Pierre Morel

If we use multiple source of interrupts, for exemple, using SCLP

s/exemple/example/

Quoted 22 lines

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 | 25 +++++++++++++++++++++++--
  1 file changed, 23 insertions(+), 2 deletions(-)

s390x/cstart64.S +0 -0

index 86dd4c4..ff05f9b 100644
@@ -118,6 +118,25 @@ memsetxc:
  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
  	.endm

Quoted 1 line

s390x/cstart64.S memsetxc: · line 121

Quoted 1 line

s390x/cstart64.S +1 -0

+	.macro SAVE_IRQ_REGS

Maybe add comments to the start of the macros like:
"Save registers on the stack, so we can have stacked interrupts."

s390x/cstart64.S memsetxc: · line 128

Quoted 8 lines

s390x/cstart64.S +7 -0

+	slgfi   %r15, 15 * 8
+	stmg    %r0, %r14, 0(%r15)
+	slgfi   %r15, 16 * 8
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8(%r15)
+	.endr
+	lgr     %r2, %r15

What's that doing?

Quoted 19 lines

s390x/cstart64.S +11 -0

+	.endm
+
+	.macro RESTORE_IRQ_REGS
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8(%r15)
+	.endr
+	algfi   %r15, 16 * 8
+	lmg     %r0, %r14, 0(%r15)
+	algfi   %r15, 15 * 8
+	.endm
+
  .section .text
  /*
   * load_reset calling convention:

@@ -154,6 +173,8 @@ diag308_load_reset:
  	lpswe	GEN_LC_SW_INT_PSW
  1:	br	%r14
 

s390x/cstart64.S diag308_load_reset: · line 177

Quoted 2 lines

s390x/cstart64.S +2 -0

+
+

Still not fixed

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

index cf6e1ca..b6bb8c1 100644

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

 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

s390x/cstart64.S +8 -7

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

@@ -214,19 +215,19 @@ svc_int:
 
 	.align	8
 reset_psw:
-	.quad	0x0008000180000000
+	.quad	PSW_EXCEPTION_MASK
 initial_psw:
-	.quad	0x0000000180000000, clear_bss_start
+	.quad	PSW_EXCEPTION_MASK, clear_bss_start
 pgm_int_psw:
-	.quad	0x0000000180000000, pgm_int
+	.quad	PSW_EXCEPTION_MASK, pgm_int
 ext_int_psw:
-	.quad	0x0000000180000000, ext_int
+	.quad	PSW_EXCEPTION_MASK, ext_int
 mcck_int_psw:
-	.quad	0x0000000180000000, mcck_int
+	.quad	PSW_EXCEPTION_MASK, mcck_int
 io_int_psw:
-	.quad	0x0000000180000000, io_int
+	.quad	PSW_EXCEPTION_MASK, io_int
 svc_int_psw:
-	.quad	0x0000000180000000, svc_int
+	.quad	PSW_EXCEPTION_MASK, svc_int
 
 
 initial_cr0:

Quoted 22 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 +0 -0

index cf6e1ca..b6bb8c1 100644
@@ -10,15 +10,22 @@
  #ifndef _ASM_S390X_ARCH_DEF_H_
  #define _ASM_S390X_ARCH_DEF_H_
 

lib/s390x/asm/arch_def.h line 13

Quoted 1 line

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

+#define PSW_MASK_IO			0x0200000000000000UL

That's new and not used in this patch, please move it to the patch where it's needed.

lib/s390x/asm/arch_def.h line 23

Quoted 11 lines

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

+#define PSW_MASK_EXT			0x0100000000000000UL
+#define PSW_MASK_DAT			0x0400000000000000UL
+#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)
+
+#ifndef __ASSEMBLER__

\n

Quoted 15 lines

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

  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

@@ -272,3 +279,4 @@ static inline int stsi(void *addr, int fc, int sel1, int sel2)
  }
 

lib/s390x/asm/arch_def.h static inline int stsi(void *addr, int fc, int sel1, int sel2) · line 282

Quoted 2 lines

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

  #endif
+#endif

Please add a comment to which ifdef this endif belongs.

Quoted 36 lines

s390x/cstart64.S +8 -7

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

@@ -214,19 +215,19 @@ svc_int:
 
  	.align	8
  reset_psw:
-	.quad	0x0008000180000000
+	.quad	PSW_EXCEPTION_MASK
  initial_psw:
-	.quad	0x0000000180000000, clear_bss_start
+	.quad	PSW_EXCEPTION_MASK, clear_bss_start
  pgm_int_psw:
-	.quad	0x0000000180000000, pgm_int
+	.quad	PSW_EXCEPTION_MASK, pgm_int
  ext_int_psw:
-	.quad	0x0000000180000000, ext_int
+	.quad	PSW_EXCEPTION_MASK, ext_int
  mcck_int_psw:
-	.quad	0x0000000180000000, mcck_int
+	.quad	PSW_EXCEPTION_MASK, mcck_int
  io_int_psw:
-	.quad	0x0000000180000000, io_int
+	.quad	PSW_EXCEPTION_MASK, io_int
  svc_int_psw:
-	.quad	0x0000000180000000, svc_int
+	.quad	PSW_EXCEPTION_MASK, svc_int

s390x/cstart64.S svc_int: · line 233

Quoted 3 lines

s390x/cstart64.S +0 -0

  initial_cr0:
  	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
  	.quad	0x0000000000040000

Could you maybe also fix that up in a separate patch and use the same constant in lib/s390x/smp.c

Define two functions to register and to unregister a call back for IO Interrupt handling.

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

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

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

lib/s390x/interrupt.c +6 -1

index 05f30be..b70aafd 100644
@@ -10,9 +10,9 @@
  * under the terms of the GNU Library General Public License version 2.
  */
 #include <libcflat.h>
-#include <asm/interrupt.h>
 #include <asm/barrier.h>
 #include <sclp.h>
+#include <interrupt.h>
 
 static bool pgm_int_expected;
 static bool ext_int_expected;

@@ -141,12 +141,33 @@ void handle_mcck_int(void)
 		     lc->mcck_old_psw.addr);
 }
 
+static void (*io_int_func)(void);
+
 void handle_io_int(void)
 {
+	if (*io_int_func)
+		return (*io_int_func)();
+
 	report_abort("Unexpected io interrupt: at %#lx",

lib/s390x/interrupt.c +9 -0

+
+int unregister_io_int_func(void (*f)(void))
+{
+	if (io_int_func != f)
+		return -1;
+	io_int_func = NULL;
+	return 0;
+}
+
 void handle_svc_int(void)
 {
 	report_abort("Unexpected supervisor call interrupt: at %#lx",

lib/s390x/interrupt.h new file +7 -0

new file mode 100644
index 0000000..e945ef7
@@ -0,0 +1,7 @@
+#ifndef __INTERRUPT_H
+#include <asm/interrupt.h>
+
+int register_io_int_func(void (*f)(void));
+int unregister_io_int_func(void (*f)(void));
+
+#endif

Quoted 2 lines · Pierre Morel

Define two functions to register and to unregister a call back for IO Interrupt handling.

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

Define two functions to register and to unregister a call back for IO Interrupt handling.

Per default we keep the old behavior, so does a successful unregister of the callback.

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

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Quoted 6 lines · Pierre Morel

Define two functions to register and to unregister a call back for IO Interrupt handling.

Per default we keep the old behavior, so does a successful unregister of the callback.

Quoted 3 lines

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

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Nearly forgot this:

Reviewed-by Janosch Frank <[email protected]>

To serve multiple times, the function get_clock_ms() is moved from intercept.c test to the new file asm/time.h.

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

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

---
 lib/s390x/asm/time.h | 26 ++++++++++++++++++++++++++
 s390x/intercept.c    | 11 +----------
 2 files changed, 27 insertions(+), 10 deletions(-)
 create mode 100644 lib/s390x/asm/time.h

lib/s390x/asm/time.h new file +26 -0

new file mode 100644
index 0000000..25c7a3c
@@ -0,0 +1,26 @@
+/*
+ * Clock utilities for s390
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *
+ * Copied from the s390/intercept test by:
+ *  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.
+ */
+#ifndef _ASM_S390X_TIME_H_
+#define _ASM_S390X_TIME_H_
+
+static inline uint64_t get_clock_ms(void)
+{
+	uint64_t clk;
+
+	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
+
+	/* Bit 51 is incrememented each microsecond */
+	return (clk >> (63 - 51)) / 1000;
+}
+
+#endif

s390x/intercept.c +1 -10

index 3696e33..80e9606 100644
@@ -13,6 +13,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
+#include <asm/time.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 

@@ -159,16 +160,6 @@ static void test_testblock(void)
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
-static uint64_t get_clock_ms(void)
-{
-	uint64_t clk;
-
-	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
-
-	/* Bit 51 is incrememented each microsecond */
-	return (clk >> (63 - 51)) / 1000;
-}
-
 struct {
 	const char *name;
 	void (*func)(void);

Quoted 2 lines · Pierre Morel

To serve multiple times, the function get_clock_ms() is moved from intercept.c test to the new file asm/time.h.

Let's move get_clock_ms() to lib/s390/asm/time.h, so it can be used in multiple places.

Quoted 1 line

Quoted 3 lines

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

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

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      | 259 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
 2 files changed, 416 insertions(+)
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c

lib/s390x/css.h new file +41 -0

new file mode 100644
index 0000000..fd086aa
@@ -0,0 +1,259 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * Author: Pierre Morel <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef CSS_H
+#define CSS_H
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20
+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw1 {
+	unsigned char code;
+	unsigned char flags;
+	unsigned short count;
+	uint32_t data_address;
+} __attribute__ ((aligned(4)));
+
+#define ORB_M_KEY	0xf0000000
+#define ORB_F_SUSPEND	0x08000000
+#define ORB_F_STREAMING	0x04000000
+#define ORB_F_MODIFCTRL	0x02000000
+#define ORB_F_SYNC	0x01000000
+#define ORB_F_FORMAT	0x00800000
+#define ORB_F_PREFETCH	0x00400000
+#define ORB_F_INIT_IRQ	0x00200000

lib/s390x/css.h new file +35 -0

+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t ccw_addr;
+	uint8_t  dev_stat;
+	uint8_t  sch_stat;
+	uint16_t count;
+};
+
+struct pmcw {
+	uint32_t intparm;
+#define PMCW_DNV        0x0001
+#define PMCW_ENABLE     0x0080
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	uint32_t flags2;
+};
+
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint8_t  md[12];
+} __attribute__ ((aligned(4)));
+
+struct irb {
+	struct scsw scsw;
+	uint32_t esw[5];

lib/s390x/css.h new file +138 -0

+		: "+d" (cc)
+		: "d" (reg1), "a" (addr), "m" (*addr)
+		: "cc", "memory");
+	return cc;
+}
+
+static inline int stsch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	   stsch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int msch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	   msch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int tsch(unsigned long schid, struct irb *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	   tsch	0(%3)\n"
+		"	   ipm	 %0\n"
+		"	   srl	 %0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int hsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	hsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int xsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	xsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int csch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	csch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int rsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	rsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int rchp(unsigned long chpid)
+{
+	register unsigned long reg1 asm("1") = chpid;
+	int cc;
+
+	asm volatile(
+		"	rchp\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+/* Debug functions */
+char *dump_pmcw_flags(uint16_t f);
+char *dump_scsw_flags(uint32_t f);
+
+#ifdef DEBUG_CSS
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);
+struct ccw *dump_ccw(struct ccw *cp);
+#else
+static inline void dump_scsw(struct scsw *scsw) {}
+static inline void dump_irb(struct irb *irbp) {}

lib/s390x/css.h new file +1 -0

+#endif

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

new file mode 100644
index 0000000..ff1a812
@@ -0,0 +1,157 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2019 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 Library General Public License version 2.
+ *
+ * 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
+ * - IRB  : Interuption response Block, describes the result of an operation
+ *          holds a SCSW and model-dependent data.
+ * - SCHIB: SubCHannel Information Block composed of:
+ *   - 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.
+ */
+
+#include <unistd.h>

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

+ */
+static const char *scsw_str = "kkkkslccfpixuzen";
+static const char *scsw_str2 = "1SHCrshcsdsAIPSs";
+static char scsw_line[64] = {};
+
+char *dump_scsw_flags(uint32_t f)
+{
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		if ((f << i) & 0x80000000)
+			scsw_line[i] = scsw_str[i];
+		else
+			scsw_line[i] = '_';
+	}
+	scsw_line[i] = ' ';
+	for (; i < 32; i++) {
+		if ((f << i) & 0x80000000)
+			scsw_line[i + 1] = scsw_str2[i - 16];

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

+ */
+static const char *pmcw_str = "11iii111ellmmdtv";
+static char pcmw_line[32] = {};
+char *dump_pmcw_flags(uint16_t f)
+{
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		if ((f << i) & 0x8000)
+			pcmw_line[i] = pmcw_str[i];
+		else
+			pcmw_line[i] = '_';
+	}
+	return pcmw_line;
+}
+
+#ifdef DEBUG_CSS
+void dump_scsw(struct scsw *s)
+{
+	dump_scsw_flags(s->ctrl);
+	printf("scsw->flags: %s\n", line);
+	printf("scsw->addr : %08x\n", s->addr);
+	printf("scsw->devs : %02x\n", s->devs);
+	printf("scsw->schs : %02x\n", s->schs);
+	printf("scsw->count: %04x\n", s->count);
+}
+
+void dump_irb(struct irb *irbp)
+{
+	int i;
+	uint32_t *p = (uint32_t *)irbp;
+
+	dump_scsw(&irbp->scsw);
+	for (i = 0; i < sizeof(*irbp)/sizeof(*p); i++, p++)
+		printf("irb[%02x] : %08x\n", i, *p);
+}
+
+void dump_pmcw(struct pmcw *p)
+{
+	int i;
+
+	printf("pmcw->intparm  : %08x\n", p->intparm);
+	printf("pmcw->flags    : %04x\n", p->flags);
+	dump_pmcw_flags(p->flags);
+	printf("pmcw->devnum   : %04x\n", p->devnum);
+	printf("pmcw->lpm      : %02x\n", p->lpm);
+	printf("pmcw->pnom     : %02x\n", p->pnom);
+	printf("pmcw->lpum     : %02x\n", p->lpum);
+	printf("pmcw->pim      : %02x\n", p->pim);
+	printf("pmcw->mbi      : %04x\n", p->mbi);
+	printf("pmcw->pom      : %02x\n", p->pom);
+	printf("pmcw->pam      : %02x\n", p->pam);
+	printf("pmcw->mbi      : %04x\n", p->mbi);
+	for (i = 0; i < 8; i++)
+		printf("pmcw->chpid[%d]: %02x\n", i, p->chpid[i]);
+	printf("pmcw->flags2  : %08x\n", p->flags2);
+}
+
+void dump_schib(struct schib *sch)
+{
+	struct pmcw *p = &sch->pmcw;
+	struct scsw *s = &sch->scsw;
+
+	printf("--SCHIB--\n");
+	dump_pmcw(p);
+	dump_scsw(s);
+}
+
+struct ccw *dump_ccw(struct ccw *cp)
+{
+	printf("CCW: code: %02x flags: %02x count: %04x data: %08x\n", cp->code,
+	    cp->flags, cp->count, cp->data);
+
+	if (cp->code == CCW_C_TIC)
+		return (struct ccw *)(long)cp->data;
+
+	return (cp->flags & CCW_F_CC) ? cp + 1 : NULL;
+}
+
+void dump_orb(struct orb *op)
+{
+	struct ccw *cp;
+
+	printf("ORB: intparm : %08x\n", op->intparm);
+	printf("ORB: ctrl    : %08x\n", op->ctrl);
+	printf("ORB: prio    : %08x\n", op->prio);
+	cp = (struct ccw *)(long) (op->cpa);
+	while (cp)
+		cp = dump_ccw(cp);
+}
+
+#endif

Quoted 61 lines · Pierre Morel

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      | 259 +++++++++++++++++++++++++++++++++++++++++++
  lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
  2 files changed, 416 insertions(+)

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

lib/s390x/css.h new file +41 -0

new file mode 100644
index 0000000..fd086aa
@@ -0,0 +1,259 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * Author: Pierre Morel <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef CSS_H
+#define CSS_H
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20
+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw1 {
+	unsigned char code;
+	unsigned char flags;
+	unsigned short count;
+	uint32_t data_address;
+} __attribute__ ((aligned(4)));
+
+#define ORB_M_KEY	0xf0000000
+#define ORB_F_SUSPEND	0x08000000
+#define ORB_F_STREAMING	0x04000000
+#define ORB_F_MODIFCTRL	0x02000000
+#define ORB_F_SYNC	0x01000000
+#define ORB_F_FORMAT	0x00800000
+#define ORB_F_PREFETCH	0x00400000
+#define ORB_F_INIT_IRQ	0x00200000

lib/s390x/css.h line 51

Quoted 10 lines

lib/s390x/css.h new file +10 -0

+#define ORB_F_ADDRLIMIT	0x00100000
+#define ORB_F_SUSP_IRQ	0x00080000
+#define ORB_F_TRANSPORT	0x00040000
+#define ORB_F_IDAW2	0x00020000
+#define ORB_F_IDAW_2K	0x00010000
+#define ORB_M_LPM	0x0000ff00
+#define ORB_F_LPM_DFLT	0x00008000
+#define ORB_F_ILSM	0x00000080
+#define ORB_F_CCW_IND	0x00000040
+#define ORB_F_ORB_EXT	0x00000001

That looks weird.

lib/s390x/css.h line 59

Quoted 8 lines

lib/s390x/css.h new file +8 -0

+
+struct orb {
+	uint32_t intparm;
+	uint32_t ctrl;
+	uint32_t cpa;
+	uint32_t prio;
+	uint32_t reserved[4];
+} __attribute__ ((aligned(4)));

Are any of these ever stack allocated and therefore need alignment?

Quoted 40 lines

lib/s390x/css.h new file +39 -0

+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t ccw_addr;
+	uint8_t  dev_stat;
+	uint8_t  sch_stat;
+	uint16_t count;
+};
+
+struct pmcw {
+	uint32_t intparm;
+#define PMCW_DNV        0x0001
+#define PMCW_ENABLE     0x0080
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	uint32_t flags2;
+};
+
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint8_t  md[12];
+} __attribute__ ((aligned(4)));
+
+struct irb {
+	struct scsw scsw;
+	uint32_t esw[5];
+	uint32_t ecw[8];
+	uint32_t emw[8];
+} __attribute__ ((aligned(4)));
+

lib/s390x/css.h line 104

Quoted 6 lines

lib/s390x/css.h new file +6 -0

+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int cc = -1;

Why is this one initialized but no other cc is?

lib/s390x/css.h line 109

Quoted 6 lines

lib/s390x/css.h new file +5 -0

+
+	asm volatile(
+		"	   ssch	0(%2)\n"
+		"0:	 ipm	 %0\n"
+		"	   srl	 %0,28\n"

Formatting?

lib/s390x/css.h line 110

Quoted 1 line

lib/s390x/css.h new file +1 -0

+		"1:\n"

What do these jump labels do?

Quoted 7 lines

lib/s390x/css.h new file +6 -0

+		: "+d" (cc)
+		: "d" (reg1), "a" (addr), "m" (*addr)
+		: "cc", "memory");
+	return cc;
+}
+

Quoted 1 line

+extern unsigned long stacktop;

That should be exported somewhere else, we also use it in lib/s390x/sclp.c. Btw. why is it in here at all?

Quoted 37 lines

lib/s390x/css.h new file +1 -0

+#endif

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

new file mode 100644
index 0000000..ff1a812
@@ -0,0 +1,157 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2019 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 Library General Public License version 2.
+ *
+ * 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
+ * - IRB  : Interuption response Block, describes the result of an operation
+ *          holds a SCSW and model-dependent data.
+ * - SCHIB: SubCHannel Information Block composed of:
+ *   - 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.
+ */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+

lib/s390x/css_dump.c line 35

Quoted 6 lines

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

+#undef DEBUG_CSS
+#include <css.h>
+
+/*
+ * Try to have a more human representation of the SCSW flags:
+ * each letter in the two strings he under represent the first

s/he under//

lib/s390x/css_dump.c line 36

Quoted 2 lines

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

+ * letter of the associated bit in the flag.

in the flag fields?

Quoted 24 lines

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

+ */
+static const char *scsw_str = "kkkkslccfpixuzen";
+static const char *scsw_str2 = "1SHCrshcsdsAIPSs";
+static char scsw_line[64] = {};
+
+char *dump_scsw_flags(uint32_t f)
+{
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		if ((f << i) & 0x80000000)
+			scsw_line[i] = scsw_str[i];
+		else
+			scsw_line[i] = '_';
+	}
+	scsw_line[i] = ' ';
+	for (; i < 32; i++) {
+		if ((f << i) & 0x80000000)
+			scsw_line[i + 1] = scsw_str2[i - 16];
+		else
+			scsw_line[i + 1] = '_';
+	}
+	return scsw_line;

lib/s390x/css_dump.c line 65

Quoted 6 lines

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

+}
+
+/*
+ * Try o have a more human representation of the PMCW flags
+ * each letter in the two strings he under represent the first
+ * letter of the associated bit in the flag.

Please rephrase

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         | 88 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 4 files changed, 95 insertions(+)
 create mode 100644 s390x/css.c

s390x/css.c new file +33 -0

new file mode 100644
index 0000000..dfab35f
@@ -0,0 +1,88 @@
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2019 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.
+ */
+
+#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/time.h>
+
+#include <css.h>
+
+#define SID_ONE		0x00010000
+
+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;

s390x/css.c new file +5 -0

+}
+
+static struct {
+	const char *name;
+	void (*func)(void);

Quoted 58 lines · On Wed, 11 Dec 2019 16:46:07 +0100 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 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         | 88 +++++++++++++++++++++++++++++++++++++++++++++
  s390x/unittests.cfg |  4 +++
  4 files changed, 95 insertions(+)

create mode 100644 s390x/css.c

s390x/css.c new file +37 -0

new file mode 100644
index 0000000..dfab35f
@@ -0,0 +1,88 @@
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2019 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.
+ */
+
+#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/time.h>
+
+#include <css.h>
+
+#define SID_ONE		0x00010000
+
+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;
+
+	for (scn = 0; scn < 0xffff; scn++) {
+		cc = stsch(scn|SID_ONE, &schib);
+		switch (cc) {

s390x/css.c line 43

Quoted 6 lines

s390x/css.c new file +6 -0

+		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 scn 0x%x", cc, scn);

Spell out "subchannel number"?

s390x/css.c line 47

Quoted 5 lines

s390x/css.c new file +4 -0

+			return;
+		}
+		if (cc)
+			break;

Isn't that redundant?

s390x/css.c line 48

Quoted 2 lines

s390x/css.c new file +1 -0

+		/* We silently only support type 0, a.k.a. I/O channels */

s/silently/currently/ ?

Quoted 4 lines

s390x/css.c new file +3 -0

+		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
+			continue;
+		/* We ignore I/O channels without valid devices */

s390x/css.c line 61

Quoted 10 lines

s390x/css.c new file +10 -0

+		if (!(pmcw->flags & PMCW_DNV))
+			continue;
+		/* We keep track of the first device as our test device */
+		if (!test_device_sid)
+			test_device_sid = scn|SID_ONE;
+		scn_found++;
+	}
+out:
+	if (!scn_found) {
+		report(0, "Devices, Tested: %d, no I/O type found", scn);

It's no I/O _devices_ found, isn't it? There might have been I/O subchannels, but none with a valid device...

s390x/css.c line 64

Quoted 4 lines

s390x/css.c new file +3 -0

+		return;
+	}
+	report(1, "Devices, tested: %d, I/O type: %d", scn, scn_found);

As you're testing this anyway: what about tracking _all_ numbers here? I.e., advance a counter for I/O subchannels as well, even if !dnv, and have an output like >
"Tested subchannels: 20, I/O subchannels: 18, I/O devices: 10"

or so?

Quoted 10 lines

s390x/css.c new file +9 -0

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

s390x/css.c line 79

Quoted 6 lines

s390x/css.c new file +6 -0

+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("Channel Sub-System");

s/Channel Sub-System/Channel Subsystem/ ?

s390x/css.c line 88

Quoted 10 lines

s390x/css.c new file +9 -0

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

This basically looks sane to me now.

Just some additional considerations (we can do that on top, no need to do surgery here right now):

I currently have the (not sure how sensible) idea to add some optional testing for vfio-ccw, and this would obviously need some I/O routines as well. So, in the long run, it would be good if something like this stsch-loop could be factored out to a kind of library function. Just some thoughts for now :)

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 success of the MSCH instruction by enabling a channel.

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

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

s390x/css.c +1 -0

index dfab35f..b8824ad 100644
@@ -19,12 +19,24 @@
 #include <asm/time.h>
 
 #include <css.h>
+#include <asm/time.h>
 
 #define SID_ONE		0x00010000
 
 static struct schib schib;
 static int test_device_sid;
 

s390x/css.c +29 -0

+	int cc;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+	/* 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;
+	}
+
+	/*

s390x/css.c +9 -0

+
+	if (!(pmcw->flags & PMCW_ENABLE)) {
+		report(0, "Enable failed. pmcw: %x", pmcw->flags);
+		return;
+	}
+	report(1, "Tested");
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
+	{ "enable (msch)", test_enable },
 	{ NULL, NULL }
 };
 

Quoted 31 lines · On Wed, 11 Dec 2019 16:46:08 +0100 Pierre Morel <[email protected]> wrote:

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 success of the MSCH instruction by enabling a channel.

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

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

s390x/css.c +1 -0

index dfab35f..b8824ad 100644
@@ -19,12 +19,24 @@
  #include <asm/time.h>
 
  #include <css.h>
+#include <asm/time.h>
 
  #define SID_ONE		0x00010000
 
  static struct schib schib;
  static int test_device_sid;
 

s390x/css.c

Quoted 10 lines

s390x/css.c +10 -0

+static inline void delay(unsigned long ms)
+{
+	unsigned long startclk;
+
+	startclk = get_clock_ms();
+	for (;;) {
+		if (get_clock_ms() - startclk > ms)
+			break;
+	}
+} >

Would this function be useful for other callers as well? I.e., should it go into a common header?

Quoted 9 lines

s390x/css.c +1 -0

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

@@ -64,11 +76,64 @@ out:
  	report(1, "Devices, tested: %d, I/O type: %d", scn, scn_found);
  }
 

s390x/css.c out: · line 82

Quoted 4 lines

s390x/css.c +4 -0

+static void test_enable(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+		int count = 0;

Odd indentation.

Quoted 35 lines

s390x/css.c +34 -0

+	int cc;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+	/* 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
+	 * insert a little delay and try 5 times.
+	 */
+	do {
+		cc = stsch(test_device_sid, &schib);

s390x/css.c out: · line 121

Quoted 5 lines

s390x/css.c +5 -0

+		if (cc) {
+			report(0, "stsch cc=%d", cc);
+			return;
+		}
+		delay(10);

That's just a short delay to avoid a busy loop, right? msch should be immediate,

and you probably should not delay on success?

s390x/css.c out: · line 122

Quoted 2 lines

s390x/css.c +1 -0

+	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);

How is this supposed to work? Doesn't the stsch overwrite the control block again, so you need to re-set the enable bit before you retry?

Quoted 47 lines · Cornelia Huck

Quoted 44 lines · On Wed, 11 Dec 2019 16:46:08 +0100 Pierre Morel <[email protected]> wrote:

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 success of the MSCH instruction by enabling a channel.

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

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

s390x/css.c +28 -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
+	 * insert a little delay and try 5 times.
+	 */
+	do {
+		cc = stsch(test_device_sid, &schib);

Quoted 8 lines

s390x/css.c out: · line 121

Quoted 5 lines

s390x/css.c +5 -0

+		if (cc) {
+			report(0, "stsch cc=%d", cc);
+			return;
+		}
+		delay(10);  

That's just a short delay to avoid a busy loop, right? msch should be immediate,

Thought you told to me that it may not be immediate in zVM did I misunderstand?

Quoted 2 lines

and you probably should not delay on success?

yes, it is not optimized, I can test PMCW_ENABLE in the loop this way we can see if, in the zVM case we need to do retries or not.

Quoted 1 line

Quoted 4 lines

s390x/css.c out: · line 122

Quoted 1 line

s390x/css.c +1 -0

+	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);  

How is this supposed to work? Doesn't the stsch overwrite the control block again, so you need to re-set the enable bit before you retry?

I do not think so, there is no msch() in the loop. Do I miss something?

Quoted 57 lines · On Thu, 12 Dec 2019 15:01:07 +0100 Pierre Morel <[email protected]> wrote:

Quoted 47 lines · Cornelia Huck

Quoted 44 lines · On Wed, 11 Dec 2019 16:46:08 +0100 Pierre Morel <[email protected]> wrote:

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 success of the MSCH instruction by enabling a channel.

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

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

s390x/css.c +28 -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
+	 * insert a little delay and try 5 times.
+	 */
+	do {
+		cc = stsch(test_device_sid, &schib);

Quoted 8 lines

s390x/css.c out: · line 121

Quoted 5 lines

s390x/css.c +5 -0

+		if (cc) {
+			report(0, "stsch cc=%d", cc);
+			return;
+		}
+		delay(10);

That's just a short delay to avoid a busy loop, right? msch should be immediate,

Quoted 2 lines

Thought you told to me that it may not be immediate in zVM did I misunderstand?

Maybe I have been confusing... what I'm referring to is this programming note for msch:

"It is recommended that the program inspect the
contents of the subchannel by subsequently
issuing STORE SUBCHANNEL when MODIFY
SUBCHANNEL sets condition code 0. Use of
STORE SUBCHANNEL is a method for deter-
mining if the designated subchannel was
changed or not. Failure to inspect the subchan-
nel following the setting of condition code 0 by
MODIFY SUBCHANNEL may result in conditions
that the program does not expect to occur."

That's exactly what we had to do under z/VM back then: do the msch, check via stsch, redo the msch if needed, check again via stsch. It usually worked with the second msch the latest.

Quoted 14 lines

Quoted 3 lines

and you probably should not delay on success?

yes, it is not optimized, I can test PMCW_ENABLE in the loop this way we can see if, in the zVM case we need to do retries or not.

Quoted 5 lines

s390x/css.c out: · line 122

Quoted 2 lines

s390x/css.c +1 -0

+	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);

How is this supposed to work? Doesn't the stsch overwrite the control block again, so you need to re-set the enable bit before you retry?

Quoted 2 lines

I do not think so, there is no msch() in the loop. Do I miss something?

Well, _I_ missed that the msch() was missing :) You need it (see above); just waiting and re-doing the stsch is useless, as msch is a synchronous instruction which has finished its processing after the cc has been set.

Quoted 77 lines · Cornelia Huck

Quoted 57 lines · On Thu, 12 Dec 2019 15:01:07 +0100 Pierre Morel <[email protected]> wrote:

Quoted 47 lines · Cornelia Huck

Quoted 44 lines · On Wed, 11 Dec 2019 16:46:08 +0100 Pierre Morel <[email protected]> wrote:

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 success of the MSCH instruction by enabling a channel.

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

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

s390x/css.c +28 -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
+	 * insert a little delay and try 5 times.
+	 */
+	do {
+		cc = stsch(test_device_sid, &schib);

Quoted 8 lines

s390x/css.c out: · line 121

Quoted 5 lines

s390x/css.c +5 -0

+		if (cc) {
+			report(0, "stsch cc=%d", cc);
+			return;
+		}
+		delay(10);  

That's just a short delay to avoid a busy loop, right? msch should be immediate,

Quoted 2 lines

Thought you told to me that it may not be immediate in zVM did I misunderstand?

Maybe I have been confusing... what I'm referring to is this programming note for msch:

"It is recommended that the program inspect the
contents of the subchannel by subsequently
issuing STORE SUBCHANNEL when MODIFY
SUBCHANNEL sets condition code 0. Use of
STORE SUBCHANNEL is a method for deter-
mining if the designated subchannel was
changed or not. Failure to inspect the subchan-
nel following the setting of condition code 0 by
MODIFY SUBCHANNEL may result in conditions
that the program does not expect to occur."

Quoted 3 lines

That's exactly what we had to do under z/VM back then: do the msch, check via stsch, redo the msch if needed, check again via stsch. It usually worked with the second msch the latest.

OK, I understand, then it is a bug in zVM that this test could enlighten.

I think we should keep it so, it allows to recognize 3 cases (after I change to test ENABLE in the loop as I said I will):
- immediate ENABLE

- asynchrone ENABLE

- failure to ENABLE

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

and you probably should not delay on success?

yes, it is not optimized, I can test PMCW_ENABLE in the loop this way we can see if, in the zVM case we need to do retries or not.

Quoted 5 lines

s390x/css.c out: · line 122

Quoted 2 lines

s390x/css.c +1 -0

+	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);  

How is this supposed to work? Doesn't the stsch overwrite the control block again, so you need to re-set the enable bit before you retry?

Quoted 8 lines

Quoted 2 lines

I do not think so, there is no msch() in the loop. Do I miss something?

Well, _I_ missed that the msch() was missing :) You need it (see above); just waiting and re-doing the stsch is useless, as msch is a synchronous instruction which has finished its processing after the cc has been set.

Since kvm-unit-test is a test system, not an OS so I think that here we have one more point to leverage the enable function:
- We need to test the enable (what I did (partially))

- We need the enable to work (your proposition) to further test the I/O

OK, I rework this part with your comment in mind.

Thanks
Pierre

Quoted 78 lines · On Thu, 12 Dec 2019 15:21:21 +0100 Pierre Morel <[email protected]> wrote:

Quoted 77 lines · Cornelia Huck

Quoted 57 lines · On Thu, 12 Dec 2019 15:01:07 +0100 Pierre Morel <[email protected]> wrote:

Quoted 47 lines · Cornelia Huck

Quoted 44 lines · On Wed, 11 Dec 2019 16:46:08 +0100 Pierre Morel <[email protected]> wrote:

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 success of the MSCH instruction by enabling a channel.

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

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

s390x/css.c +28 -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
+	 * insert a little delay and try 5 times.
+	 */
+	do {
+		cc = stsch(test_device_sid, &schib);

Quoted 8 lines

s390x/css.c out: · line 121

Quoted 5 lines

s390x/css.c +5 -0

+		if (cc) {
+			report(0, "stsch cc=%d", cc);
+			return;
+		}
+		delay(10);

That's just a short delay to avoid a busy loop, right? msch should be immediate,

Quoted 2 lines

Thought you told to me that it may not be immediate in zVM did I misunderstand?

Maybe I have been confusing... what I'm referring to is this programming note for msch:

"It is recommended that the program inspect the
contents of the subchannel by subsequently
issuing STORE SUBCHANNEL when MODIFY
SUBCHANNEL sets condition code 0. Use of
STORE SUBCHANNEL is a method for deter-
mining if the designated subchannel was
changed or not. Failure to inspect the subchan-
nel following the setting of condition code 0 by
MODIFY SUBCHANNEL may result in conditions
that the program does not expect to occur."

Quoted 5 lines

Quoted 3 lines

That's exactly what we had to do under z/VM back then: do the msch, check via stsch, redo the msch if needed, check again via stsch. It usually worked with the second msch the latest.

OK, I understand, then it is a bug in zVM that this test could enlighten.

Probably more a quirk than a bug... the explanation there is not explicit about that :)

Quoted 4 lines

I think we should keep it so, it allows to recognize 3 cases (after I change to test ENABLE in the loop as I said I will):
- immediate ENABLE

This is the good case.

Quoted 1 line

- asynchrone ENABLE

This one I would consider an architecture violation.

Quoted 1 line

- failure to ENABLE

This is the quirk above.

But I'm not quite sure how you would be able to distinguish the last two cases?

Quoted 24 lines

Quoted 23 lines

Quoted 10 lines

Quoted 4 lines

and you probably should not delay on success?

yes, it is not optimized, I can test PMCW_ENABLE in the loop this way we can see if, in the zVM case we need to do retries or not.

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

s390x/css.c out: · line 122

Quoted 1 line

s390x/css.c +1 -0

+	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);

How is this supposed to work? Doesn't the stsch overwrite the control block again, so you need to re-set the enable bit before you retry?

I do not think so, there is no msch() in the loop. Do I miss something?

Well, _I_ missed that the msch() was missing :) You need it (see above); just waiting and re-doing the stsch is useless, as msch is a synchronous instruction which has finished its processing after the cc has been set.

Quoted 3 lines

Since kvm-unit-test is a test system, not an OS so I think that here we have one more point to leverage the enable function:
- We need to test the enable (what I did (partially))

Maybe also log if you needed to retry? Not as an error, but as additional information?

Quoted 97 lines · Cornelia Huck

Quoted 76 lines · On Thu, 12 Dec 2019 15:21:21 +0100 Pierre Morel <[email protected]> wrote:

Quoted 75 lines · Cornelia Huck

Quoted 56 lines · On Thu, 12 Dec 2019 15:01:07 +0100 Pierre Morel <[email protected]> wrote:

Quoted 46 lines · Cornelia Huck

Quoted 44 lines · On Wed, 11 Dec 2019 16:46:08 +0100 Pierre Morel <[email protected]> wrote:

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 success of the MSCH instruction by enabling a channel.

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

---
    s390x/css.c | 65
+++++++++++++++++++++++++++++++++++++++++++++++++++++

    1 file changed, 65 insertions(+)

+    /* Read the SCHIB for this subchannel */
+    cc = stsch(test_device_sid, &schib);
+    if (cc) {
+        report(0, "stsch cc=%d", cc);
+        return;
+    }

(quoted diff) +22 -0

+
+    /* 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
+     * insert a little delay and try 5 times.
+     */
+    do {
+        cc = stsch(test_device_sid, &schib);

Quoted 8 lines

Quoted 5 lines

+        if (cc) {
+            report(0, "stsch cc=%d", cc);
+            return;
+        }
+        delay(10);

That's just a short delay to avoid a busy loop, right? msch should be immediate,

Quoted 2 lines

Thought you told to me that it may not be immediate in zVM did I misunderstand?

Maybe I have been confusing... what I'm referring to is this programming note for msch:

"It is recommended that the program inspect the
contents of the subchannel by subsequently
issuing STORE SUBCHANNEL when MODIFY
SUBCHANNEL sets condition code 0. Use of
STORE SUBCHANNEL is a method for deter-
mining if the designated subchannel was
changed or not. Failure to inspect the subchan-
nel following the setting of condition code 0 by
MODIFY SUBCHANNEL may result in conditions
that the program does not expect to occur."

Quoted 5 lines

Quoted 3 lines

That's exactly what we had to do under z/VM back then: do the msch, check via stsch, redo the msch if needed, check again via stsch. It usually worked with the second msch the latest.

OK, I understand, then it is a bug in zVM that this test could enlighten.

Probably more a quirk than a bug... the explanation there is not explicit about that :)

Quoted 4 lines

I think we should keep it so, it allows to recognize 3 cases (after I change to test ENABLE in the loop as I said I will):
- immediate ENABLE

This is the good case.

Quoted 1 line

- asynchrone ENABLE

Quoted 8 lines

This one I would consider an architecture violation.

Quoted 1 line

- failure to ENABLE

This is the quirk above.

But I'm not quite sure how you would be able to distinguish the last two cases?

This is where the delay can help.
But yes, not sure that we can differentiate this without to know how long we should delay.

Quoted 23 lines

Quoted 22 lines

Quoted 11 lines

Quoted 11 lines

Quoted 2 lines

and you probably should not delay on success?

yes, it is not optimized, I can test PMCW_ENABLE in the loop this way we can see if, in the zVM case we need to do retries or not.

Quoted 4 lines

Quoted 1 line

+    } while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);

How is this supposed to work? Doesn't the stsch overwrite the control block again, so you need to re-set the enable bit before you retry?

Quoted 7 lines

Quoted 2 lines

I do not think so, there is no msch() in the loop. Do I miss something?

Well, _I_ missed that the msch() was missing :) You need it (see above); just waiting and re-doing the stsch is useless, as msch is a synchronous instruction which has finished its processing after the cc has been set.

Since kvm-unit-test is a test system, not an OS so I think that here we have one more point to leverage the enable function:
- We need to test the enable (what I did (partially))

Quoted 2 lines

Maybe also log if you needed to retry? Not as an error, but as additional information?

Yes.

Regards,
Pierre

After all, I make it simple by testing if the MSCH works as expected, no retry, no delay.
This is just a test.

I will add a new patch to add a library function to enable the channel, with retry to serve when we really need to enable the channel, not to test.

Quoted 3 lines · On Thu, 12 Dec 2019 18:33:15 +0100 Pierre Morel <[email protected]> wrote:

After all, I make it simple by testing if the MSCH works as expected, no retry, no delay.
This is just a test.

That's probably fine if you only run under kvm (not sure what your further plans here are).

Quoted 4 lines

I will add a new patch to add a library function to enable the channel, with retry to serve when we really need to enable the channel, not to test.

A simple enable should be enough for kvm-only usage, we can add a retry easily if needed.

We probably can also defer adding the library function until after we get another user :)

When a channel is enabled we can start a SENSE 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/css.h |  13 ++++
 s390x/css.c     | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+)

s390x/css.c +12 -0

+static void irq_io(void)
+{
+	int ret = 0;
+	char *flags;
+	int sid;
+
+	report_prefix_push("Interrupt");
+	if (lowcore->io_int_param != CSS_TEST_INT_PARAM) {
+		report(0, "Bad io_int_param: %x", lowcore->io_int_param);
+		report_prefix_pop();
+		return;
+	}

s390x/css.c +34 -0

+		break;
+	}
+pop:
+	report_prefix_pop();
+}
+
+static int start_subchannel(int code, char *data, int count)
+{
+	int ret;
+	struct pmcw *p = &schib.pmcw;
+	struct orb *orb_p = &orb[0];
+
+	/* Verify that a test subchannel has been set */
+	if (!test_device_sid) {
+		report_skip("No device");
+		return 0;
+	}
+
+	if ((unsigned long)data >= 0x80000000UL) {
+		report(0, "Data above 2G! %p", data);
+		return 0;
+	}
+
+	/* Verify that the subchannel has been enabled */
+	ret = stsch(test_device_sid, &schib);
+	if (ret) {
+		report(0, "Err %d on stsch on sid %08x", ret, test_device_sid);
+		return 0;
+	}
+	if (!(p->flags & PMCW_ENABLE)) {
+		report_skip("Device (sid %08x) not enabled", test_device_sid);
+		return 0;
+	}
+