[kvm-unit-tests PATCH v7 00/12] s390x: Testing the Channel Subsystem I/O

[kvm-unit-tests PATCH v7 00/12] s390x: Testing the Channel Subsystem I/O

[Pierre Morel]

Goal of the series is to have a framework to test Channel-Subsystem I/O with
QEMU/KVM.
  
To be able to support interrupt for CSS I/O and for SCLP we need to modify
the interrupt framework to allow re-entrant interruptions.
  
We add a registration for IRQ callbacks to the test program to define its own
interrupt handler. We need to do special work under interrupt like acknowledging
the interrupt.
  
Being working on PSW bits to allow I/O interrupt, we define new PSW bits
in arch_def.h and use __ASSEMBLER__ define to be able to include this header
in an assembler source file.

This series presents four major tests:
- Enumeration:
        The CSS is enumerated using the STSCH instruction recursively on all
        potentially existing channels.
        Keeping the first channel found as a reference for future use.
        Checks STSCH
 
- Enable:
        If the enumeration succeeded the tests enables the reference
        channel with MSCH and verifies with STSCH that the channel is
        effectively enabled, retrying a predefined count on failure
	to enable the channel
        Checks MSCH       
 
- Sense:
        If the channel is enabled this test sends a SENSE_ID command
        to the reference channel, analyzing the answer and expecting
        the Control unit type being 0xc0ca
        Checks SSCH(READ) and IO-IRQ

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


Note:
- The following patches may be pulled first:
  s390x: saving regs for interrupts
  s390x: Use PSW bits definitions in cstart
  s390x: Move control register bit definitions and add AFP to them
  s390x: export the clock get_clock_ms() utility
  s390x: use get_clock_ms() to calculate a delay in ms

- I think this one if it receives reviewed-by can also be pulled now:
  s390x: define function to wait for interrupt

- this patch has a comment from Janosch who asks change so... need opinion:
  but since I need reviews for the next patches I let it here unchanged.
  s390x: interrupt registration

- These 5 patches are really I/O oriented and need reviewed-by:
  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

Regards,
Pierre

Pierre Morel (12):
  s390x: saving regs for interrupts
  s390x: Use PSW bits definitions in cstart
  s390x: Move control register bit definitions and add AFP to them
  s390x: interrupt registration
  s390x: export the clock get_clock_ms() utility
  s390x: use get_clock_ms() to calculate a delay in ms
  s390x: Library resources for CSS tests
  s390x: css: stsch, enumeration test
  s390x: css: msch, enable test
  s390x: define function to wait for interrupt
  s390x: css: ssch/tsch with sense and interrupt
  s390x: css: ping pong

 lib/s390x/asm/arch_def.h |  32 +++-
 lib/s390x/asm/time.h     |  36 ++++
 lib/s390x/css.h          | 279 ++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c     | 157 +++++++++++++++++
 lib/s390x/css_lib.c      |  55 ++++++
 lib/s390x/interrupt.c    |  23 ++-
 lib/s390x/interrupt.h    |   8 +
 s390x/Makefile           |   3 +
 s390x/css.c              | 355 +++++++++++++++++++++++++++++++++++++++
 s390x/cstart64.S         |  58 +++++--
 s390x/intercept.c        |  11 +-
 s390x/unittests.cfg      |   4 +
 12 files changed, 995 insertions(+), 26 deletions(-)
 create mode 100644 lib/s390x/asm/time.h
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c
 create mode 100644 lib/s390x/css_lib.c
 create mode 100644 lib/s390x/interrupt.h
 create mode 100644 s390x/css.c

-- 
2.25.1

Changelog:

from v6 to v7

* s390x: saving regs for interrupts
- macro name modificatio for SAVE_REGS_STACK
  (David)
- saving the FPC
  (David)

* s390x: Use PSW bits definitions in cstart
- suppress definition for PSW_RESET_MASK
  use PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW
  (David)

* s390x: Library resources for CSS tests
* s390x: css: stsch, enumeration test
  move library definitions from stsch patche
  to the Library patch
  add the library to the s390 Makefile here
  (Janosch)
  
* s390x: css: msch, enable test
  Add retries when enable fails
  (Connie)
  Re-introduce the patches for delay implementation
  to add a delay between retries

* s390x: define function to wait for interrupt
  Changed name from wfi to wait_for_interrupt
  (Janosch)

* s390x: css: ssch/tsch with sense and interrupt
  add a flag parameter to ssch and use it to add
  SLI (Suppress Length Indication) flag for SENSE_ID
  (Connie)


from v5 to v6
- Added comments for IRQ handling in
  s390x: saving regs for interrupts
  (Janosch) 

- fixed BUG on reset_psw, added PSW_MASK_PSW_SHORT
  and PSW_RESET_MASK

- fixed several lines over 80 chars

- fixed licenses, use GPL V2 (no more LGPL)

- replacing delay() with wfi() (Wait For Interrupt)
  during the css tests
  (suggested by Connie's comments)

- suppressing delay() induces suppressing the patch
  "s390x: export the clock get_clock_ms() utility"
  which is already reviewed but can be picked from
  the v5 series.

- changed the logic of the tests, the 4 css tests
  are always run one after the other so no need to 
  re-run enumeration and enabling at the begining
  of each tests, it has alredy been done.
  This makes code simpler.

from v4 to v5
- add a patch to explicitely define the initial_cr0
  value
  (Janosch)
- add RB from Janosh on interrupt registration
- several formating, typo correction and removing
  unnecessary initialization in "linrary resources..."
  (Janosch)
- several formating and typo corrections on
  "stsch enumeration test"
  (Connie)
- reworking the msch test
  (Connie)
- reworking of ssch test, pack the sense-id structure
  (Connie)

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

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

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

[kvm-unit-tests PATCH v7 01/12] s390x: saving regs for interrupts

[Pierre Morel]

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

Instead of saving at a static memory address, let's save the base
registers, the floating point registers and the floating point
control register on the stack in case of I/O interrupts

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

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 9af6bb3..3c7d8a9 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -118,6 +118,43 @@ memsetxc:
 	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	.endm
 
+/* Save registers on the stack (r15), so we can have stacked interrupts. */
+	.macro SAVE_REGS_STACK
+	/* Allocate a stack frame for 15 general registers */
+	slgfi   %r15, 15 * 8
+	/* Store all registers from r0 to r14 on the stack */
+	stmg    %r0, %r14, 0(%r15)
+	/* Allocate a stack frame for 16 floating point registers */
+	/* The size of a FP register is the size of an double word */
+	slgfi   %r15, 16 * 8
+	/* Save fp register on stack: offset to SP is multiple of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8(%r15)
+	.endr
+	/* Save fpc, but keep stack aligned on 64bits */
+	slgfi   %r15, 8
+	efpc	%r0
+	stg	%r0, 0(%r15)
+	.endm
+
+/* Restore the register in reverse order */
+	.macro RESTORE_REGS_STACK
+	/* Restore fpc */
+	lfpc	0(%r15)
+	algfi	%r15, 8
+	/* Restore fp register from stack: SP still where it was left */
+	/* and offset to SP is a multile of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8(%r15)
+	.endr
+	/* Now it is done, rewind the stack pointer by 16 double word */
+	algfi   %r15, 16 * 8
+	/* Load the registers from stack */
+	lmg     %r0, %r14, 0(%r15)
+	/* Rewind the stack by 15 double word */
+	algfi   %r15, 15 * 8
+	.endm
+
 .section .text
 /*
  * load_reset calling convention:
@@ -182,9 +219,9 @@ mcck_int:
 	lpswe	GEN_LC_MCCK_OLD_PSW
 
 io_int:
-	SAVE_REGS
+	SAVE_REGS_STACK
 	brasl	%r14, handle_io_int
-	RESTORE_REGS
+	RESTORE_REGS_STACK
 	lpswe	GEN_LC_IO_OLD_PSW
 
 svc_int:
-- 
2.25.1

[kvm-unit-tests PATCH v7 02/12] s390x: Use PSW bits definitions in cstart

[Pierre Morel]

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

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 15 +++++++++++----
 s390x/cstart64.S         | 15 ++++++++-------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 15a4d49..820af93 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,15 +10,21 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_EXT			0x0100000000000000UL
+#define PSW_MASK_DAT			0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_PSTATE			0x0001000000000000UL
+#define PSW_MASK_BA			0x0000000080000000UL
+#define PSW_MASK_EA			0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
+
+#ifndef __ASSEMBLER__
 struct psw {
 	uint64_t	mask;
 	uint64_t	addr;
 };
 
-#define PSW_MASK_EXT			0x0100000000000000UL
-#define PSW_MASK_DAT			0x0400000000000000UL
-#define PSW_MASK_PSTATE			0x0001000000000000UL
-
 #define CR0_EXTM_SCLP			0X0000000000000200UL
 #define CR0_EXTM_EXTC			0X0000000000002000UL
 #define CR0_EXTM_EMGC			0X0000000000004000UL
@@ -297,4 +303,5 @@ static inline uint32_t get_prefix(void)
 	return current_prefix;
 }
 
+#endif /* __ASSEMBLER */
 #endif
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 3c7d8a9..e890568 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -12,6 +12,7 @@
  */
 #include <asm/asm-offsets.h>
 #include <asm/sigp.h>
+#include <asm/arch_def.h>
 
 .section .init
 
@@ -232,19 +233,19 @@ svc_int:
 
 	.align	8
 reset_psw:
-	.quad	0x0008000180000000
+	.quad	PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW
 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.25.1

[kvm-unit-tests PATCH v7 03/12] s390x: Move control register bit definitions and add AFP to them

[Pierre Morel]

While adding the definition for the AFP-Register control bit, move all
existing definitions for CR0 out of the C zone to the assmbler zone to
keep the definitions concerning CR0 together.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/asm/arch_def.h | 11 ++++++-----
 s390x/cstart64.S         |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 820af93..54ffd0b 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -19,17 +19,18 @@
 
 #define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
 
+#define CR0_EXTM_SCLP			0X0000000000000200UL
+#define CR0_EXTM_EXTC			0X0000000000002000UL
+#define CR0_EXTM_EMGC			0X0000000000004000UL
+#define CR0_EXTM_MASK			0X0000000000006200UL
+#define CR0_AFP_REG_CRTL		0x0000000000040000UL
+
 #ifndef __ASSEMBLER__
 struct psw {
 	uint64_t	mask;
 	uint64_t	addr;
 };
 
-#define CR0_EXTM_SCLP			0X0000000000000200UL
-#define CR0_EXTM_EXTC			0X0000000000002000UL
-#define CR0_EXTM_EMGC			0X0000000000004000UL
-#define CR0_EXTM_MASK			0X0000000000006200UL
-
 struct lowcore {
 	uint8_t		pad_0x0000[0x0080 - 0x0000];	/* 0x0000 */
 	uint32_t	ext_int_param;			/* 0x0080 */
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index e890568..0679fce 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -248,4 +248,4 @@ svc_int_psw:
 	.quad	PSW_EXCEPTION_MASK, svc_int
 initial_cr0:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
-	.quad	0x0000000000040000
+	.quad	CR0_AFP_REG_CRTL
-- 
2.25.1

[kvm-unit-tests PATCH v7 04/12] s390x: interrupt registration

[Pierre Morel]

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

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

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 3a40cac..243b9c2 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -10,9 +10,9 @@
  * under the terms of the GNU Library General Public License version 2.
  */
 #include <libcflat.h>
-#include <asm/interrupt.h>
 #include <asm/barrier.h>
 #include <sclp.h>
+#include <interrupt.h>
 
 static bool pgm_int_expected;
 static bool ext_int_expected;
@@ -144,12 +144,33 @@ void handle_mcck_int(void)
 		     stap(), lc->mcck_old_psw.addr);
 }
 
+static void (*io_int_func)(void);
+
 void handle_io_int(void)
 {
+	if (io_int_func)
+		return io_int_func();
+
 	report_abort("Unexpected io interrupt: on cpu %d at %#lx",
 		     stap(), lc->io_old_psw.addr);
 }
 
+int register_io_int_func(void (*f)(void))
+{
+	if (io_int_func)
+		return -1;
+	io_int_func = f;
+	return 0;
+}
+
+int unregister_io_int_func(void (*f)(void))
+{
+	if (io_int_func != f)
+		return -1;
+	io_int_func = NULL;
+	return 0;
+}
+
 void handle_svc_int(void)
 {
 	report_abort("Unexpected supervisor call interrupt: on cpu %d at %#lx",
diff --git a/lib/s390x/interrupt.h b/lib/s390x/interrupt.h
new file mode 100644
index 0000000..323258d
--- /dev/null
+++ b/lib/s390x/interrupt.h
@@ -0,0 +1,8 @@
+#ifndef __INTERRUPT_H
+#define __INTERRUPT_H
+#include <asm/interrupt.h>
+
+int register_io_int_func(void (*f)(void));
+int unregister_io_int_func(void (*f)(void));
+
+#endif /* __INTERRUPT_H */
-- 
2.25.1

[kvm-unit-tests PATCH v7 05/12] s390x: export the clock get_clock_ms() utility

[Pierre Morel]

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/asm/time.h | 26 ++++++++++++++++++++++++++
 s390x/intercept.c    | 11 +----------
 2 files changed, 27 insertions(+), 10 deletions(-)
 create mode 100644 lib/s390x/asm/time.h

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
new file mode 100644
index 0000000..25c7a3c
--- /dev/null
+++ b/lib/s390x/asm/time.h
@@ -0,0 +1,26 @@
+/*
+ * Clock utilities for s390
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *
+ * Copied from the s390/intercept test by:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+#ifndef _ASM_S390X_TIME_H_
+#define _ASM_S390X_TIME_H_
+
+static inline uint64_t get_clock_ms(void)
+{
+	uint64_t clk;
+
+	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
+
+	/* Bit 51 is incrememented each microsecond */
+	return (clk >> (63 - 51)) / 1000;
+}
+
+#endif
diff --git a/s390x/intercept.c b/s390x/intercept.c
index 5f46b82..2e38257 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -14,6 +14,7 @@
 #include <asm/interrupt.h>
 #include <asm/page.h>
 #include <asm/facility.h>
+#include <asm/time.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
@@ -153,16 +154,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.25.1

[kvm-unit-tests PATCH v7 06/12] s390x: use get_clock_ms() to calculate a delay in ms

[Pierre Morel]

use get_clock_ms() to calculate a delay in ms

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/time.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
index 25c7a3c..931a119 100644
--- a/lib/s390x/asm/time.h
+++ b/lib/s390x/asm/time.h
@@ -23,4 +23,14 @@ static inline uint64_t get_clock_ms(void)
 	return (clk >> (63 - 51)) / 1000;
 }
 
+static inline void mdelay(unsigned long ms)
+{
+	unsigned long startclk;
+
+	startclk = get_clock_ms();
+	for (;;)
+		if (get_clock_ms() - startclk > ms)
+			break;
+}
+
 #endif
-- 
2.25.1

[kvm-unit-tests PATCH v7 07/12] s390x: Library resources for CSS tests

[Pierre Morel]

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

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h      | 259 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
 s390x/Makefile       |   1 +
 3 files changed, 417 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..e0d3a98
--- /dev/null
+++ b/lib/s390x/css.h
@@ -0,0 +1,259 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2020
+ * Author: Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef CSS_H
+#define CSS_H
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20
+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw1 {
+	unsigned char code;
+	unsigned char flags;
+	unsigned short count;
+	uint32_t data_address;
+} __attribute__ ((aligned(4)));
+
+#define SID_ONE		0x00010000
+
+#define ORB_M_KEY	0xf0000000
+#define ORB_F_SUSPEND	0x08000000
+#define ORB_F_STREAMING	0x04000000
+#define ORB_F_MODIFCTRL	0x02000000
+#define ORB_F_SYNC	0x01000000
+#define ORB_F_FORMAT	0x00800000
+#define ORB_F_PREFETCH	0x00400000
+#define ORB_F_INIT_IRQ	0x00200000
+#define ORB_F_ADDRLIMIT	0x00100000
+#define ORB_F_SUSP_IRQ	0x00080000
+#define ORB_F_TRANSPORT	0x00040000
+#define ORB_F_IDAW2	0x00020000
+#define ORB_F_IDAW_2K	0x00010000
+#define ORB_M_LPM	0x0000ff00
+#define ORB_F_LPM_DFLT	0x00008000
+#define ORB_F_ILSM	0x00000080
+#define ORB_F_CCW_IND	0x00000040
+#define ORB_F_ORB_EXT	0x00000001
+
+struct orb {
+	uint32_t intparm;
+	uint32_t ctrl;
+	uint32_t cpa;
+	uint32_t prio;
+	uint32_t reserved[4];
+} __attribute__ ((aligned(4)));
+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t ccw_addr;
+	uint8_t  dev_stat;
+	uint8_t  sch_stat;
+	uint16_t count;
+};
+
+struct pmcw {
+	uint32_t intparm;
+#define PMCW_DNV        0x0001
+#define PMCW_ENABLE     0x0080
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	uint32_t flags2;
+};
+#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
+
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint8_t  md[12];
+} __attribute__ ((aligned(4)));
+
+struct irb {
+	struct scsw scsw;
+	uint32_t esw[5];
+	uint32_t ecw[8];
+	uint32_t emw[8];
+} __attribute__ ((aligned(4)));
+
+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	ssch	0(%2)\n"
+		"	ipm	%0\n"
+		"	srl	%0,28\n"
+		: "=d" (cc)
+		: "d" (reg1), "a" (addr), "m" (*addr)
+		: "cc", "memory");
+	return cc;
+}
+
+static inline int stsch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	stsch	0(%3)\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int msch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	msch	0(%3)\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int tsch(unsigned long schid, struct irb *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	tsch	0(%3)\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");
+	return cc;
+}
+
+static inline int hsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	hsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int xsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	xsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int csch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	csch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int rsch(unsigned long schid)
+{
+	register unsigned long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(
+		"	rsch\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+static inline int rchp(unsigned long chpid)
+{
+	register unsigned long reg1 asm("1") = chpid;
+	int cc;
+
+	asm volatile(
+		"	rchp\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}
+
+/* Debug functions */
+char *dump_pmcw_flags(uint16_t f);
+char *dump_scsw_flags(uint32_t f);
+
+#ifdef DEBUG_CSS
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);
+struct ccw *dump_ccw(struct ccw *cp);
+#else
+static inline void dump_scsw(struct scsw *scsw) {}
+static inline void dump_irb(struct irb *irbp) {}
+static inline void dump_pmcw(struct pmcw *p) {}
+static inline void dump_schib(struct schib *sch) {}
+static inline void dump_orb(struct orb *op) {}
+static inline struct ccw *dump_ccw(struct ccw *cp)
+{
+	return NULL;
+}
+#endif /* DEBUG_CSS */
+#endif
diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
new file mode 100644
index 0000000..c1e8a53
--- /dev/null
+++ b/lib/s390x/css_dump.c
@@ -0,0 +1,157 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2020 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.
+ *
+ * Description:
+ * Provides the dumping functions for various structures used by subchannels:
+ * - ORB  : Operation request block, describes the I/O operation and points to
+ *          a CCW chain
+ * - CCW  : Channel Command Word, describes the command, data and flow control
+ * - IRB  : Interuption response Block, describes the result of an operation;
+ *          holds a SCSW and model-dependent data.
+ * - SCHIB: SubCHannel Information Block composed of:
+ *   - SCSW: SubChannel Status Word, status of the channel.
+ *   - PMCW: Path Management Control Word
+ * You need the QEMU ccw-pong device in QEMU to answer the I/O transfers.
+ */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+
+#undef DEBUG_CSS
+#include <css.h>
+
+/*
+ * Try to have a more human representation of the SCSW flags:
+ * each letter in the two strings represent the first
+ * letter of the associated bit in the flag fields.
+ */
+static const char *scsw_str = "kkkkslccfpixuzen";
+static const char *scsw_str2 = "1SHCrshcsdsAIPSs";
+static char scsw_line[64] = {};
+
+char *dump_scsw_flags(uint32_t f)
+{
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		if ((f << i) & 0x80000000)
+			scsw_line[i] = scsw_str[i];
+		else
+			scsw_line[i] = '_';
+	}
+	scsw_line[i] = ' ';
+	for (; i < 32; i++) {
+		if ((f << i) & 0x80000000)
+			scsw_line[i + 1] = scsw_str2[i - 16];
+		else
+			scsw_line[i + 1] = '_';
+	}
+	return scsw_line;
+}
+
+/*
+ * Try o have a more human representation of the PMCW flags
+ * each letter in the string represent the first
+ * letter of the associated bit in the flag fields.
+ */
+static const char *pmcw_str = "11iii111ellmmdtv";
+static char pcmw_line[32] = {};
+char *dump_pmcw_flags(uint16_t f)
+{
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		if ((f << i) & 0x8000)
+			pcmw_line[i] = pmcw_str[i];
+		else
+			pcmw_line[i] = '_';
+	}
+	return pcmw_line;
+}
+
+#ifdef DEBUG_CSS
+void dump_scsw(struct scsw *s)
+{
+	dump_scsw_flags(s->ctrl);
+	printf("scsw->flags: %s\n", line);
+	printf("scsw->addr : %08x\n", s->addr);
+	printf("scsw->devs : %02x\n", s->devs);
+	printf("scsw->schs : %02x\n", s->schs);
+	printf("scsw->count: %04x\n", s->count);
+}
+
+void dump_irb(struct irb *irbp)
+{
+	int i;
+	uint32_t *p = (uint32_t *)irbp;
+
+	dump_scsw(&irbp->scsw);
+	for (i = 0; i < sizeof(*irbp)/sizeof(*p); i++, p++)
+		printf("irb[%02x] : %08x\n", i, *p);
+}
+
+void dump_pmcw(struct pmcw *p)
+{
+	int i;
+
+	printf("pmcw->intparm  : %08x\n", p->intparm);
+	printf("pmcw->flags    : %04x\n", p->flags);
+	dump_pmcw_flags(p->flags);
+	printf("pmcw->devnum   : %04x\n", p->devnum);
+	printf("pmcw->lpm      : %02x\n", p->lpm);
+	printf("pmcw->pnom     : %02x\n", p->pnom);
+	printf("pmcw->lpum     : %02x\n", p->lpum);
+	printf("pmcw->pim      : %02x\n", p->pim);
+	printf("pmcw->mbi      : %04x\n", p->mbi);
+	printf("pmcw->pom      : %02x\n", p->pom);
+	printf("pmcw->pam      : %02x\n", p->pam);
+	printf("pmcw->mbi      : %04x\n", p->mbi);
+	for (i = 0; i < 8; i++)
+		printf("pmcw->chpid[%d]: %02x\n", i, p->chpid[i]);
+	printf("pmcw->flags2  : %08x\n", p->flags2);
+}
+
+void dump_schib(struct schib *sch)
+{
+	struct pmcw *p = &sch->pmcw;
+	struct scsw *s = &sch->scsw;
+
+	printf("--SCHIB--\n");
+	dump_pmcw(p);
+	dump_scsw(s);
+}
+
+struct ccw *dump_ccw(struct ccw *cp)
+{
+	printf("CCW: code: %02x flags: %02x count: %04x data: %08x\n", cp->code,
+	    cp->flags, cp->count, cp->data);
+
+	if (cp->code == CCW_C_TIC)
+		return (struct ccw *)(long)cp->data;
+
+	return (cp->flags & CCW_F_CC) ? cp + 1 : NULL;
+}
+
+void dump_orb(struct orb *op)
+{
+	struct ccw *cp;
+
+	printf("ORB: intparm : %08x\n", op->intparm);
+	printf("ORB: ctrl    : %08x\n", op->ctrl);
+	printf("ORB: prio    : %08x\n", op->prio);
+	cp = (struct ccw *)(long) (op->cpa);
+	while (cp)
+		cp = dump_ccw(cp);
+}
+
+#endif
diff --git a/s390x/Makefile b/s390x/Makefile
index ddb4b48..050c40b 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -51,6 +51,7 @@ cflatobjs += lib/s390x/sclp-console.o
 cflatobjs += lib/s390x/interrupt.o
 cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
+cflatobjs += lib/s390x/css_dump.o
 
 OBJDIRS += lib/s390x
 
-- 
2.25.1

[kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Pierre Morel]

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

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

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

diff --git a/s390x/Makefile b/s390x/Makefile
index 050c40b..baebf18 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -17,6 +17,7 @@ tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
diff --git a/s390x/css.c b/s390x/css.c
new file mode 100644
index 0000000..d7989d8
--- /dev/null
+++ b/s390x/css.c
@@ -0,0 +1,89 @@
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2020 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+
+#include <css.h>
+
+static struct schib schib;
+static int test_device_sid;
+
+static void test_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+	int scn;
+	int scn_found = 0;
+	int dev_found = 0;
+
+	for (scn = 0; scn < 0xffff; scn++) {
+		cc = stsch(scn|SID_ONE, &schib);
+		switch (cc) {
+		case 0:		/* 0 means SCHIB stored */
+			break;
+		case 3:		/* 3 means no more channels */
+			goto out;
+		default:	/* 1 or 2 should never happened for STSCH */
+			report(0, "Unexpected cc=%d on subchannel number 0x%x",
+			       cc, scn);
+			return;
+		}
+
+		/* We currently only support type 0, a.k.a. I/O channels */
+		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
+			continue;
+
+		/* We ignore I/O channels without valid devices */
+		scn_found++;
+		if (!(pmcw->flags & PMCW_DNV))
+			continue;
+
+		/* We keep track of the first device as our test device */
+		if (!test_device_sid)
+			test_device_sid = scn | SID_ONE;
+
+		dev_found++;
+	}
+
+out:
+	report(dev_found,
+	       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+	       scn, scn_found, dev_found);
+}
+
+static struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "enumerate (stsch)", test_enumerate },
+	{ NULL, NULL }
+};
+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("Channel Subsystem");
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		tests[i].func();
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 07013b2..a436ec0 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -83,3 +83,7 @@ extra_params = -m 1G
 [sclp-3g]
 file = sclp.elf
 extra_params = -m 3G
+
+[css]
+file = css.elf
+extra_params =-device ccw-pong
-- 
2.25.1

[kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Pierre Morel]

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

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

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

diff --git a/s390x/css.c b/s390x/css.c
index d7989d8..1b60a47 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -16,6 +16,7 @@
 #include <string.h>
 #include <interrupt.h>
 #include <asm/arch_def.h>
+#include <asm/time.h>
 
 #include <css.h>
 
@@ -65,11 +66,77 @@ out:
 	       scn, scn_found, dev_found);
 }
 
+#define MAX_ENABLE_RETRIES	5
+static void test_enable(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int retry_count = 0;
+	int cc;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	/* Read the SCHIB for this subchannel */
+	cc = stsch(test_device_sid, &schib);
+	if (cc) {
+		report(0, "stsch cc=%d", cc);
+		return;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report(1, "stsch: sch %08x already enabled", test_device_sid);
+		return;
+	}
+
+retry:
+	/* Update the SCHIB to enable the channel */
+	pmcw->flags |= PMCW_ENABLE;
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(test_device_sid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report(0, "msch cc=%d", cc);
+		return;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(test_device_sid, &schib);
+	if (cc) {
+		report(0, "stsch cc=%d", cc);
+		return;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report(1, "msch: sch %08x enabled after %d retries",
+		       test_device_sid, retry_count);
+		return;
+	}
+
+	if (retry_count++ < MAX_ENABLE_RETRIES) {
+		mdelay(10); /* the hardware was not ready, let it some time */
+		goto retry;
+	}
+
+	report(0,
+	       "msch: enabling sch %08x failed after %d retries. pmcw: %x",
+	       test_device_sid, retry_count, pmcw->flags);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
+	{ "enable (msch)", test_enable },
 	{ NULL, NULL }
 };
 
-- 
2.25.1

[kvm-unit-tests PATCH v7 10/12] s390x: define function to wait for interrupt

[Pierre Morel]

Allow the program to wait for an interrupt.

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 54ffd0b..f200e28 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,9 +10,11 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_IO			0x0200000000000000UL
 #define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_WAIT			0x0002000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
 #define PSW_MASK_BA			0x0000000080000000UL
 #define PSW_MASK_EA			0x0000000100000000UL
@@ -253,6 +255,18 @@ static inline void load_psw_mask(uint64_t mask)
 		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
 }
 
+static inline void wait_for_interrupt(uint64_t irq_mask)
+{
+	uint64_t psw_mask = extract_psw_mask();
+
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
+	/*
+	 * After being woken and having processed the interrupt, let's restore
+	 * the PSW mask.
+	 */
+	load_psw_mask(psw_mask);
+}
+
 static inline void enter_pstate(void)
 {
 	uint64_t mask;
-- 
2.25.1

[kvm-unit-tests PATCH v7 11/12] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

We add a new css_lib file to contain the I/O functions we may
share with different tests.
First function is the subchannel_enable() function.

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

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

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     |  20 ++++++
 lib/s390x/css_lib.c |  55 +++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/css.c         | 145 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 221 insertions(+)
 create mode 100644 lib/s390x/css_lib.c

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index e0d3a98..f4df7bc 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -99,6 +99,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(4))) __attribute__ ((packed));
+
 /* CSS low level access functions */
 
 static inline int ssch(unsigned long schid, struct orb *addr)
@@ -256,4 +269,11 @@ static inline struct ccw *dump_ccw(struct ccw *cp)
 	return NULL;
 }
 #endif /* DEBUG_CSS */
+
+#define SID_ONE         0x00010000
+
+/* Library functions */
+int enable_subchannel(unsigned int sid);
+int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
new file mode 100644
index 0000000..80b9359
--- /dev/null
+++ b/lib/s390x/css_lib.c
@@ -0,0 +1,55 @@
+/*
+ * Channel subsystem library functions
+ *
+ * Copyright (c) 2020 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 <stdint.h>
+#include <stddef.h>
+#include <css.h>
+
+int enable_subchannel(unsigned int sid)
+{
+	struct schib schib;
+	struct pmcw *pmcw = &schib.pmcw;
+	int try_count = 5;
+	int cc;
+
+	if (!(sid & SID_ONE))
+		return -1;
+
+	cc = stsch(sid, &schib);
+	if (cc)
+		return -cc;
+
+	do {
+		pmcw->flags |= PMCW_ENABLE;
+
+		cc = msch(sid, &schib);
+		if (cc)
+			return -cc;
+
+		cc = stsch(sid, &schib);
+		if (cc)
+			return -cc;
+
+	} while (!(pmcw->flags & PMCW_ENABLE) && --try_count);
+
+	return try_count;
+}
+
+int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
+{
+	struct orb orb;
+
+	orb.intparm = sid;
+	orb.ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	orb.cpa = (unsigned int) (unsigned long)ccw;
+
+	return ssch(sid, &orb);
+}
diff --git a/s390x/Makefile b/s390x/Makefile
index baebf18..166cb5c 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -53,6 +53,7 @@ cflatobjs += lib/s390x/interrupt.o
 cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/css_dump.o
+cflatobjs += lib/s390x/css_lib.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/css.c b/s390x/css.c
index 1b60a47..c94a916 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -20,8 +20,24 @@
 
 #include <css.h>
 
+#define PONG_CU_TYPE		0xc0ca
+
+struct lowcore *lowcore = (void *)0x0;
+
 static struct schib schib;
 static int test_device_sid;
+#define NUM_CCW  100
+static struct ccw1 ccw[NUM_CCW];
+static struct irb irb;
+static struct senseid senseid;
+
+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 test_enumerate(void)
 {
@@ -131,12 +147,140 @@ retry:
 	       test_device_sid, retry_count, pmcw->flags);
 }
 
+static void enable_io_isc(void)
+{
+	/* Let's enable all ISCs for I/O interrupt */
+	set_io_irq_subclass_mask(0x00000000ff000000);
+}
+
+static void irq_io(void)
+{
+	int ret = 0;
+	char *flags;
+	int sid;
+
+	report_prefix_push("Interrupt");
+	/* Lowlevel set the SID as interrupt parameter. */
+	if (lowcore->io_int_param != test_device_sid) {
+		report(0,
+		       "Bad io_int_param: %x expected %x",
+		       lowcore->io_int_param, test_device_sid);
+		goto pop;
+	}
+	report_prefix_pop();
+
+	report_prefix_push("tsch");
+	sid = lowcore->subsys_id_word;
+	ret = tsch(sid, &irb);
+	switch (ret) {
+	case 1:
+		dump_irb(&irb);
+		flags = dump_scsw_flags(irb.scsw.ctrl);
+		report(0,
+		       "I/O interrupt, CC 1 but tsch reporting sch %08x as not status pending: %s",
+		       sid, flags);
+		break;
+	case 2:
+		report(0, "tsch returns unexpected CC 2");
+		break;
+	case 3:
+		report(0, "tsch reporting sch %08x as not operational", sid);
+		break;
+	case 0:
+		/* Stay humble on success */
+		break;
+	}
+pop:
+	report_prefix_pop();
+	lowcore->io_old_psw.mask &= ~PSW_MASK_WAIT;
+}
+
+static int start_subchannel(int code, void *data, int count,
+			    unsigned char flags)
+{
+	int ret;
+
+	report_prefix_push("start_senseid");
+	/* Build the CCW chain with a single CCW */
+	ccw[0].code = code;
+	ccw[0].flags = flags; /* No flags need to be set */
+	ccw[0].count = count;
+	ccw[0].data_address = (int)(unsigned long)data;
+
+	ret = start_ccw1_chain(test_device_sid, ccw);
+	if (ret) {
+		report(0, "start_ccw_chain failed ret=%d", ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+/*
+ * test_sense
+ * Pre-requisits:
+ * - We need the QEMU PONG device as the first recognized
+ *   device by the enumeration.
+ * - ./s390x-run s390x/css.elf -device ccw-pong,cu_type=0xc0ca
+ */
+static void test_sense(void)
+{
+	int ret;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	ret = enable_subchannel(test_device_sid);
+	if (ret < 0) {
+		report(0,
+		       "Could not enable the subchannel: %08x",
+		       test_device_sid);
+		return;
+	}
+
+	ret = register_io_int_func(irq_io);
+	if (ret) {
+		report(0, "Could not register IRQ handler");
+		goto unreg_cb;
+	}
+
+	lowcore->io_int_param = 0;
+
+	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid),
+			       CCW_F_SLI);
+	if (!ret) {
+		report(0, "ssch failed for SENSE ID on sch %08x",
+		       test_device_sid);
+		goto unreg_cb;
+	}
+
+	wait_for_interrupt(PSW_MASK_IO);
+
+	if (lowcore->io_int_param != test_device_sid)
+		goto unreg_cb;
+
+	report_info("reserved %02x cu_type %04x cu_model %02x dev_type %04x dev_model %02x",
+		    senseid.reserved, senseid.cu_type, senseid.cu_model,
+		    senseid.dev_type, senseid.dev_model);
+
+	report((senseid.cu_type == PONG_CU),
+	       "cu_type: expect 0x%04x got 0x%04x",
+	       PONG_CU_TYPE, senseid.cu_type);
+
+unreg_cb:
+	unregister_io_int_func(irq_io);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
+	{ "sense (ssch/tsch)", test_sense },
 	{ NULL, NULL }
 };
 
@@ -145,6 +289,7 @@ int main(int argc, char *argv[])
 	int i;
 
 	report_prefix_push("Channel Subsystem");
+	enable_io_isc();
 	for (i = 0; tests[i].name; i++) {
 		report_prefix_push(tests[i].name);
 		tests[i].func();
-- 
2.25.1

[kvm-unit-tests PATCH v7 12/12] s390x: css: ping pong

[Pierre Morel]

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

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

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

diff --git a/s390x/css.c b/s390x/css.c
index c94a916..da80574 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -21,6 +21,12 @@
 #include <css.h>
 
 #define PONG_CU_TYPE		0xc0ca
+/* Channel Commands for PONG device */
+#define PONG_WRITE	0x21 /* Write */
+#define PONG_READ	0x22 /* Read buffer */
+
+#define BUFSZ	9
+static char buffer[BUFSZ];
 
 struct lowcore *lowcore = (void *)0x0;
 
@@ -274,6 +280,53 @@ unreg_cb:
 	unregister_io_int_func(irq_io);
 }
 
+static void test_ping(void)
+{
+	int success, result;
+	int cnt = 0, max = 4;
+
+	if (senseid.cu_type != PONG_CU) {
+		report_skip("Device is not a pong device.");
+		return;
+	}
+
+	result = register_io_int_func(irq_io);
+	if (result) {
+		report(0, "Could not register IRQ handler");
+		return;
+	}
+
+	while (cnt++ < max) {
+		snprintf(buffer, BUFSZ, "%08x\n", cnt);
+		success = start_subchannel(PONG_WRITE, buffer, BUFSZ, 0);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+
+		wait_for_interrupt(PSW_MASK_IO);
+
+		success = start_subchannel(PONG_READ, buffer, BUFSZ, 0);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+
+		wait_for_interrupt(PSW_MASK_IO);
+
+		result = atol(buffer);
+		if (result != (cnt + 1)) {
+			report(0, "Bad answer from pong: %08x - %08x",
+			       cnt, result);
+			goto unreg_cb;
+		}
+	}
+	report(1, "ping-pong count 0x%08x", cnt);
+
+unreg_cb:
+	unregister_io_int_func(irq_io);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -281,6 +334,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.25.1

Re: [kvm-unit-tests PATCH v7 03/12] s390x: Move control register bit definitions and add AFP to them

[Thomas Huth]

On 18/05/2020 18.07, Pierre Morel wrote:
> While adding the definition for the AFP-Register control bit, move all
> existing definitions for CR0 out of the C zone to the assmbler zone to
> keep the definitions concerning CR0 together.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/asm/arch_def.h | 11 ++++++-----
>  s390x/cstart64.S         |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 820af93..54ffd0b 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -19,17 +19,18 @@
>  
>  #define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
>  
> +#define CR0_EXTM_SCLP			0X0000000000000200UL
> +#define CR0_EXTM_EXTC			0X0000000000002000UL
> +#define CR0_EXTM_EMGC			0X0000000000004000UL
> +#define CR0_EXTM_MASK			0X0000000000006200UL
> +#define CR0_AFP_REG_CRTL		0x0000000000040000UL
> +
>  #ifndef __ASSEMBLER__
>  struct psw {
>  	uint64_t	mask;
>  	uint64_t	addr;
>  };
>  
> -#define CR0_EXTM_SCLP			0X0000000000000200UL
> -#define CR0_EXTM_EXTC			0X0000000000002000UL
> -#define CR0_EXTM_EMGC			0X0000000000004000UL
> -#define CR0_EXTM_MASK			0X0000000000006200UL

This patch does not apply anymore due to commit f7df29115f736b ...
please switch to lower-case "0x"s in the next version.

 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Thomas Huth]

On 18/05/2020 18.07, Pierre Morel wrote:
> First step for testing the channel subsystem is to enumerate the css and
> retrieve the css devices.
> 
> This tests the success of STSCH I/O instruction, we do not test the
> reaction of the VM for an instruction with wrong parameters.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/Makefile      |  1 +
>  s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 ++
>  3 files changed, 94 insertions(+)
>  create mode 100644 s390x/css.c
[...]
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 07013b2..a436ec0 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -83,3 +83,7 @@ extra_params = -m 1G
>  [sclp-3g]
>  file = sclp.elf
>  extra_params = -m 3G
> +
> +[css]
> +file = css.elf
> +extra_params =-device ccw-pong

I gave your patch series a try on a normal upstream QEMU (that does not
have the ccw-pong device yet), and the css test of course fails there,
since QEMU bails out with:

 -device ccw-pong: 'ccw-pong' is not a valid device model name

This is unfortunate - I think we likely have to deal with QEMUs for
quite a while that do not have this device enabled. Could you maybe add
some kind of check to the kvm-unit-tests scripts that only run a test if
a given device is available, and skip the test otherwise?

 Thomas

Re: [kvm-unit-tests PATCH v7 02/12] s390x: Use PSW bits definitions in cstart

[Janosch Frank]

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

On 5/18/20 6:07 PM, Pierre Morel wrote:
> This patch defines the PSW bits EA/BA used to initialize the PSW masks
> for exceptions.
> 
> Since some PSW mask definitions exist already in arch_def.h we add these
> definitions there.
> We move all PSW definitions together and protect assembler code against
> C syntax.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Nice

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

> ---
>  lib/s390x/asm/arch_def.h | 15 +++++++++++----
>  s390x/cstart64.S         | 15 ++++++++-------
>  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 15a4d49..820af93 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -10,15 +10,21 @@
>  #ifndef _ASM_S390X_ARCH_DEF_H_
>  #define _ASM_S390X_ARCH_DEF_H_
>  
> +#define PSW_MASK_EXT			0x0100000000000000UL
> +#define PSW_MASK_DAT			0x0400000000000000UL
> +#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
> +#define PSW_MASK_PSTATE			0x0001000000000000UL
> +#define PSW_MASK_BA			0x0000000080000000UL
> +#define PSW_MASK_EA			0x0000000100000000UL
> +
> +#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
> +
> +#ifndef __ASSEMBLER__
>  struct psw {
>  	uint64_t	mask;
>  	uint64_t	addr;
>  };
>  
> -#define PSW_MASK_EXT			0x0100000000000000UL
> -#define PSW_MASK_DAT			0x0400000000000000UL
> -#define PSW_MASK_PSTATE			0x0001000000000000UL
> -
>  #define CR0_EXTM_SCLP			0X0000000000000200UL
>  #define CR0_EXTM_EXTC			0X0000000000002000UL
>  #define CR0_EXTM_EMGC			0X0000000000004000UL
> @@ -297,4 +303,5 @@ static inline uint32_t get_prefix(void)
>  	return current_prefix;
>  }
>  
> +#endif /* __ASSEMBLER */
>  #endif
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 3c7d8a9..e890568 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -12,6 +12,7 @@
>   */
>  #include <asm/asm-offsets.h>
>  #include <asm/sigp.h>
> +#include <asm/arch_def.h>
>  
>  .section .init
>  
> @@ -232,19 +233,19 @@ svc_int:
>  
>  	.align	8
>  reset_psw:
> -	.quad	0x0008000180000000
> +	.quad	PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW
>  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
> 



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

Re: [kvm-unit-tests PATCH v7 01/12] s390x: saving regs for interrupts

[Janosch Frank]

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

On 5/18/20 6:07 PM, Pierre Morel wrote:
> If we use multiple source of interrupts, for example, using SCLP
> console to print information while using I/O interrupts, we need
> to have a re-entrant register saving interruption handling.
> 
> Instead of saving at a static memory address, let's save the base
> registers, the floating point registers and the floating point
> control register on the stack in case of I/O interrupts
> 
> Note that we keep the static register saving to recover from the
> RESET tests.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Some nits below

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


> ---
>  s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 9af6bb3..3c7d8a9 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -118,6 +118,43 @@ memsetxc:
>  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
>  	.endm
>  
> +/* Save registers on the stack (r15), so we can have stacked interrupts. */
> +	.macro SAVE_REGS_STACK
> +	/* Allocate a stack frame for 15 general registers */
> +	slgfi   %r15, 15 * 8
> +	/* Store all registers from r0 to r14 on the stack */

/* Store registers r0 to r14 on the stack */

> +	stmg    %r0, %r14, 0(%r15)
> +	/* Allocate a stack frame for 16 floating point registers */
> +	/* The size of a FP register is the size of an double word */
> +	slgfi   %r15, 16 * 8
> +	/* Save fp register on stack: offset to SP is multiple of reg number */
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	std	\i, \i * 8(%r15)
> +	.endr
> +	/* Save fpc, but keep stack aligned on 64bits */
> +	slgfi   %r15, 8
> +	efpc	%r0
> +	stg	%r0, 0(%r15)
> +	.endm
> +
> +/* Restore the register in reverse order */
> +	.macro RESTORE_REGS_STACK
> +	/* Restore fpc */
> +	lfpc	0(%r15)
> +	algfi	%r15, 8
> +	/* Restore fp register from stack: SP still where it was left */
> +	/* and offset to SP is a multile of reg number */

multile?
Multiple?

> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	ld	\i, \i * 8(%r15)
> +	.endr
> +	/* Now it is done, rewind the stack pointer by 16 double word */

Now that we're done, rewind the stack pointer by 16 double words

> +	algfi   %r15, 16 * 8
> +	/* Load the registers from stack */
> +	lmg     %r0, %r14, 0(%r15)
> +	/* Rewind the stack by 15 double word */
> +	algfi   %r15, 15 * 8
> +	.endm
> +
>  .section .text
>  /*
>   * load_reset calling convention:
> @@ -182,9 +219,9 @@ mcck_int:
>  	lpswe	GEN_LC_MCCK_OLD_PSW
>  
>  io_int:
> -	SAVE_REGS
> +	SAVE_REGS_STACK
>  	brasl	%r14, handle_io_int
> -	RESTORE_REGS
> +	RESTORE_REGS_STACK
>  	lpswe	GEN_LC_IO_OLD_PSW
>  
>  svc_int:
> 



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

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Janosch Frank]

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

On 5/25/20 9:12 PM, Thomas Huth wrote:
> On 18/05/2020 18.07, Pierre Morel wrote:
>> First step for testing the channel subsystem is to enumerate the css and
>> retrieve the css devices.
>>
>> This tests the success of STSCH I/O instruction, we do not test the
>> reaction of the VM for an instruction with wrong parameters.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  s390x/Makefile      |  1 +
>>  s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |  4 ++
>>  3 files changed, 94 insertions(+)
>>  create mode 100644 s390x/css.c
> [...]
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 07013b2..a436ec0 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -83,3 +83,7 @@ extra_params = -m 1G
>>  [sclp-3g]
>>  file = sclp.elf
>>  extra_params = -m 3G
>> +
>> +[css]
>> +file = css.elf
>> +extra_params =-device ccw-pong
> 
> I gave your patch series a try on a normal upstream QEMU (that does not
> have the ccw-pong device yet), and the css test of course fails there,
> since QEMU bails out with:
> 
>  -device ccw-pong: 'ccw-pong' is not a valid device model name
> 
> This is unfortunate - I think we likely have to deal with QEMUs for
> quite a while that do not have this device enabled. Could you maybe add
> some kind of check to the kvm-unit-tests scripts that only run a test if
> a given device is available, and skip the test otherwise?
> 
>  Thomas
> 

Could we for now remove it from unittests.cfg and let Pierre come up
with a solution without delaying this whole series? I expect changes to
run_tests.sh to attract a rather long discussion...


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

Re: [kvm-unit-tests PATCH v7 10/12] s390x: define function to wait for interrupt

[Janosch Frank]

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

On 5/18/20 6:07 PM, Pierre Morel wrote:
> Allow the program to wait for an interrupt.
> 
> The interrupt handler is in charge to remove the WAIT bit
> when it finished handling the interrupt.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

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

> ---
>  lib/s390x/asm/arch_def.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 54ffd0b..f200e28 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -10,9 +10,11 @@
>  #ifndef _ASM_S390X_ARCH_DEF_H_
>  #define _ASM_S390X_ARCH_DEF_H_
>  
> +#define PSW_MASK_IO			0x0200000000000000UL
>  #define PSW_MASK_EXT			0x0100000000000000UL
>  #define PSW_MASK_DAT			0x0400000000000000UL
>  #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
> +#define PSW_MASK_WAIT			0x0002000000000000UL
>  #define PSW_MASK_PSTATE			0x0001000000000000UL
>  #define PSW_MASK_BA			0x0000000080000000UL
>  #define PSW_MASK_EA			0x0000000100000000UL
> @@ -253,6 +255,18 @@ static inline void load_psw_mask(uint64_t mask)
>  		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
>  }
>  
> +static inline void wait_for_interrupt(uint64_t irq_mask)
> +{
> +	uint64_t psw_mask = extract_psw_mask();
> +
> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
> +	/*
> +	 * After being woken and having processed the interrupt, let's restore
> +	 * the PSW mask.
> +	 */
> +	load_psw_mask(psw_mask);
> +}
> +
>  static inline void enter_pstate(void)
>  {
>  	uint64_t mask;
> 



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

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Thomas Huth]

On 26/05/2020 12.41, Janosch Frank wrote:
> On 5/25/20 9:12 PM, Thomas Huth wrote:
>> On 18/05/2020 18.07, Pierre Morel wrote:
>>> First step for testing the channel subsystem is to enumerate the css and
>>> retrieve the css devices.
>>>
>>> This tests the success of STSCH I/O instruction, we do not test the
>>> reaction of the VM for an instruction with wrong parameters.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>  s390x/Makefile      |  1 +
>>>  s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>>>  s390x/unittests.cfg |  4 ++
>>>  3 files changed, 94 insertions(+)
>>>  create mode 100644 s390x/css.c
>> [...]
>>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>>> index 07013b2..a436ec0 100644
>>> --- a/s390x/unittests.cfg
>>> +++ b/s390x/unittests.cfg
>>> @@ -83,3 +83,7 @@ extra_params = -m 1G
>>>  [sclp-3g]
>>>  file = sclp.elf
>>>  extra_params = -m 3G
>>> +
>>> +[css]
>>> +file = css.elf
>>> +extra_params =-device ccw-pong
>>
>> I gave your patch series a try on a normal upstream QEMU (that does not
>> have the ccw-pong device yet), and the css test of course fails there,
>> since QEMU bails out with:
>>
>>  -device ccw-pong: 'ccw-pong' is not a valid device model name
>>
>> This is unfortunate - I think we likely have to deal with QEMUs for
>> quite a while that do not have this device enabled. Could you maybe add
>> some kind of check to the kvm-unit-tests scripts that only run a test if
>> a given device is available, and skip the test otherwise?
>>
>>  Thomas
>>
> 
> Could we for now remove it from unittests.cfg and let Pierre come up
> with a solution without delaying this whole series? I expect changes to
> run_tests.sh to attract a rather long discussion...

Fine for me, too.

 Thomas

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Pierre Morel]

On 2020-05-26 12:49, Thomas Huth wrote:
> On 26/05/2020 12.41, Janosch Frank wrote:
>> On 5/25/20 9:12 PM, Thomas Huth wrote:
>>> On 18/05/2020 18.07, Pierre Morel wrote:
>>>> First step for testing the channel subsystem is to enumerate the css and
>>>> retrieve the css devices.

...snip...

>>> I gave your patch series a try on a normal upstream QEMU (that does not
>>> have the ccw-pong device yet), and the css test of course fails there,
>>> since QEMU bails out with:
>>>
>>>   -device ccw-pong: 'ccw-pong' is not a valid device model name
>>>
>>> This is unfortunate - I think we likely have to deal with QEMUs for
>>> quite a while that do not have this device enabled. Could you maybe add
>>> some kind of check to the kvm-unit-tests scripts that only run a test if
>>> a given device is available, and skip the test otherwise?
>>>
>>>   Thomas
>>>
>>
>> Could we for now remove it from unittests.cfg and let Pierre come up
>> with a solution without delaying this whole series? I expect changes to
>> run_tests.sh to attract a rather long discussion...
> 
> Fine for me, too.
> 
>   Thomas
> 

OK, thanks you both, I do so, take the unittest.cfg out from this series 
and come back later with a proposition for unittest.cfg.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 01/12] s390x: saving regs for interrupts

[Pierre Morel]

On 2020-05-26 12:30, Janosch Frank wrote:
> On 5/18/20 6:07 PM, Pierre Morel wrote:
>> If we use multiple source of interrupts, for example, using SCLP
>> console to print information while using I/O interrupts, we need
>> to have a re-entrant register saving interruption handling.
>>
>> Instead of saving at a static memory address, let's save the base
>> registers, the floating point registers and the floating point
>> control register on the stack in case of I/O interrupts
>>
>> Note that we keep the static register saving to recover from the
>> RESET tests.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Some nits below
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> 
> 

Thanks, I will modify the nits as you proposed.

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 02/12] s390x: Use PSW bits definitions in cstart

[Pierre Morel]

On 2020-05-26 12:17, Janosch Frank wrote:
> On 5/18/20 6:07 PM, Pierre Morel wrote:
>> This patch defines the PSW bits EA/BA used to initialize the PSW masks
>> for exceptions.
>>
>> Since some PSW mask definitions exist already in arch_def.h we add these
>> definitions there.
>> We move all PSW definitions together and protect assembler code against
>> C syntax.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Nice
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 10/12] s390x: define function to wait for interrupt

[Pierre Morel]

On 2020-05-26 12:42, Janosch Frank wrote:
> On 5/18/20 6:07 PM, Pierre Morel wrote:
>> Allow the program to wait for an interrupt.
>>
>> The interrupt handler is in charge to remove the WAIT bit
>> when it finished handling the interrupt.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 03/12] s390x: Move control register bit definitions and add AFP to them

[Pierre Morel]

On 2020-05-25 20:57, Thomas Huth wrote:
> On 18/05/2020 18.07, Pierre Morel wrote:
>> While adding the definition for the AFP-Register control bit, move all
>> existing definitions for CR0 out of the C zone to the assmbler zone to
>> keep the definitions concerning CR0 together.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   lib/s390x/asm/arch_def.h | 11 ++++++-----
>>   s390x/cstart64.S         |  2 +-
>>   2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 820af93..54ffd0b 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -19,17 +19,18 @@
>>   
>>   #define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
>>   
>> +#define CR0_EXTM_SCLP			0X0000000000000200UL
>> +#define CR0_EXTM_EXTC			0X0000000000002000UL
>> +#define CR0_EXTM_EMGC			0X0000000000004000UL
>> +#define CR0_EXTM_MASK			0X0000000000006200UL
>> +#define CR0_AFP_REG_CRTL		0x0000000000040000UL
>> +
>>   #ifndef __ASSEMBLER__
>>   struct psw {
>>   	uint64_t	mask;
>>   	uint64_t	addr;
>>   };
>>   
>> -#define CR0_EXTM_SCLP			0X0000000000000200UL
>> -#define CR0_EXTM_EXTC			0X0000000000002000UL
>> -#define CR0_EXTM_EMGC			0X0000000000004000UL
>> -#define CR0_EXTM_MASK			0X0000000000006200UL
> 
> This patch does not apply anymore due to commit f7df29115f736b ...
> please switch to lower-case "0x"s in the next version.
> 
>   Thanks,
>    Thomas
> 

OK, I will rebase.
Thanks,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 07/12] s390x: Library resources for CSS tests

[Cornelia Huck]

On Mon, 18 May 2020 18:07:26 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Provide some definitions and library routines that can be used by
> tests targeting the channel subsystem.
> 
> Debug function can be activated by defining DEBUG_CSS before the
> inclusion of the css.h header file.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h      | 259 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
>  s390x/Makefile       |   1 +
>  3 files changed, 417 insertions(+)
>  create mode 100644 lib/s390x/css.h
>  create mode 100644 lib/s390x/css_dump.c
> 

(...)

> +struct ccw1 {
> +	unsigned char code;
> +	unsigned char flags;
> +	unsigned short count;

I'm wondering why you're using unsigned {char,short} here, instead of
the uint*_t types everywhere else? It's not wrong, but probably better
to be consistent?

> +	uint32_t data_address;
> +} __attribute__ ((aligned(4)));
> +
> +#define SID_ONE		0x00010000
> +

I think it would be beneficial for the names to somewhat match the
naming in Linux and/or QEMU -- or more speaking names (as you do for
some), which is also good.

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

ORB_F_ISIC? (As it does not refer to 'initialization', but 'initial'.)

> +#define ORB_F_ADDRLIMIT	0x00100000
> +#define ORB_F_SUSP_IRQ	0x00080000

ORB_F_SSIC? (As it deals with suppression.)

> +#define ORB_F_TRANSPORT	0x00040000
> +#define ORB_F_IDAW2	0x00020000

ORB_F_IDAW_FMT2?

Or following Linux/QEMU, use ORB_F_C64 for a certain retro appeal :)

> +#define ORB_F_IDAW_2K	0x00010000
> +#define ORB_M_LPM	0x0000ff00
> +#define ORB_F_LPM_DFLT	0x00008000

That's a default lpm of 0x80, right? It's a bit buried between the orb
definitions, and it also seems to be more of a implementation choice --
move it out from the flags here?

> +#define ORB_F_ILSM	0x00000080

ORB_F_ILS?

> +#define ORB_F_CCW_IND	0x00000040

ORB_F_MIDAW? I had a hard time figuring out that one :)

> +#define ORB_F_ORB_EXT	0x00000001

(...)

> +/*
> + * Try o have a more human representation of the PMCW flags

s/o/to/

> + * each letter in the string represent the first

s/represent/represents/

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

(...)

Generally, looks good to me.

Re: [kvm-unit-tests PATCH v7 04/12] s390x: interrupt registration

[Thomas Huth]

On 18/05/2020 18.07, Pierre Morel wrote:
> Let's make it possible to add and remove a custom io interrupt handler,
> that can be used instead of the normal one.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
>  lib/s390x/interrupt.h |  8 ++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>  create mode 100644 lib/s390x/interrupt.h
...
> diff --git a/lib/s390x/interrupt.h b/lib/s390x/interrupt.h
> new file mode 100644
> index 0000000..323258d
> --- /dev/null
> +++ b/lib/s390x/interrupt.h
> @@ -0,0 +1,8 @@
> +#ifndef __INTERRUPT_H
> +#define __INTERRUPT_H

Looking at this patch again, I noticed another nit: No double
underscores at the beginning of header guards, please! That's reserved
namespace. Simply use INTERRUPT_H or S390X_INTERRUPT_H or something
similar instead.

 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v7 05/12] s390x: export the clock get_clock_ms() utility

[Thomas Huth]

On 18/05/2020 18.07, Pierre Morel wrote:
> To serve multiple times, the function get_clock_ms() is moved
> from intercept.c test to the new file asm/time.h.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/asm/time.h | 26 ++++++++++++++++++++++++++
>  s390x/intercept.c    | 11 +----------
>  2 files changed, 27 insertions(+), 10 deletions(-)
>  create mode 100644 lib/s390x/asm/time.h
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> new file mode 100644
> index 0000000..25c7a3c
> --- /dev/null
> +++ b/lib/s390x/asm/time.h
> @@ -0,0 +1,26 @@
> +/*
> + * Clock utilities for s390
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *
> + * Copied from the s390/intercept test by:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
> +#ifndef _ASM_S390X_TIME_H_
> +#define _ASM_S390X_TIME_H_

Please also remove the underscores at the beginning (and preferably also
at the end) here.

 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v7 06/12] s390x: use get_clock_ms() to calculate a delay in ms

[Thomas Huth]

On 18/05/2020 18.07, Pierre Morel wrote:
> use get_clock_ms() to calculate a delay in ms
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/time.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> index 25c7a3c..931a119 100644
> --- a/lib/s390x/asm/time.h
> +++ b/lib/s390x/asm/time.h
> @@ -23,4 +23,14 @@ static inline uint64_t get_clock_ms(void)
>  	return (clk >> (63 - 51)) / 1000;
>  }
>  
> +static inline void mdelay(unsigned long ms)
> +{
> +	unsigned long startclk;
> +
> +	startclk = get_clock_ms();
> +	for (;;)
> +		if (get_clock_ms() - startclk > ms)
> +			break;

Maybe rather:

    for (;get_clock_ms() - startclk <= ms;)
	;

?
Or:

    while (get_clock_ms() - startclk <= ms)
        ;
?

 Thomas

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Cornelia Huck]

On Mon, 18 May 2020 18:07:27 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> First step for testing the channel subsystem is to enumerate the css and
> retrieve the css devices.
> 
> This tests the success of STSCH I/O instruction, we do not test the
> reaction of the VM for an instruction with wrong parameters.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/Makefile      |  1 +
>  s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 ++
>  3 files changed, 94 insertions(+)
>  create mode 100644 s390x/css.c

(...)

> +static void test_enumerate(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int cc;
> +	int scn;
> +	int scn_found = 0;
> +	int dev_found = 0;
> +
> +	for (scn = 0; scn < 0xffff; scn++) {
> +		cc = stsch(scn|SID_ONE, &schib);
> +		switch (cc) {
> +		case 0:		/* 0 means SCHIB stored */
> +			break;
> +		case 3:		/* 3 means no more channels */
> +			goto out;
> +		default:	/* 1 or 2 should never happened for STSCH */
> +			report(0, "Unexpected cc=%d on subchannel number 0x%x",
> +			       cc, scn);
> +			return;
> +		}
> +
> +		/* We currently only support type 0, a.k.a. I/O channels */
> +		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
> +			continue;
> +
> +		/* We ignore I/O channels without valid devices */
> +		scn_found++;
> +		if (!(pmcw->flags & PMCW_DNV))
> +			continue;
> +
> +		/* We keep track of the first device as our test device */
> +		if (!test_device_sid)
> +			test_device_sid = scn | SID_ONE;
> +
> +		dev_found++;
> +	}
> +
> +out:
> +	report(dev_found,
> +	       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
> +	       scn, scn_found, dev_found);

Just wondering: with the current invocation, you expect to find exactly
one subchannel with a valid device, right?

> +}
> +
> +static struct {
> +	const char *name;
> +	void (*func)(void);
> +} tests[] = {
> +	{ "enumerate (stsch)", test_enumerate },
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +	int i;
> +
> +	report_prefix_push("Channel Subsystem");
> +	for (i = 0; tests[i].name; i++) {
> +		report_prefix_push(tests[i].name);
> +		tests[i].func();
> +		report_prefix_pop();
> +	}
> +	report_prefix_pop();
> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 07013b2..a436ec0 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -83,3 +83,7 @@ extra_params = -m 1G
>  [sclp-3g]
>  file = sclp.elf
>  extra_params = -m 3G
> +
> +[css]
> +file = css.elf
> +extra_params =-device ccw-pong

Hm... you could test enumeration even with a QEMU that does not include
support for the pong device, right? Would it be worthwhile to split out
a set of css tests that use e.g. a virtio-net-ccw device, and have a
css-pong set of tests that require the pong device?

Re: [kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Cornelia Huck]

On Mon, 18 May 2020 18:07:28 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> A second step when testing the channel subsystem is to prepare a channel
> for use.
> This includes:
> - Get the current subchannel Information Block (SCHIB) using STSCH
> - Update it in memory to set the ENABLE bit
> - Tell the CSS that the SCHIB has been modified using MSCH
> - Get the SCHIB from the CSS again to verify that the subchannel is
>   enabled.
> - If the subchannel is not enabled retry a predefined retries count.
> 
> This tests the MSCH instruction to enable a channel succesfuly.
> This is NOT a routine to really enable the channel, no retry is done,
> in case of error, a report is made.

Hm... so you retry if the subchannel is not enabled after cc 0, but you
don't retry if the cc indicates busy/status pending? Makes sense, as we
don't expect the subchannel to be busy, but a more precise note in the
patch description would be good :)

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index d7989d8..1b60a47 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -16,6 +16,7 @@
>  #include <string.h>
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
> +#include <asm/time.h>
>  
>  #include <css.h>
>  
> @@ -65,11 +66,77 @@ out:
>  	       scn, scn_found, dev_found);
>  }
>  
> +#define MAX_ENABLE_RETRIES	5
> +static void test_enable(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int retry_count = 0;
> +	int cc;
> +
> +	if (!test_device_sid) {
> +		report_skip("No device");
> +		return;
> +	}
> +
> +	/* Read the SCHIB for this subchannel */
> +	cc = stsch(test_device_sid, &schib);
> +	if (cc) {
> +		report(0, "stsch cc=%d", cc);
> +		return;
> +	}
> +
> +	if (pmcw->flags & PMCW_ENABLE) {
> +		report(1, "stsch: sch %08x already enabled", test_device_sid);
> +		return;
> +	}
> +
> +retry:
> +	/* Update the SCHIB to enable the channel */
> +	pmcw->flags |= PMCW_ENABLE;
> +
> +	/* Tell the CSS we want to modify the subchannel */
> +	cc = msch(test_device_sid, &schib);
> +	if (cc) {
> +		/*
> +		 * If the subchannel is status pending or
> +		 * if a function is in progress,
> +		 * we consider both cases as errors.

Could also be cc 3, but that would be even more weird. Just logging the
cc seems fine, though.

> +		 */
> +		report(0, "msch cc=%d", cc);
> +		return;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again to verify the enablement
> +	 */
> +	cc = stsch(test_device_sid, &schib);
> +	if (cc) {
> +		report(0, "stsch cc=%d", cc);
> +		return;
> +	}
> +
> +	if (pmcw->flags & PMCW_ENABLE) {
> +		report(1, "msch: sch %08x enabled after %d retries",
> +		       test_device_sid, retry_count);
> +		return;
> +	}
> +
> +	if (retry_count++ < MAX_ENABLE_RETRIES) {
> +		mdelay(10); /* the hardware was not ready, let it some time */

s/let/give/

> +		goto retry;
> +	}
> +
> +	report(0,
> +	       "msch: enabling sch %08x failed after %d retries. pmcw: %x",
> +	       test_device_sid, retry_count, pmcw->flags);
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
>  } tests[] = {
>  	{ "enumerate (stsch)", test_enumerate },
> +	{ "enable (msch)", test_enable },
>  	{ NULL, NULL }
>  };
>  

Otherwise,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH v7 10/12] s390x: define function to wait for interrupt

[Cornelia Huck]

On Mon, 18 May 2020 18:07:29 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Allow the program to wait for an interrupt.
> 
> The interrupt handler is in charge to remove the WAIT bit
> when it finished handling the interrupt.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH v7 11/12] s390x: css: ssch/tsch with sense and interrupt

[Cornelia Huck]

On Mon, 18 May 2020 18:07:30 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We add a new css_lib file to contain the I/O functions we may
> share with different tests.
> First function is the subchannel_enable() function.
> 
> When a channel is enabled we can start a SENSE_ID command using
> the SSCH instruction to recognize the control unit and device.
> 
> This tests the success of SSCH, the I/O interruption and the TSCH
> instructions.
> 
> The test expects a device with a control unit type of 0xC0CA as the
> first subchannel of the CSS.

It might make sense to extend this to be able to check for any expected
type (e.g. 0x3832, should my suggestion to split css tests and css-pong
tests make sense.)

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  20 ++++++
>  lib/s390x/css_lib.c |  55 +++++++++++++++++
>  s390x/Makefile      |   1 +
>  s390x/css.c         | 145 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 221 insertions(+)
>  create mode 100644 lib/s390x/css_lib.c

(...)

> +int enable_subchannel(unsigned int sid)
> +{
> +	struct schib schib;
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int try_count = 5;
> +	int cc;
> +
> +	if (!(sid & SID_ONE))
> +		return -1;

Hm... this error is indistinguishable for the caller from a cc 1 for
the msch. Use something else (as this is a coding error)?

> +
> +	cc = stsch(sid, &schib);
> +	if (cc)
> +		return -cc;
> +
> +	do {
> +		pmcw->flags |= PMCW_ENABLE;
> +
> +		cc = msch(sid, &schib);
> +		if (cc)
> +			return -cc;
> +
> +		cc = stsch(sid, &schib);
> +		if (cc)
> +			return -cc;
> +
> +	} while (!(pmcw->flags & PMCW_ENABLE) && --try_count);
> +
> +	return try_count;

How useful is that information for the caller? I don't see the code
below making use of it.

> +}
> +
> +int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
> +{
> +	struct orb orb;
> +
> +	orb.intparm = sid;

Just an idea: If you use something else here (maybe the cpa), and set
the intparm to the sid in msch, you can test something else: Does msch
properly set the intparm, and is that intparm overwritten by a
successful ssch, until the next ssch or msch comes around?

> +	orb.ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
> +	orb.cpa = (unsigned int) (unsigned long)ccw;

Use a struct initializer, so that unset fields are 0?

> +
> +	return ssch(sid, &orb);
> +}

(...)

> +/*
> + * test_sense
> + * Pre-requisits:
> + * - We need the QEMU PONG device as the first recognized
> + *   device by the enumeration.
> + * - ./s390x-run s390x/css.elf -device ccw-pong,cu_type=0xc0ca
> + */
> +static void test_sense(void)
> +{
> +	int ret;
> +
> +	if (!test_device_sid) {
> +		report_skip("No device");
> +		return;
> +	}
> +
> +	ret = enable_subchannel(test_device_sid);
> +	if (ret < 0) {
> +		report(0,
> +		       "Could not enable the subchannel: %08x",
> +		       test_device_sid);
> +		return;
> +	}
> +
> +	ret = register_io_int_func(irq_io);
> +	if (ret) {
> +		report(0, "Could not register IRQ handler");
> +		goto unreg_cb;
> +	}
> +
> +	lowcore->io_int_param = 0;
> +
> +	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid),
> +			       CCW_F_SLI);

Clear senseid, before actually sending the program?

> +	if (!ret) {
> +		report(0, "ssch failed for SENSE ID on sch %08x",
> +		       test_device_sid);
> +		goto unreg_cb;
> +	}
> +
> +	wait_for_interrupt(PSW_MASK_IO);
> +
> +	if (lowcore->io_int_param != test_device_sid)
> +		goto unreg_cb;
> +
> +	report_info("reserved %02x cu_type %04x cu_model %02x dev_type %04x dev_model %02x",
> +		    senseid.reserved, senseid.cu_type, senseid.cu_model,
> +		    senseid.dev_type, senseid.dev_model);
> +

I'd also recommend checking that senseid.reserved is indeed 0xff -- in
combination with senseid clearing before the ssch, that ensures that
the senseid structure has actually been written to and is not pure
garbage. (It's also a cu type agnostic test :)

It also might make sense to check how much data you actually got, as
you set SLI.


> +	report((senseid.cu_type == PONG_CU),
> +	       "cu_type: expect 0x%04x got 0x%04x",
> +	       PONG_CU_TYPE, senseid.cu_type);
> +
> +unreg_cb:
> +	unregister_io_int_func(irq_io);
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
>  } tests[] = {
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
> +	{ "sense (ssch/tsch)", test_sense },
>  	{ NULL, NULL }
>  };
>  
> @@ -145,6 +289,7 @@ int main(int argc, char *argv[])
>  	int i;
>  
>  	report_prefix_push("Channel Subsystem");
> +	enable_io_isc();
>  	for (i = 0; tests[i].name; i++) {
>  		report_prefix_push(tests[i].name);
>  		tests[i].func();

Re: [kvm-unit-tests PATCH v7 04/12] s390x: interrupt registration

[Pierre Morel]

On 2020-05-26 20:08, Thomas Huth wrote:
> On 18/05/2020 18.07, Pierre Morel wrote:
>> Let's make it possible to add and remove a custom io interrupt handler,
>> that can be used instead of the normal one.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
>>   lib/s390x/interrupt.h |  8 ++++++++
>>   2 files changed, 30 insertions(+), 1 deletion(-)
>>   create mode 100644 lib/s390x/interrupt.h
> ...
>> diff --git a/lib/s390x/interrupt.h b/lib/s390x/interrupt.h
>> new file mode 100644
>> index 0000000..323258d
>> --- /dev/null
>> +++ b/lib/s390x/interrupt.h
>> @@ -0,0 +1,8 @@
>> +#ifndef __INTERRUPT_H
>> +#define __INTERRUPT_H
> 
> Looking at this patch again, I noticed another nit: No double
> underscores at the beginning of header guards, please! That's reserved
> namespace. Simply use INTERRUPT_H or S390X_INTERRUPT_H or something
> similar instead.
> 
>   Thanks,
>    Thomas
> 

OK, thanks
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 05/12] s390x: export the clock get_clock_ms() utility

[Pierre Morel]

On 2020-05-26 20:10, Thomas Huth wrote:
> On 18/05/2020 18.07, Pierre Morel wrote:
>> To serve multiple times, the function get_clock_ms() is moved
>> from intercept.c test to the new file asm/time.h.

...snip...

>> index 0000000..25c7a3c
>> --- /dev/null
>> +++ b/lib/s390x/asm/time.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Clock utilities for s390
>> + *
>> + * Authors:
>> + *  Thomas Huth <thuth@redhat.com>
>> + *
>> + * Copied from the s390/intercept test by:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2.
>> + */
>> +#ifndef _ASM_S390X_TIME_H_
>> +#define _ASM_S390X_TIME_H_
> 
> Please also remove the underscores at the beginning (and preferably also
> at the end) here.

OK,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 06/12] s390x: use get_clock_ms() to calculate a delay in ms

[Pierre Morel]

On 2020-05-26 20:16, Thomas Huth wrote:
> On 18/05/2020 18.07, Pierre Morel wrote:
>> use get_clock_ms() to calculate a delay in ms
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/time.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
>> index 25c7a3c..931a119 100644
>> --- a/lib/s390x/asm/time.h
>> +++ b/lib/s390x/asm/time.h
>> @@ -23,4 +23,14 @@ static inline uint64_t get_clock_ms(void)
>>   	return (clk >> (63 - 51)) / 1000;
>>   }
>>   
>> +static inline void mdelay(unsigned long ms)
>> +{
>> +	unsigned long startclk;
>> +
>> +	startclk = get_clock_ms();
>> +	for (;;)
>> +		if (get_clock_ms() - startclk > ms)
>> +			break;
> 
> Maybe rather:
> 
>      for (;get_clock_ms() - startclk <= ms;)
> 	;
> 
> ?
> Or:
> 
>      while (get_clock_ms() - startclk <= ms)
>          ;
> ?
> 
>   Thomas
> 

Hi,

your comment made me realize I did not take care on the wrapping.
I will rework this.

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 07/12] s390x: Library resources for CSS tests

[Pierre Morel]

On 2020-05-26 18:30, Cornelia Huck wrote:
> On Mon, 18 May 2020 18:07:26 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Provide some definitions and library routines that can be used by
>> tests targeting the channel subsystem.
>>
>> Debug function can be activated by defining DEBUG_CSS before the
>> inclusion of the css.h header file.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 259 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
>>   s390x/Makefile       |   1 +
>>   3 files changed, 417 insertions(+)
>>   create mode 100644 lib/s390x/css.h
>>   create mode 100644 lib/s390x/css_dump.c
>>
> 
> (...)
> 
>> +struct ccw1 {
>> +	unsigned char code;
>> +	unsigned char flags;
>> +	unsigned short count;
> 
> I'm wondering why you're using unsigned {char,short} here, instead of
> the uint*_t types everywhere else? It's not wrong, but probably better
> to be consistent?
> 
>> +	uint32_t data_address;
>> +} __attribute__ ((aligned(4)));
>> +
>> +#define SID_ONE		0x00010000
>> +
> 
> I think it would be beneficial for the names to somewhat match the
> naming in Linux and/or QEMU -- or more speaking names (as you do for
> some), which is also good.
> 
>> +#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
> 
> ORB_F_ISIC? (As it does not refer to 'initialization', but 'initial'.)
> 
>> +#define ORB_F_ADDRLIMIT	0x00100000
>> +#define ORB_F_SUSP_IRQ	0x00080000
> 
> ORB_F_SSIC? (As it deals with suppression.)
> 
>> +#define ORB_F_TRANSPORT	0x00040000
>> +#define ORB_F_IDAW2	0x00020000
> 
> ORB_F_IDAW_FMT2?
> 
> Or following Linux/QEMU, use ORB_F_C64 for a certain retro appeal :)
> 
>> +#define ORB_F_IDAW_2K	0x00010000
>> +#define ORB_M_LPM	0x0000ff00
>> +#define ORB_F_LPM_DFLT	0x00008000
> 
> That's a default lpm of 0x80, right? It's a bit buried between the orb
> definitions, and it also seems to be more of a implementation choice --
> move it out from the flags here?
> 
>> +#define ORB_F_ILSM	0x00000080
> 
> ORB_F_ILS?
> 
>> +#define ORB_F_CCW_IND	0x00000040
> 
> ORB_F_MIDAW? I had a hard time figuring out that one :)
> 
>> +#define ORB_F_ORB_EXT	0x00000001
> 
> (...)
> 
>> +/*
>> + * Try o have a more human representation of the PMCW flags
> 
> s/o/to/
> 
>> + * each letter in the string represent the first
> 
> s/represent/represents/
> 
>> + * letter of the associated bit in the flag fields.
>> + */
> 
> (...)
> 
> Generally, looks good to me.
> 

Thanks a lot,
I take all your remarks and will update the names for better ones as you 
suggest.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Pierre Morel]

On 2020-05-27 10:55, Cornelia Huck wrote:
> On Mon, 18 May 2020 18:07:27 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> First step for testing the channel subsystem is to enumerate the css and
>> retrieve the css devices.
>>
>> This tests the success of STSCH I/O instruction, we do not test the
>> reaction of the VM for an instruction with wrong parameters.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/Makefile      |  1 +
>>   s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |  4 ++
>>   3 files changed, 94 insertions(+)
>>   create mode 100644 s390x/css.c
> 
> (...)
> 
>> +static void test_enumerate(void)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int cc;
>> +	int scn;
>> +	int scn_found = 0;
>> +	int dev_found = 0;
>> +
>> +	for (scn = 0; scn < 0xffff; scn++) {
>> +		cc = stsch(scn|SID_ONE, &schib);
>> +		switch (cc) {
>> +		case 0:		/* 0 means SCHIB stored */
>> +			break;
>> +		case 3:		/* 3 means no more channels */
>> +			goto out;
>> +		default:	/* 1 or 2 should never happened for STSCH */
>> +			report(0, "Unexpected cc=%d on subchannel number 0x%x",
>> +			       cc, scn);
>> +			return;
>> +		}
>> +
>> +		/* We currently only support type 0, a.k.a. I/O channels */
>> +		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
>> +			continue;
>> +
>> +		/* We ignore I/O channels without valid devices */
>> +		scn_found++;
>> +		if (!(pmcw->flags & PMCW_DNV))
>> +			continue;
>> +
>> +		/* We keep track of the first device as our test device */
>> +		if (!test_device_sid)
>> +			test_device_sid = scn | SID_ONE;
>> +
>> +		dev_found++;
>> +	}
>> +
>> +out:
>> +	report(dev_found,
>> +	       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
>> +	       scn, scn_found, dev_found);
> 
> Just wondering: with the current invocation, you expect to find exactly
> one subchannel with a valid device, right?
...snip...

>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 07013b2..a436ec0 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -83,3 +83,7 @@ extra_params = -m 1G
>>   [sclp-3g]
>>   file = sclp.elf
>>   extra_params = -m 3G
>> +
>> +[css]
>> +file = css.elf
>> +extra_params =-device ccw-pong
> 
> Hm... you could test enumeration even with a QEMU that does not include
> support for the pong device, right? Would it be worthwhile to split out
> a set of css tests that use e.g. a virtio-net-ccw device, and have a
> css-pong set of tests that require the pong device?
> 

Yes, you are right, using a virtio-net-ccw will allow to keep this test 
without waiting for the PONG device to exist.

@Thomas, what do you think? I will still have to figure something out 
for PONG tests but here, it should be OK with virtio-net-ccw.

Thanks a lot for the solution, :)

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Thomas Huth]

On 04/06/2020 13.35, Pierre Morel wrote:
> 
> 
> On 2020-05-27 10:55, Cornelia Huck wrote:
>> On Mon, 18 May 2020 18:07:27 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> First step for testing the channel subsystem is to enumerate the css and
>>> retrieve the css devices.
>>>
>>> This tests the success of STSCH I/O instruction, we do not test the
>>> reaction of the VM for an instruction with wrong parameters.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   s390x/Makefile      |  1 +
>>>   s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>>>   s390x/unittests.cfg |  4 ++
>>>   3 files changed, 94 insertions(+)
>>>   create mode 100644 s390x/css.c
>>
>> (...)
>>
>>> +static void test_enumerate(void)
>>> +{
>>> +    struct pmcw *pmcw = &schib.pmcw;
>>> +    int cc;
>>> +    int scn;
>>> +    int scn_found = 0;
>>> +    int dev_found = 0;
>>> +
>>> +    for (scn = 0; scn < 0xffff; scn++) {
>>> +        cc = stsch(scn|SID_ONE, &schib);
>>> +        switch (cc) {
>>> +        case 0:        /* 0 means SCHIB stored */
>>> +            break;
>>> +        case 3:        /* 3 means no more channels */
>>> +            goto out;
>>> +        default:    /* 1 or 2 should never happened for STSCH */
>>> +            report(0, "Unexpected cc=%d on subchannel number 0x%x",
>>> +                   cc, scn);
>>> +            return;
>>> +        }
>>> +
>>> +        /* We currently only support type 0, a.k.a. I/O channels */
>>> +        if (PMCW_CHANNEL_TYPE(pmcw) != 0)
>>> +            continue;
>>> +
>>> +        /* We ignore I/O channels without valid devices */
>>> +        scn_found++;
>>> +        if (!(pmcw->flags & PMCW_DNV))
>>> +            continue;
>>> +
>>> +        /* We keep track of the first device as our test device */
>>> +        if (!test_device_sid)
>>> +            test_device_sid = scn | SID_ONE;
>>> +
>>> +        dev_found++;
>>> +    }
>>> +
>>> +out:
>>> +    report(dev_found,
>>> +           "Tested subchannels: %d, I/O subchannels: %d, I/O
>>> devices: %d",
>>> +           scn, scn_found, dev_found);
>>
>> Just wondering: with the current invocation, you expect to find exactly
>> one subchannel with a valid device, right?
> ...snip...
> 
>>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>>> index 07013b2..a436ec0 100644
>>> --- a/s390x/unittests.cfg
>>> +++ b/s390x/unittests.cfg
>>> @@ -83,3 +83,7 @@ extra_params = -m 1G
>>>   [sclp-3g]
>>>   file = sclp.elf
>>>   extra_params = -m 3G
>>> +
>>> +[css]
>>> +file = css.elf
>>> +extra_params =-device ccw-pong
>>
>> Hm... you could test enumeration even with a QEMU that does not include
>> support for the pong device, right? Would it be worthwhile to split out
>> a set of css tests that use e.g. a virtio-net-ccw device, and have a
>> css-pong set of tests that require the pong device?
>>
> 
> Yes, you are right, using a virtio-net-ccw will allow to keep this test
> without waiting for the PONG device to exist.
> 
> @Thomas, what do you think? I will still have to figure something out
> for PONG tests but here, it should be OK with virtio-net-ccw.

Sure, sounds good. We can go with -device virtio-net-ccw for now, and
then later add an additional entry a la:

[css-pong]
file = css.elf
device = ccw-pong

... where the test scripts then check for the availability of the device
first before starting the test?

 Thomas

Re: [kvm-unit-tests PATCH v7 08/12] s390x: css: stsch, enumeration test

[Pierre Morel]

On 2020-06-04 13:45, Thomas Huth wrote:
> On 04/06/2020 13.35, Pierre Morel wrote:
>>
>>
>> On 2020-05-27 10:55, Cornelia Huck wrote:
>>> On Mon, 18 May 2020 18:07:27 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>
>>>> First step for testing the channel subsystem is to enumerate the css and
>>>> retrieve the css devices.
>>>>
>>>> This tests the success of STSCH I/O instruction, we do not test the
>>>> reaction of the VM for an instruction with wrong parameters.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    s390x/Makefile      |  1 +
>>>>    s390x/css.c         | 89 +++++++++++++++++++++++++++++++++++++++++++++
>>>>    s390x/unittests.cfg |  4 ++
>>>>    3 files changed, 94 insertions(+)
>>>>    create mode 100644 s390x/css.c
>>>
>>> (...)
>>>
>>>> +static void test_enumerate(void)
>>>> +{
>>>> +    struct pmcw *pmcw = &schib.pmcw;
>>>> +    int cc;
>>>> +    int scn;
>>>> +    int scn_found = 0;
>>>> +    int dev_found = 0;
>>>> +
>>>> +    for (scn = 0; scn < 0xffff; scn++) {
>>>> +        cc = stsch(scn|SID_ONE, &schib);
>>>> +        switch (cc) {
>>>> +        case 0:        /* 0 means SCHIB stored */
>>>> +            break;
>>>> +        case 3:        /* 3 means no more channels */
>>>> +            goto out;
>>>> +        default:    /* 1 or 2 should never happened for STSCH */
>>>> +            report(0, "Unexpected cc=%d on subchannel number 0x%x",
>>>> +                   cc, scn);
>>>> +            return;
>>>> +        }
>>>> +
>>>> +        /* We currently only support type 0, a.k.a. I/O channels */
>>>> +        if (PMCW_CHANNEL_TYPE(pmcw) != 0)
>>>> +            continue;
>>>> +
>>>> +        /* We ignore I/O channels without valid devices */
>>>> +        scn_found++;
>>>> +        if (!(pmcw->flags & PMCW_DNV))
>>>> +            continue;
>>>> +
>>>> +        /* We keep track of the first device as our test device */
>>>> +        if (!test_device_sid)
>>>> +            test_device_sid = scn | SID_ONE;
>>>> +
>>>> +        dev_found++;
>>>> +    }
>>>> +
>>>> +out:
>>>> +    report(dev_found,
>>>> +           "Tested subchannels: %d, I/O subchannels: %d, I/O
>>>> devices: %d",
>>>> +           scn, scn_found, dev_found);
>>>
>>> Just wondering: with the current invocation, you expect to find exactly
>>> one subchannel with a valid device, right?
>> ...snip...
>>
>>>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>>>> index 07013b2..a436ec0 100644
>>>> --- a/s390x/unittests.cfg
>>>> +++ b/s390x/unittests.cfg
>>>> @@ -83,3 +83,7 @@ extra_params = -m 1G
>>>>    [sclp-3g]
>>>>    file = sclp.elf
>>>>    extra_params = -m 3G
>>>> +
>>>> +[css]
>>>> +file = css.elf
>>>> +extra_params =-device ccw-pong
>>>
>>> Hm... you could test enumeration even with a QEMU that does not include
>>> support for the pong device, right? Would it be worthwhile to split out
>>> a set of css tests that use e.g. a virtio-net-ccw device, and have a
>>> css-pong set of tests that require the pong device?
>>>
>>
>> Yes, you are right, using a virtio-net-ccw will allow to keep this test
>> without waiting for the PONG device to exist.
>>
>> @Thomas, what do you think? I will still have to figure something out
>> for PONG tests but here, it should be OK with virtio-net-ccw.
> 
> Sure, sounds good. We can go with -device virtio-net-ccw for now, and
> then later add an additional entry a la:
> 
> [css-pong]
> file = css.elf
> device = ccw-pong
> 
> ... where the test scripts then check for the availability of the device
> first before starting the test?
> 
>   Thomas
> 

yes,
thanks,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Pierre Morel]

On 2020-05-27 11:42, Cornelia Huck wrote:
> On Mon, 18 May 2020 18:07:28 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> A second step when testing the channel subsystem is to prepare a channel
>> for use.
>> This includes:
>> - Get the current subchannel Information Block (SCHIB) using STSCH
>> - Update it in memory to set the ENABLE bit
>> - Tell the CSS that the SCHIB has been modified using MSCH
>> - Get the SCHIB from the CSS again to verify that the subchannel is
>>    enabled.
>> - If the subchannel is not enabled retry a predefined retries count.
>>
>> This tests the MSCH instruction to enable a channel succesfuly.
>> This is NOT a routine to really enable the channel, no retry is done,
>> in case of error, a report is made.
> 
> Hm... so you retry if the subchannel is not enabled after cc 0, but you
> don't retry if the cc indicates busy/status pending? Makes sense, as we
> don't expect the subchannel to be busy, but a more precise note in the
> patch description would be good :)

OK, I add something like
"
- If the command succeed but subchannel is not enabled retry a
   predefined retries count.
- If the command fails, report the failure and do not retry, even
   if cc indicates a busy/status as we do not expect this.
"

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 67 insertions(+)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index d7989d8..1b60a47 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -16,6 +16,7 @@
>>   #include <string.h>
>>   #include <interrupt.h>
>>   #include <asm/arch_def.h>
>> +#include <asm/time.h>
>>   
>>   #include <css.h>
>>   
>> @@ -65,11 +66,77 @@ out:
>>   	       scn, scn_found, dev_found);
>>   }
>>   
>> +#define MAX_ENABLE_RETRIES	5
>> +static void test_enable(void)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int retry_count = 0;
>> +	int cc;
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
>> +
>> +	/* Read the SCHIB for this subchannel */
>> +	cc = stsch(test_device_sid, &schib);
>> +	if (cc) {
>> +		report(0, "stsch cc=%d", cc);
>> +		return;
>> +	}
>> +
>> +	if (pmcw->flags & PMCW_ENABLE) {
>> +		report(1, "stsch: sch %08x already enabled", test_device_sid);
>> +		return;
>> +	}
>> +
>> +retry:
>> +	/* Update the SCHIB to enable the channel */
>> +	pmcw->flags |= PMCW_ENABLE;
>> +
>> +	/* Tell the CSS we want to modify the subchannel */
>> +	cc = msch(test_device_sid, &schib);
>> +	if (cc) {
>> +		/*
>> +		 * If the subchannel is status pending or
>> +		 * if a function is in progress,
>> +		 * we consider both cases as errors.
> 
> Could also be cc 3, but that would be even more weird. Just logging the
> cc seems fine, though.
> 
>> +		 */
>> +		report(0, "msch cc=%d", cc);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Read the SCHIB again to verify the enablement
>> +	 */
>> +	cc = stsch(test_device_sid, &schib);
>> +	if (cc) {
>> +		report(0, "stsch cc=%d", cc);
>> +		return;
>> +	}
>> +
>> +	if (pmcw->flags & PMCW_ENABLE) {
>> +		report(1, "msch: sch %08x enabled after %d retries",
>> +		       test_device_sid, retry_count);
>> +		return;
>> +	}
>> +
>> +	if (retry_count++ < MAX_ENABLE_RETRIES) {
>> +		mdelay(10); /* the hardware was not ready, let it some time */
> 
> s/let/give/
> 
>> +		goto retry;
>> +	}
>> +
>> +	report(0,
>> +	       "msch: enabling sch %08x failed after %d retries. pmcw: %x",
>> +	       test_device_sid, retry_count, pmcw->flags);
>> +}
>> +
>>   static struct {
>>   	const char *name;
>>   	void (*func)(void);
>>   } tests[] = {
>>   	{ "enumerate (stsch)", test_enumerate },
>> +	{ "enable (msch)", test_enable },
>>   	{ NULL, NULL }
>>   };
>>   
> 
> Otherwise,
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks a lot.
I make the corrections.

regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 10/12] s390x: define function to wait for interrupt

[Pierre Morel]

On 2020-05-27 11:45, Cornelia Huck wrote:
> On Mon, 18 May 2020 18:07:29 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Allow the program to wait for an interrupt.
>>
>> The interrupt handler is in charge to remove the WAIT bit
>> when it finished handling the interrupt.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Cornelia Huck]

On Thu, 4 Jun 2020 14:46:05 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-05-27 11:42, Cornelia Huck wrote:
> > On Mon, 18 May 2020 18:07:28 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> A second step when testing the channel subsystem is to prepare a channel
> >> for use.
> >> This includes:
> >> - Get the current subchannel Information Block (SCHIB) using STSCH
> >> - Update it in memory to set the ENABLE bit
> >> - Tell the CSS that the SCHIB has been modified using MSCH
> >> - Get the SCHIB from the CSS again to verify that the subchannel is
> >>    enabled.
> >> - If the subchannel is not enabled retry a predefined retries count.
> >>
> >> This tests the MSCH instruction to enable a channel succesfuly.
> >> This is NOT a routine to really enable the channel, no retry is done,
> >> in case of error, a report is made.  
> > 
> > Hm... so you retry if the subchannel is not enabled after cc 0, but you
> > don't retry if the cc indicates busy/status pending? Makes sense, as we
> > don't expect the subchannel to be busy, but a more precise note in the
> > patch description would be good :)  
> 
> OK, I add something like
> "
> - If the command succeed but subchannel is not enabled retry a

s/succeed/succeeds/ :)

>    predefined retries count.
> - If the command fails, report the failure and do not retry, even
>    if cc indicates a busy/status as we do not expect this.

"indicates busy/status pending" ?

> "

Re: [kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Pierre Morel]

On 2020-06-04 15:29, Cornelia Huck wrote:
> On Thu, 4 Jun 2020 14:46:05 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-05-27 11:42, Cornelia Huck wrote:
>>> On Mon, 18 May 2020 18:07:28 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> A second step when testing the channel subsystem is to prepare a channel
>>>> for use.
>>>> This includes:
>>>> - Get the current subchannel Information Block (SCHIB) using STSCH
>>>> - Update it in memory to set the ENABLE bit
>>>> - Tell the CSS that the SCHIB has been modified using MSCH
>>>> - Get the SCHIB from the CSS again to verify that the subchannel is
>>>>     enabled.
>>>> - If the subchannel is not enabled retry a predefined retries count.
>>>>
>>>> This tests the MSCH instruction to enable a channel succesfuly.
>>>> This is NOT a routine to really enable the channel, no retry is done,
>>>> in case of error, a report is made.
>>>
>>> Hm... so you retry if the subchannel is not enabled after cc 0, but you
>>> don't retry if the cc indicates busy/status pending? Makes sense, as we
>>> don't expect the subchannel to be busy, but a more precise note in the
>>> patch description would be good :)
>>
>> OK, I add something like
>> "
>> - If the command succeed but subchannel is not enabled retry a
> 
> s/succeed/succeeds/ :)
> 
>>     predefined retries count.
>> - If the command fails, report the failure and do not retry, even
>>     if cc indicates a busy/status as we do not expect this.
> 
> "indicates busy/status pending" ?
> 
>> "
> 
done, thanks,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 11/12] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

On 2020-05-27 12:09, Cornelia Huck wrote:
> On Mon, 18 May 2020 18:07:30 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We add a new css_lib file to contain the I/O functions we may
>> share with different tests.
>> First function is the subchannel_enable() function.
>>
>> When a channel is enabled we can start a SENSE_ID command using
>> the SSCH instruction to recognize the control unit and device.
>>
>> This tests the success of SSCH, the I/O interruption and the TSCH
>> instructions.
>>
>> The test expects a device with a control unit type of 0xC0CA as the
>> first subchannel of the CSS.
> 
> It might make sense to extend this to be able to check for any expected
> type (e.g. 0x3832, should my suggestion to split css tests and css-pong
> tests make sense.)

right.

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  20 ++++++
>>   lib/s390x/css_lib.c |  55 +++++++++++++++++
>>   s390x/Makefile      |   1 +
>>   s390x/css.c         | 145 ++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 221 insertions(+)
>>   create mode 100644 lib/s390x/css_lib.c
> 
> (...)
> 
>> +int enable_subchannel(unsigned int sid)
>> +{
>> +	struct schib schib;
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +	int try_count = 5;
>> +	int cc;
>> +
>> +	if (!(sid & SID_ONE))
>> +		return -1;
> 
> Hm... this error is indistinguishable for the caller from a cc 1 for
> the msch. Use something else (as this is a coding error)?

right it is a coding error -> assert ?

> 
>> +
>> +	cc = stsch(sid, &schib);
>> +	if (cc)
>> +		return -cc;
>> +
>> +	do {
>> +		pmcw->flags |= PMCW_ENABLE;
>> +
>> +		cc = msch(sid, &schib);
>> +		if (cc)
>> +			return -cc;
>> +
>> +		cc = stsch(sid, &schib);
>> +		if (cc)
>> +			return -cc;
>> +
>> +	} while (!(pmcw->flags & PMCW_ENABLE) && --try_count);
>> +
>> +	return try_count;
> 
> How useful is that information for the caller? I don't see the code
> below making use of it.

right,
I will change the fail cases to a report_info and return 0 in case of 
success.

> 
>> +}
>> +
>> +int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
>> +{
>> +	struct orb orb;
>> +
>> +	orb.intparm = sid;
> 
> Just an idea: If you use something else here (maybe the cpa), and set
> the intparm to the sid in msch, you can test something else: Does msch
> properly set the intparm, and is that intparm overwritten by a
> successful ssch, until the next ssch or msch comes around?

good idea.
Using cpa is all what we need at the current development.


> 
>> +	orb.ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
>> +	orb.cpa = (unsigned int) (unsigned long)ccw;
> 
> Use a struct initializer, so that unset fields are 0?

not only more beautifull but effective. thanks!

> 
>> +
>> +	return ssch(sid, &orb);
>> +}
> 
> (...)
> 
>> +/*
>> + * test_sense
>> + * Pre-requisits:
>> + * - We need the QEMU PONG device as the first recognized
>> + *   device by the enumeration.
>> + * - ./s390x-run s390x/css.elf -device ccw-pong,cu_type=0xc0ca
>> + */
>> +static void test_sense(void)
>> +{
>> +	int ret;
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
>> +
>> +	ret = enable_subchannel(test_device_sid);
>> +	if (ret < 0) {
>> +		report(0,
>> +		       "Could not enable the subchannel: %08x",
>> +		       test_device_sid);
>> +		return;
>> +	}
>> +
>> +	ret = register_io_int_func(irq_io);
>> +	if (ret) {
>> +		report(0, "Could not register IRQ handler");
>> +		goto unreg_cb;
>> +	}
>> +
>> +	lowcore->io_int_param = 0;
>> +
>> +	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid),
>> +			       CCW_F_SLI);
> 
> Clear senseid, before actually sending the program?

yes.

> 
>> +	if (!ret) {
>> +		report(0, "ssch failed for SENSE ID on sch %08x",
>> +		       test_device_sid);
>> +		goto unreg_cb;
>> +	}
>> +
>> +	wait_for_interrupt(PSW_MASK_IO);
>> +
>> +	if (lowcore->io_int_param != test_device_sid)
>> +		goto unreg_cb;
>> +
>> +	report_info("reserved %02x cu_type %04x cu_model %02x dev_type %04x dev_model %02x",
>> +		    senseid.reserved, senseid.cu_type, senseid.cu_model,
>> +		    senseid.dev_type, senseid.dev_model);
>> +
> 
> I'd also recommend checking that senseid.reserved is indeed 0xff -- in
> combination with senseid clearing before the ssch, that ensures that
> the senseid structure has actually been written to and is not pure
> garbage. (It's also a cu type agnostic test :)

good idea, thanks.

> 
> It also might make sense to check how much data you actually got, as
> you set SLI.
> 
> 

Yes, will do.

Thanks for the comments, I make the changes for the next revision.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Pierre Morel]

Hi Connie,

for the next series, I will move more code of the css.c to the css_lib.c 
to be able to reuse it for other tests.
One of your suggestions IIRC.
So I will not be able to keep your RB until you have a look at the changes.
I will keep the same algorithms but still...

regards,
Pierre

On 2020-06-05 09:37, Pierre Morel wrote:
> 
> 
> On 2020-06-04 15:29, Cornelia Huck wrote:
>> On Thu, 4 Jun 2020 14:46:05 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 2020-05-27 11:42, Cornelia Huck wrote:
>>>> On Mon, 18 May 2020 18:07:28 +0200
>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>> A second step when testing the channel subsystem is to prepare a 
>>>>> channel
>>>>> for use.
>>>>> This includes:
>>>>> - Get the current subchannel Information Block (SCHIB) using STSCH
>>>>> - Update it in memory to set the ENABLE bit
>>>>> - Tell the CSS that the SCHIB has been modified using MSCH
>>>>> - Get the SCHIB from the CSS again to verify that the subchannel is
>>>>>     enabled.
>>>>> - If the subchannel is not enabled retry a predefined retries count.
>>>>>
>>>>> This tests the MSCH instruction to enable a channel succesfuly.
>>>>> This is NOT a routine to really enable the channel, no retry is done,
>>>>> in case of error, a report is made.
>>>>
>>>> Hm... so you retry if the subchannel is not enabled after cc 0, but you
>>>> don't retry if the cc indicates busy/status pending? Makes sense, as we
>>>> don't expect the subchannel to be busy, but a more precise note in the
>>>> patch description would be good :)
>>>
>>> OK, I add something like
>>> "
>>> - If the command succeed but subchannel is not enabled retry a
>>
>> s/succeed/succeeds/ :)
>>
>>>     predefined retries count.
>>> - If the command fails, report the failure and do not retry, even
>>>     if cc indicates a busy/status as we do not expect this.
>>
>> "indicates busy/status pending" ?
>>
>>> "
>>
> done, thanks,
> 
> Pierre
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v7 09/12] s390x: css: msch, enable test

[Cornelia Huck]

On Fri, 5 Jun 2020 10:24:56 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Hi Connie,
> 
> for the next series, I will move more code of the css.c to the css_lib.c 
> to be able to reuse it for other tests.
> One of your suggestions IIRC.
> So I will not be able to keep your RB until you have a look at the changes.
> I will keep the same algorithms but still...

np, I'll look at it again.

Re: [kvm-unit-tests PATCH v7 11/12] s390x: css: ssch/tsch with sense and interrupt

[Cornelia Huck]

On Fri, 5 Jun 2020 09:37:39 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-05-27 12:09, Cornelia Huck wrote:
> > On Mon, 18 May 2020 18:07:30 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> We add a new css_lib file to contain the I/O functions we may
> >> share with different tests.
> >> First function is the subchannel_enable() function.
> >>
> >> When a channel is enabled we can start a SENSE_ID command using
> >> the SSCH instruction to recognize the control unit and device.
> >>
> >> This tests the success of SSCH, the I/O interruption and the TSCH
> >> instructions.
> >>
> >> The test expects a device with a control unit type of 0xC0CA as the
> >> first subchannel of the CSS.  
> > 
> > It might make sense to extend this to be able to check for any expected
> > type (e.g. 0x3832, should my suggestion to split css tests and css-pong
> > tests make sense.)  
> 
> right.
> 
> >   
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >>   lib/s390x/css.h     |  20 ++++++
> >>   lib/s390x/css_lib.c |  55 +++++++++++++++++
> >>   s390x/Makefile      |   1 +
> >>   s390x/css.c         | 145 ++++++++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 221 insertions(+)
> >>   create mode 100644 lib/s390x/css_lib.c  
> > 
> > (...)
> >   
> >> +int enable_subchannel(unsigned int sid)
> >> +{
> >> +	struct schib schib;
> >> +	struct pmcw *pmcw = &schib.pmcw;
> >> +	int try_count = 5;
> >> +	int cc;
> >> +
> >> +	if (!(sid & SID_ONE))
> >> +		return -1;  
> > 
> > Hm... this error is indistinguishable for the caller from a cc 1 for
> > the msch. Use something else (as this is a coding error)?  
> 
> right it is a coding error -> assert ?

Sounds good to me.