[kvm-unit-tests PATCH v4 0/9] s390x: Testing the Channel Subsystem I/O

[kvm-unit-tests PATCH v4 0/9] s390x: Testing the Channel Subsystem I/O

[Pierre Morel]

Goal of the series is to have a framwork to test Channel-Subsystem I/O with
QEMU/KVM.
  
To be able to support interrupt for CSS I/O and for SCLP we need to modify
the interrupt framework to allow re-entrant interruptions.
  
We add a registration for IRQ callbacks to the test programm to define its own
interrupt handler. We need to do special work under interrupt like acknoledging
the interrupt.
  
Being working on PSW bits to allow I/O interrupt, we define new PSW bits
in arch_def.h and use __ASSEMBLER__ define to be able to include this header
in an assembler source file. 
 
This series presents four major tests:
- Enumeration:
        The CSS is enumerated using the STSCH instruction recursively on all
        potentially existing channels.
        Keeping the first channel found as a reference for future use.
        Checks STSCH
 
- Enable:
        If the enumeration succeeded the tests enables the reference
        channel with MSCH and verifies with STSCH that the channel is
        effectively enabled
        Checks MSCH 
 
- Sense:
        If the channel is enabled this test sends a SENSE_ID command
        to the reference channel, analysing the answer and expecting
        the Control unit type being 0xc0ca
        Checks SSCH(READ) and IO-IRQ

- ping-pong:
        If the reference channel leads to the PONG device (0xc0ca),
        the test exchanges a string containing a 9 digit number with
        the PONG device and expecting this number to be incremented
        by the PONG device.
        Checks SSCH(WRITE)


Pierre Morel (9):
  s390x: saving regs for interrupts
  s390x: Use PSW bits definitions in cstart
  s390x: interrupt registration
  s390x: export the clock get_clock_ms() utility
  s390x: Library resources for CSS tests
  s390x: css: stsch, enumeration test
  s390x: css: msch, enable test
  s390x: css: ssch/tsch with sense and interrupt
  s390x: css: ping pong

 lib/s390x/asm/arch_def.h |  16 +-
 lib/s390x/asm/time.h     |  26 +++
 lib/s390x/css.h          | 273 ++++++++++++++++++++++++++++
 lib/s390x/css_dump.c     | 157 ++++++++++++++++
 lib/s390x/interrupt.c    |  23 ++-
 lib/s390x/interrupt.h    |   7 +
 s390x/Makefile           |   2 +
 s390x/css.c              | 374 +++++++++++++++++++++++++++++++++++++++
 s390x/cstart64.S         |  40 ++++-
 s390x/intercept.c        |  11 +-
 s390x/unittests.cfg      |   4 +
 11 files changed, 909 insertions(+), 24 deletions(-)
 create mode 100644 lib/s390x/asm/time.h
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c
 create mode 100644 lib/s390x/interrupt.h
 create mode 100644 s390x/css.c

-- 
2.17.0

Changelog:
from v3 to v4
- add RB from David and Thomas for patchs 
  (3) irq registrqtion and (4) clock export
- rework the PSW bit definitions
  (Thomas)
- Suppress undef DEBUG from css_dump
  (Thomas)
- rework report() functions using new scheme
  (Thomas)
- suppress un-necessary report_info()
- more spelling corrections
- add a loop around enable bit testing
  (Connie)
- rework IRQ testing
  (Connie)
- Test data addresses to be under 2G
  (Connie)

from v2 to v3:
- Rework spelling
  (Connie)
- More descriptions
  (Connie)
- use __ASSEMBLER__ preprocessing to keep
  bits definitions and C structures in the same file
  (David)
- rename the new file clock.h as time.h
  (Janosch, David?)
- use registration for the IO interruption
  (David, Thomas)
- test the SCHIB to verify it has really be modified
  (Connie)
- Lot of simplifications in the tests
  (Connie)

from v1 to v2:
- saving floating point registers (David, Janosh)
- suppress unused PSW bits defintions (Janosh)
- added Thomas reviewed-by
- style and comments modifications (Connie, Janosh)
- moved get_clock_ms() into headers and use it (Thomas)
- separate header and library utility from tests
- Suppress traces, separate tests, make better usage of reports

[kvm-unit-tests PATCH v4 1/9] s390x: saving regs for interrupts

[Pierre Morel]

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 <pmorel@linux.ibm.com>
---
 s390x/cstart64.S | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 86dd4c4..ff05f9b 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -118,6 +118,25 @@ memsetxc:
 	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	.endm
 
+	.macro SAVE_IRQ_REGS
+	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
+	.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
 
+
+
 .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:
-- 
2.17.0

[kvm-unit-tests PATCH v4 2/9] s390x: Use PSW bits definitions in cstart

[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 <pmorel@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 16 ++++++++++++----
 s390x/cstart64.S         | 15 ++++++++-------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index cf6e1ca..b6bb8c1 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,15 +10,22 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_IO			0x0200000000000000UL
+#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__
 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)
 }
 
 #endif
+#endif
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index ff05f9b..56a2045 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -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:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
 	.quad	0x0000000000040000
-- 
2.17.0

[kvm-unit-tests PATCH v4 3/9] s390x: interrupt registration

[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.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
 lib/s390x/interrupt.h |  7 +++++++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 lib/s390x/interrupt.h

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 05f30be..b70aafd 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -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",
 		     lc->io_old_psw.addr);
 }
 
+int register_io_int_func(void (*f)(void))
+{
+	if (io_int_func)
+		return -1;
+	io_int_func = f;
+	return 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",
diff --git a/lib/s390x/interrupt.h b/lib/s390x/interrupt.h
new file mode 100644
index 0000000..e945ef7
--- /dev/null
+++ b/lib/s390x/interrupt.h
@@ -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
-- 
2.17.0

[kvm-unit-tests PATCH v4 4/9] s390x: export the clock get_clock_ms() utility

[Pierre Morel]

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 <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 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

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
new file mode 100644
index 0000000..25c7a3c
--- /dev/null
+++ b/lib/s390x/asm/time.h
@@ -0,0 +1,26 @@
+/*
+ * Clock utilities for s390
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *
+ * Copied from the s390/intercept test by:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * 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
diff --git a/s390x/intercept.c b/s390x/intercept.c
index 3696e33..80e9606 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -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);
-- 
2.17.0

[kvm-unit-tests PATCH v4 5/9] s390x: Library resources for CSS tests

[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 <pmorel@linux.ibm.com>
---
 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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
new file mode 100644
index 0000000..fd086aa
--- /dev/null
+++ b/lib/s390x/css.h
@@ -0,0 +1,259 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * Author: Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * 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
+#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
+
+struct orb {
+	uint32_t intparm;
+	uint32_t ctrl;
+	uint32_t cpa;
+	uint32_t prio;
+	uint32_t reserved[4];
+} __attribute__ ((aligned(4)));
+
+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)));
+
+/* 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;
+
+	asm volatile(
+		"	   ssch	0(%2)\n"
+		"0:	 ipm	 %0\n"
+		"	   srl	 %0,28\n"
+		"1:\n"
+		: "+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) {}
+static inline void dump_pmcw(struct pmcw *p) {}
+static inline void dump_schib(struct schib *sch) {}
+static inline void dump_orb(struct orb *op) {}
+static inline struct ccw *dump_ccw(struct ccw *cp)
+{
+	return NULL;
+}
+#endif
+
+extern unsigned long stacktop;
+#endif
diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
new file mode 100644
index 0000000..ff1a812
--- /dev/null
+++ b/lib/s390x/css_dump.c
@@ -0,0 +1,157 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2019 IBM Corp.
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * 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>
+
+#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
+ * letter of the associated bit in the flag.
+ */
+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;
+}
+
+/*
+ * 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.
+ */
+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
-- 
2.17.0

[kvm-unit-tests PATCH v4 6/9] s390x: css: stsch, enumeration test

[Pierre Morel]

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 <pmorel@linux.ibm.com>
---
 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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index fd086aa..06f048b 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -82,6 +82,7 @@ struct pmcw {
 	uint8_t  chpid[8];
 	uint32_t flags2;
 };
+#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
 
 struct schib {
 	struct pmcw pmcw;
diff --git a/s390x/Makefile b/s390x/Makefile
index 3744372..9ebbb84 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
 tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
@@ -50,6 +51,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
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/css.c b/s390x/css.c
new file mode 100644
index 0000000..dfab35f
--- /dev/null
+++ b/s390x/css.c
@@ -0,0 +1,88 @@
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * 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) {
+		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);
+			return;
+		}
+		if (cc)
+			break;
+		/* We silently only support type 0, a.k.a. I/O channels */
+		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
+			continue;
+		/* We ignore I/O channels without valid devices */
+		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);
+		return;
+	}
+	report(1, "Devices, tested: %d, I/O type: %d", scn, scn_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 Sub-System");
+	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();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f1b07cd..1755d9e 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -75,3 +75,7 @@ file = stsi.elf
 [smp]
 file = smp.elf
 extra_params =-smp 2
+
+[css]
+file = css.elf
+extra_params =-device ccw-pong
-- 
2.17.0

[kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[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.

This tests the success of the MSCH instruction by enabling a channel.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/css.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/s390x/css.c b/s390x/css.c
index dfab35f..b8824ad 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -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;
 
+static inline void delay(unsigned long ms)
+{
+	unsigned long startclk;
+
+	startclk = get_clock_ms();
+	for (;;) {
+		if (get_clock_ms() - startclk > ms)
+			break;
+	}
+}
+
 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);
 }
 
+static void test_enable(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+		int count = 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);
+		if (cc) {
+			report(0, "stsch cc=%d", cc);
+			return;
+		}
+		delay(10);
+	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);
+
+	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 }
 };
 
-- 
2.17.0

[kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

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 <pmorel@linux.ibm.com>
---
 lib/s390x/css.h |  13 ++++
 s390x/css.c     | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 06f048b..983e502 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -97,6 +97,19 @@ struct irb {
 	uint32_t emw[8];
 } __attribute__ ((aligned(4)));
 
+#define CCW_CMD_SENSE_ID	0xe4
+#define PONG_CU			0xc0ca
+struct senseid {
+	/* common part */
+	uint8_t reserved;        /* always 0x'FF' */
+	uint16_t cu_type;        /* control unit type */
+	uint8_t cu_model;        /* control unit model */
+	uint16_t dev_type;       /* device type */
+	uint8_t dev_model;       /* device model */
+	uint8_t unused;          /* padding byte */
+	uint8_t padding[256 - 10]; /* Extra padding for CCW */
+} __attribute__ ((aligned(8)));
+
 /* CSS low level access functions */
 
 static inline int ssch(unsigned long schid, struct orb *addr)
diff --git a/s390x/css.c b/s390x/css.c
index b8824ad..7b9bdb1 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -22,9 +22,23 @@
 #include <asm/time.h>
 
 #define SID_ONE		0x00010000
+#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
+
+#define CSS_TEST_INT_PARAM	0xcafec0ca
+#define PONG_CU_TYPE		0xc0ca
+
+struct lowcore *lowcore = (void *)0x0;
 
 static struct schib schib;
 static int test_device_sid;
+#define NUM_CCW  100
+static struct ccw1 ccw[NUM_CCW];
+#define NUM_ORB  100
+static struct orb orb[NUM_ORB];
+static struct irb irb;
+#define BUF_SZ  0x1000
+static char buffer[BUF_SZ] __attribute__ ((aligned(8)));
+static struct senseid senseid;
 
 static inline void delay(unsigned long ms)
 {
@@ -37,6 +51,22 @@ static inline void delay(unsigned long ms)
 	}
 }
 
+static void set_io_irq_subclass_mask(uint64_t const new_mask)
+{
+	asm volatile (
+		"lctlg %%c6, %%c6, %[source]\n"
+		: /* No outputs */
+		: [source] "R" (new_mask));
+}
+
+static void set_system_mask(uint8_t new_mask)
+{
+	asm volatile (
+		"ssm %[source]\n"
+		: /* No outputs */
+		: [source] "R" (new_mask));
+}
+
 static void test_enumerate(void)
 {
 	struct pmcw *pmcw = &schib.pmcw;
@@ -128,12 +158,157 @@ static void test_enable(void)
 	report(1, "Tested");
 }
 
+static void enable_io_irq(void)
+{
+	/* Let's enable all ISCs for I/O interrupt */
+	set_io_irq_subclass_mask(0x00000000ff000000);
+	set_system_mask(PSW_PRG_MASK >> 56);
+}
+
+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;
+	}
+	report_prefix_pop();
+
+	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, "IRB scsw flags: %s", flags);
+		goto pop;
+	case 2:
+		report(0, "TSCH return unexpected CC 2");
+		goto pop;
+	case 3:
+		report(0, "Subchannel %08x not operational", sid);
+		goto pop;
+	case 0:
+		/* Stay humble on success */
+		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;
+	}
+
+	report_prefix_push("ssch");
+	/* Build the CCW chain with a single CCW */
+	ccw[0].code = code;
+	ccw[0].flags = 0; /* No flags need to be set */
+	ccw[0].count = count;
+	ccw[0].data_address = (int)(unsigned long)data;
+	orb_p->intparm = CSS_TEST_INT_PARAM;
+	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	if ((unsigned long)&ccw[0] >= 0x80000000UL) {
+		report(0, "CCW above 2G! %016lx", (unsigned long)&ccw[0]);
+		report_prefix_pop();
+		return 0;
+	}
+	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
+
+	ret = ssch(test_device_sid, orb_p);
+	if (ret) {
+		report(0, "ssch cc=%d", ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+/*
+ * 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;
+
+	ret = register_io_int_func(irq_io);
+	if (ret) {
+		report(0, "Could not register IRQ handler");
+		goto unreg_cb;
+	}
+
+	enable_io_irq();
+	lowcore->io_int_param = 0;
+
+	ret = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
+	if (!ret) {
+		report(0, "start_subchannel failed");
+		goto unreg_cb;
+	}
+
+	delay(100);
+	if (lowcore->io_int_param != CSS_TEST_INT_PARAM) {
+		report(0, "cu_type: expect 0x%08x, got 0x%08x",
+		       CSS_TEST_INT_PARAM, lowcore->io_int_param);
+		goto unreg_cb;
+	}
+
+	senseid.cu_type = buffer[2] | (buffer[1] << 8);
+
+	/* Sense ID is non packed cut_type is at offset +1 byte */
+	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);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
+	{ "sense (ssch/tsch)", test_sense },
 	{ NULL, NULL }
 };
 
-- 
2.17.0

[kvm-unit-tests PATCH v4 9/9] s390x: css: ping pong

[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 <pmorel@linux.ibm.com>
---
 s390x/css.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/s390x/css.c b/s390x/css.c
index 7b9bdb1..a09cdff 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -26,6 +26,9 @@
 
 #define CSS_TEST_INT_PARAM	0xcafec0ca
 #define PONG_CU_TYPE		0xc0ca
+/* Channel Commands for PONG device */
+#define PONG_WRITE	0x21 /* Write */
+#define PONG_READ	0x22 /* Read buffer */
 
 struct lowcore *lowcore = (void *)0x0;
 
@@ -302,6 +305,48 @@ unreg_cb:
 	unregister_io_int_func(irq_io);
 }
 
+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");
+		return;
+	}
+
+	result = register_io_int_func(irq_io);
+	if (result) {
+		report(0, "Could not register IRQ handler");
+		return;
+	}
+
+	while (cnt++ < max) {
+		snprintf(buffer, BUF_SZ, "%08x\n", cnt);
+		success = start_subchannel(PONG_WRITE, buffer, 8);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+		delay(100);
+		success = start_subchannel(PONG_READ, buffer, 8);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+		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);
@@ -309,6 +354,7 @@ static struct {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "sense (ssch/tsch)", test_sense },
+	{ "ping-pong (ssch/tsch)", test_ping },
 	{ NULL, NULL }
 };
 
-- 
2.17.0

Re: [kvm-unit-tests PATCH v4 1/9] s390x: saving regs for interrupts

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1985 bytes --]

On 12/11/19 4:46 PM, Pierre Morel wrote:
> If we use multiple source of interrupts, for exemple, using SCLP

s/exemple/example/

> 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 <pmorel@linux.ibm.com>
> ---
>  s390x/cstart64.S | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 86dd4c4..ff05f9b 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -118,6 +118,25 @@ memsetxc:
>  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
>  	.endm
> > +	.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."

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

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

Still not fixed

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v4 2/9] s390x: Use PSW bits definitions in cstart

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3088 bytes --]

On 12/11/19 4:46 PM, Pierre Morel wrote:
> 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 <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 16 ++++++++++++----
>  s390x/cstart64.S         | 15 ++++++++-------
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index cf6e1ca..b6bb8c1 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -10,15 +10,22 @@
>  #ifndef _ASM_S390X_ARCH_DEF_H_
>  #define _ASM_S390X_ARCH_DEF_H_
>  
> +#define PSW_MASK_IO			0x0200000000000000UL

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

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

>  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)
>  }
>  
>  #endif
> +#endif

Please add a comment to which ifdef this endif belongs.

> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index ff05f9b..56a2045 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -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:
>  	/* 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

> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v4 3/9] s390x: interrupt registration

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2244 bytes --]

On 12/11/19 4:46 PM, Pierre Morel wrote:
> 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.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
>  lib/s390x/interrupt.h |  7 +++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>  create mode 100644 lib/s390x/interrupt.h
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 05f30be..b70aafd 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -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",
>  		     lc->io_old_psw.addr);
>  }
>  
> +int register_io_int_func(void (*f)(void))
> +{
> +	if (io_int_func)
> +		return -1;
> +	io_int_func = f;
> +	return 0;
> +}

Not completely sure why this isn't a bool, but ok.

> +
> +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",
> diff --git a/lib/s390x/interrupt.h b/lib/s390x/interrupt.h
> new file mode 100644
> index 0000000..e945ef7
> --- /dev/null
> +++ b/lib/s390x/interrupt.h
> @@ -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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v4 4/9] s390x: export the clock get_clock_ms() utility

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2319 bytes --]

On 12/11/19 4:46 PM, Pierre Morel wrote:
> 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.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>  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
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> new file mode 100644
> index 0000000..25c7a3c
> --- /dev/null
> +++ b/lib/s390x/asm/time.h
> @@ -0,0 +1,26 @@
> +/*
> + * Clock utilities for s390
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *
> + * Copied from the s390/intercept test by:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * 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
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 3696e33..80e9606 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -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);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v4 3/9] s390x: interrupt registration

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 470 bytes --]

On 12/11/19 4:46 PM, Pierre Morel wrote:
> 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 <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Nearly forgot this:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v4 5/9] s390x: Library resources for CSS tests

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 11788 bytes --]

On 12/11/19 4:46 PM, Pierre Morel wrote:
> 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 <pmorel@linux.ibm.com>
> ---
>  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
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> new file mode 100644
> index 0000000..fd086aa
> --- /dev/null
> +++ b/lib/s390x/css.h
> @@ -0,0 +1,259 @@
> +/*
> + * CSS definitions
> + *
> + * Copyright IBM, Corp. 2019
> + * Author: Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * 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
> +#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.

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

> +
> +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)));
> +
> +/* 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?

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

Formatting?

> +		"1:\n"

What do these jump labels do?

> +		: "+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) {}
> +static inline void dump_pmcw(struct pmcw *p) {}
> +static inline void dump_schib(struct schib *sch) {}
> +static inline void dump_orb(struct orb *op) {}
> +static inline struct ccw *dump_ccw(struct ccw *cp)
> +{
> +	return NULL;
> +}
> +#endif
> +
> +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?

> +#endif
> diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
> new file mode 100644
> index 0000000..ff1a812
> --- /dev/null
> +++ b/lib/s390x/css_dump.c
> @@ -0,0 +1,157 @@
> +/*
> + * Channel subsystem structures dumping
> + *
> + * Copyright (c) 2019 IBM Corp.
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * 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>
> +
> +#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//

> + * letter of the associated bit in the flag.

in the flag fields?

> + */
> +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;
> +}
> +
> +/*
> + * 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

> + */
> +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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v4 6/9] s390x: css: stsch, enumeration test

[Cornelia Huck]

On Wed, 11 Dec 2019 16:46:07 +0100
Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
> ---
>  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

> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..dfab35f
> --- /dev/null
> +++ b/s390x/css.c
> @@ -0,0 +1,88 @@
> +/*
> + * Channel Subsystem tests
> + *
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * 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) {
> +		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"?

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

Isn't that redundant?

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

s/silently/currently/ ?

> +		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
> +			continue;
> +		/* We ignore I/O channels without valid devices */
> +		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...

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

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

s/Channel Sub-System/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();
> +}

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

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Cornelia Huck]

On Wed, 11 Dec 2019 16:46:08 +0100
Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index dfab35f..b8824ad 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -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;
>  
> +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?

> +
>  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);
>  }
>  
> +static void test_enable(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +		int count = 0;

Odd indentation.

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

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

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

Re: [kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Cornelia Huck]

On Wed, 11 Dec 2019 16:46:09 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> When a channel is enabled we can start a SENSE command using the SSCH

s/SENSE/SENSE ID/

SENSE is for getting sense data after a unit check :)

> 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 <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h |  13 ++++
>  s390x/css.c     | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 188 insertions(+)

> +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;
> +	}
> +	report_prefix_pop();
> +
> +	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, "IRB scsw flags: %s", flags);

I guess that should only happen if the I/O interrupt was for another
subchannel, and we only enable one subchannel, right?

Maybe log "I/O interrupt, but sch not status pending: <flags>"? (No
idea how log the logged messages can be for kvm unit tests.)

> +		goto pop;
> +	case 2:
> +		report(0, "TSCH return unexpected CC 2");

s/return/returns/

> +		goto pop;
> +	case 3:
> +		report(0, "Subchannel %08x not operational", sid);
> +		goto pop;
> +	case 0:
> +		/* Stay humble on success */

:)

> +		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;
> +	}
> +
> +	report_prefix_push("ssch");
> +	/* Build the CCW chain with a single CCW */
> +	ccw[0].code = code;
> +	ccw[0].flags = 0; /* No flags need to be set */
> +	ccw[0].count = count;
> +	ccw[0].data_address = (int)(unsigned long)data;
> +	orb_p->intparm = CSS_TEST_INT_PARAM;
> +	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> +	if ((unsigned long)&ccw[0] >= 0x80000000UL) {
> +		report(0, "CCW above 2G! %016lx", (unsigned long)&ccw[0]);

Maybe check before filling out the ccw?

> +		report_prefix_pop();
> +		return 0;
> +	}
> +	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
> +
> +	ret = ssch(test_device_sid, orb_p);
> +	if (ret) {
> +		report(0, "ssch cc=%d", ret);
> +		report_prefix_pop();
> +		return 0;
> +	}
> +	report_prefix_pop();
> +	return 1;
> +}
> +
> +/*
> + * 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;
> +
> +	ret = register_io_int_func(irq_io);
> +	if (ret) {
> +		report(0, "Could not register IRQ handler");
> +		goto unreg_cb;
> +	}
> +
> +	enable_io_irq();
> +	lowcore->io_int_param = 0;
> +
> +	ret = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
> +	if (!ret) {
> +		report(0, "start_subchannel failed");
> +		goto unreg_cb;
> +	}
> +
> +	delay(100);
> +	if (lowcore->io_int_param != CSS_TEST_INT_PARAM) {
> +		report(0, "cu_type: expect 0x%08x, got 0x%08x",
> +		       CSS_TEST_INT_PARAM, lowcore->io_int_param);
> +		goto unreg_cb;
> +	}

This still looks like that odd "delay and hope an interrupt has arrived
in the mean time" pattern.

Also, doesn't the interrupt handler check for the intparm already?

> +
> +	senseid.cu_type = buffer[2] | (buffer[1] << 8);

This still looks odd; why not have the ccw fill out the senseid
structure directly?

> +
> +	/* Sense ID is non packed cut_type is at offset +1 byte */

I have trouble parsing this sentence...

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

Didn't you want to check for ff in the reserved field as well?

> +
> +unreg_cb:
> +	unregister_io_int_func(irq_io);
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
>  } tests[] = {
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
> +	{ "sense (ssch/tsch)", test_sense },
>  	{ NULL, NULL }
>  };
>  

Re: [kvm-unit-tests PATCH v4 1/9] s390x: saving regs for interrupts

[Pierre Morel]

On 2019-12-12 10:24, Janosch Frank wrote:
> On 12/11/19 4:46 PM, Pierre Morel wrote:
>> If we use multiple source of interrupts, for exemple, using SCLP
> 
> s/exemple/example/

OK, thanks

> 
>> 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 <pmorel@linux.ibm.com>
>> ---
>>   s390x/cstart64.S | 25 +++++++++++++++++++++++--
>>   1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index 86dd4c4..ff05f9b 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -118,6 +118,25 @@ memsetxc:
>>   	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
>>   	.endm
>>> +	.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."

OK.

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

Passing a parameter to the saved registers to the handler.
makes me think that since I reworked the interrupt handler to add 
registration the parameter disappeared...

I will remove this line and come back with a new series at the time we 
need to access the registers.

> 
>> +	.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
>>   
>> +
>> +
> 
> Still not fixed

sorry I did not see this

Thanks for review,
Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/9] s390x: Use PSW bits definitions in cstart

[Pierre Morel]

On 2019-12-12 10:31, Janosch Frank wrote:
> On 12/11/19 4:46 PM, Pierre Morel wrote:
>> 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 <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h | 16 ++++++++++++----
>>   s390x/cstart64.S         | 15 ++++++++-------
>>   2 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index cf6e1ca..b6bb8c1 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -10,15 +10,22 @@
>>   #ifndef _ASM_S390X_ARCH_DEF_H_
>>   #define _ASM_S390X_ARCH_DEF_H_
>>   
>> +#define PSW_MASK_IO			0x0200000000000000UL
> 
> That's new and not used in this patch, please move it to the patch where
> it's needed.

OK, I can do this.

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

OK

>>   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)
>>   }
>>   
>>   #endif
>> +#endif
> 
> Please add a comment to which ifdef this endif belongs.
> 

OK

>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index ff05f9b..56a2045 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -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:
>>   	/* 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

Yes, OK.

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 3/9] s390x: interrupt registration

[Pierre Morel]

On 2019-12-12 10:39, Janosch Frank wrote:
> On 12/11/19 4:46 PM, Pierre Morel wrote:
>> 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.

OK, thanks.
Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 3/9] s390x: interrupt registration

[Pierre Morel]

On 2019-12-12 10:41, Janosch Frank wrote:
> On 12/11/19 4:46 PM, Pierre Morel wrote:
>> 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 <pmorel@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> Nearly forgot this:
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 

:)

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 4/9] s390x: export the clock get_clock_ms() utility

[Pierre Morel]

On 2019-12-12 10:40, Janosch Frank wrote:
> On 12/11/19 4:46 PM, Pierre Morel wrote:
>> 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.
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 5/9] s390x: Library resources for CSS tests

[Pierre Morel]

On 2019-12-12 10:51, Janosch Frank wrote:
> On 12/11/19 4:46 PM, Pierre Morel wrote:
>> 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 <pmorel@linux.ibm.com>
>> ---
>>   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
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> new file mode 100644
>> index 0000000..fd086aa
>> --- /dev/null
>> +++ b/lib/s390x/css.h
>> @@ -0,0 +1,259 @@
>> +/*
>> + * CSS definitions
>> + *
>> + * Copyright IBM, Corp. 2019
>> + * Author: Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * 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
>> +#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.
> 
>> +
>> +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?

ORB can be eventually allocated on the stack.

scsw and pmcw not but the SCHIB or the IRB could.

> 
>> +
>> +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)));
>> +
>> +/* 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?

it does not need to.
will disapear.

> 
>> +
>> +	asm volatile(
>> +		"	   ssch	0(%2)\n"
>> +		"0:	 ipm	 %0\n"
>> +		"	   srl	 %0,28\n"
> 
> Formatting?
> 
>> +		"1:\n"
> 
> What do these jump labels do?

forgotten from a previous implementation.
will disapear

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

...

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

also forgotten from a previous implementation.

> 
>> +#endif
>> diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
>> new file mode 100644
>> index 0000000..ff1a812
>> --- /dev/null
>> +++ b/lib/s390x/css_dump.c
>> @@ -0,0 +1,157 @@
>> +/*
>> + * Channel subsystem structures dumping
>> + *
>> + * Copyright (c) 2019 IBM Corp.
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * 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>
>> +
>> +#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//

yes, thanks.

> 
>> + * letter of the associated bit in the flag.
> 
> in the flag fields?

OK

> 
>> + */
>> +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;
>> +}
>> +
>> +/*
>> + * 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
> 
OK.

Thanks.
regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 6/9] s390x: css: stsch, enumeration test

[Pierre Morel]

On 2019-12-12 11:18, Cornelia Huck wrote:
> On Wed, 11 Dec 2019 16:46:07 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
>> ---
>>   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
> 
>> diff --git a/s390x/css.c b/s390x/css.c
>> new file mode 100644
>> index 0000000..dfab35f
>> --- /dev/null
>> +++ b/s390x/css.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * Channel Subsystem tests
>> + *
>> + * Copyright (c) 2019 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * 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) {
>> +		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"?

Yes I can do this.

> 
>> +			return;
>> +		}
>> +		if (cc)
>> +			break;
> 
> Isn't that redundant?
fully.

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

OK

> 
>> +		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
>> +			continue;
>> +		/* We ignore I/O channels without valid devices */
>> +		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...

yes, I will update the stats.

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

Yes, will do.


> 
>> +}
>> +
>> +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 Sub-System");
> 
> s/Channel Sub-System/Channel Subsystem/ ?

OK, (it was because of "CSS").

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

Yes, could be useful.

Thanks,
Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Pierre Morel]

On 2019-12-12 13:01, Cornelia Huck wrote:
> On Wed, 11 Dec 2019 16:46:08 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index dfab35f..b8824ad 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -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;
>>   
>> +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?

Yes, I wanted to put it in the new time.h with the get_clock_ms()  but 
did not since I already got the RB.
I also did not want to add a patch to the series, but since you ask, I 
can put it in a separate patch to keep the RB and to add it in the time.h

> 
>> +
>>   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);
>>   }
>>   
>> +static void test_enable(void)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +		int count = 0;
> 
> Odd indentation.

indeed!

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

> 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.


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

Thanks for the review,
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Cornelia Huck]

On Thu, 12 Dec 2019 15:01:07 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-12 13:01, Cornelia Huck wrote:
> > On Wed, 11 Dec 2019 16:46:08 +0100
> > Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
> >> ---
> >>   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;
> >> +	}
> >> +
> >> +	/* 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);
> >> +		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?

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.

> 
> > 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.
> 
> 
> >   
> >> +	} 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.

Re: [kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

On 2019-12-12 13:26, Cornelia Huck wrote:
> On Wed, 11 Dec 2019 16:46:09 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> When a channel is enabled we can start a SENSE command using the SSCH
> 
> s/SENSE/SENSE ID/
> 
> SENSE is for getting sense data after a unit check :)

Yes, thanks.

> 
>> 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 <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h |  13 ++++
>>   s390x/css.c     | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 188 insertions(+)
> 
>> +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;
>> +	}
>> +	report_prefix_pop();
>> +
>> +	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, "IRB scsw flags: %s", flags);
> 
> I guess that should only happen if the I/O interrupt was for another
> subchannel, and we only enable one subchannel, right?
> 
> Maybe log "I/O interrupt, but sch not status pending: <flags>"? (No
> idea how log the logged messages can be for kvm unit tests.)

Yes, the log message I had was not very useful at first sight.

> 
>> +		goto pop;
>> +	case 2:
>> +		report(0, "TSCH return unexpected CC 2");
> 
> s/return/returns/
> 
>> +		goto pop;
>> +	case 3:
>> +		report(0, "Subchannel %08x not operational", sid);
>> +		goto pop;
>> +	case 0:
>> +		/* Stay humble on success */
> 
> :)
> 
>> +		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;
>> +	}
>> +
>> +	report_prefix_push("ssch");
>> +	/* Build the CCW chain with a single CCW */
>> +	ccw[0].code = code;
>> +	ccw[0].flags = 0; /* No flags need to be set */
>> +	ccw[0].count = count;
>> +	ccw[0].data_address = (int)(unsigned long)data;
>> +	orb_p->intparm = CSS_TEST_INT_PARAM;
>> +	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
>> +	if ((unsigned long)&ccw[0] >= 0x80000000UL) {
>> +		report(0, "CCW above 2G! %016lx", (unsigned long)&ccw[0]);
> 
> Maybe check before filling out the ccw?

Yes. Also I wonder if we should not make sure the all kvm-test-text and 
data are under 2G by construct, because I am quite sure that this sort 
of tests will repeat all over the kvm-unit-test code.

Will provide a separate patch for this, in between just do as you said, 
it is the logical thing to do here.

> 
>> +		report_prefix_pop();
>> +		return 0;
>> +	}
>> +	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
>> +
>> +	ret = ssch(test_device_sid, orb_p);
>> +	if (ret) {
>> +		report(0, "ssch cc=%d", ret);
>> +		report_prefix_pop();
>> +		return 0;
>> +	}
>> +	report_prefix_pop();
>> +	return 1;
>> +}
>> +
>> +/*
>> + * 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;
>> +
>> +	ret = register_io_int_func(irq_io);
>> +	if (ret) {
>> +		report(0, "Could not register IRQ handler");
>> +		goto unreg_cb;
>> +	}
>> +
>> +	enable_io_irq();
>> +	lowcore->io_int_param = 0;
>> +
>> +	ret = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
>> +	if (!ret) {
>> +		report(0, "start_subchannel failed");
>> +		goto unreg_cb;
>> +	}
>> +
>> +	delay(100);
>> +	if (lowcore->io_int_param != CSS_TEST_INT_PARAM) {
>> +		report(0, "cu_type: expect 0x%08x, got 0x%08x",
>> +		       CSS_TEST_INT_PARAM, lowcore->io_int_param);
>> +		goto unreg_cb;
>> +	}
> 
> This still looks like that odd "delay and hope an interrupt has arrived
> in the mean time" pattern.

yes.

> 
> Also, doesn't the interrupt handler check for the intparm already?

Yes, if the interrupt fires.

> 
>> +
>> +	senseid.cu_type = buffer[2] | (buffer[1] << 8);
> 
> This still looks odd; why not have the ccw fill out the senseid
> structure directly?

Oh sorry, you already said and I forgot to modify this.
thanks

> 
>> +
>> +	/* Sense ID is non packed cut_type is at offset +1 byte */
> 
> I have trouble parsing this sentence...
> 
>> +	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);
> 
> Didn't you want to check for ff in the reserved field as well?

It was not intended as a check for SENSE_ID but for STSCH/READ.
But, while at this... why not.

Thanks for the review.
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Pierre Morel]

On 2019-12-12 15:10, Cornelia Huck wrote:
> On Thu, 12 Dec 2019 15:01:07 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2019-12-12 13:01, Cornelia Huck wrote:
>>> On Wed, 11 Dec 2019 16:46:08 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
>>>> ---
>>>>    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;
>>>> +	}
>>>> +
>>>> +	/* 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);
>>>> +		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?
> 
> 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.

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


> 
>>
>>> 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.
>>
>>
>>>    
>>>> +	} 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.
> 

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


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Cornelia Huck]

On Thu, 12 Dec 2019 15:21:21 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-12 15:10, Cornelia Huck wrote:
> > On Thu, 12 Dec 2019 15:01:07 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> On 2019-12-12 13:01, Cornelia Huck wrote:  
> >>> On Wed, 11 Dec 2019 16:46:08 +0100
> >>> Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
> >>>> ---
> >>>>    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;
> >>>> +	}
> >>>> +
> >>>> +	/* 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);
> >>>> +		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?  
> > 
> > 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.  
> 
> 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 :)

> 
> 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.

> - asynchrone ENABLE

This one I would consider an architecture violation.

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

> >   
> >>  
> >>> 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.
> >>
> >>  
> >>>      
> >>>> +	} 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.
> >   
> 
> 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?

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

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Pierre Morel]

On 2019-12-12 15:33, Cornelia Huck wrote:
> On Thu, 12 Dec 2019 15:21:21 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2019-12-12 15:10, Cornelia Huck wrote:
>>> On Thu, 12 Dec 2019 15:01:07 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> On 2019-12-12 13:01, Cornelia Huck wrote:
>>>>> On Wed, 11 Dec 2019 16:46:08 +0100
>>>>> Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>     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;
>>>>>> +	}
>>>>>> +
>>>>>> +	/* 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);
>>>>>> +		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?
>>>
>>> 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.
>>
>> 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 :)
> 
>>
>> 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.
> 
>> - asynchrone ENABLE
> 
> This one I would consider an architecture violation.
> 
>> - 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.


> 
>>>    
>>>>   
>>>>> 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.
>>>>
>>>>   
>>>>>       
>>>>>> +	} 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.
>>>    
>>
>> 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?

Yes.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Pierre Morel]

On 2019-12-12 17:05, Pierre Morel wrote:
> 
> 
> On 2019-12-12 15:33, Cornelia Huck wrote:
>> On Thu, 12 Dec 2019 15:21:21 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 2019-12-12 15:10, Cornelia Huck wrote:
>>>> On Thu, 12 Dec 2019 15:01:07 +0100
>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>> On 2019-12-12 13:01, Cornelia Huck wrote:
>>>>>> On Wed, 11 Dec 2019 16:46:08 +0100
>>>>>> Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com>
>>>>>>> ---
>>>>>>>     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;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* 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);
>>>>>>> +        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?
>>>>
>>>> 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.
>>>
>>> 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 :)
>>
>>>
>>> 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.
>>
>>> - asynchrone ENABLE
>>
>> This one I would consider an architecture violation.
>>
>>> - 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.
> 
> 
>>
>>>>>> 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.
>>>>>
>>>>>>> +    } 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.
>>>
>>> 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?
> 
> 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.

Regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

On 2019-12-12 15:10, Pierre Morel wrote:
> 
> 
> On 2019-12-12 13:26, Cornelia Huck wrote:
>> On Wed, 11 Dec 2019 16:46:09 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>

...


> 
>>
>> Also, doesn't the interrupt handler check for the intparm already?
> 
> Yes, if the interrupt fires.
> 
>>
>>> +
>>> +    senseid.cu_type = buffer[2] | (buffer[1] << 8);
>>
>> This still looks odd; why not have the ccw fill out the senseid
>> structure directly?
> 
> Oh sorry, you already said and I forgot to modify this.
> thanks

hum, sorry, I forgot, the sense structure is not padded so I need this.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Cornelia Huck]

On Thu, 12 Dec 2019 18:33:15 +0100
Pierre Morel <pmorel@linux.ibm.com> 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).

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

Re: [kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Cornelia Huck]

On Thu, 12 Dec 2019 19:20:07 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-12 15:10, Pierre Morel wrote:
> > 
> > 
> > On 2019-12-12 13:26, Cornelia Huck wrote:  
> >> On Wed, 11 Dec 2019 16:46:09 +0100
> >> Pierre Morel <pmorel@linux.ibm.com> wrote:

> >>> +
> >>> +    senseid.cu_type = buffer[2] | (buffer[1] << 8);  
> >>
> >> This still looks odd; why not have the ccw fill out the senseid
> >> structure directly?  
> > 
> > Oh sorry, you already said and I forgot to modify this.
> > thanks  
> 
> hum, sorry, I forgot, the sense structure is not padded so I need this.

Very confused; I see padding in the senseid structure? (And what does
padding have to do with it?)

Also, you only copy the cu type... it would really be much better if
you looked at the whole structure you got back from the hypervisor;
that would also allow you to do some more sanity checks etc. If you
really can't pass in a senseid structure directly, just copy everything
you got?

Re: [kvm-unit-tests PATCH v4 9/9] s390x: css: ping pong

[Cornelia Huck]

On Wed, 11 Dec 2019 16:46:10 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> 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 <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index 7b9bdb1..a09cdff 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -26,6 +26,9 @@
>  
>  #define CSS_TEST_INT_PARAM	0xcafec0ca
>  #define PONG_CU_TYPE		0xc0ca
> +/* Channel Commands for PONG device */
> +#define PONG_WRITE	0x21 /* Write */
> +#define PONG_READ	0x22 /* Read buffer */
>  
>  struct lowcore *lowcore = (void *)0x0;
>  
> @@ -302,6 +305,48 @@ unreg_cb:
>  	unregister_io_int_func(irq_io);
>  }
>  
> +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");
> +		return;
> +	}
> +
> +	result = register_io_int_func(irq_io);
> +	if (result) {
> +		report(0, "Could not register IRQ handler");
> +		return;
> +	}
> +
> +	while (cnt++ < max) {
> +		snprintf(buffer, BUF_SZ, "%08x\n", cnt);
> +		success = start_subchannel(PONG_WRITE, buffer, 8);

Magic value? Maybe introduce a #define for the lengths of the
reads/writes?

[This also got me thinking about your start_subchannel function
again... do you also want to allow flags like e.g. SLI? It's not
unusual for commands to return different lengths of data depending on
what features are available; it might be worthwhile to allow short data
if you're not sure that e.g. a command returns either the short or the
long version of a structure.]


> +		if (!success) {
> +			report(0, "start_subchannel failed");
> +			goto unreg_cb;
> +		}
> +		delay(100);
> +		success = start_subchannel(PONG_READ, buffer, 8);
> +		if (!success) {
> +			report(0, "start_subchannel failed");
> +			goto unreg_cb;
> +		}
> +		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);
> @@ -309,6 +354,7 @@ static struct {
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
>  	{ "sense (ssch/tsch)", test_sense },
> +	{ "ping-pong (ssch/tsch)", test_ping },
>  	{ NULL, NULL }
>  };
>  

Re: [kvm-unit-tests PATCH v4 7/9] s390x: css: msch, enable test

[Pierre Morel]

On 2019-12-13 10:33, Cornelia Huck wrote:
> On Thu, 12 Dec 2019 18:33:15 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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).

If the test fails, it fails.

However, for other tests we need the enable to work.
For example before making a SENSE_ID, in this case we will make retries.



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

I already work on it in the prepared next series, I have
- the enable test
and
- a enable_sch to use as a librairy function

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

On 2019-12-13 10:43, Cornelia Huck wrote:
> On Thu, 12 Dec 2019 19:20:07 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2019-12-12 15:10, Pierre Morel wrote:
>>>
>>>
>>> On 2019-12-12 13:26, Cornelia Huck wrote:
>>>> On Wed, 11 Dec 2019 16:46:09 +0100
>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>>> +
>>>>> +    senseid.cu_type = buffer[2] | (buffer[1] << 8);
>>>>
>>>> This still looks odd; why not have the ccw fill out the senseid
>>>> structure directly?
>>>
>>> Oh sorry, you already said and I forgot to modify this.
>>> thanks
>>
>> hum, sorry, I forgot, the sense structure is not padded so I need this.
> 
> Very confused; I see padding in the senseid structure? (And what does
> padding have to do with it?)

Sorry my fault:
I wanted to say packed and... I forgot to pack the senseid structure.
So I change this.

> 
> Also, you only copy the cu type... it would really be much better if
> you looked at the whole structure you got back from the hypervisor;
> that would also allow you to do some more sanity checks etc. If you
> really can't pass in a senseid structure directly, just copy everything
> you got?
> 

No I can, just need to work properly ;)

I will only check on the cu_type but report_info() on all fields.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 8/9] s390x: css: ssch/tsch with sense and interrupt

[Cornelia Huck]

On Fri, 13 Dec 2019 16:24:18 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-13 10:43, Cornelia Huck wrote:
> > On Thu, 12 Dec 2019 19:20:07 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> On 2019-12-12 15:10, Pierre Morel wrote:  
> >>>
> >>>
> >>> On 2019-12-12 13:26, Cornelia Huck wrote:  
> >>>> On Wed, 11 Dec 2019 16:46:09 +0100
> >>>> Pierre Morel <pmorel@linux.ibm.com> wrote:  
> >   
> >>>>> +
> >>>>> +    senseid.cu_type = buffer[2] | (buffer[1] << 8);  
> >>>>
> >>>> This still looks odd; why not have the ccw fill out the senseid
> >>>> structure directly?  
> >>>
> >>> Oh sorry, you already said and I forgot to modify this.
> >>> thanks  
> >>
> >> hum, sorry, I forgot, the sense structure is not padded so I need this.  
> > 
> > Very confused; I see padding in the senseid structure? (And what does
> > padding have to do with it?)  
> 
> Sorry my fault:
> I wanted to say packed and... I forgot to pack the senseid structure.
> So I change this.

Ah, good :)

> 
> > 
> > Also, you only copy the cu type... it would really be much better if
> > you looked at the whole structure you got back from the hypervisor;
> > that would also allow you to do some more sanity checks etc. If you
> > really can't pass in a senseid structure directly, just copy everything
> > you got?
> >   
> 
> No I can, just need to work properly ;)
> 
> I will only check on the cu_type but report_info() on all fields.

Sounds good!

Re: [kvm-unit-tests PATCH v4 9/9] s390x: css: ping pong

[Pierre Morel]

On 2019-12-13 10:50, Cornelia Huck wrote:
> On Wed, 11 Dec 2019 16:46:10 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> 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 <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index 7b9bdb1..a09cdff 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -26,6 +26,9 @@
>>   
>>   #define CSS_TEST_INT_PARAM	0xcafec0ca
>>   #define PONG_CU_TYPE		0xc0ca
>> +/* Channel Commands for PONG device */
>> +#define PONG_WRITE	0x21 /* Write */
>> +#define PONG_READ	0x22 /* Read buffer */
>>   
>>   struct lowcore *lowcore = (void *)0x0;
>>   
>> @@ -302,6 +305,48 @@ unreg_cb:
>>   	unregister_io_int_func(irq_io);
>>   }
>>   
>> +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");
>> +		return;
>> +	}
>> +
>> +	result = register_io_int_func(irq_io);
>> +	if (result) {
>> +		report(0, "Could not register IRQ handler");
>> +		return;
>> +	}
>> +
>> +	while (cnt++ < max) {
>> +		snprintf(buffer, BUF_SZ, "%08x\n", cnt);
>> +		success = start_subchannel(PONG_WRITE, buffer, 8);
> 
> Magic value? Maybe introduce a #define for the lengths of the
> reads/writes?

OK, and I also do not need a buffer so big.

> 
> [This also got me thinking about your start_subchannel function
> again... do you also want to allow flags like e.g. SLI? It's not
> unusual for commands to return different lengths of data depending on
> what features are available; it might be worthwhile to allow short data
> if you're not sure that e.g. a command returns either the short or the
> long version of a structure.]

I would prefer to keep simple it in this series if you agree.

AFAIU the current QEMU implementation use a fix length and if a short 
read occurs it is an error.
Since we test on PONG, there should be no error.

I agree that for a general test we should change this, but currently the 
goal is just to verify that the remote device is PONG.

If we accept variable length, we need to check the length of what we 
received, and this could need some infrastructure changes that I would 
like to do later.

When the series is accepted I will begin to do more complicated things like:
- checking the exceptions for wrong parameters
   This is the first I will add.
- checking the response difference on flags (SLI, SKP)
- using CC and CD flags for chaining
- TIC, NOP, suspend/resume and PCI

These last one will be fun, we can also trying to play with prefetch 
while at it. :)

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 9/9] s390x: css: ping pong

[Cornelia Huck]

On Fri, 13 Dec 2019 17:50:02 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-13 10:50, Cornelia Huck wrote:

> > [This also got me thinking about your start_subchannel function
> > again... do you also want to allow flags like e.g. SLI? It's not
> > unusual for commands to return different lengths of data depending on
> > what features are available; it might be worthwhile to allow short data
> > if you're not sure that e.g. a command returns either the short or the
> > long version of a structure.]  
> 
> I would prefer to keep simple it in this series if you agree.

Sure, that's fine.

> 
> AFAIU the current QEMU implementation use a fix length and if a short 
> read occurs it is an error.
> Since we test on PONG, there should be no error.

It all depends on how the QEMU device is supposed to work. For a
command reading/writing some data, I'd usually expect the following:

- If SLI is not set, require the length to be the exact value expected
  by the device; otherwise, generate an error.
- If SLI is set, require the length to be between the minimum length
  that makes sense and the full length of the buffer; otherwise,
  generate an error.

Of course, if minimum length == full length, SLI has no real effect :)

> 
> I agree that for a general test we should change this, but currently the 
> goal is just to verify that the remote device is PONG.
> 
> If we accept variable length, we need to check the length of what we 
> received, and this could need some infrastructure changes that I would 
> like to do later.

You mean at the device level? At the driver level (== here), you should
simply get an error or not, I guess.

> 
> When the series is accepted I will begin to do more complicated things like:
> - checking the exceptions for wrong parameters
>    This is the first I will add.

Agreed, that's probably the most useful one.

> - checking the response difference on flags (SLI, SKP)
> - using CC and CD flags for chaining
> - TIC, NOP, suspend/resume and PCI
> 
> These last one will be fun, we can also trying to play with prefetch 
> while at it. :)

I think any kind of ccw chain will already be fun :) It's probably not
so well tested anyway, as virtio is basically single-command.