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

[kvm-unit-tests PATCH v2 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.

Making the interrupt handler weak allows 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 all PSW bits in a
dedicated file.

The simple test tests the I/O reading by the SUB Channel. It needs QEMU to
be patched to have the pong device defined.
The pong device answers, for now, with a Hello World to the read request.


Pierre Morel (9):
  s390x: saving regs for interrupts
  s390x: Define the PSW bits
  s390x: irq: make IRQ handler weak
  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_bits.h |  20 +++
 lib/s390x/asm/arch_def.h  |   6 +-
 lib/s390x/asm/clock.h     |  25 ++++
 lib/s390x/css.h           | 282 ++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c      | 147 +++++++++++++++++++
 lib/s390x/interrupt.c     |   2 +-
 s390x/Makefile            |   4 +-
 s390x/css.c               | 294 ++++++++++++++++++++++++++++++++++++++
 s390x/cstart64.S          |  38 +++--
 s390x/intercept.c         |  11 +-
 s390x/unittests.cfg       |   4 +
 11 files changed, 809 insertions(+), 24 deletions(-)
 create mode 100644 lib/s390x/asm/arch_bits.h
 create mode 100644 lib/s390x/asm/clock.h
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c
 create mode 100644 s390x/css.c

-- 
2.17.0

Changelog:
- 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 v2 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 399ae9b..525c464 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 v2 2/9] s390x: Define the PSW bits

[Pierre Morel]

Let's define the PSW bits  explicitly, it will clarify their
usage.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
 lib/s390x/asm/arch_def.h  |  6 ++----
 s390x/cstart64.S          | 13 +++++++------
 3 files changed, 29 insertions(+), 10 deletions(-)
 create mode 100644 lib/s390x/asm/arch_bits.h

diff --git a/lib/s390x/asm/arch_bits.h b/lib/s390x/asm/arch_bits.h
new file mode 100644
index 0000000..a77489a
--- /dev/null
+++ b/lib/s390x/asm/arch_bits.h
@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
+#ifndef _ASM_S390X_ARCH_BITS_H_
+#define _ASM_S390X_ARCH_BITS_H_
+
+#define PSW_MASK_IO		0x0200000000000000
+#define PSW_MASK_EXT		0x0100000000000000
+#define PSW_MASK_EA		0x0000000100000000
+#define PSW_MASK_BA		0x0000000080000000
+
+#define PSW_EXCEPTION_MASK (PSW_MASK_EA|PSW_MASK_BA)
+
+#endif
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index cf6e1ca..ee7ace2 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,15 +10,13 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#include <asm/arch_bits.h>
+
 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
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 525c464..44caf7a 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -11,6 +11,7 @@
  * under the terms of the GNU Library General Public License version 2.
  */
 #include <asm/asm-offsets.h>
+#include <asm/arch_bits.h>
 #include <asm/sigp.h>
 
 .section .init
@@ -216,17 +217,17 @@ svc_int:
 reset_psw:
 	.quad	0x0008000180000000
 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 v2 3/9] s390x: irq: make IRQ handler weak

[Pierre Morel]

Having a weak function allows the tests programm to declare its own
IRQ handler.
This is helpfull for I/O tests to have the I/O IRQ handler having
its special work to do.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 3e07867..d70fde3 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -140,7 +140,7 @@ void handle_mcck_int(void)
 		     lc->mcck_old_psw.addr);
 }
 
-void handle_io_int(void)
+__attribute__((weak)) void handle_io_int(void)
 {
 	report_abort("Unexpected io interrupt: at %#lx",
 		     lc->io_old_psw.addr);
-- 
2.17.0

[kvm-unit-tests PATCH v2 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/clock.h.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/clock.h | 25 +++++++++++++++++++++++++
 s390x/intercept.c     | 11 +----------
 2 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 lib/s390x/asm/clock.h

diff --git a/lib/s390x/asm/clock.h b/lib/s390x/asm/clock.h
new file mode 100644
index 0000000..e293741
--- /dev/null
+++ b/lib/s390x/asm/clock.h
@@ -0,0 +1,25 @@
+/*
+ * Clock utilities for s390
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *  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_CLOCK_H_
+#define _ASM_S390X_CLOCK_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 404b4c6..ab08533 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/clock.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 v2 5/9] s390x: Library resources for CSS tests

[Pierre Morel]

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h      | 269 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 147 +++++++++++++++++++++++
 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..95dec72
--- /dev/null
+++ b/lib/s390x/css.h
@@ -0,0 +1,269 @@
+/*
+ * 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 ccw {
+	unsigned char code;
+	unsigned char flags;
+	unsigned short count;
+	unsigned int data;
+} __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 {
+	unsigned int	intparm;
+	unsigned int	ctrl;
+	unsigned int	cpa;
+	unsigned int	prio;
+	unsigned int	reserved[4];
+} __attribute__ ((aligned(4)));
+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t addr;
+	uint8_t  devs;
+	uint8_t  schs;
+	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];
+	struct {
+		uint8_t res0;
+		uint8_t st:3;
+		uint8_t :5;
+		uint16_t :13;
+		uint16_t f:1;
+		uint16_t x:1;
+		uint16_t s:1;
+	};
+};
+
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint32_t  md0;
+	uint32_t  md1;
+	uint32_t  md2;
+} __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);
+#undef DEBUG
+#ifdef DEBUG
+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..5356df2
--- /dev/null
+++ b/lib/s390x/css_dump.c
@@ -0,0 +1,147 @@
+/*
+ * Channel Sub-System 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, describe the I/O operation and point to
+ *          a CCW chain
+ * - CCW  : Channel Command Word, describe the data and flow control
+ * - IRB  : Interuption response Block, describe the result of an operation
+ *          hold a SCSW and several channel type dependent fields.
+ * - SCHIB: SubChannel Information Block composed of:
+ *   - SCSW: SubChannel Status Word, status of the channel as a result of an
+ *           operation when in IRB.
+ *   - 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>
+
+#include <css.h>
+
+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;
+}
+
+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
+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->flag2);
+}
+
+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 v2 6/9] s390x: css: stsch, enumeration test

[Pierre Morel]

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

This test the success of STSCH I/O instruction.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/Makefile      |  4 ++-
 s390x/css.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 s390x/css.c

diff --git a/s390x/Makefile b/s390x/Makefile
index e9da618..167ba05 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -1,4 +1,4 @@
-tests = $(TEST_DIR)/selftest.elf
+#tests = $(TEST_DIR)/selftest.elf
 tests += $(TEST_DIR)/intercept.elf
 tests += $(TEST_DIR)/emulator.elf
 tests += $(TEST_DIR)/sieve.elf
@@ -19,6 +19,7 @@ tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/uv-guest.elf
 tests += $(TEST_DIR)/uv-host.elf
 tests += $(TEST_DIR)/uv-limit.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 tests_img = $(patsubst %.elf,%.img,$(tests))
 
@@ -56,6 +57,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..8186f55
--- /dev/null
+++ b/s390x/css.c
@@ -0,0 +1,86 @@
+/*
+ * Channel Sub-System 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 <css.h>
+
+#define SID_ONE		0x00010000
+
+static struct schib schib;
+
+static const char *Channel_type[3] = {
+	"I/O", "CHSC", "MSG"
+};
+
+static int test_device_sid;
+
+static void test_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int sid;
+	int ret, i;
+	int found = 0;
+
+	for (sid = 0; sid < 0xffff; sid++) {
+		ret = stsch(sid|SID_ONE, &schib);
+		if (!ret && (pmcw->flags & PMCW_DNV)) {
+			report_info("SID %04x Type %s PIM %x", sid,
+				     Channel_type[pmcw->st], pmcw->pim);
+			for (i = 0; i < 8; i++)  {
+				if ((pmcw->pim << i) & 0x80) {
+					report_info("CHPID[%d]: %02x", i,
+						    pmcw->chpid[i]);
+					break;
+				}
+			}
+			found++;
+	
+		}
+		if (found && !test_device_sid)
+			test_device_sid = sid|SID_ONE;
+	}
+	if (!found) {
+		report("Found %d devices", 0, found);
+		return;
+	}
+	ret = stsch(test_device_sid, &schib);
+	if (ret) {
+		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
+		return;
+	}
+	report("Tested", 1);
+	return;
+}
+
+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 ff4c088..efdf954 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -78,3 +78,7 @@ extra_params =-smp 2
 
 [uv-guest]
 file = uv-guest.elf
+
+[css]
+file = css.elf
+extra_params =-device ccw-pong
-- 
2.17.0

[kvm-unit-tests PATCH v2 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 tests the success of the MSCH instruction by enabling a channel.

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

diff --git a/s390x/css.c b/s390x/css.c
index 8186f55..e42dc2f 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -62,11 +62,38 @@ static void test_enumerate(void)
 	return;
 }
 
+static void set_schib(void)
+{
+	struct pmcw *p = &schib.pmcw;
+
+	p->intparm = 0xdeadbeef;
+	p->flags |= PMCW_ENABLE;
+}
+
+static void test_enable(void)
+{
+	int ret;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+	set_schib();
+	dump_schib(&schib);
+
+	ret = msch(test_device_sid, &schib);
+	if (ret)
+		report("msch cc=%d", 0, ret);
+	else
+		report("Tested", 1);
+}
+
 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 v2 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 expect a device with a control unit type of 0xC0CA.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h |  13 +++++
 s390x/css.c     | 137 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 95dec72..59114c6 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -106,6 +106,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 e42dc2f..534864f 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -11,12 +11,28 @@
  */
 
 #include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <asm/interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/clock.h>
 
 #include <css.h>
 
 #define SID_ONE		0x00010000
+#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
+
+struct lowcore *lowcore = (void *)0x0;
 
 static struct schib schib;
+#define NB_CCW  100
+static struct ccw ccw[NB_CCW];
+#define NB_ORB  100
+static struct orb orb[NB_ORB];
+static struct irb irb;
+static char buffer[0x1000] __attribute__ ((aligned(8)));
+static struct senseid senseid;
 
 static const char *Channel_type[3] = {
 	"I/O", "CHSC", "MSG"
@@ -24,6 +40,34 @@ static const char *Channel_type[3] = {
 
 static int test_device_sid;
 
+static void delay(unsigned long ms)
+{
+	unsigned long startclk;
+
+	startclk = get_clock_ms();
+	for (;;) {
+		if (get_clock_ms() - startclk > ms)
+			break;
+	}
+}
+
+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;
@@ -88,12 +132,105 @@ static void test_enable(void)
 		report("Tested", 1);
 }
 
+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);
+}
+
+void handle_io_int(void)
+{
+	int ret = 0;
+	char *flags;
+
+	report_prefix_push("Interrupt");
+	if (lowcore->io_int_param != 0xcafec0ca) {
+		report("Bad io_int_param: %x", 0, lowcore->io_int_param);
+		report_prefix_pop();
+		return;
+	}
+	report("io_int_param: %x", 1, lowcore->io_int_param);
+	report_prefix_pop();
+
+	ret = tsch(lowcore->subsys_id_word, &irb);
+	dump_irb(&irb);
+	flags = dump_scsw_flags(irb.scsw.ctrl);
+
+	if (ret)
+		report("IRB scsw flags: %s", 0, flags);
+	else
+		report("IRB scsw flags: %s", 1, flags);
+	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];
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return 0;
+	}
+	ret = stsch(test_device_sid, &schib);
+	if (ret) {
+		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
+		return 0;
+	}
+	if (!(p->flags & PMCW_ENABLE)) {
+		report_skip("Device (sid %08x) not enabled", test_device_sid);
+		return 0;
+	}
+	ccw[0].code = code ;
+	ccw[0].flags = CCW_F_PCI;
+	ccw[0].count = count;
+	ccw[0].data = (int)(unsigned long)data;
+	orb_p->intparm = 0xcafec0ca;
+	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
+
+	report_prefix_push("Start Subchannel");
+	ret = ssch(test_device_sid, orb_p);
+	if (ret) {
+		report("ssch cc=%d", 0, ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+static void test_sense(void)
+{
+	int success;
+
+	enable_io_irq();
+
+	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
+	if (!success) {
+		report("start_subchannel failed", 0);
+		return;
+	}
+
+	senseid.cu_type = buffer[2] | (buffer[1] << 8);
+	delay(1000);
+
+	/* Sense ID is non packed cut_type is at offset +1 byte */
+	if (senseid.cu_type == PONG_CU)
+		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
+	else
+		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
+}
+
 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 v2 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 respond to PONG_WRITE requests by incrementing an
integer, stored as a string at offset 0 of the CCW data.

This is only a success test, no error expected.

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

diff --git a/s390x/css.c b/s390x/css.c
index 534864f..0761e70 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -23,6 +23,10 @@
 #define SID_ONE		0x00010000
 #define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
 
+/* Local Channel Commands */
+#define PONG_WRITE	0x21 /* Write */
+#define PONG_READ	0x22 /* Read buffer */
+
 struct lowcore *lowcore = (void *)0x0;
 
 static struct schib schib;
@@ -31,7 +35,8 @@ static struct ccw ccw[NB_CCW];
 #define NB_ORB  100
 static struct orb orb[NB_ORB];
 static struct irb irb;
-static char buffer[0x1000] __attribute__ ((aligned(8)));
+#define BUF_SZ	0x1000
+static char buffer[BUF_SZ] __attribute__ ((aligned(8)));
 static struct senseid senseid;
 
 static const char *Channel_type[3] = {
@@ -224,6 +229,44 @@ static void test_sense(void)
 		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
 }
 
+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;
+	}
+
+	enable_io_irq();
+
+	while (cnt++ < max) {
+report_info("cnt..: %08x", cnt);
+		snprintf(buffer, BUF_SZ, "%08x\n", cnt);
+		success = start_subchannel(PONG_WRITE, buffer, 8);
+		if (!success) {
+			report("start_subchannel failed", 0);
+			return;
+		}
+		delay(100);
+		success = start_subchannel(PONG_READ, buffer, 8);
+		if (!success) {
+			report("start_subchannel failed", 0);
+			return;
+		}
+		result = atol(buffer);
+		if (result != (cnt + 1)) {
+			report("Bad answer from pong: %08x - %08x", 0, cnt, result);
+			return;
+		} else 
+			report_info("%08x - %08x", cnt, result);
+
+		delay(100);
+	}
+	report("ping-pong count 0x%08x", 1, cnt);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -231,6 +274,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 v2 2/9] s390x: Define the PSW bits

[David Hildenbrand]

On 28.11.19 13:46, Pierre Morel wrote:
> Let's define the PSW bits  explicitly, it will clarify their
> usage.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>  lib/s390x/asm/arch_def.h  |  6 ++----

I'm sorry, but I don't really see a reason to move these 4/5 defines to
a separate header. Can you just keep them in arch_def.h and extend?

(none of your other patches touch arch_bits.h - and it is somewhat a
weird name. Where to put something new: arch_def.h or arch_bits.h? I
would have understood "psw.h", but even that, I don't consider necessary)

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 2/9] s390x: Define the PSW bits

[Pierre Morel]

On 2019-11-28 15:36, David Hildenbrand wrote:
> On 28.11.19 13:46, Pierre Morel wrote:
>> Let's define the PSW bits  explicitly, it will clarify their
>> usage.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>>   lib/s390x/asm/arch_def.h  |  6 ++----
> I'm sorry, but I don't really see a reason to move these 4/5 defines to
> a separate header. Can you just keep them in arch_def.h and extend?

no because arch_def.h contains C structures and inline.


>
> (none of your other patches touch arch_bits.h - and it is somewhat a
> weird name. Where to put something new: arch_def.h or arch_bits.h? I
> would have understood "psw.h", but even that, I don't consider necessary)
>
I can use a name like psw.h or let fall and keep the hexa.


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 2/9] s390x: Define the PSW bits

[David Hildenbrand]

> Am 28.11.2019 um 20:16 schrieb Pierre Morel <pmorel@linux.ibm.com>:
> 
> 
>> On 2019-11-28 15:36, David Hildenbrand wrote:
>>> On 28.11.19 13:46, Pierre Morel wrote:
>>> Let's define the PSW bits  explicitly, it will clarify their
>>> usage.
>>> 
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>>>  lib/s390x/asm/arch_def.h  |  6 ++----
>> I'm sorry, but I don't really see a reason to move these 4/5 defines to
>> a separate header. Can you just keep them in arch_def.h and extend?
> 
> no because arch_def.h contains C structures and inline.

(resend because the iOS Mail app does crazy html thingies)

You‘re looking for

#ifndef __ASSEMBLER__
...

See lib/s390x/asm/sigp.h

Re: [kvm-unit-tests PATCH v2 3/9] s390x: irq: make IRQ handler weak

[David Hildenbrand]

On 28.11.19 13:46, Pierre Morel wrote:
> Having a weak function allows the tests programm to declare its own
> IRQ handler.
> This is helpfull for I/O tests to have the I/O IRQ handler having
> its special work to do.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/interrupt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 3e07867..d70fde3 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -140,7 +140,7 @@ void handle_mcck_int(void)
>  		     lc->mcck_old_psw.addr);
>  }
>  
> -void handle_io_int(void)
> +__attribute__((weak)) void handle_io_int(void)
>  {
>  	report_abort("Unexpected io interrupt: at %#lx",
>  		     lc->io_old_psw.addr);
> 

The clear alternative would be a way to register a callback function.
That way you can modify the callback during the tests. As long as not
registered, wrong I/Os can be caught easily here. @Thomas?

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 28.11.19 13:46, Pierre Morel wrote:
> To serve multiple times, the function get_clock_ms() is moved
> from intercept.c test to the new file asm/clock.h.

I'd probably call this "tod.h" instead. Nevermind.

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 2/9] s390x: Define the PSW bits

[Pierre Morel]

On 2019-11-28 20:48, David Hildenbrand wrote:
> 
> 
>> Am 28.11.2019 um 20:16 schrieb Pierre Morel <pmorel@linux.ibm.com>:
>>
>> 
>>> On 2019-11-28 15:36, David Hildenbrand wrote:
>>>> On 28.11.19 13:46, Pierre Morel wrote:
>>>> Let's define the PSW bits  explicitly, it will clarify their
>>>> usage.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>>>>   lib/s390x/asm/arch_def.h  |  6 ++----
>>> I'm sorry, but I don't really see a reason to move these 4/5 defines to
>>> a separate header. Can you just keep them in arch_def.h and extend?
>>
>> no because arch_def.h contains C structures and inline.
> 
> (resend because the iOS Mail app does crazy html thingies)
> 
> You‘re looking for
> 
> #ifndef __ASSEMBLER__
> ...
> 
> See lib/s390x/asm/sigp.h
> 

Yes, better. :)

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-11-29 13:03, David Hildenbrand wrote:
> On 28.11.19 13:46, Pierre Morel wrote:
>> To serve multiple times, the function get_clock_ms() is moved
>> from intercept.c test to the new file asm/clock.h.
> 
> I'd probably call this "tod.h" instead. Nevermind.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/9] s390x: irq: make IRQ handler weak

[Thomas Huth]

On 29/11/2019 13.01, David Hildenbrand wrote:
> On 28.11.19 13:46, Pierre Morel wrote:
>> Having a weak function allows the tests programm to declare its own
>> IRQ handler.
>> This is helpfull for I/O tests to have the I/O IRQ handler having
>> its special work to do.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/s390x/interrupt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 3e07867..d70fde3 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -140,7 +140,7 @@ void handle_mcck_int(void)
>>  		     lc->mcck_old_psw.addr);
>>  }
>>  
>> -void handle_io_int(void)
>> +__attribute__((weak)) void handle_io_int(void)
>>  {
>>  	report_abort("Unexpected io interrupt: at %#lx",
>>  		     lc->io_old_psw.addr);
>>
> 
> The clear alternative would be a way to register a callback function.
> That way you can modify the callback during the tests. As long as not
> registered, wrong I/Os can be caught easily here. @Thomas?

I don't mind too much, but I think I'd also slightly prefer a registered
callback function here instead.

 Thomas

Re: [kvm-unit-tests PATCH v2 2/9] s390x: Define the PSW bits

[Janosch Frank]

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

On 11/28/19 3:36 PM, David Hildenbrand wrote:
> On 28.11.19 13:46, Pierre Morel wrote:
>> Let's define the PSW bits  explicitly, it will clarify their
>> usage.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>>  lib/s390x/asm/arch_def.h  |  6 ++----
> 
> I'm sorry, but I don't really see a reason to move these 4/5 defines to
> a separate header. Can you just keep them in arch_def.h and extend?
> 
> (none of your other patches touch arch_bits.h - and it is somewhat a
> weird name. Where to put something new: arch_def.h or arch_bits.h? I
> would have understood "psw.h", but even that, I don't consider necessary)
> 

On a related note:
I'd still like to split up the file soonish, maybe moving the functions
into a new file?

@Thomas/David: What's your opinion on that?


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

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

[Janosch Frank]

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

On 11/29/19 1:03 PM, David Hildenbrand wrote:
> On 28.11.19 13:46, Pierre Morel wrote:
>> To serve multiple times, the function get_clock_ms() is moved
>> from intercept.c test to the new file asm/clock.h.
> 
> I'd probably call this "tod.h" instead. Nevermind.

time.h / timing.h?
I'm planning on adding cpu timer functions somewhen next year...

> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 



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

Re: [kvm-unit-tests PATCH v2 2/9] s390x: Define the PSW bits

[David Hildenbrand]

On 02.12.19 12:11, Janosch Frank wrote:
> On 11/28/19 3:36 PM, David Hildenbrand wrote:
>> On 28.11.19 13:46, Pierre Morel wrote:
>>> Let's define the PSW bits  explicitly, it will clarify their
>>> usage.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>>>  lib/s390x/asm/arch_def.h  |  6 ++----
>>
>> I'm sorry, but I don't really see a reason to move these 4/5 defines to
>> a separate header. Can you just keep them in arch_def.h and extend?
>>
>> (none of your other patches touch arch_bits.h - and it is somewhat a
>> weird name. Where to put something new: arch_def.h or arch_bits.h? I
>> would have understood "psw.h", but even that, I don't consider necessary)
>>
> 
> On a related note:
> I'd still like to split up the file soonish, maybe moving the functions
> into a new file?
> 
> @Thomas/David: What's your opinion on that?

Sure, as long as the file header is not longer as its actual content :D

-- 
Thanks,

David / dhildenb

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

[Cornelia Huck]

On Thu, 28 Nov 2019 13:46:03 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> These are the include and library utilities for the css tests patch
> series.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h      | 269 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 147 +++++++++++++++++++++++
>  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..95dec72
> --- /dev/null
> +++ b/lib/s390x/css.h
> @@ -0,0 +1,269 @@
> +/*
> + * 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 ccw {

ccw1, to make it clear that this is a format-1 ccw?

(Do you have any plans to test this with format-0 ccws as well?)

> +	unsigned char code;
> +	unsigned char flags;
> +	unsigned short count;
> +	unsigned int data;

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 {
> +	unsigned int	intparm;
> +	unsigned int	ctrl;
> +	unsigned int	cpa;
> +	unsigned int	prio;
> +	unsigned int	reserved[4];
> +} __attribute__ ((aligned(4)));
> +
> +struct scsw {
> +	uint32_t ctrl;
> +	uint32_t addr;

ccw_addr?

> +	uint8_t  devs;
> +	uint8_t  schs;

Maybe dev_stat/sch_stat?

> +	uint16_t count;
> +};

Out of curiousity (I'm not familiar with the conventions in
kvm-unit-tests): You use the explicit uint32_t et al. types here, while
you used unsigned int et al. in the other definitions... maybe it would
be better to use one or the other?

Also, you probably always want to test against a QEMU-provided device
anyway, so you can probably ignore transport mode and stick with
command mode only, right?

> +
> +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];
> +	struct {
> +		uint8_t res0;
> +		uint8_t st:3;
> +		uint8_t :5;
> +		uint16_t :13;
> +		uint16_t f:1;
> +		uint16_t x:1;
> +		uint16_t s:1;
> +	};

Hm... you used masks for the other fields, any reason you didn't here?

> +};
> +
> +struct schib {
> +	struct pmcw pmcw;
> +	struct scsw scsw;
> +	uint32_t  md0;
> +	uint32_t  md1;
> +	uint32_t  md2;

Hm, both Linux and QEMU express the fields you called md<n> as a 64 bit
measurement-block address and a four bytes model-dependent area...
would it make sense to do so here as well? If the extended measurement
block facility is not installed, we'd get a 12 bytes model-dependent
area, which IMHO would also look better here.

> +} __attribute__ ((aligned(4)));
> +
> +struct irb {
> +	struct scsw scsw;
> +	uint32_t esw[5];
> +	uint32_t ecw[8];
> +	uint32_t emw[8];

If I read the PoP correctly, esw, ecw, and emw are defined bytewise,
not u32-wise.

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

Looking at the Linux code, stsch, msch, and ssch all set up an
exception handler. IIRC, I had introduced that for stsch for multiple
subchannels sets, not sure about the others. Are we sure we never need
to catch exceptions here?

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

Does rchp actually do anything useful on QEMU? Or is this mainly for
completeness' sake?

> +
> +/* Debug functions */
> +char *dump_pmcw_flags(uint16_t f);
> +char *dump_scsw_flags(uint32_t f);
> +#undef DEBUG
> +#ifdef DEBUG
> +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..5356df2
> --- /dev/null
> +++ b/lib/s390x/css_dump.c
> @@ -0,0 +1,147 @@
> +/*
> + * Channel Sub-System structures dumping

I think "subsystem" is the more usual spelling.

> + *
> + * 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, describe the I/O operation and point to

s/describe/describes/
s/point/points/

> + *          a CCW chain
> + * - CCW  : Channel Command Word, describe the data and flow control

s/describe/describes/

but maybe better:

"contains the command, parameters, and a pointer to data"

?

Also this is format-1 only, isn't it?

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

s/describe/describes/

> + *          hold a SCSW and several channel type dependent fields.

s/hold/holds/

s/several channel type dependent fields/model-dependent data/ ?

> + * - SCHIB: SubChannel Information Block composed of:

s/SubChannel/Subchannel/

> + *   - SCSW: SubChannel Status Word, status of the channel as a result of an

s/SubChannel/Subchannel/

> + *           operation when in IRB.

I think that description is a bit confusing: An SCSW always contains
the subchannel status; it's just when it is contained in an IRB that
the status is associated to the last event on that subchannel (as the
result of an operation, or as an unsolicited status.)

> + *   - 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>
> +
> +#include <css.h>
> +
> +static const char *scsw_str = "kkkkslccfpixuzen";
> +static const char *scsw_str2 = "1SHCrshcsdsAIPSs";

Nice, random strings? :)

(...)

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

[Cornelia Huck]

On Thu, 28 Nov 2019 13:46:04 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> First step by testing the channel subsystem is to enumerate the css and

s/by/for/

> retrieve the css devices.
> 
> This test the success of STSCH I/O instruction.

s/test/tests/

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/Makefile      |  4 ++-
>  s390x/css.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  3 files changed, 93 insertions(+), 1 deletion(-)
>  create mode 100644 s390x/css.c
> 

(...)

> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..8186f55
> --- /dev/null
> +++ b/s390x/css.c
> @@ -0,0 +1,86 @@
> +/*
> + * Channel Sub-System tests

s/Sub-System/Subsystem/

> + *
> + * 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 <css.h>
> +
> +#define SID_ONE		0x00010000
> +
> +static struct schib schib;
> +
> +static const char *Channel_type[3] = {
> +	"I/O", "CHSC", "MSG"

No EADM? :)

I don't think we plan to emulate anything beyond I/O in QEMU, though.

> +};
> +
> +static int test_device_sid;
> +
> +static void test_enumerate(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int sid;
> +	int ret, i;
> +	int found = 0;
> +
> +	for (sid = 0; sid < 0xffff; sid++) {
> +		ret = stsch(sid|SID_ONE, &schib);

This seems a bit odd. You are basically putting the subchannel number
into sid, OR in the one, and then use the resulting value as the sid
(subchannel identifier).

> +		if (!ret && (pmcw->flags & PMCW_DNV)) {
> +			report_info("SID %04x Type %s PIM %x", sid,

That's not a sid, but the subchannel number (see above).

> +				     Channel_type[pmcw->st], pmcw->pim);
> +			for (i = 0; i < 8; i++)  {
> +				if ((pmcw->pim << i) & 0x80) {
> +					report_info("CHPID[%d]: %02x", i,
> +						    pmcw->chpid[i]);
> +					break;
> +				}
> +			}
> +			found++;
> +	
> +		}

Here, you iterate over the 0-0xffff range, even if you got a condition
code 3 (indicating no more subchannels in that set). Is that
intentional?

> +		if (found && !test_device_sid)
> +			test_device_sid = sid|SID_ONE;

You set test_device_sid to the last valid subchannel? Why?

> +	}
> +	if (!found) {
> +		report("Found %d devices", 0, found);
> +		return;
> +	}
> +	ret = stsch(test_device_sid, &schib);

Why do you do a stsch() again?

> +	if (ret) {
> +		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
> +		return;
> +	}
> +	report("Tested", 1);
> +	return;

I don't think you need this return statement.

Your test only enumerates devices in the first subchannel set. Do you
plan to enhance the test to enable the MSS facility and iterate over
all subchannel sets?

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

(...)

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

[Cornelia Huck]

On Thu, 28 Nov 2019 13:46:05 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> A second step when testing the channel subsystem is to prepare a channel
> for use.
> 
> This tests the success of the MSCH instruction by enabling a channel.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index 8186f55..e42dc2f 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -62,11 +62,38 @@ static void test_enumerate(void)
>  	return;
>  }
>  
> +static void set_schib(void)
> +{
> +	struct pmcw *p = &schib.pmcw;
> +
> +	p->intparm = 0xdeadbeef;
> +	p->flags |= PMCW_ENABLE;
> +}
> +
> +static void test_enable(void)
> +{
> +	int ret;
> +
> +	if (!test_device_sid) {
> +		report_skip("No device");
> +		return;
> +	}
> +	set_schib();
> +	dump_schib(&schib);
> +
> +	ret = msch(test_device_sid, &schib);
> +	if (ret)
> +		report("msch cc=%d", 0, ret);

Maybe do a stsch and then check/dump the contents of the schib again?

Background: The architecture allows that msch returns success, but that
the fields modified by the issuer remain unchanged at the subchannel
regardless. That should not happen with QEMU; but I remember versions
of z/VM where we sometimes had to call msch twice to make changes stick.

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

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

[Cornelia Huck]

On Thu, 28 Nov 2019 13:46:06 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> 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 expect a device with a control unit type of 0xC0CA.

s/expect/expects/

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h |  13 +++++
>  s390x/css.c     | 137 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 150 insertions(+)
> 

(...)

> diff --git a/s390x/css.c b/s390x/css.c
> index e42dc2f..534864f 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -11,12 +11,28 @@
>   */
>  
>  #include <libcflat.h>
> +#include <alloc_phys.h>
> +#include <asm/page.h>
> +#include <string.h>
> +#include <asm/interrupt.h>
> +#include <asm/arch_def.h>
> +#include <asm/clock.h>
>  
>  #include <css.h>
>  
>  #define SID_ONE		0x00010000
> +#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
> +
> +struct lowcore *lowcore = (void *)0x0;
>  
>  static struct schib schib;
> +#define NB_CCW  100

s/NB_CCW/NUM_CCWS/ ?

I was scratching my head a bit when I first saw that define.

> +static struct ccw ccw[NB_CCW];
> +#define NB_ORB  100
> +static struct orb orb[NB_ORB];
> +static struct irb irb;
> +static char buffer[0x1000] __attribute__ ((aligned(8)));
> +static struct senseid senseid;
>  
>  static const char *Channel_type[3] = {
>  	"I/O", "CHSC", "MSG"
> @@ -24,6 +40,34 @@ static const char *Channel_type[3] = {
>  
>  static int test_device_sid;
>  

(...)

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

Should you accommodate unsolicited interrupts as well?

> +	report("io_int_param: %x", 1, lowcore->io_int_param);
> +	report_prefix_pop();
> +
> +	ret = tsch(lowcore->subsys_id_word, &irb);
> +	dump_irb(&irb);
> +	flags = dump_scsw_flags(irb.scsw.ctrl);
> +
> +	if (ret)
> +		report("IRB scsw flags: %s", 0, flags);
> +	else
> +		report("IRB scsw flags: %s", 1, flags);
> +	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];
> +
> +	if (!test_device_sid) {
> +		report_skip("No device");
> +		return 0;
> +	}
> +	ret = stsch(test_device_sid, &schib);

That schib is a global variable, isn't it? Why do you need to re-check?

> +	if (ret) {
> +		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
> +		return 0;
> +	}
> +	if (!(p->flags & PMCW_ENABLE)) {
> +		report_skip("Device (sid %08x) not enabled", test_device_sid);
> +		return 0;
> +	}
> +	ccw[0].code = code ;

Extra ' ' before ';'

> +	ccw[0].flags = CCW_F_PCI;

Huh, what's that PCI for?

> +	ccw[0].count = count;
> +	ccw[0].data = (int)(unsigned long)data;

Can you be sure that data is always below 2G?

> +	orb_p->intparm = 0xcafec0ca;
> +	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> +	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
> +
> +	report_prefix_push("Start Subchannel");
> +	ret = ssch(test_device_sid, orb_p);
> +	if (ret) {
> +		report("ssch cc=%d", 0, ret);
> +		report_prefix_pop();
> +		return 0;
> +	}
> +	report_prefix_pop();
> +	return 1;
> +}
> +
> +static void test_sense(void)
> +{
> +	int success;
> +
> +	enable_io_irq();
> +
> +	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
> +	if (!success) {
> +		report("start_subchannel failed", 0);
> +		return;
> +	}
> +
> +	senseid.cu_type = buffer[2] | (buffer[1] << 8);
> +	delay(1000);
> +
> +	/* Sense ID is non packed cut_type is at offset +1 byte */
> +	if (senseid.cu_type == PONG_CU)
> +		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
> +	else
> +		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
> +}

I'm not really convinced by that logic here. This will fall apart if
you don't have your pong device exactly in the right place, and it does
not make it easy to extend this for more devices in the future.

What about the following:
- do a stsch() loop (basically re-use your first patch)
- for each I/O subchannel with dnv=1, do SenseID
- use the first (?) device with a c0ca CU type as your test device

Or maybe I'm overthinking this? It just does not strike me as very
robust and reusable.

> +
>  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 v2 9/9] s390x: css: ping pong

[Cornelia Huck]

On Thu, 28 Nov 2019 13:46:07 +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.

"We want to test some read/write ccws via the SSCH instruction with a
QEMU device with control unit type 0xC0CA." ?

> 
> This type of device respond to PONG_WRITE requests by incrementing an

s/respond/responds/

> integer, stored as a string at offset 0 of the CCW data.
> 
> This is only a success test, no error expected.

Nobody expects the Spanish Inquisition^W^W^W an error :)

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index 534864f..0761e70 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -23,6 +23,10 @@
>  #define SID_ONE		0x00010000
>  #define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
>  
> +/* Local Channel Commands */

/* Channel commands for the PONG device */

?

> +#define PONG_WRITE	0x21 /* Write */
> +#define PONG_READ	0x22 /* Read buffer */
> +
>  struct lowcore *lowcore = (void *)0x0;
>  
>  static struct schib schib;
> @@ -31,7 +35,8 @@ static struct ccw ccw[NB_CCW];
>  #define NB_ORB  100
>  static struct orb orb[NB_ORB];
>  static struct irb irb;
> -static char buffer[0x1000] __attribute__ ((aligned(8)));
> +#define BUF_SZ	0x1000
> +static char buffer[BUF_SZ] __attribute__ ((aligned(8)));

Merge this with the introduction of this variable?

>  static struct senseid senseid;
>  
>  static const char *Channel_type[3] = {
> @@ -224,6 +229,44 @@ static void test_sense(void)
>  		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
>  }
>  
> +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");

:D

> +		return;
> +	}
> +
> +	enable_io_irq();

Hasn't that been enabled already for doing SenseID?

> +
> +	while (cnt++ < max) {
> +report_info("cnt..: %08x", cnt);

Wrong indentation?

> +		snprintf(buffer, BUF_SZ, "%08x\n", cnt);
> +		success = start_subchannel(PONG_WRITE, buffer, 8);
> +		if (!success) {
> +			report("start_subchannel failed", 0);
> +			return;
> +		}
> +		delay(100);
> +		success = start_subchannel(PONG_READ, buffer, 8);
> +		if (!success) {
> +			report("start_subchannel failed", 0);
> +			return;
> +		}
> +		result = atol(buffer);
> +		if (result != (cnt + 1)) {
> +			report("Bad answer from pong: %08x - %08x", 0, cnt, result);
> +			return;
> +		} else 
> +			report_info("%08x - %08x", cnt, result);
> +
> +		delay(100);
> +	}
> +	report("ping-pong count 0x%08x", 1, cnt);
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -231,6 +274,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 v2 2/9] s390x: Define the PSW bits

[Pierre Morel]

On 2019-12-02 12:17, David Hildenbrand wrote:
> On 02.12.19 12:11, Janosch Frank wrote:
>> On 11/28/19 3:36 PM, David Hildenbrand wrote:
>>> On 28.11.19 13:46, Pierre Morel wrote:
>>>> Let's define the PSW bits  explicitly, it will clarify their
>>>> usage.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>   lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
>>>>   lib/s390x/asm/arch_def.h  |  6 ++----
>>>
>>> I'm sorry, but I don't really see a reason to move these 4/5 defines to
>>> a separate header. Can you just keep them in arch_def.h and extend?
>>>
>>> (none of your other patches touch arch_bits.h - and it is somewhat a
>>> weird name. Where to put something new: arch_def.h or arch_bits.h? I
>>> would have understood "psw.h", but even that, I don't consider necessary)
>>>
>>
>> On a related note:
>> I'd still like to split up the file soonish, maybe moving the functions
>> into a new file?
>>
>> @Thomas/David: What's your opinion on that?
> 
> Sure, as long as the file header is not longer as its actual content :D
> 

No problem for me.
What should the file header be?

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/9] s390x: irq: make IRQ handler weak

[Pierre Morel]

On 2019-12-02 11:41, Thomas Huth wrote:
> On 29/11/2019 13.01, David Hildenbrand wrote:
>> On 28.11.19 13:46, Pierre Morel wrote:
>>> Having a weak function allows the tests programm to declare its own
>>> IRQ handler.
>>> This is helpfull for I/O tests to have the I/O IRQ handler having
>>> its special work to do.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   lib/s390x/interrupt.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>>> index 3e07867..d70fde3 100644
>>> --- a/lib/s390x/interrupt.c
>>> +++ b/lib/s390x/interrupt.c
>>> @@ -140,7 +140,7 @@ void handle_mcck_int(void)
>>>   		     lc->mcck_old_psw.addr);
>>>   }
>>>   
>>> -void handle_io_int(void)
>>> +__attribute__((weak)) void handle_io_int(void)
>>>   {
>>>   	report_abort("Unexpected io interrupt: at %#lx",
>>>   		     lc->io_old_psw.addr);
>>>
>>
>> The clear alternative would be a way to register a callback function.
>> That way you can modify the callback during the tests. As long as not
>> registered, wrong I/Os can be caught easily here. @Thomas?
> 
> I don't mind too much, but I think I'd also slightly prefer a registered
> callback function here instead.
> 
>   Thomas
> 

As you like but I wonder why you prefer the complicated solution.
The kvm-unit-test is single task, if a test really need something 
complicated it can be done in the test not in the common code.

Anyway I do like you want.

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 12:13, Janosch Frank wrote:
> On 11/29/19 1:03 PM, David Hildenbrand wrote:
>> On 28.11.19 13:46, Pierre Morel wrote:
>>> To serve multiple times, the function get_clock_ms() is moved
>>> from intercept.c test to the new file asm/clock.h.
>>
>> I'd probably call this "tod.h" instead. Nevermind.
> 
> time.h / timing.h?
> I'm planning on adding cpu timer functions somewhen next year...

as long as you both agree, I take the name you want.

> 
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
>>
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 15:06, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 13:46:03 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> These are the include and library utilities for the css tests patch
>> series.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 269 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 147 +++++++++++++++++++++++
>>   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..95dec72
>> --- /dev/null
>> +++ b/lib/s390x/css.h
>> @@ -0,0 +1,269 @@
>> +/*
>> + * 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 ccw {
> 
> ccw1, to make it clear that this is a format-1 ccw?
> 
> (Do you have any plans to test this with format-0 ccws as well?)

not in a near future

> 
>> +	unsigned char code;
>> +	unsigned char flags;
>> +	unsigned short count;
>> +	unsigned int data;
> 
> data_address?

OK

> 
>> +} __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 {
>> +	unsigned int	intparm;
>> +	unsigned int	ctrl;
>> +	unsigned int	cpa;
>> +	unsigned int	prio;
>> +	unsigned int	reserved[4];
>> +} __attribute__ ((aligned(4)));
>> +
>> +struct scsw {
>> +	uint32_t ctrl;
>> +	uint32_t addr;
> 
> ccw_addr?

OK

> 
>> +	uint8_t  devs;
>> +	uint8_t  schs;
> 
> Maybe dev_stat/sch_stat?

OK

> 
>> +	uint16_t count;
>> +};
> 
> Out of curiousity (I'm not familiar with the conventions in
> kvm-unit-tests): You use the explicit uint32_t et al. types here, while
> you used unsigned int et al. in the other definitions... maybe it would
> be better to use one or the other?

OK, I suppose that if we want to be working with TCG uintxxx_t is better.
So I will take care to be coherent.

> 
> Also, you probably always want to test against a QEMU-provided device
> anyway, so you can probably ignore transport mode and stick with
> command mode only, right?

Yes

> 
>> +
>> +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];
>> +	struct {
>> +		uint8_t res0;
>> +		uint8_t st:3;
>> +		uint8_t :5;
>> +		uint16_t :13;
>> +		uint16_t f:1;
>> +		uint16_t x:1;
>> +		uint16_t s:1;
>> +	};
> 
> Hm... you used masks for the other fields, any reason you didn't here?

Here too , I must be coherent.
Reason was that I use the flags differently, the test of the bit fields 
is easier to read than shift & mask but ... anyway I prefer masks for 
hardware close programing.

> 
>> +};
>> +
>> +struct schib {
>> +	struct pmcw pmcw;
>> +	struct scsw scsw;
>> +	uint32_t  md0;
>> +	uint32_t  md1;
>> +	uint32_t  md2;
> 
> Hm, both Linux and QEMU express the fields you called md<n> as a 64 bit
> measurement-block address and a four bytes model-dependent area...
> would it make sense to do so here as well? If the extended measurement
> block facility is not installed, we'd get a 12 bytes model-dependent
> area, which IMHO would also look better here.

OK, IMHO too

> 
>> +} __attribute__ ((aligned(4)));
>> +
>> +struct irb {
>> +	struct scsw scsw;
>> +	uint32_t esw[5];
>> +	uint32_t ecw[8];
>> +	uint32_t emw[8];
> 
> If I read the PoP correctly, esw, ecw, and emw are defined bytewise,
> not u32-wise.

Hum, I found them referenced u32-wise.
I intended to define them as a structure at the moment they are used.
If you really prefer I can use uint8_t esw[40] uint8_t ecw[64] etc.
but I like better word aligned structures.

> 
>> +} __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;
>> +}
> 
> Looking at the Linux code, stsch, msch, and ssch all set up an
> exception handler. IIRC, I had introduced that for stsch for multiple
> subchannels sets, not sure about the others. Are we sure we never need
> to catch exceptions here?

This is the next and important step.
I want to surround each call with expect_pgm_int() / check_pgm_int_code()

Also testing each error case.
So we do not need exception handler here, we will use the kvm-test ones.

> 
>> +
>> +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 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;
>> +}
> 
> Does rchp actually do anything useful on QEMU? Or is this mainly for
> completeness' sake?

It is for completness for this first series.
However I would like to do something with it in a following one.
to be ready for QEMU implementation or to test real hardware when the 
kvm-unit-test run on LPAR.

> 
>> +
>> +/* Debug functions */
>> +char *dump_pmcw_flags(uint16_t f);
>> +char *dump_scsw_flags(uint32_t f);
>> +#undef DEBUG
>> +#ifdef DEBUG
>> +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..5356df2
>> --- /dev/null
>> +++ b/lib/s390x/css_dump.c
>> @@ -0,0 +1,147 @@
>> +/*
>> + * Channel Sub-System structures dumping
> 
> I think "subsystem" is the more usual spelling.

OK, I found it much fun so :)

> 
>> + *
>> + * 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, describe the I/O operation and point to
> 
> s/describe/describes/
> s/point/points/

thanks

> 
>> + *          a CCW chain
>> + * - CCW  : Channel Command Word, describe the data and flow control
> 
> s/describe/describes/
> 
> but maybe better:
> 
> "contains the command, parameters, and a pointer to data"
> 
> ?

yes, thanks

> 
> Also this is format-1 only, isn't it?

yes, I add this

> 
>> + * - IRB  : Interuption response Block, describe the result of an operation
> 
> s/describe/describes/

again!
thanks :)

> 
>> + *          hold a SCSW and several channel type dependent fields.
> 
> s/hold/holds/
hum...
> 
> s/several channel type dependent fields/model-dependent data/ ?

yes

> 
>> + * - SCHIB: SubChannel Information Block composed of:
> 
> s/SubChannel/Subchannel/
> 
>> + *   - SCSW: SubChannel Status Word, status of the channel as a result of an
> 
> s/SubChannel/Subchannel/
OK
> 
>> + *           operation when in IRB.
> 
> I think that description is a bit confusing: An SCSW always contains
> the subchannel status; it's just when it is contained in an IRB that
> the status is associated to the last event on that subchannel (as the
> result of an operation, or as an unsolicited status.)

yes, the "when in IRB has nothing to do there" IRB defintion is above.
I will make it clearer

> 
>> + *   - 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>
>> +
>> +#include <css.h>
>> +
>> +static const char *scsw_str = "kkkkslccfpixuzen";
>> +static const char *scsw_str2 = "1SHCrshcsdsAIPSs";
> 
> Nice, random strings? :)

these are the bit definitions associated with the scsw flags.
Easier to read than a bit field ... as long as you have the 
documentation (Pop)


Thanks a lot for the review.
Seems I have some work for the v3

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 15:22, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 13:46:04 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> First step by testing the channel subsystem is to enumerate the css and
> 
> s/by/for/

ok

> 
>> retrieve the css devices.
>>
>> This test the success of STSCH I/O instruction.
> 
> s/test/tests/

yes

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/Makefile      |  4 ++-
>>   s390x/css.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |  4 +++
>>   3 files changed, 93 insertions(+), 1 deletion(-)
>>   create mode 100644 s390x/css.c
>>
> 
> (...)
> 
>> diff --git a/s390x/css.c b/s390x/css.c
>> new file mode 100644
>> index 0000000..8186f55
>> --- /dev/null
>> +++ b/s390x/css.c
>> @@ -0,0 +1,86 @@
>> +/*
>> + * Channel Sub-System tests
> 
> s/Sub-System/Subsystem/

yes too

> 
>> + *
>> + * 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 <css.h>
>> +
>> +#define SID_ONE		0x00010000
>> +
>> +static struct schib schib;
>> +
>> +static const char *Channel_type[3] = {
>> +	"I/O", "CHSC", "MSG"
> 
> No EADM? :)

I forgot EADM, I will add it!

> 
> I don't think we plan to emulate anything beyond I/O in QEMU, though.

Even, yes, no plan to use it for now.

> 
>> +};
>> +
>> +static int test_device_sid;
>> +
>> +static void test_enumerate(void)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int sid;
>> +	int ret, i;
>> +	int found = 0;
>> +
>> +	for (sid = 0; sid < 0xffff; sid++) {
>> +		ret = stsch(sid|SID_ONE, &schib);
> 
> This seems a bit odd. You are basically putting the subchannel number
> into sid, OR in the one, and then use the resulting value as the sid
> (subchannel identifier).
> 
>> +		if (!ret && (pmcw->flags & PMCW_DNV)) {
>> +			report_info("SID %04x Type %s PIM %x", sid,
> 
> That's not a sid, but the subchannel number (see above).
> 
>> +				     Channel_type[pmcw->st], pmcw->pim);
>> +			for (i = 0; i < 8; i++)  {
>> +				if ((pmcw->pim << i) & 0x80) {
>> +					report_info("CHPID[%d]: %02x", i,
>> +						    pmcw->chpid[i]);
>> +					break;
>> +				}
>> +			}
>> +			found++;
>> +	
>> +		}
> 
> Here, you iterate over the 0-0xffff range, even if you got a condition
> code 3 (indicating no more subchannels in that set). Is that
> intentional?

I thought there could be more subchannels.
I need then a break in the loop when this happens.
I will reread the PoP to see how to find that no more subchannel are in 
that set.

> 
>> +		if (found && !test_device_sid)
>> +			test_device_sid = sid|SID_ONE;
> 
> You set test_device_sid to the last valid subchannel? Why?

The last ? I wanted the first one

I wanted something easy but I should have explain.

To avoid doing complicated things like doing a sense on each valid 
subchannel I just take the first one.
Should be enough as we do not go to the device in this test.

> 
>> +	}
>> +	if (!found) {
>> +		report("Found %d devices", 0, found);
>> +		return;
>> +	}
>> +	ret = stsch(test_device_sid, &schib);
> 
> Why do you do a stsch() again?

right, no need.
In an internal version I used to print some informations from the SCHIB.
Since in between I overwrote the SHIB, I did it again.
But in this version; no need.

> 
>> +	if (ret) {
>> +		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
>> +		return;
>> +	}
>> +	report("Tested", 1);
>> +	return;
> 
> I don't think you need this return statement.

right I have enough work. :)

> 
> Your test only enumerates devices in the first subchannel set. Do you
> plan to enhance the test to enable the MSS facility and iterate over
> all subchannel sets?

Yes, it is something we can do in a following series

> 
>> +}
>> +
>> +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/Sub-System/Subsystem/

yes, again.


Thanks for the review.
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 15:30, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 13:46:05 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> A second step when testing the channel subsystem is to prepare a channel
>> for use.
>>
>> This tests the success of the MSCH instruction by enabling a channel.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index 8186f55..e42dc2f 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -62,11 +62,38 @@ static void test_enumerate(void)
>>   	return;
>>   }
>>   
>> +static void set_schib(void)
>> +{
>> +	struct pmcw *p = &schib.pmcw;
>> +
>> +	p->intparm = 0xdeadbeef;
>> +	p->flags |= PMCW_ENABLE;
>> +}
>> +
>> +static void test_enable(void)
>> +{
>> +	int ret;
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
>> +	set_schib();
>> +	dump_schib(&schib);
>> +
>> +	ret = msch(test_device_sid, &schib);
>> +	if (ret)
>> +		report("msch cc=%d", 0, ret);
> 
> Maybe do a stsch and then check/dump the contents of the schib again?
> 
> Background: The architecture allows that msch returns success, but that
> the fields modified by the issuer remain unchanged at the subchannel
> regardless. That should not happen with QEMU; but I remember versions
> of z/VM where we sometimes had to call msch twice to make changes stick.

OK, thanks, good advice

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Mon, 2 Dec 2019 18:53:16 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-02 15:22, Cornelia Huck wrote:
> > On Thu, 28 Nov 2019 13:46:04 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> +static int test_device_sid;
> >> +
> >> +static void test_enumerate(void)
> >> +{
> >> +	struct pmcw *pmcw = &schib.pmcw;
> >> +	int sid;
> >> +	int ret, i;
> >> +	int found = 0;
> >> +
> >> +	for (sid = 0; sid < 0xffff; sid++) {
> >> +		ret = stsch(sid|SID_ONE, &schib);  
> > 
> > This seems a bit odd. You are basically putting the subchannel number
> > into sid, OR in the one, and then use the resulting value as the sid
> > (subchannel identifier).
> >   
> >> +		if (!ret && (pmcw->flags & PMCW_DNV)) {
> >> +			report_info("SID %04x Type %s PIM %x", sid,  
> > 
> > That's not a sid, but the subchannel number (see above).
> >   
> >> +				     Channel_type[pmcw->st], pmcw->pim);
> >> +			for (i = 0; i < 8; i++)  {
> >> +				if ((pmcw->pim << i) & 0x80) {
> >> +					report_info("CHPID[%d]: %02x", i,
> >> +						    pmcw->chpid[i]);
> >> +					break;
> >> +				}
> >> +			}
> >> +			found++;
> >> +	
> >> +		}  
> > 
> > Here, you iterate over the 0-0xffff range, even if you got a condition
> > code 3 (indicating no more subchannels in that set). Is that
> > intentional?  
> 
> I thought there could be more subchannels.
> I need then a break in the loop when this happens.
> I will reread the PoP to see how to find that no more subchannel are in 
> that set.

The fact that cc 3 for stsch == no more subchannels is unfortunately a
bit scattered across the PoP :/ Dug it out some time ago, maybe it's
still in the archives somewhere...

> 
> >   
> >> +		if (found && !test_device_sid)
> >> +			test_device_sid = sid|SID_ONE;  
> > 
> > You set test_device_sid to the last valid subchannel? Why?  
> 
> The last ? I wanted the first one

It is indeed the first one, -ENOCOFFEE.

> 
> I wanted something easy but I should have explain.
> 
> To avoid doing complicated things like doing a sense on each valid 
> subchannel I just take the first one.
> Should be enough as we do not go to the device in this test.

Yes; but you plan to reuse that code, don't you?

> 
> >   
> >> +	}
> >> +	if (!found) {
> >> +		report("Found %d devices", 0, found);

Now that I look at this again: If you got here, you always found 0
devices, so that message is not super helpful :)

> >> +		return;
> >> +	}
> >> +	ret = stsch(test_device_sid, &schib);  
> > 
> > Why do you do a stsch() again?  
> 
> right, no need.
> In an internal version I used to print some informations from the SCHIB.
> Since in between I overwrote the SHIB, I did it again.
> But in this version; no need.

You could copy the schib of the subchannel to be tested to a different
place, but I'm not sure it's worth it.

> 
> >   
> >> +	if (ret) {
> >> +		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
> >> +		return;
> >> +	}
> >> +	report("Tested", 1);
> >> +	return;  
> > 
> > I don't think you need this return statement.  
> 
> right I have enough work. :)
> 
> > 
> > Your test only enumerates devices in the first subchannel set. Do you
> > plan to enhance the test to enable the MSS facility and iterate over
> > all subchannel sets?  
> 
> Yes, it is something we can do in a following series

Sure, just asked out of interest :)

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

[Pierre Morel]

On 2019-12-02 15:55, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 13:46:06 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> 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 expect a device with a control unit type of 0xC0CA.
> 
> s/expect/expects/

... :(

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h |  13 +++++
>>   s390x/css.c     | 137 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 150 insertions(+)
>>
> 
> (...)
> 
>> diff --git a/s390x/css.c b/s390x/css.c
>> index e42dc2f..534864f 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -11,12 +11,28 @@
>>    */
>>   
>>   #include <libcflat.h>
>> +#include <alloc_phys.h>
>> +#include <asm/page.h>
>> +#include <string.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/arch_def.h>
>> +#include <asm/clock.h>
>>   
>>   #include <css.h>
>>   
>>   #define SID_ONE		0x00010000
>> +#define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
>> +
>> +struct lowcore *lowcore = (void *)0x0;
>>   
>>   static struct schib schib;
>> +#define NB_CCW  100
> 
> s/NB_CCW/NUM_CCWS/ ?
> 
> I was scratching my head a bit when I first saw that define.

French and english.... sorry
of course better, I change it

> 
>> +static struct ccw ccw[NB_CCW];
>> +#define NB_ORB  100
>> +static struct orb orb[NB_ORB];
>> +static struct irb irb;
>> +static char buffer[0x1000] __attribute__ ((aligned(8)));
>> +static struct senseid senseid;
>>   
>>   static const char *Channel_type[3] = {
>>   	"I/O", "CHSC", "MSG"
>> @@ -24,6 +40,34 @@ static const char *Channel_type[3] = {
>>   
>>   static int test_device_sid;
>>   
> 
> (...)
> 
>> +void handle_io_int(void)
>> +{
>> +	int ret = 0;
>> +	char *flags;
>> +
>> +	report_prefix_push("Interrupt");
>> +	if (lowcore->io_int_param != 0xcafec0ca) {
>> +		report("Bad io_int_param: %x", 0, lowcore->io_int_param);
>> +		report_prefix_pop();
>> +		return;
>> +	}
> 
> Should you accommodate unsolicited interrupts as well?

Yet, I do not expect unsolicited interrupt.
But should be at least kept in mind.

May be I add this for v3.

> 
>> +	report("io_int_param: %x", 1, lowcore->io_int_param);
>> +	report_prefix_pop();
>> +
>> +	ret = tsch(lowcore->subsys_id_word, &irb);
>> +	dump_irb(&irb);
>> +	flags = dump_scsw_flags(irb.scsw.ctrl);
>> +
>> +	if (ret)
>> +		report("IRB scsw flags: %s", 0, flags);
>> +	else
>> +		report("IRB scsw flags: %s", 1, flags);
>> +	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];
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return 0;
>> +	}
>> +	ret = stsch(test_device_sid, &schib);
> 
> That schib is a global variable, isn't it? Why do you need to re-check?

In the principe the previous tests, storing the SHIB could have been 
disabled.

> 
>> +	if (ret) {
>> +		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
>> +		return 0;
>> +	}
>> +	if (!(p->flags & PMCW_ENABLE)) {
>> +		report_skip("Device (sid %08x) not enabled", test_device_sid);
>> +		return 0;
>> +	}
>> +	ccw[0].code = code ;
> 
> Extra ' ' before ';'

yes, thanks

> 
>> +	ccw[0].flags = CCW_F_PCI;
> 
> Huh, what's that PCI for? 

Program Control Interruption

I will add a comment :)

> 
>> +	ccw[0].count = count;
>> +	ccw[0].data = (int)(unsigned long)data;
> 
> Can you be sure that data is always below 2G?

Currently yes, the program is loaded at 0x10000 and is quite small
also doing a test does not hurt for the case the function is used in 
another test someday.

> 
>> +	orb_p->intparm = 0xcafec0ca;
>> +	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
>> +	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
>> +
>> +	report_prefix_push("Start Subchannel");
>> +	ret = ssch(test_device_sid, orb_p);
>> +	if (ret) {
>> +		report("ssch cc=%d", 0, ret);
>> +		report_prefix_pop();
>> +		return 0;
>> +	}
>> +	report_prefix_pop();
>> +	return 1;
>> +}
>> +
>> +static void test_sense(void)
>> +{
>> +	int success;
>> +
>> +	enable_io_irq();
>> +
>> +	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
>> +	if (!success) {
>> +		report("start_subchannel failed", 0);
>> +		return;
>> +	}
>> +
>> +	senseid.cu_type = buffer[2] | (buffer[1] << 8);
>> +	delay(1000);
>> +
>> +	/* Sense ID is non packed cut_type is at offset +1 byte */
>> +	if (senseid.cu_type == PONG_CU)
>> +		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
>> +	else
>> +		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
>> +}
> 
> I'm not really convinced by that logic here. This will fall apart if
> you don't have your pong device exactly in the right place, and it does
> not make it easy to extend this for more devices in the future.

Wanted to keep things simple. PONG must be the first valid channel.
also, should be documented at least.

> 
> What about the following:
> - do a stsch() loop (basically re-use your first patch)
> - for each I/O subchannel with dnv=1, do SenseID
> - use the first (?) device with a c0ca CU type as your test device
> 
> Or maybe I'm overthinking this? It just does not strike me as very
> robust and reusable.

I can do it.

Thanks for the comments,

Best regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 16:03, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 13:46:07 +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.
> 
> "We want to test some read/write ccws via the SSCH instruction with a
> QEMU device with control unit type 0xC0CA." ?
> 
>>
>> This type of device respond to PONG_WRITE requests by incrementing an
> 
> s/respond/responds/

oiiinnnn..... but yes

> 
>> integer, stored as a string at offset 0 of the CCW data.
>>
>> This is only a success test, no error expected.
> 
> Nobody expects the Spanish Inquisition^W^W^W an error :)
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index 534864f..0761e70 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -23,6 +23,10 @@
>>   #define SID_ONE		0x00010000
>>   #define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
>>   
>> +/* Local Channel Commands */
> 
> /* Channel commands for the PONG device */
> 
> ?

better, thanks

> 
>> +#define PONG_WRITE	0x21 /* Write */
>> +#define PONG_READ	0x22 /* Read buffer */
>> +
>>   struct lowcore *lowcore = (void *)0x0;
>>   
>>   static struct schib schib;
>> @@ -31,7 +35,8 @@ static struct ccw ccw[NB_CCW];
>>   #define NB_ORB  100
>>   static struct orb orb[NB_ORB];
>>   static struct irb irb;
>> -static char buffer[0x1000] __attribute__ ((aligned(8)));
>> +#define BUF_SZ	0x1000
>> +static char buffer[BUF_SZ] __attribute__ ((aligned(8)));
> 
> Merge this with the introduction of this variable?

yes, better too

> 
>>   static struct senseid senseid;
>>   
>>   static const char *Channel_type[3] = {
>> @@ -224,6 +229,44 @@ static void test_sense(void)
>>   		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
>>   }
>>   
>> +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");
> 
> :D
> 
>> +		return;
>> +	}
>> +
>> +	enable_io_irq();
> 
> Hasn't that been enabled already for doing SenseID?

Yes, but same remark as before, the sub tests here are independant, 
started from the test[] table.
If the sense test is commented out, the...

> 
>> +
>> +	while (cnt++ < max) {
>> +report_info("cnt..: %08x", cnt);
> 
> Wrong indentation?

wrong report !
forgotten from a test.

Thanks for the review,
Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 19:15, Cornelia Huck wrote:
> On Mon, 2 Dec 2019 18:53:16 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2019-12-02 15:22, Cornelia Huck wrote:
>>> On Thu, 28 Nov 2019 13:46:04 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> +static int test_device_sid;
>>>> +
>>>> +static void test_enumerate(void)
>>>> +{
>>>> +	struct pmcw *pmcw = &schib.pmcw;
>>>> +	int sid;
>>>> +	int ret, i;
>>>> +	int found = 0;
>>>> +
>>>> +	for (sid = 0; sid < 0xffff; sid++) {
>>>> +		ret = stsch(sid|SID_ONE, &schib);
>>>
>>> This seems a bit odd. You are basically putting the subchannel number
>>> into sid, OR in the one, and then use the resulting value as the sid
>>> (subchannel identifier).
>>>    
>>>> +		if (!ret && (pmcw->flags & PMCW_DNV)) {
>>>> +			report_info("SID %04x Type %s PIM %x", sid,
>>>
>>> That's not a sid, but the subchannel number (see above).
>>>    
>>>> +				     Channel_type[pmcw->st], pmcw->pim);
>>>> +			for (i = 0; i < 8; i++)  {
>>>> +				if ((pmcw->pim << i) & 0x80) {
>>>> +					report_info("CHPID[%d]: %02x", i,
>>>> +						    pmcw->chpid[i]);
>>>> +					break;
>>>> +				}
>>>> +			}
>>>> +			found++;
>>>> +	
>>>> +		}
>>>
>>> Here, you iterate over the 0-0xffff range, even if you got a condition
>>> code 3 (indicating no more subchannels in that set). Is that
>>> intentional?
>>
>> I thought there could be more subchannels.
>> I need then a break in the loop when this happens.
>> I will reread the PoP to see how to find that no more subchannel are in
>> that set.
> 
> The fact that cc 3 for stsch == no more subchannels is unfortunately a
> bit scattered across the PoP :/ Dug it out some time ago, maybe it's
> still in the archives somewhere...

So the the subchannel are always one after the other?

> 
>>
>>>    
>>>> +		if (found && !test_device_sid)
>>>> +			test_device_sid = sid|SID_ONE;
>>>
>>> You set test_device_sid to the last valid subchannel? Why?
>>
>> The last ? I wanted the first one
> 
> It is indeed the first one, -ENOCOFFEE.

Would never happend to me.


> 
>>
>> I wanted something easy but I should have explain.
>>
>> To avoid doing complicated things like doing a sense on each valid
>> subchannel I just take the first one.
>> Should be enough as we do not go to the device in this test.
> 
> Yes; but you plan to reuse that code, don't you?

yes, so I must think about this

> 
>>
>>>    
>>>> +	}
>>>> +	if (!found) {
>>>> +		report("Found %d devices", 0, found);
> 
> Now that I look at this again: If you got here, you always found 0
> devices, so that message is not super helpful :)

yes, found is too much.
A cut and past from the time I was happy to find even one! :)

> 
>>>> +		return;
>>>> +	}
>>>> +	ret = stsch(test_device_sid, &schib);
>>>
>>> Why do you do a stsch() again?
>>
>> right, no need.
>> In an internal version I used to print some informations from the SCHIB.
>> Since in between I overwrote the SHIB, I did it again.
>> But in this version; no need.
> 
> You could copy the schib of the subchannel to be tested to a different
> place, but I'm not sure it's worth it.
> 
>>
>>>    
>>>> +	if (ret) {
>>>> +		report("Err %d on stsch on sid %08x", 0, ret, test_device_sid);
>>>> +		return;
>>>> +	}
>>>> +	report("Tested", 1);
>>>> +	return;
>>>
>>> I don't think you need this return statement.
>>
>> right I have enough work. :)
>>
>>>
>>> Your test only enumerates devices in the first subchannel set. Do you
>>> plan to enhance the test to enable the MSS facility and iterate over
>>> all subchannel sets?
>>
>> Yes, it is something we can do in a following series
> 
> Sure, just asked out of interest :)
> 

Thanks,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Mon, 2 Dec 2019 19:33:59 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-02 19:15, Cornelia Huck wrote:
> > On Mon, 2 Dec 2019 18:53:16 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> On 2019-12-02 15:22, Cornelia Huck wrote:  
> >>> On Thu, 28 Nov 2019 13:46:04 +0100
> >>> Pierre Morel <pmorel@linux.ibm.com> wrote:  
> >   
> >>>> +static int test_device_sid;
> >>>> +
> >>>> +static void test_enumerate(void)
> >>>> +{
> >>>> +	struct pmcw *pmcw = &schib.pmcw;
> >>>> +	int sid;
> >>>> +	int ret, i;
> >>>> +	int found = 0;
> >>>> +
> >>>> +	for (sid = 0; sid < 0xffff; sid++) {
> >>>> +		ret = stsch(sid|SID_ONE, &schib);  
> >>>
> >>> This seems a bit odd. You are basically putting the subchannel number
> >>> into sid, OR in the one, and then use the resulting value as the sid
> >>> (subchannel identifier).
> >>>      
> >>>> +		if (!ret && (pmcw->flags & PMCW_DNV)) {
> >>>> +			report_info("SID %04x Type %s PIM %x", sid,  
> >>>
> >>> That's not a sid, but the subchannel number (see above).
> >>>      
> >>>> +				     Channel_type[pmcw->st], pmcw->pim);
> >>>> +			for (i = 0; i < 8; i++)  {
> >>>> +				if ((pmcw->pim << i) & 0x80) {
> >>>> +					report_info("CHPID[%d]: %02x", i,
> >>>> +						    pmcw->chpid[i]);
> >>>> +					break;
> >>>> +				}
> >>>> +			}
> >>>> +			found++;
> >>>> +	
> >>>> +		}  
> >>>
> >>> Here, you iterate over the 0-0xffff range, even if you got a condition
> >>> code 3 (indicating no more subchannels in that set). Is that
> >>> intentional?  
> >>
> >> I thought there could be more subchannels.
> >> I need then a break in the loop when this happens.
> >> I will reread the PoP to see how to find that no more subchannel are in
> >> that set.  
> > 
> > The fact that cc 3 for stsch == no more subchannels is unfortunately a
> > bit scattered across the PoP :/ Dug it out some time ago, maybe it's
> > still in the archives somewhere...  
> 
> So the the subchannel are always one after the other?

While QEMU (and z/VM) usually do that, they can really be scattered
around. For the in-between I/O subchannels that don't lead to a device,
you'll still get cc 0, it's just the dnv bit that is 0. The cc 3
basically just tells you that you can stop looking.

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

[Cornelia Huck]

On Mon, 2 Dec 2019 19:18:20 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2019-12-02 15:55, Cornelia Huck wrote:
> > On Thu, 28 Nov 2019 13:46:06 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> +	ccw[0].code = code ;  
> > 
> > Extra ' ' before ';'  
> 
> yes, thanks
> 
> >   
> >> +	ccw[0].flags = CCW_F_PCI;  
> > 
> > Huh, what's that PCI for?   
> 
> Program Control Interruption

Yes; but why do you need it? Doesn't the QEMU device provide you with
an interrupt for the final status? I don't think PCI makes sense unless
you want a notification for the progress through a chain.

> 
> I will add a comment :)

Good idea; the PCI is bound to confuse people :)

> 
> >   
> >> +	ccw[0].count = count;
> >> +	ccw[0].data = (int)(unsigned long)data;  
> > 
> > Can you be sure that data is always below 2G?  
> 
> Currently yes, the program is loaded at 0x10000 and is quite small
> also doing a test does not hurt for the case the function is used in 
> another test someday.

Nod.

> 
> >   
> >> +	orb_p->intparm = 0xcafec0ca;
> >> +	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> >> +	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
> >> +
> >> +	report_prefix_push("Start Subchannel");
> >> +	ret = ssch(test_device_sid, orb_p);
> >> +	if (ret) {
> >> +		report("ssch cc=%d", 0, ret);
> >> +		report_prefix_pop();
> >> +		return 0;
> >> +	}
> >> +	report_prefix_pop();
> >> +	return 1;
> >> +}
> >> +
> >> +static void test_sense(void)
> >> +{
> >> +	int success;
> >> +
> >> +	enable_io_irq();
> >> +
> >> +	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
> >> +	if (!success) {
> >> +		report("start_subchannel failed", 0);
> >> +		return;
> >> +	}
> >> +
> >> +	senseid.cu_type = buffer[2] | (buffer[1] << 8);
> >> +	delay(1000);
> >> +
> >> +	/* Sense ID is non packed cut_type is at offset +1 byte */
> >> +	if (senseid.cu_type == PONG_CU)
> >> +		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
> >> +	else
> >> +		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
> >> +}  
> > 
> > I'm not really convinced by that logic here. This will fall apart if
> > you don't have your pong device exactly in the right place, and it does
> > not make it easy to extend this for more devices in the future.  
> 
> Wanted to keep things simple. PONG must be the first valid channel.
> also, should be documented at least.

Yes, please :)

> 
> > 
> > What about the following:
> > - do a stsch() loop (basically re-use your first patch)
> > - for each I/O subchannel with dnv=1, do SenseID
> > - use the first (?) device with a c0ca CU type as your test device
> > 
> > Or maybe I'm overthinking this? It just does not strike me as very
> > robust and reusable.  
> 
> I can do it.
> 
> Thanks for the comments,
> 
> Best regards,
> Pierre
> 

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

[Pierre Morel]

On 2019-12-02 20:49, Cornelia Huck wrote:
> On Mon, 2 Dec 2019 19:33:59 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2019-12-02 19:15, Cornelia Huck wrote:
>>> On Mon, 2 Dec 2019 18:53:16 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> On 2019-12-02 15:22, Cornelia Huck wrote:
>>>>> On Thu, 28 Nov 2019 13:46:04 +0100
>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>>>> +static int test_device_sid;
>>>>>> +
>>>>>> +static void test_enumerate(void)
>>>>>> +{
>>>>>> +	struct pmcw *pmcw = &schib.pmcw;
>>>>>> +	int sid;
>>>>>> +	int ret, i;
>>>>>> +	int found = 0;
>>>>>> +
>>>>>> +	for (sid = 0; sid < 0xffff; sid++) {
>>>>>> +		ret = stsch(sid|SID_ONE, &schib);
>>>>>
>>>>> This seems a bit odd. You are basically putting the subchannel number
>>>>> into sid, OR in the one, and then use the resulting value as the sid
>>>>> (subchannel identifier).
>>>>>       
>>>>>> +		if (!ret && (pmcw->flags & PMCW_DNV)) {
>>>>>> +			report_info("SID %04x Type %s PIM %x", sid,
>>>>>
>>>>> That's not a sid, but the subchannel number (see above).
>>>>>       
>>>>>> +				     Channel_type[pmcw->st], pmcw->pim);
>>>>>> +			for (i = 0; i < 8; i++)  {
>>>>>> +				if ((pmcw->pim << i) & 0x80) {
>>>>>> +					report_info("CHPID[%d]: %02x", i,
>>>>>> +						    pmcw->chpid[i]);
>>>>>> +					break;
>>>>>> +				}
>>>>>> +			}
>>>>>> +			found++;
>>>>>> +	
>>>>>> +		}
>>>>>
>>>>> Here, you iterate over the 0-0xffff range, even if you got a condition
>>>>> code 3 (indicating no more subchannels in that set). Is that
>>>>> intentional?
>>>>
>>>> I thought there could be more subchannels.
>>>> I need then a break in the loop when this happens.
>>>> I will reread the PoP to see how to find that no more subchannel are in
>>>> that set.
>>>
>>> The fact that cc 3 for stsch == no more subchannels is unfortunately a
>>> bit scattered across the PoP :/ Dug it out some time ago, maybe it's
>>> still in the archives somewhere...
>>
>> So the the subchannel are always one after the other?
> 
> While QEMU (and z/VM) usually do that, they can really be scattered
> around. For the in-between I/O subchannels that don't lead to a device,
> you'll still get cc 0, it's just the dnv bit that is 0. The cc 3
> basically just tells you that you can stop looking.
> 

Thanks for the explanation, I will change the code accordingly.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 20:54, Cornelia Huck wrote:
> On Mon, 2 Dec 2019 19:18:20 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2019-12-02 15:55, Cornelia Huck wrote:
>>> On Thu, 28 Nov 2019 13:46:06 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> +	ccw[0].code = code ;
>>>
>>> Extra ' ' before ';'
>>
>> yes, thanks
>>
>>>    
>>>> +	ccw[0].flags = CCW_F_PCI;
>>>
>>> Huh, what's that PCI for?
>>
>> Program Control Interruption
> 
> Yes; but why do you need it? Doesn't the QEMU device provide you with
> an interrupt for the final status? I don't think PCI makes sense unless
> you want a notification for the progress through a chain.

Right, but this is for a later test.
So I do not need it. Will remove it.

> 
>>
>> I will add a comment :)
> 
> Good idea; the PCI is bound to confuse people :)
> 
>>
>>>    
>>>> +	ccw[0].count = count;
>>>> +	ccw[0].data = (int)(unsigned long)data;
>>>
>>> Can you be sure that data is always below 2G?
>>
>> Currently yes, the program is loaded at 0x10000 and is quite small
>> also doing a test does not hurt for the case the function is used in
>> another test someday.
> 
> Nod.

Will do.

> 
>>
>>>    
>>>> +	orb_p->intparm = 0xcafec0ca;
>>>> +	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
>>>> +	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
>>>> +
>>>> +	report_prefix_push("Start Subchannel");
>>>> +	ret = ssch(test_device_sid, orb_p);
>>>> +	if (ret) {
>>>> +		report("ssch cc=%d", 0, ret);
>>>> +		report_prefix_pop();
>>>> +		return 0;
>>>> +	}
>>>> +	report_prefix_pop();
>>>> +	return 1;
>>>> +}
>>>> +
>>>> +static void test_sense(void)
>>>> +{
>>>> +	int success;
>>>> +
>>>> +	enable_io_irq();
>>>> +
>>>> +	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
>>>> +	if (!success) {
>>>> +		report("start_subchannel failed", 0);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	senseid.cu_type = buffer[2] | (buffer[1] << 8);
>>>> +	delay(1000);
>>>> +
>>>> +	/* Sense ID is non packed cut_type is at offset +1 byte */
>>>> +	if (senseid.cu_type == PONG_CU)
>>>> +		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
>>>> +	else
>>>> +		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
>>>> +}
>>>
>>> I'm not really convinced by that logic here. This will fall apart if
>>> you don't have your pong device exactly in the right place, and it does
>>> not make it easy to extend this for more devices in the future.
>>
>> Wanted to keep things simple. PONG must be the first valid channel.
>> also, should be documented at least.
> 
> Yes, please :)
> 

Thanks.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2019-12-02 19:25, Pierre Morel wrote:
> 
> 
> On 2019-12-02 16:03, Cornelia Huck wrote:
>> On Thu, 28 Nov 2019 13:46:07 +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.
>>
>> "We want to test some read/write ccws via the SSCH instruction with a
>> QEMU device with control unit type 0xC0CA." ?
>>
>>>
>>> This type of device respond to PONG_WRITE requests by incrementing an
>>
>> s/respond/responds/
> 
> oiiinnnn..... but yes
> 
>>
>>> integer, stored as a string at offset 0 of the CCW data.
>>>
>>> This is only a success test, no error expected.
>>
>> Nobody expects the Spanish Inquisition^W^W^W an error :)
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   s390x/css.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/s390x/css.c b/s390x/css.c
>>> index 534864f..0761e70 100644
>>> --- a/s390x/css.c
>>> +++ b/s390x/css.c
>>> @@ -23,6 +23,10 @@
>>>   #define SID_ONE        0x00010000
>>>   #define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
>>> +/* Local Channel Commands */
>>
>> /* Channel commands for the PONG device */
>>
>> ?
> 
> better, thanks
> 
>>
>>> +#define PONG_WRITE    0x21 /* Write */
>>> +#define PONG_READ    0x22 /* Read buffer */
>>> +
>>>   struct lowcore *lowcore = (void *)0x0;
>>>   static struct schib schib;
>>> @@ -31,7 +35,8 @@ static struct ccw ccw[NB_CCW];
>>>   #define NB_ORB  100
>>>   static struct orb orb[NB_ORB];
>>>   static struct irb irb;
>>> -static char buffer[0x1000] __attribute__ ((aligned(8)));
>>> +#define BUF_SZ    0x1000
>>> +static char buffer[BUF_SZ] __attribute__ ((aligned(8)));
>>
>> Merge this with the introduction of this variable?
> 
> yes, better too
> 
>>
>>>   static struct senseid senseid;
>>>   static const char *Channel_type[3] = {
>>> @@ -224,6 +229,44 @@ static void test_sense(void)
>>>           report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
>>>   }
>>> +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");
>>
>> :D
>>
>>> +        return;
>>> +    }
>>> +
>>> +    enable_io_irq();
>>
>> Hasn't that been enabled already for doing SenseID?
> 
> Yes, but same remark as before, the sub tests here are independant, 
> started from the test[] table.
> If the sense test is commented out, the...

hum, coming back here again, the tests depends one from the other and 
are absolutely not independent as I pretended. so...
You are right I do not need to enable IRQ here.

> 
>>
>>> +
>>> +    while (cnt++ < max) {
>>> +report_info("cnt..: %08x", cnt);
>>
>> Wrong indentation?
> 
> wrong report !
> forgotten from a test.
> 
> Thanks for the review,
> Regards,
> Pierre
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen