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

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

[Pierre Morel]

Hi All,

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 0x3832, a.k.a. virtio-net-ccw.
        Checks SSCH(READ) and IO-IRQ

Note:
- The following patches are general usage and may be pulled first:
  s390x: Use PSW bits definitions in cstart
  s390x: Move control register bit definitions and add AFP to them
  s390x: saving regs for interrupts
  s390x: interrupt registration
  s390x: export the clock get_clock_ms() utility
  s390x: clock and delays caluculations
  s390x: define function to wait for interrupt
  s390x: retrieve decimal and hexadecimal kernel parameters

- These 4 patches are really I/O oriented:
  s390x: Library resources for CSS tests
  s390x: css: stsch, enumeration test
  s390x: css: msch, enable test
  s390x: css: ssch/tsch with sense and interrupt

Regards,
Pierre

Pierre Morel (12):
  s390x: Use PSW bits definitions in cstart
  s390x: Move control register bit definitions and add AFP to them
  s390x: saving regs for interrupts
  s390x: interrupt registration
  s390x: export the clock get_clock_ms() utility
  s390x: clock and delays caluculations
  s390x: define function to wait for interrupt
  s390x: retrieve decimal and hexadecimal kernel parameters
  s390x: Library resources for CSS tests
  s390x: css: stsch, enumeration test
  s390x: css: msch, enable test
  s390x: css: ssch/tsch with sense and interrupt

 lib/s390x/asm/arch_def.h |  32 ++++-
 lib/s390x/asm/time.h     |  51 ++++++++
 lib/s390x/css.h          | 276 +++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c     | 153 ++++++++++++++++++++++
 lib/s390x/css_lib.c      | 176 +++++++++++++++++++++++++
 lib/s390x/interrupt.c    |  23 +++-
 lib/s390x/interrupt.h    |   8 ++
 lib/s390x/kernel-args.c  |  60 +++++++++
 lib/s390x/kernel-args.h  |  18 +++
 s390x/Makefile           |   4 +
 s390x/css.c              | 208 +++++++++++++++++++++++++++++
 s390x/cstart64.S         |  58 ++++++--
 s390x/intercept.c        |  11 +-
 s390x/unittests.cfg      |   4 +
 14 files changed, 1056 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 lib/s390x/kernel-args.c
 create mode 100644 lib/s390x/kernel-args.h
 create mode 100644 s390x/css.c

-- 
2.25.1

Changelog:

from v8 to v9:

- rename PSW_EXCEPTION_MASK to PSW_MASK_ON_EXCEPTION
  (Thomas)

- changed false max microseconds in delay calculation
  (Thomas)

- fix bug in decimal parameter calculation
  (Thomas)

- fix bug in msch inline assembly
  (Thomas)

- add report_abort() for unprobable result of tsch
  in I/O IRQ
  (Thomas)

- use of existing lctlg() wrapper instead of new function
  (Thomas)

from v7 to v8

* Suppress ccw-pong specific remote device
  (Thomas, Janosch)

* use virtio-net-ccw as device instead of a specific
  device: no more need for QEMU patch
  (Connie)

* Add kernel parameter to access a different device.
  (Connie)

* Add tests on subschannel reading, length, garbage.
  (Connie)

* Several naming changes and reorganizations of definitions.
  (Connie)

* Take wrapping into account for delay calculation

* Align CCW1 on 8 bytes boundary

* Reorganize the first three patches
  (Janosch)

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 v9 01/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>
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 1b3bb0c..b5d7aca 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_MASK_ON_EXCEPTION	(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 e084f13..d386f35 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
 
@@ -198,19 +199,19 @@ svc_int:
 
 	.align	8
 reset_psw:
-	.quad	0x0008000180000000
+	.quad	PSW_MASK_ON_EXCEPTION | PSW_MASK_SHORT_PSW
 initial_psw:
-	.quad	0x0000000180000000, clear_bss_start
+	.quad	PSW_MASK_ON_EXCEPTION, clear_bss_start
 pgm_int_psw:
-	.quad	0x0000000180000000, pgm_int
+	.quad	PSW_MASK_ON_EXCEPTION, pgm_int
 ext_int_psw:
-	.quad	0x0000000180000000, ext_int
+	.quad	PSW_MASK_ON_EXCEPTION, ext_int
 mcck_int_psw:
-	.quad	0x0000000180000000, mcck_int
+	.quad	PSW_MASK_ON_EXCEPTION, mcck_int
 io_int_psw:
-	.quad	0x0000000180000000, io_int
+	.quad	PSW_MASK_ON_EXCEPTION, io_int
 svc_int_psw:
-	.quad	0x0000000180000000, svc_int
+	.quad	PSW_MASK_ON_EXCEPTION, 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 v9 02/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>
Acked-by: Thomas Huth <thuth@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 b5d7aca..fa71653 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -19,17 +19,18 @@
 
 #define PSW_MASK_ON_EXCEPTION	(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 d386f35..e7327ea 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -214,4 +214,4 @@ svc_int_psw:
 	.quad	PSW_MASK_ON_EXCEPTION, 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 v9 03/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>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index e7327ea..7ca9496 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -119,6 +119,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 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 multiple 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 that we're 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:
@@ -186,9 +223,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 v9 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..1973d26
--- /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 v9 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..1791380
--- /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 v9 06/12] s390x: clock and delays caluculations

[Pierre Morel]

The hardware gives us a good definition of the microsecond,
let's keep this information and let the routine accessing
the hardware keep all the information and return microseconds.

Calculate delays in microseconds and take care about wrapping
around zero.

Define values with macros and use inlines to keep the
milliseconds interface.

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

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
index 1791380..7f1d891 100644
--- a/lib/s390x/asm/time.h
+++ b/lib/s390x/asm/time.h
@@ -13,14 +13,39 @@
 #ifndef ASM_S390X_TIME_H
 #define ASM_S390X_TIME_H
 
-static inline uint64_t get_clock_ms(void)
+#define STCK_SHIFT_US	(63 - 51)
+#define STCK_MAX	((1UL << 52) - 1)
+
+static inline uint64_t get_clock_us(void)
 {
 	uint64_t clk;
 
 	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
 
 	/* Bit 51 is incrememented each microsecond */
-	return (clk >> (63 - 51)) / 1000;
+	return clk >> STCK_SHIFT_US;
+}
+
+static inline void udelay(unsigned long us)
+{
+	unsigned long startclk = get_clock_us();
+	unsigned long c;
+
+	do {
+		c = get_clock_us();
+		if (c < startclk)
+			c += STCK_MAX;
+	} while (c < startclk + us);
+}
+
+static inline void mdelay(unsigned long ms)
+{
+	udelay(ms * 1000);
+}
+
+static inline uint64_t get_clock_ms(void)
+{
+	return get_clock_us() / 1000;
 }
 
 #endif
-- 
2.25.1

[kvm-unit-tests PATCH v9 07/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>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.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 fa71653..ff2ee20 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 v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Pierre Morel]

We often need to retrieve hexadecimal kernel parameters.
Let's implement a shared utility to do it.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
 lib/s390x/kernel-args.h | 18 +++++++++++++
 s390x/Makefile          |  1 +
 3 files changed, 79 insertions(+)
 create mode 100644 lib/s390x/kernel-args.c
 create mode 100644 lib/s390x/kernel-args.h

diff --git a/lib/s390x/kernel-args.c b/lib/s390x/kernel-args.c
new file mode 100644
index 0000000..2d3b2c2
--- /dev/null
+++ b/lib/s390x/kernel-args.c
@@ -0,0 +1,60 @@
+/*
+ * Retrieving kernel arguments
+ *
+ * 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 <string.h>
+#include <asm/arch_def.h>
+#include <kernel-args.h>
+
+static const char *hex_digit = "0123456789abcdef";
+
+static unsigned long htol(char *s)
+{
+	unsigned long v = 0, shift = 0, value = 0;
+	int i, digit, len = strlen(s);
+
+	for (shift = 0, i = len - 1; i >= 0; i--, shift += 4) {
+		digit = s[i] | 0x20;	/* Set lowercase */
+		if (!strchr(hex_digit, digit))
+			return 0;	/* this is not a digit ! */
+
+		if (digit <= '9')
+			v = digit - '0';
+		else
+			v = digit - 'a' + 10;
+		value += (v << shift);
+	}
+
+	return value;
+}
+
+int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val)
+{
+	int i, ret;
+	char *p, *q;
+
+	for (i = 0; i < argc; i++) {
+		ret = strncmp(argv[i], str, strlen(str));
+		if (ret)
+			continue;
+		p = strchr(argv[i], '=');
+		if (!p)
+			return -1;
+		q = strchr(p, 'x');
+		if (!q)
+			*val = atol(p + 1);
+		else
+			*val = htol(q + 1);
+		return 0;
+	}
+	return -2;
+}
diff --git a/lib/s390x/kernel-args.h b/lib/s390x/kernel-args.h
new file mode 100644
index 0000000..a88e34e
--- /dev/null
+++ b/lib/s390x/kernel-args.h
@@ -0,0 +1,18 @@
+/*
+ * Kernel argument
+ *
+ * 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.
+ */
+
+#ifndef KERNEL_ARGS_H
+#define KERNEL_ARGS_H
+
+int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val);
+
+#endif
diff --git a/s390x/Makefile b/s390x/Makefile
index ddb4b48..47a94cc 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/kernel-args.o
 
 OBJDIRS += lib/s390x
 
-- 
2.25.1

[kvm-unit-tests PATCH v9 09/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.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
 s390x/Makefile       |   1 +
 3 files changed, 410 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..0ddceb1
--- /dev/null
+++ b/lib/s390x/css.h
@@ -0,0 +1,256 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2020
+ * Author: 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 CSS_H
+#define CSS_H
+
+/* subchannel ID bit 16 must always be one */
+#define SCHID_ONE	0x00010000
+
+#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 {
+	uint8_t code;
+	uint8_t flags;
+	uint16_t count;
+	uint32_t data_address;
+} __attribute__ ((aligned(8)));
+
+#define ORB_CTRL_KEY	0xf0000000
+#define ORB_CTRL_SPND	0x08000000
+#define ORB_CTRL_STR	0x04000000
+#define ORB_CTRL_MOD	0x02000000
+#define ORB_CTRL_SYNC	0x01000000
+#define ORB_CTRL_FMT	0x00800000
+#define ORB_CTRL_PFCH	0x00400000
+#define ORB_CTRL_ISIC	0x00200000
+#define ORB_CTRL_ALCC	0x00100000
+#define ORB_CTRL_SSIC	0x00080000
+#define ORB_CTRL_CPTC	0x00040000
+#define ORB_CTRL_C64	0x00020000
+#define ORB_CTRL_I2K	0x00010000
+#define ORB_CTRL_LPM	0x0000ff00
+#define ORB_CTRL_ILS	0x00000080
+#define ORB_CTRL_MIDAW	0x00000040
+#define ORB_CTRL_ORBX	0x00000001
+
+#define ORB_LPM_DFLT	0x00008000
+
+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)
+		: "d" (reg1), "m" (*addr), "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);
+
+void dump_scsw(struct scsw *scsw);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);
+struct ccw1 *dump_ccw(struct ccw1 *cp);
+void dump_irb(struct irb *irbp);
+void dump_pmcw(struct pmcw *p);
+void dump_orb(struct orb *op);
+
+int css_enumerate(void);
+#define MAX_ENABLE_RETRIES      5
+int css_enable(int schid);
+
+#endif
diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
new file mode 100644
index 0000000..0c2b64e
--- /dev/null
+++ b/lib/s390x/css_dump.c
@@ -0,0 +1,153 @@
+/*
+ * 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 <libcflat.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <css.h>
+
+/*
+ * Try to have a more human representation of the SCSW flags:
+ * each letter in the two strings represents 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 to have a more human representation of the PMCW flags
+ * each letter in the string represents 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;
+}
+
+void dump_scsw(struct scsw *s)
+{
+	dump_scsw_flags(s->ctrl);
+	printf("scsw->flags: %s\n", scsw_line);
+	printf("scsw->addr : %08x\n", s->ccw_addr);
+	printf("scsw->devs : %02x\n", s->dev_stat);
+	printf("scsw->schs : %02x\n", s->sch_stat);
+	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 ccw1 *dump_ccw(struct ccw1 *cp)
+{
+	printf("CCW: code: %02x flags: %02x count: %04x data: %08x\n", cp->code,
+	    cp->flags, cp->count, cp->data_address);
+
+	if (cp->code == CCW_C_TIC)
+		return (struct ccw1 *)(long)cp->data_address;
+
+	return (cp->flags & CCW_F_CC) ? cp + 1 : NULL;
+}
+
+void dump_orb(struct orb *op)
+{
+	struct ccw1 *cp;
+
+	printf("ORB: intparm : %08x\n", op->intparm);
+	printf("ORB: ctrl    : %08x\n", op->ctrl);
+	printf("ORB: prio    : %08x\n", op->prio);
+	cp = (struct ccw1 *)(long) (op->cpa);
+	while (cp)
+		cp = dump_ccw(cp);
+}
diff --git a/s390x/Makefile b/s390x/Makefile
index 47a94cc..3cb97da 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -52,6 +52,7 @@ cflatobjs += lib/s390x/interrupt.o
 cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/kernel-args.o
+cflatobjs += lib/s390x/css_dump.o
 
 OBJDIRS += lib/s390x
 
-- 
2.25.1

[kvm-unit-tests PATCH v9 10/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>
---
 lib/s390x/css_lib.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/Makefile      |  2 ++
 s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 4 files changed, 131 insertions(+)
 create mode 100644 lib/s390x/css_lib.c
 create mode 100644 s390x/css.c

diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
new file mode 100644
index 0000000..b0a0294
--- /dev/null
+++ b/lib/s390x/css_lib.c
@@ -0,0 +1,70 @@
+/*
+ * Channel Subsystem tests library
+ *
+ * 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;
+
+/*
+ * css_enumerate:
+ * On success return the first subchannel ID found.
+ * On error return an invalid subchannel ID containing cc
+ */
+int css_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int scn_found = 0;
+	int dev_found = 0;
+	int schid = 0;
+	int cc;
+	int scn;
+
+	for (scn = 0; scn < 0xffff; scn++) {
+		cc = stsch(scn | SCHID_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_abort("Unexpected error %d on subchannel %08x",
+				     cc, scn | SCHID_ONE);
+			return cc;
+		}
+
+		/* 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 (!schid)
+			schid = scn | SCHID_ONE;
+		report_info("Found subchannel %08x", scn | SCHID_ONE);
+		dev_found++;
+	}
+
+out:
+	report_info("Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+		    scn, scn_found, dev_found);
+	return schid;
+}
diff --git a/s390x/Makefile b/s390x/Makefile
index 3cb97da..afd2c9b 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
@@ -53,6 +54,7 @@ cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/kernel-args.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
new file mode 100644
index 0000000..230adf5
--- /dev/null
+++ b/s390x/css.c
@@ -0,0 +1,55 @@
+/*
+ * 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 int test_device_sid;
+
+static void test_enumerate(void)
+{
+	test_device_sid = css_enumerate();
+	if (test_device_sid & SCHID_ONE) {
+		report(1, "First device schid: 0x%08x", test_device_sid);
+		return;
+	}
+	report(0, "No I/O device 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 b307329..0f156af 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -84,3 +84,7 @@ extra_params = -m 1G
 [sclp-3g]
 file = sclp.elf
 extra_params = -m 3G
+
+[css]
+file = css.elf
+extra_params = -device virtio-net-ccw
-- 
2.25.1

[kvm-unit-tests PATCH v9 11/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 command succeeds 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 pending as we do not expect this.

This tests the MSCH instruction to enable a channel succesfuly.
This some retries are done and in case of error, and if the retries
count is exceeded, a report is made.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css_lib.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/css.c         | 15 ++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index b0a0294..06a76db 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -15,6 +15,7 @@
 #include <string.h>
 #include <interrupt.h>
 #include <asm/arch_def.h>
+#include <asm/time.h>
 
 #include <css.h>
 
@@ -68,3 +69,62 @@ out:
 		    scn, scn_found, dev_found);
 	return schid;
 }
+
+int css_enable(int schid)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int retry_count = 0;
+	int cc;
+
+	/* Read the SCHIB for this subchannel */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch failed with cc=%d", cc);
+		return cc;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report_info("stsch: sch %08x already enabled", schid);
+		return 0;
+	}
+
+retry:
+	/* Update the SCHIB to enable the channel */
+	pmcw->flags |= PMCW_ENABLE;
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(schid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report_info("msch failed with cc=%d", cc);
+		return cc;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch failed with cc=%d", cc);
+		return cc;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report_info("Subchannel %08x enabled after %d retries",
+			    schid, retry_count);
+		return 0;
+	}
+
+	if (retry_count++ < MAX_ENABLE_RETRIES) {
+		mdelay(10); /* the hardware was not ready, give it some time */
+		goto retry;
+	}
+
+	report_info("msch: enabling sch %08x failed after %d retries. pmcw flags: %x",
+		    schid, retry_count, pmcw->flags);
+	return -1;
+}
diff --git a/s390x/css.c b/s390x/css.c
index 230adf5..6948d73 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -31,11 +31,26 @@ static void test_enumerate(void)
 	report(0, "No I/O device found");
 }
 
+static void test_enable(void)
+{
+	int cc;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	cc = css_enable(test_device_sid);
+
+	report(cc == 0, "Enable subchannel %08x", test_device_sid);
+}
+
 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 v9 12/12] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

After a channel is enabled we 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 SENSE_ID command response is tested to report 0xff inside
its reserved field and to report the same control unit type
as the cu_type kernel argument.

Without the cu_type kernel argument, the test expects a device
with a default control unit type of 0x3832, a.k.a virtio-net-ccw.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     |  20 +++++++
 lib/s390x/css_lib.c |  46 +++++++++++++++
 s390x/css.c         | 140 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 205 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 0ddceb1..3091d63 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -100,6 +100,19 @@ struct irb {
 	uint32_t emw[8];
 } __attribute__ ((aligned(4)));
 
+#define CCW_CMD_SENSE_ID	0xe4
+#define CSS_SENSEID_COMMON_LEN	8
+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)
@@ -253,4 +266,11 @@ int css_enumerate(void);
 #define MAX_ENABLE_RETRIES      5
 int css_enable(int schid);
 
+
+/* Library functions */
+int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
+int start_subchannel(unsigned int sid, int code, void *data, int count,
+		     unsigned char flags);
+int sch_read_len(int sid);
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 06a76db..c3d93d3 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -128,3 +128,49 @@ retry:
 		    schid, retry_count, pmcw->flags);
 	return -1;
 }
+
+int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
+{
+	struct orb orb = {
+		.intparm = sid,
+		.ctrl = ORB_CTRL_ISIC|ORB_CTRL_FMT|ORB_LPM_DFLT,
+		.cpa = (unsigned int) (unsigned long)ccw,
+	};
+
+	return ssch(sid, &orb);
+}
+
+/*
+ * In the next revisions we will implement the possibility to handle
+ * CCW chains doing this we will need to work with ccw1 pointers.
+ * For now we only need a unique CCW.
+ */
+static struct ccw1 unique_ccw;
+
+int start_subchannel(unsigned int sid, int code, void *data, int count,
+		     unsigned char flags)
+{
+	int cc;
+	struct ccw1 *ccw = &unique_ccw;
+
+	report_prefix_push("start_subchannel");
+	/* Build the CCW chain with a single CCW */
+	ccw->code = code;
+	ccw->flags = flags; /* No flags need to be set */
+	ccw->count = count;
+	ccw->data_address = (int)(unsigned long)data;
+
+	cc = start_ccw1_chain(sid, ccw);
+	if (cc) {
+		report(0, "start_ccw_chain failed ret=%d", cc);
+		report_prefix_pop();
+		return cc;
+	}
+	report_prefix_pop();
+	return 0;
+}
+
+int sch_read_len(int sid)
+{
+	return unique_ccw.count;
+}
diff --git a/s390x/css.c b/s390x/css.c
index 6948d73..6b618a1 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -16,10 +16,18 @@
 #include <string.h>
 #include <interrupt.h>
 #include <asm/arch_def.h>
+#include <kernel-args.h>
 
 #include <css.h>
 
+#define DEFAULT_CU_TYPE		0x3832
+static unsigned long cu_type = DEFAULT_CU_TYPE;
+
+struct lowcore *lowcore = (void *)0x0;
+
 static int test_device_sid;
+static struct irb irb;
+static struct senseid senseid;
 
 static void test_enumerate(void)
 {
@@ -45,20 +53,150 @@ static void test_enable(void)
 	report(cc == 0, "Enable subchannel %08x", test_device_sid);
 }
 
+static void enable_io_isc(void)
+{
+	/* Let's enable all ISCs for I/O interrupt */
+	lctlg(6, 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;
+}
+
+/*
+ * test_sense
+ * Pre-requisits:
+ * - We need the test device as the first recognized
+ *   device by the enumeration.
+ */
+static void test_sense(void)
+{
+	int ret;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	ret = css_enable(test_device_sid);
+	if (ret) {
+		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;
+
+	memset(&senseid, 0, sizeof(senseid));
+	ret = start_subchannel(test_device_sid, CCW_CMD_SENSE_ID,
+			       &senseid, sizeof(senseid), CCW_F_SLI);
+	if (ret) {
+		report(0, "ssch failed for SENSE ID on sch %08x with cc %d",
+		       test_device_sid, ret);
+		goto unreg_cb;
+	}
+
+	wait_for_interrupt(PSW_MASK_IO);
+
+	ret = sch_read_len(test_device_sid);
+	if (ret < CSS_SENSEID_COMMON_LEN) {
+		report(0,
+		       "ssch succeeded for SENSE ID but report a too short length: %d",
+		       ret);
+		goto unreg_cb;
+	}
+
+	if (senseid.reserved != 0xff) {
+		report(0,
+		       "ssch succeeded for SENSE ID but reports garbage: %x",
+		       senseid.reserved);
+		goto unreg_cb;
+	}
+
+	if (lowcore->io_int_param != test_device_sid)
+		goto unreg_cb;
+
+	report_info("senseid length read: %d", ret);
+	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 == cu_type, "cu_type: expect 0x%04x got 0x%04x",
+	       (uint16_t) 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 }
 };
 
+static unsigned long value;
+
 int main(int argc, char *argv[])
 {
-	int i;
+	int i, ret;
+
+	ret = kernel_arg(argc, argv, "cu_type=", &value);
+	if (!ret)
+		cu_type = (uint16_t)value;
+	else
+		report_info("Using cu_type default value: 0x%04lx", cu_type);
 
 	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

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

[Thomas Huth]

On 15/06/2020 11.31, 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>
> 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(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v9 06/12] s390x: clock and delays caluculations

[Thomas Huth]

On 15/06/2020 11.31, Pierre Morel wrote:
> The hardware gives us a good definition of the microsecond,
> let's keep this information and let the routine accessing
> the hardware keep all the information and return microseconds.
> 
> Calculate delays in microseconds and take care about wrapping
> around zero.
> 
> Define values with macros and use inlines to keep the
> milliseconds interface.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)

In case you respin, there is a typo in the title ("caluculations") ...
otherwise this can be fixed when the patch gets picked up.

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Pierre Morel]

On 2020-06-16 11:31, Thomas Huth wrote:
> On 15/06/2020 11.31, 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>
>> 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(-)
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v9 06/12] s390x: clock and delays caluculations

[Pierre Morel]

On 2020-06-16 11:35, Thomas Huth wrote:
> On 15/06/2020 11.31, Pierre Morel wrote:
>> The hardware gives us a good definition of the microsecond,
>> let's keep this information and let the routine accessing
>> the hardware keep all the information and return microseconds.
>>
>> Calculate delays in microseconds and take care about wrapping
>> around zero.
>>
>> Define values with macros and use inlines to keep the
>> milliseconds interface.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
> 
> In case you respin, there is a typo in the title ("caluculations") ...
> otherwise this can be fixed when the patch gets picked up.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Yes if I respin I modifify it :)

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Thomas Huth]

On 15/06/2020 11.31, Pierre Morel wrote:
> We often need to retrieve hexadecimal kernel parameters.
> Let's implement a shared utility to do it.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/kernel-args.h | 18 +++++++++++++
>  s390x/Makefile          |  1 +
>  3 files changed, 79 insertions(+)
>  create mode 100644 lib/s390x/kernel-args.c
>  create mode 100644 lib/s390x/kernel-args.h

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 15/06/2020 11.31, Pierre Morel wrote:
> Provide some definitions and library routines that can be used by
> tests targeting the channel subsystem.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
>  s390x/Makefile       |   1 +
>  3 files changed, 410 insertions(+)
>  create mode 100644 lib/s390x/css.h
>  create mode 100644 lib/s390x/css_dump.c

I can't say much about the gory IO details, but at least the inline
assembly looks fine to me now.

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 15/06/2020 11.31, 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>
> ---
>  lib/s390x/css_lib.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/Makefile      |  2 ++
>  s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  4 files changed, 131 insertions(+)
>  create mode 100644 lib/s390x/css_lib.c
>  create mode 100644 s390x/css.c

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 15/06/2020 11.32, Pierre Morel 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 command succeeds 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 pending as we do not expect this.
> 
> This tests the MSCH instruction to enable a channel succesfuly.

s/succesfuly/successfully/

> This some retries are done and in case of error, and if the retries
> count is exceeded, a report is made.

That sentence needs some fixes, too ;-)

With the commit description fixed:

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 15/06/2020 11.31, 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>
> 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 1b3bb0c..b5d7aca 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_MASK_ON_EXCEPTION	(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 e084f13..d386f35 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
>  
> @@ -198,19 +199,19 @@ svc_int:
>  
>  	.align	8
>  reset_psw:
> -	.quad	0x0008000180000000
> +	.quad	PSW_MASK_ON_EXCEPTION | PSW_MASK_SHORT_PSW
>  initial_psw:
> -	.quad	0x0000000180000000, clear_bss_start
> +	.quad	PSW_MASK_ON_EXCEPTION, clear_bss_start
>  pgm_int_psw:
> -	.quad	0x0000000180000000, pgm_int
> +	.quad	PSW_MASK_ON_EXCEPTION, pgm_int
>  ext_int_psw:
> -	.quad	0x0000000180000000, ext_int
> +	.quad	PSW_MASK_ON_EXCEPTION, ext_int
>  mcck_int_psw:
> -	.quad	0x0000000180000000, mcck_int
> +	.quad	PSW_MASK_ON_EXCEPTION, mcck_int
>  io_int_psw:
> -	.quad	0x0000000180000000, io_int
> +	.quad	PSW_MASK_ON_EXCEPTION, io_int
>  svc_int_psw:
> -	.quad	0x0000000180000000, svc_int
> +	.quad	PSW_MASK_ON_EXCEPTION, svc_int
>  initial_cr0:
>  	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>  	.quad	0x0000000000040000
> 

I'm afraid, by when I compile this on RHEL7, the toolchain complains:

s390x/cstart64.S: Assembler messages:
s390x/cstart64.S:239: Error: found 'L', expected: ')'
s390x/cstart64.S:239: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:241: Error: found 'L', expected: ')'
s390x/cstart64.S:241: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:243: Error: found 'L', expected: ')'
s390x/cstart64.S:243: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:245: Error: found 'L', expected: ')'
s390x/cstart64.S:245: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:247: Error: found 'L', expected: ')'
s390x/cstart64.S:247: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:249: Error: found 'L', expected: ')'
s390x/cstart64.S:249: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:251: Error: found 'L', expected: ')'
s390x/cstart64.S:251: Error: junk at end of line, first unrecognized
character is `L'
s390x/cstart64.S:254: Error: junk at end of line, first unrecognized
character is `L'

Shall we skip the update to the assembler file for now?

 Thomas

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

[Pierre Morel]

On 2020-06-16 13:37, Thomas Huth wrote:
> On 15/06/2020 11.31, 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>
>> ---
>>   lib/s390x/css_lib.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/Makefile      |  2 ++
>>   s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |  4 +++
>>   4 files changed, 131 insertions(+)
>>   create mode 100644 lib/s390x/css_lib.c
>>   create mode 100644 s390x/css.c
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-16 12:31, Thomas Huth wrote:
> On 15/06/2020 11.31, Pierre Morel wrote:
>> Provide some definitions and library routines that can be used by
>> tests targeting the channel subsystem.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
>>   s390x/Makefile       |   1 +
>>   3 files changed, 410 insertions(+)
>>   create mode 100644 lib/s390x/css.h
>>   create mode 100644 lib/s390x/css_dump.c
> 
> I can't say much about the gory IO details, but at least the inline
> assembly looks fine to me now.
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Pierre Morel]

On 2020-06-16 11:47, Thomas Huth wrote:
> On 15/06/2020 11.31, Pierre Morel wrote:
>> We often need to retrieve hexadecimal kernel parameters.
>> Let's implement a shared utility to do it.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/kernel-args.h | 18 +++++++++++++
>>   s390x/Makefile          |  1 +
>>   3 files changed, 79 insertions(+)
>>   create mode 100644 lib/s390x/kernel-args.c
>>   create mode 100644 lib/s390x/kernel-args.h
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-16 13:43, Thomas Huth wrote:
> On 15/06/2020 11.32, Pierre Morel 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 command succeeds 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 pending as we do not expect this.
>>
>> This tests the MSCH instruction to enable a channel succesfuly.
> 
> s/succesfuly/successfully/

yes.

> 
>> This some retries are done and in case of error, and if the retries
>> count is exceeded, a report is made.
> 
> That sentence needs some fixes, too ;-)
> 
> With the commit description fixed:
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-16 15:13, Thomas Huth wrote:
> On 15/06/2020 11.31, 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>
>> 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 1b3bb0c..b5d7aca 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_MASK_ON_EXCEPTION	(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 e084f13..d386f35 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
>>   
>> @@ -198,19 +199,19 @@ svc_int:
>>   
>>   	.align	8
>>   reset_psw:
>> -	.quad	0x0008000180000000
>> +	.quad	PSW_MASK_ON_EXCEPTION | PSW_MASK_SHORT_PSW
>>   initial_psw:
>> -	.quad	0x0000000180000000, clear_bss_start
>> +	.quad	PSW_MASK_ON_EXCEPTION, clear_bss_start
>>   pgm_int_psw:
>> -	.quad	0x0000000180000000, pgm_int
>> +	.quad	PSW_MASK_ON_EXCEPTION, pgm_int
>>   ext_int_psw:
>> -	.quad	0x0000000180000000, ext_int
>> +	.quad	PSW_MASK_ON_EXCEPTION, ext_int
>>   mcck_int_psw:
>> -	.quad	0x0000000180000000, mcck_int
>> +	.quad	PSW_MASK_ON_EXCEPTION, mcck_int
>>   io_int_psw:
>> -	.quad	0x0000000180000000, io_int
>> +	.quad	PSW_MASK_ON_EXCEPTION, io_int
>>   svc_int_psw:
>> -	.quad	0x0000000180000000, svc_int
>> +	.quad	PSW_MASK_ON_EXCEPTION, svc_int
>>   initial_cr0:
>>   	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>>   	.quad	0x0000000000040000
>>
> 
> I'm afraid, by when I compile this on RHEL7, the toolchain complains:

I will try to figure out why.
which version are you using? and which compiler?

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-16 15:58, Pierre Morel wrote:
> 
> 
> On 2020-06-16 15:13, Thomas Huth wrote:
>> On 15/06/2020 11.31, 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>
>>> 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 1b3bb0c..b5d7aca 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_MASK_ON_EXCEPTION    (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 e084f13..d386f35 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
>>> @@ -198,19 +199,19 @@ svc_int:
>>>       .align    8
>>>   reset_psw:
>>> -    .quad    0x0008000180000000
>>> +    .quad    PSW_MASK_ON_EXCEPTION | PSW_MASK_SHORT_PSW
>>>   initial_psw:
>>> -    .quad    0x0000000180000000, clear_bss_start
>>> +    .quad    PSW_MASK_ON_EXCEPTION, clear_bss_start
>>>   pgm_int_psw:
>>> -    .quad    0x0000000180000000, pgm_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, pgm_int
>>>   ext_int_psw:
>>> -    .quad    0x0000000180000000, ext_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, ext_int
>>>   mcck_int_psw:
>>> -    .quad    0x0000000180000000, mcck_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, mcck_int
>>>   io_int_psw:
>>> -    .quad    0x0000000180000000, io_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, io_int
>>>   svc_int_psw:
>>> -    .quad    0x0000000180000000, svc_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, svc_int
>>>   initial_cr0:
>>>       /* enable AFP-register control, so FP regs (+BFP instr) can be 
>>> used */
>>>       .quad    0x0000000000040000
>>>
>>
>> I'm afraid, by when I compile this on RHEL7, the toolchain complains:
> 
> I will try to figure out why.

What if you suppress the parenthesis?

-#define PSW_MASK_ON_EXCEPTION  (PSW_MASK_EA | PSW_MASK_BA)
+#define PSW_MASK_ON_EXCEPTION  PSW_MASK_EA | PSW_MASK_BA


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Thomas Huth]

On 16/06/2020 15.58, Pierre Morel wrote:
> 
> 
> On 2020-06-16 15:13, Thomas Huth wrote:
>> On 15/06/2020 11.31, 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>
>>> 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 1b3bb0c..b5d7aca 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_MASK_ON_EXCEPTION    (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 e084f13..d386f35 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
>>>   @@ -198,19 +199,19 @@ svc_int:
>>>         .align    8
>>>   reset_psw:
>>> -    .quad    0x0008000180000000
>>> +    .quad    PSW_MASK_ON_EXCEPTION | PSW_MASK_SHORT_PSW
>>>   initial_psw:
>>> -    .quad    0x0000000180000000, clear_bss_start
>>> +    .quad    PSW_MASK_ON_EXCEPTION, clear_bss_start
>>>   pgm_int_psw:
>>> -    .quad    0x0000000180000000, pgm_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, pgm_int
>>>   ext_int_psw:
>>> -    .quad    0x0000000180000000, ext_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, ext_int
>>>   mcck_int_psw:
>>> -    .quad    0x0000000180000000, mcck_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, mcck_int
>>>   io_int_psw:
>>> -    .quad    0x0000000180000000, io_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, io_int
>>>   svc_int_psw:
>>> -    .quad    0x0000000180000000, svc_int
>>> +    .quad    PSW_MASK_ON_EXCEPTION, svc_int
>>>   initial_cr0:
>>>       /* enable AFP-register control, so FP regs (+BFP instr) can be
>>> used */
>>>       .quad    0x0000000000040000
>>>
>>
>> I'm afraid, by when I compile this on RHEL7, the toolchain complains:
> 
> I will try to figure out why.
> which version are you using? and which compiler?

$ as --version
GNU assembler version 2.27-43.base.el7
$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39)

I think the problem are the "UL" suffixes ... IIRC they are only
supported by newer versions of the binutils.

 Thomas

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

[Pierre Morel]

On 2020-06-16 15:13, Thomas Huth wrote:
> On 15/06/2020 11.31, Pierre Morel wrote:
>> This patch defines the PSW bits EA/BA used to initialize the PSW masks
>> for exceptions.

...snip...

>>   svc_int_psw:
>> -	.quad	0x0000000180000000, svc_int
>> +	.quad	PSW_MASK_ON_EXCEPTION, svc_int
>>   initial_cr0:
>>   	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>>   	.quad	0x0000000000040000
>>
> 
> I'm afraid, by when I compile this on RHEL7, the toolchain complains:
> 
> s390x/cstart64.S: Assembler messages:
> s390x/cstart64.S:239: Error: found 'L', expected: ')'
> s390x/cstart64.S:239: Error: junk at end of line, first unrecognized
...snip...
> character is `L'
> s390x/cstart64.S:254: Error: junk at end of line, first unrecognized
> character is `L'
> 
> Shall we skip the update to the assembler file for now?

I think it is the best to do to go rapidly forward.

alternative:
I propose a local macro for the PSW_MASK_ON_EXCEPTION and 
PSW_MASK_SHORT_PSW inside the assembler in the respin, put it at the end 
of the serie and we choose if we keep the changes of csstart.S

this would not slow us.

Should I keep your RB for the arch_def.h ?

thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-16 18:08, Pierre Morel wrote:
> 
> 
> On 2020-06-16 15:13, Thomas Huth wrote:
>> On 15/06/2020 11.31, Pierre Morel wrote:
>>> This patch defines the PSW bits EA/BA used to initialize the PSW masks
>>> for exceptions.
> 

> 
> Should I keep your RB for the arch_def.h ?
> 

Hi, sorry, the changes in arch_def go are not used any more so...

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Mon, 15 Jun 2020 11:31:52 +0200
Pierre Morel <pmorel@linux.ibm.com> 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>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---
>  s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)

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

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

[Cornelia Huck]

On Mon, 15 Jun 2020 11:31:53 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

Subject: "s390: I/O interrupt registration" ?

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

Not sure if we really need these checks, but they don't hurt, either.

> +	io_int_func = NULL;
> +	return 0;
> +}
> +
>  void handle_svc_int(void)
>  {
>  	report_abort("Unexpected supervisor call interrupt: on cpu %d at %#lx",

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

Re: [kvm-unit-tests PATCH v9 06/12] s390x: clock and delays caluculations

[Cornelia Huck]

On Mon, 15 Jun 2020 11:31:55 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> The hardware gives us a good definition of the microsecond,
> let's keep this information and let the routine accessing
> the hardware keep all the information and return microseconds.
> 
> Calculate delays in microseconds and take care about wrapping
> around zero.
> 
> Define values with macros and use inlines to keep the
> milliseconds interface.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> index 1791380..7f1d891 100644
> --- a/lib/s390x/asm/time.h
> +++ b/lib/s390x/asm/time.h
> @@ -13,14 +13,39 @@
>  #ifndef ASM_S390X_TIME_H
>  #define ASM_S390X_TIME_H
>  
> -static inline uint64_t get_clock_ms(void)
> +#define STCK_SHIFT_US	(63 - 51)
> +#define STCK_MAX	((1UL << 52) - 1)
> +
> +static inline uint64_t get_clock_us(void)
>  {
>  	uint64_t clk;
>  
>  	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
>  
>  	/* Bit 51 is incrememented each microsecond */

That comment may now be a tad non-obvious, because the details are
hidden behind the #define? Anyway, no strong opinion.

> -	return (clk >> (63 - 51)) / 1000;
> +	return clk >> STCK_SHIFT_US;
> +}
> +
> +static inline void udelay(unsigned long us)
> +{
> +	unsigned long startclk = get_clock_us();
> +	unsigned long c;
> +
> +	do {
> +		c = get_clock_us();
> +		if (c < startclk)
> +			c += STCK_MAX;
> +	} while (c < startclk + us);
> +}
> +
> +static inline void mdelay(unsigned long ms)
> +{
> +	udelay(ms * 1000);
> +}
> +
> +static inline uint64_t get_clock_ms(void)
> +{
> +	return get_clock_us() / 1000;
>  }
>  
>  #endif

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

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Cornelia Huck]

On Mon, 15 Jun 2020 11:31:57 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We often need to retrieve hexadecimal kernel parameters.
> Let's implement a shared utility to do it.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/kernel-args.h | 18 +++++++++++++
>  s390x/Makefile          |  1 +
>  3 files changed, 79 insertions(+)
>  create mode 100644 lib/s390x/kernel-args.c
>  create mode 100644 lib/s390x/kernel-args.h
> 

(...)

> diff --git a/lib/s390x/kernel-args.h b/lib/s390x/kernel-args.h
> new file mode 100644
> index 0000000..a88e34e
> --- /dev/null
> +++ b/lib/s390x/kernel-args.h
> @@ -0,0 +1,18 @@
> +/*
> + * Kernel argument
> + *
> + * 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.
> + */
> +
> +#ifndef KERNEL_ARGS_H
> +#define KERNEL_ARGS_H
> +
> +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val);

<bikeshed>get_kernel_arg()?</bikeshed>

> +
> +#endif

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

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

[Cornelia Huck]

On Tue, 16 Jun 2020 12:31:40 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 15/06/2020 11.31, Pierre Morel wrote:
> > Provide some definitions and library routines that can be used by
> > tests targeting the channel subsystem.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
> >  lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
> >  s390x/Makefile       |   1 +
> >  3 files changed, 410 insertions(+)
> >  create mode 100644 lib/s390x/css.h
> >  create mode 100644 lib/s390x/css_dump.c  
> 
> I can't say much about the gory IO details, but at least the inline
> assembly looks fine to me now.
> 
> Acked-by: Thomas Huth <thuth@redhat.com>

I've looked at the gory I/O details, and they seem fine to me.

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

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

[Cornelia Huck]

On Mon, 15 Jun 2020 11:31:59 +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.

Maybe worth adding a note:

"We currently don't enable multiple subchannel sets and therefore only
look in subchannel set 0."

> 
> This tests the success of STSCH I/O instruction, we do not test the
> reaction of the VM for an instruction with wrong parameters.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css_lib.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/Makefile      |  2 ++
>  s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 +++
>  4 files changed, 131 insertions(+)
>  create mode 100644 lib/s390x/css_lib.c
>  create mode 100644 s390x/css.c
> 
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> new file mode 100644
> index 0000000..b0a0294
> --- /dev/null
> +++ b/lib/s390x/css_lib.c
> @@ -0,0 +1,70 @@
> +/*
> + * Channel Subsystem tests library
> + *
> + * 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;
> +
> +/*
> + * css_enumerate:
> + * On success return the first subchannel ID found.
> + * On error return an invalid subchannel ID containing cc
> + */
> +int css_enumerate(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int scn_found = 0;
> +	int dev_found = 0;
> +	int schid = 0;
> +	int cc;
> +	int scn;
> +
> +	for (scn = 0; scn < 0xffff; scn++) {
> +		cc = stsch(scn | SCHID_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 */

s/happened/happen/

> +			report_abort("Unexpected error %d on subchannel %08x",
> +				     cc, scn | SCHID_ONE);
> +			return cc;
> +		}
> +
> +		/* 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 (!schid)
> +			schid = scn | SCHID_ONE;
> +		report_info("Found subchannel %08x", scn | SCHID_ONE);
> +		dev_found++;
> +	}
> +
> +out:
> +	report_info("Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
> +		    scn, scn_found, dev_found);
> +	return schid;
> +}

(...)

> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..230adf5
> --- /dev/null
> +++ b/s390x/css.c
> @@ -0,0 +1,55 @@
> +/*
> + * 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 int test_device_sid;
> +
> +static void test_enumerate(void)
> +{
> +	test_device_sid = css_enumerate();
> +	if (test_device_sid & SCHID_ONE) {
> +		report(1, "First device schid: 0x%08x", test_device_sid);

Maybe "Schid of first I/O device" ?

> +		return;
> +	}
> +	report(0, "No I/O device 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();
> +}

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

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

[Cornelia Huck]

On Mon, 15 Jun 2020 11:32:00 +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 command succeeds 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 pending as we do not expect this.
> 
> This tests the MSCH instruction to enable a channel succesfuly.
> This some retries are done and in case of error, and if the retries
> count is exceeded, a report is made.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css_lib.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/css.c         | 15 ++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index b0a0294..06a76db 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -15,6 +15,7 @@
>  #include <string.h>
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
> +#include <asm/time.h>
>  
>  #include <css.h>
>  
> @@ -68,3 +69,62 @@ out:
>  		    scn, scn_found, dev_found);
>  	return schid;
>  }
> +
> +int css_enable(int schid)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int retry_count = 0;
> +	int cc;
> +
> +	/* Read the SCHIB for this subchannel */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch failed with cc=%d", cc);

Mention the schid in the message?

> +		return cc;
> +	}
> +
> +	if (pmcw->flags & PMCW_ENABLE) {
> +		report_info("stsch: sch %08x already enabled", schid);
> +		return 0;
> +	}
> +
> +retry:
> +	/* Update the SCHIB to enable the channel */
> +	pmcw->flags |= PMCW_ENABLE;
> +
> +	/* Tell the CSS we want to modify the subchannel */
> +	cc = msch(schid, &schib);
> +	if (cc) {
> +		/*
> +		 * If the subchannel is status pending or
> +		 * if a function is in progress,
> +		 * we consider both cases as errors.
> +		 */
> +		report_info("msch failed with cc=%d", cc);
> +		return cc;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again to verify the enablement
> +	 */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch failed with cc=%d", cc);

Also add the schid here? Maybe also add a marker to distinguish the two
cases?

> +		return cc;
> +	}
> +
> +	if (pmcw->flags & PMCW_ENABLE) {
> +		report_info("Subchannel %08x enabled after %d retries",
> +			    schid, retry_count);
> +		return 0;
> +	}
> +
> +	if (retry_count++ < MAX_ENABLE_RETRIES) {
> +		mdelay(10); /* the hardware was not ready, give it some time */
> +		goto retry;
> +	}
> +
> +	report_info("msch: enabling sch %08x failed after %d retries. pmcw flags: %x",
> +		    schid, retry_count, pmcw->flags);
> +	return -1;
> +}

With the messages updated,

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

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

[Cornelia Huck]

On Mon, 15 Jun 2020 11:32:01 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> After a channel is enabled we 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 SENSE_ID command response is tested to report 0xff inside
> its reserved field and to report the same control unit type
> as the cu_type kernel argument.
> 
> Without the cu_type kernel argument, the test expects a device
> with a default control unit type of 0x3832, a.k.a virtio-net-ccw.

0x3832 is any virtio-ccw device; you could also test for the cu model
to make sure that it is a net device, but that probably doesn't add
much additional coverage.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  20 +++++++
>  lib/s390x/css_lib.c |  46 +++++++++++++++
>  s390x/css.c         | 140 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 205 insertions(+), 1 deletion(-)

(...)

> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 06a76db..c3d93d3 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -128,3 +128,49 @@ retry:
>  		    schid, retry_count, pmcw->flags);
>  	return -1;
>  }
> +
> +int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
> +{
> +	struct orb orb = {
> +		.intparm = sid,
> +		.ctrl = ORB_CTRL_ISIC|ORB_CTRL_FMT|ORB_LPM_DFLT,
> +		.cpa = (unsigned int) (unsigned long)ccw,
> +	};
> +
> +	return ssch(sid, &orb);
> +}
> +
> +/*
> + * In the next revisions we will implement the possibility to handle
> + * CCW chains doing this we will need to work with ccw1 pointers.

"In the future, we want to implement support for CCW chains; for that,
we will need to work with ccw1 pointers."

?

> + * For now we only need a unique CCW.
> + */
> +static struct ccw1 unique_ccw;
> +
> +int start_subchannel(unsigned int sid, int code, void *data, int count,
> +		     unsigned char flags)
> +{
> +	int cc;
> +	struct ccw1 *ccw = &unique_ccw;

Hm... it might better to call this function "start_single_ccw" or
something like that.

> +
> +	report_prefix_push("start_subchannel");
> +	/* Build the CCW chain with a single CCW */
> +	ccw->code = code;
> +	ccw->flags = flags; /* No flags need to be set */
> +	ccw->count = count;
> +	ccw->data_address = (int)(unsigned long)data;
> +
> +	cc = start_ccw1_chain(sid, ccw);
> +	if (cc) {
> +		report(0, "start_ccw_chain failed ret=%d", cc);
> +		report_prefix_pop();
> +		return cc;
> +	}
> +	report_prefix_pop();
> +	return 0;
> +}
> +
> +int sch_read_len(int sid)
> +{
> +	return unique_ccw.count;
> +}

This function is very odd... it takes a subchannel id as a parameter,
which it ignores, and instead returns the count field of the static ccw
used when starting I/O. What is the purpose of this function? Grab the
data length for the last I/O operation that was started on the
subchannel? If yes, it might be better to store that information along
with the sid? If it is the length for the last I/O operation that the
code _thinks_ it started, it might be better to reuse that information
from further up in the function instead.

> diff --git a/s390x/css.c b/s390x/css.c
> index 6948d73..6b618a1 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -16,10 +16,18 @@
>  #include <string.h>
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
> +#include <kernel-args.h>
>  
>  #include <css.h>
>  
> +#define DEFAULT_CU_TYPE		0x3832

Maybe append /* virtio-ccw */

> +static unsigned long cu_type = DEFAULT_CU_TYPE;
> +
> +struct lowcore *lowcore = (void *)0x0;
> +
>  static int test_device_sid;
> +static struct irb irb;
> +static struct senseid senseid;
>  
>  static void test_enumerate(void)
>  {
> @@ -45,20 +53,150 @@ static void test_enable(void)
>  	report(cc == 0, "Enable subchannel %08x", test_device_sid);
>  }
>  
> +static void enable_io_isc(void)
> +{
> +	/* Let's enable all ISCs for I/O interrupt */
> +	lctlg(6, 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;
> +}
> +
> +/*
> + * test_sense
> + * Pre-requisits:
> + * - We need the test device as the first recognized
> + *   device by the enumeration.
> + */
> +static void test_sense(void)
> +{
> +	int ret;
> +
> +	if (!test_device_sid) {
> +		report_skip("No device");
> +		return;
> +	}
> +
> +	ret = css_enable(test_device_sid);
> +	if (ret) {
> +		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;
> +
> +	memset(&senseid, 0, sizeof(senseid));
> +	ret = start_subchannel(test_device_sid, CCW_CMD_SENSE_ID,
> +			       &senseid, sizeof(senseid), CCW_F_SLI);
> +	if (ret) {
> +		report(0, "ssch failed for SENSE ID on sch %08x with cc %d",
> +		       test_device_sid, ret);
> +		goto unreg_cb;
> +	}
> +
> +	wait_for_interrupt(PSW_MASK_IO);
> +
> +	ret = sch_read_len(test_device_sid);
> +	if (ret < CSS_SENSEID_COMMON_LEN) {
> +		report(0,
> +		       "ssch succeeded for SENSE ID but report a too short length: %d",
> +		       ret);
> +		goto unreg_cb;
> +	}

Oh, so you want to check something even different: You know what you
put in the request, and you expect a certain minimal length back. But
that length is contained in the scsw, not in the started ccw, isn't it?

> +
> +	if (senseid.reserved != 0xff) {
> +		report(0,
> +		       "ssch succeeded for SENSE ID but reports garbage: %x",
> +		       senseid.reserved);
> +		goto unreg_cb;
> +	}
> +
> +	if (lowcore->io_int_param != test_device_sid)
> +		goto unreg_cb;

You probably want to check this further up? But doesn't irq_io()
already check this?

> +
> +	report_info("senseid length read: %d", ret);
> +	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 == cu_type, "cu_type: expect 0x%04x got 0x%04x",
> +	       (uint16_t) 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 }
>  };
>  
> +static unsigned long value;
> +
>  int main(int argc, char *argv[])
>  {
> -	int i;
> +	int i, ret;
> +
> +	ret = kernel_arg(argc, argv, "cu_type=", &value);
> +	if (!ret)
> +		cu_type = (uint16_t)value;
> +	else
> +		report_info("Using cu_type default value: 0x%04lx", cu_type);
>  
>  	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 v9 04/12] s390x: interrupt registration

[Pierre Morel]

On 2020-06-17 10:20, Cornelia Huck wrote:
> On Mon, 15 Jun 2020 11:31:53 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> Subject: "s390: I/O interrupt registration" ?

yes, better.

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

...snip...

>> +int unregister_io_int_func(void (*f)(void))
>> +{
>> +	if (io_int_func != f)
>> +		return -1;
> 
> Not sure if we really need these checks, but they don't hurt, either.

Not sure neither but I prefer to let it here to avoid more discussions 
since already reviewed-by...

> 
>> +	io_int_func = NULL;
>> +	return 0;
>> +}
>> +
>>   void handle_svc_int(void)
>>   {
>>   	report_abort("Unexpected supervisor call interrupt: on cpu %d at %#lx",
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v9 06/12] s390x: clock and delays caluculations

[Pierre Morel]

On 2020-06-17 10:27, Cornelia Huck wrote:
> On Mon, 15 Jun 2020 11:31:55 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> The hardware gives us a good definition of the microsecond,
>> let's keep this information and let the routine accessing
>> the hardware keep all the information and return microseconds.
>>
>> Calculate delays in microseconds and take care about wrapping
>> around zero.
>>
>> Define values with macros and use inlines to keep the
>> milliseconds interface.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
>> index 1791380..7f1d891 100644
>> --- a/lib/s390x/asm/time.h
>> +++ b/lib/s390x/asm/time.h
>> @@ -13,14 +13,39 @@
>>   #ifndef ASM_S390X_TIME_H
>>   #define ASM_S390X_TIME_H
>>   
>> -static inline uint64_t get_clock_ms(void)
>> +#define STCK_SHIFT_US	(63 - 51)
>> +#define STCK_MAX	((1UL << 52) - 1)
>> +
>> +static inline uint64_t get_clock_us(void)
>>   {
>>   	uint64_t clk;
>>   
>>   	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
>>   
>>   	/* Bit 51 is incrememented each microsecond */
> 
> That comment may now be a tad non-obvious, because the details are
> hidden behind the #define? Anyway, no strong opinion.

You are right.
I find the macro's name is explicit so I remove the comment.

> 
>> -	return (clk >> (63 - 51)) / 1000;
>> +	return clk >> STCK_SHIFT_US;
>> +}
>> +
>> +static inline void udelay(unsigned long us)
>> +{
>> +	unsigned long startclk = get_clock_us();
>> +	unsigned long c;
>> +
>> +	do {
>> +		c = get_clock_us();
>> +		if (c < startclk)
>> +			c += STCK_MAX;
>> +	} while (c < startclk + us);
>> +}
>> +
>> +static inline void mdelay(unsigned long ms)
>> +{
>> +	udelay(ms * 1000);
>> +}
>> +
>> +static inline uint64_t get_clock_ms(void)
>> +{
>> +	return get_clock_us() / 1000;
>>   }
>>   
>>   #endif
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Pierre Morel]

On 2020-06-17 10:37, Cornelia Huck wrote:
> On Mon, 15 Jun 2020 11:31:57 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We often need to retrieve hexadecimal kernel parameters.
>> Let's implement a shared utility to do it.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/kernel-args.h | 18 +++++++++++++
>>   s390x/Makefile          |  1 +
>>   3 files changed, 79 insertions(+)
>>   create mode 100644 lib/s390x/kernel-args.c
>>   create mode 100644 lib/s390x/kernel-args.h
>>
> 
> (...)
> 
>> diff --git a/lib/s390x/kernel-args.h b/lib/s390x/kernel-args.h
>> new file mode 100644
>> index 0000000..a88e34e
>> --- /dev/null
>> +++ b/lib/s390x/kernel-args.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * Kernel argument
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef KERNEL_ARGS_H
>> +#define KERNEL_ARGS_H
>> +
>> +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val);
> 
> <bikeshed>get_kernel_arg()?</bikeshed>

OK, is more explicit.

> 
>> +
>> +#endif
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-17 10:42, Cornelia Huck wrote:
> On Tue, 16 Jun 2020 12:31:40 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 15/06/2020 11.31, Pierre Morel wrote:
>>> Provide some definitions and library routines that can be used by
>>> tests targeting the channel subsystem.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
>>>   s390x/Makefile       |   1 +
>>>   3 files changed, 410 insertions(+)
>>>   create mode 100644 lib/s390x/css.h
>>>   create mode 100644 lib/s390x/css_dump.c
>>
>> I can't say much about the gory IO details, but at least the inline
>> assembly looks fine to me now.
>>
>> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> I've looked at the gory I/O details, and they seem fine to me.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-17 10:48, Cornelia Huck wrote:
> On Mon, 15 Jun 2020 11:31:59 +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.
> 
> Maybe worth adding a note:
> 
> "We currently don't enable multiple subchannel sets and therefore only
> look in subchannel set 0."

right, thanks

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

...snip...

>> +	for (scn = 0; scn < 0xffff; scn++) {
>> +		cc = stsch(scn | SCHID_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 */
> 
> s/happened/happen/

yes

...snip...


>> diff --git a/s390x/css.c b/s390x/css.c
...
>> +static void test_enumerate(void)
>> +{
>> +	test_device_sid = css_enumerate();
>> +	if (test_device_sid & SCHID_ONE) {
>> +		report(1, "First device schid: 0x%08x", test_device_sid);
> 
> Maybe "Schid of first I/O device" ?

yes, better.

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

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-17 10:54, Cornelia Huck wrote:
> On Mon, 15 Jun 2020 11:32:00 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> A second step when testing the channel subsystem is to prepare a channel
>> for use.
...snip...
>> +
>> +	/* Read the SCHIB for this subchannel */
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch failed with cc=%d", cc);
> 
> Mention the schid in the message?

Yes:
report_info("stsch: sch %08x failed with cc=%d", schid, cc);

> 
>> +		return cc;
>> +	}
>> +
>> +	if (pmcw->flags & PMCW_ENABLE) {
>> +		report_info("stsch: sch %08x already enabled", schid);
>> +		return 0;
>> +	}
>> +
>> +retry:
>> +	/* Update the SCHIB to enable the channel */
>> +	pmcw->flags |= PMCW_ENABLE;
>> +
>> +	/* Tell the CSS we want to modify the subchannel */
>> +	cc = msch(schid, &schib);
>> +	if (cc) {
>> +		/*
>> +		 * If the subchannel is status pending or
>> +		 * if a function is in progress,
>> +		 * we consider both cases as errors.
>> +		 */
>> +		report_info("msch failed with cc=%d", cc);

added schid here too

>> +		return cc;
>> +	}
>> +
>> +	/*
>> +	 * Read the SCHIB again to verify the enablement
>> +	 */
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch failed with cc=%d", cc);
> 
> Also add the schid here? Maybe also add a marker to distinguish the two
> cases?

changed to:
report_info("stsch: updating sch %08x failed with cc=%d",schid, cc);
                     ^^^
> 
>> +		return cc;
>> +	}
>> +
>> +	if (pmcw->flags & PMCW_ENABLE) {
>> +		report_info("Subchannel %08x enabled after %d retries",
>> +			    schid, retry_count);
>> +		return 0;
>> +	}
>> +
>> +	if (retry_count++ < MAX_ENABLE_RETRIES) {
>> +		mdelay(10); /* the hardware was not ready, give it some time */
>> +		goto retry;
>> +	}
>> +
>> +	report_info("msch: enabling sch %08x failed after %d retries. pmcw flags: %x",
>> +		    schid, retry_count, pmcw->flags);
>> +	return -1;
>> +}
> 
> With the messages updated,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-17 11:54, Cornelia Huck wrote:
> On Mon, 15 Jun 2020 11:32:01 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> After a channel is enabled we 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 SENSE_ID command response is tested to report 0xff inside
>> its reserved field and to report the same control unit type
>> as the cu_type kernel argument.
>>
>> Without the cu_type kernel argument, the test expects a device
>> with a default control unit type of 0x3832, a.k.a virtio-net-ccw.
> 
> 0x3832 is any virtio-ccw device; you could also test for the cu model
> to make sure that it is a net device, but that probably doesn't add
> much additional coverage.
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  20 +++++++
>>   lib/s390x/css_lib.c |  46 +++++++++++++++
>>   s390x/css.c         | 140 +++++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 205 insertions(+), 1 deletion(-)
> 
...snip...

>> +/*
>> + * In the next revisions we will implement the possibility to handle
>> + * CCW chains doing this we will need to work with ccw1 pointers.
> 
> "In the future, we want to implement support for CCW chains; for that,
> we will need to work with ccw1 pointers."
> 
> ?

yes, better, thanks.

> 
>> + * For now we only need a unique CCW.
>> + */
>> +static struct ccw1 unique_ccw;
>> +
>> +int start_subchannel(unsigned int sid, int code, void *data, int count,
>> +		     unsigned char flags)
>> +{
>> +	int cc;
>> +	struct ccw1 *ccw = &unique_ccw;
> 
> Hm... it might better to call this function "start_single_ccw" or
> something like that.

You are right.
I will rework this.
What about differentiating this badly named "start_subchannel()" into:

ccw_setup_ccw(ccw, data, cnt, flgs);
ccw_setup_orb(orb, ccw, flgs)
ccw_start_request(schid, orb);

would be much clearer I think.

> 
>> +
>> +	report_prefix_push("start_subchannel");
>> +	/* Build the CCW chain with a single CCW */
>> +	ccw->code = code;
>> +	ccw->flags = flags; /* No flags need to be set */
>> +	ccw->count = count;
>> +	ccw->data_address = (int)(unsigned long)data;
>> +
>> +	cc = start_ccw1_chain(sid, ccw);
>> +	if (cc) {
>> +		report(0, "start_ccw_chain failed ret=%d", cc);
>> +		report_prefix_pop();
>> +		return cc;
>> +	}
>> +	report_prefix_pop();
>> +	return 0;
>> +}
>> +
>> +int sch_read_len(int sid)
>> +{
>> +	return unique_ccw.count;
>> +}
> 
> This function is very odd... it takes a subchannel id as a parameter,
> which it ignores, and instead returns the count field of the static ccw
> used when starting I/O. What is the purpose of this function? Grab the
> data length for the last I/O operation that was started on the
> subchannel? If yes, it might be better to store that information along
> with the sid? If it is the length for the last I/O operation that the
> code _thinks_ it started, it might be better to reuse that information
> from further up in the function instead.

agreed, I forgot to update this, totally confused.
will rework this.


> 
>> diff --git a/s390x/css.c b/s390x/css.c
>> index 6948d73..6b618a1 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -16,10 +16,18 @@
>>   #include <string.h>
>>   #include <interrupt.h>
>>   #include <asm/arch_def.h>
>> +#include <kernel-args.h>
>>   
>>   #include <css.h>
>>   
>> +#define DEFAULT_CU_TYPE		0x3832
> 
> Maybe append /* virtio-ccw */

yes, thanks

> 
>> +static unsigned long cu_type = DEFAULT_CU_TYPE;
>> +
>> +struct lowcore *lowcore = (void *)0x0;
>> +
>>   static int test_device_sid;
>> +static struct irb irb;
>> +static struct senseid senseid;
>>   
>>   static void test_enumerate(void)
>>   {
>> @@ -45,20 +53,150 @@ static void test_enable(void)
>>   	report(cc == 0, "Enable subchannel %08x", test_device_sid);
>>   }
>>   
>> +static void enable_io_isc(void)
>> +{
>> +	/* Let's enable all ISCs for I/O interrupt */
>> +	lctlg(6, 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;
>> +}
>> +
>> +/*
>> + * test_sense
>> + * Pre-requisits:
>> + * - We need the test device as the first recognized
>> + *   device by the enumeration.
>> + */
>> +static void test_sense(void)
>> +{
>> +	int ret;
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
>> +
>> +	ret = css_enable(test_device_sid);
>> +	if (ret) {
>> +		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;
>> +
>> +	memset(&senseid, 0, sizeof(senseid));
>> +	ret = start_subchannel(test_device_sid, CCW_CMD_SENSE_ID,
>> +			       &senseid, sizeof(senseid), CCW_F_SLI);
>> +	if (ret) {
>> +		report(0, "ssch failed for SENSE ID on sch %08x with cc %d",
>> +		       test_device_sid, ret);
>> +		goto unreg_cb;
>> +	}
>> +
>> +	wait_for_interrupt(PSW_MASK_IO);
>> +
>> +	ret = sch_read_len(test_device_sid);
>> +	if (ret < CSS_SENSEID_COMMON_LEN) {
>> +		report(0,
>> +		       "ssch succeeded for SENSE ID but report a too short length: %d",
>> +		       ret);
>> +		goto unreg_cb;
>> +	}
> 
> Oh, so you want to check something even different: You know what you
> put in the request, and you expect a certain minimal length back. But
> that length is contained in the scsw, not in the started ccw, isn't it?

yes it is.

> 
>> +
>> +	if (senseid.reserved != 0xff) {
>> +		report(0,
>> +		       "ssch succeeded for SENSE ID but reports garbage: %x",
>> +		       senseid.reserved);
>> +		goto unreg_cb;
>> +	}
>> +
>> +	if (lowcore->io_int_param != test_device_sid)
>> +		goto unreg_cb;
> 
> You probably want to check this further up? But doesn't irq_io()
> already check this?

yes it does

Thanks for the comments,

I will rework this.

- rework the start_subchannel()
- rework the read_len() if we ever need this

Also thinking to put the irq_io routine inside the library, it will be 
reused by other tests.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Wed, 17 Jun 2020 13:55:52 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-06-17 11:54, Cornelia Huck wrote:
> > On Mon, 15 Jun 2020 11:32:01 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

(...)

> >> +int start_subchannel(unsigned int sid, int code, void *data, int count,
> >> +		     unsigned char flags)
> >> +{
> >> +	int cc;
> >> +	struct ccw1 *ccw = &unique_ccw;  
> > 
> > Hm... it might better to call this function "start_single_ccw" or
> > something like that.  
> 
> You are right.
> I will rework this.
> What about differentiating this badly named "start_subchannel()" into:
> 
> ccw_setup_ccw(ccw, data, cnt, flgs);
> ccw_setup_orb(orb, ccw, flgs)
> ccw_start_request(schid, orb);
> 
> would be much clearer I think.

Not sure about ccw_setup_ccw; might get a bit non-obvious if you're
trying to build a chain.

Let's see how this turns out.

(...)

> I will rework this.
> 
> - rework the start_subchannel()
> - rework the read_len() if we ever need this

I think checking the count after the request concluded is actually a
good idea. In the future, we could also add a check that it matches the
requested length for a request where SLI was not specified.

> 
> Also thinking to put the irq_io routine inside the library, it will be 
> reused by other tests.

Yes, that probably makes sense as well.

Re: [kvm-unit-tests PATCH v9 06/12] s390x: clock and delays caluculations

[Janosch Frank]

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

On 6/15/20 11:31 AM, Pierre Morel wrote:
> The hardware gives us a good definition of the microsecond,
> let's keep this information and let the routine accessing
> the hardware keep all the information and return microseconds.
> 
> Calculate delays in microseconds and take care about wrapping
> around zero.
> 
> Define values with macros and use inlines to keep the
> milliseconds interface.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Small nit below.


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

> ---
>  lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> index 1791380..7f1d891 100644
> --- a/lib/s390x/asm/time.h
> +++ b/lib/s390x/asm/time.h
> @@ -13,14 +13,39 @@
>  #ifndef ASM_S390X_TIME_H
>  #define ASM_S390X_TIME_H
>  
> -static inline uint64_t get_clock_ms(void)
> +#define STCK_SHIFT_US	(63 - 51)
> +#define STCK_MAX	((1UL << 52) - 1)
> +
> +static inline uint64_t get_clock_us(void)
>  {
>  	uint64_t clk;
>  
>  	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
>  
>  	/* Bit 51 is incrememented each microsecond */
> -	return (clk >> (63 - 51)) / 1000;
> +	return clk >> STCK_SHIFT_US;
> +}
> +
> +static inline void udelay(unsigned long us)
> +{
> +	unsigned long startclk = get_clock_us();
> +	unsigned long c;
> +
> +	do {
> +		c = get_clock_us();
> +		if (c < startclk)
> +			c += STCK_MAX;
> +	} while (c < startclk + us);
> +}
> +
> +static inline void mdelay(unsigned long ms)
> +{
> +	udelay(ms * 1000);
> +}
> +
> +static inline uint64_t get_clock_ms(void)
> +{
> +	return get_clock_us() / 1000;
>  }

Why don't you put that below to the get_clock_us()?

>  
>  #endif
> 



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

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Janosch Frank]

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

On 6/15/20 11:31 AM, Pierre Morel wrote:
> We often need to retrieve hexadecimal kernel parameters.
> Let's implement a shared utility to do it.

Often?

My main problem with this patch is that it doesn't belong into the s390
library. atol() is already in string.c so htol() can be next to it.

util.c already has parse_keyval() so you should be able to extend it a
bit for hex values and add a function below that goes through argv[].

CCing Andrew as he wrote most of the common library

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/kernel-args.h | 18 +++++++++++++
>  s390x/Makefile          |  1 +
>  3 files changed, 79 insertions(+)
>  create mode 100644 lib/s390x/kernel-args.c
>  create mode 100644 lib/s390x/kernel-args.h
> 
> diff --git a/lib/s390x/kernel-args.c b/lib/s390x/kernel-args.c
> new file mode 100644
> index 0000000..2d3b2c2
> --- /dev/null
> +++ b/lib/s390x/kernel-args.c
> @@ -0,0 +1,60 @@
> +/*
> + * Retrieving kernel arguments
> + *
> + * 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 <string.h>
> +#include <asm/arch_def.h>
> +#include <kernel-args.h>
> +
> +static const char *hex_digit = "0123456789abcdef";
> +
> +static unsigned long htol(char *s)
> +{
> +	unsigned long v = 0, shift = 0, value = 0;
> +	int i, digit, len = strlen(s);
> +
> +	for (shift = 0, i = len - 1; i >= 0; i--, shift += 4) {
> +		digit = s[i] | 0x20;	/* Set lowercase */
> +		if (!strchr(hex_digit, digit))
> +			return 0;	/* this is not a digit ! */
> +
> +		if (digit <= '9')
> +			v = digit - '0';
> +		else
> +			v = digit - 'a' + 10;
> +		value += (v << shift);
> +	}
> +
> +	return value;
> +}
> +
> +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val)
> +{
> +	int i, ret;
> +	char *p, *q;
> +
> +	for (i = 0; i < argc; i++) {
> +		ret = strncmp(argv[i], str, strlen(str));
> +		if (ret)
> +			continue;
> +		p = strchr(argv[i], '=');
> +		if (!p)
> +			return -1;
> +		q = strchr(p, 'x');
> +		if (!q)
> +			*val = atol(p + 1);
> +		else
> +			*val = htol(q + 1);
> +		return 0;
> +	}
> +	return -2;
> +}
> diff --git a/lib/s390x/kernel-args.h b/lib/s390x/kernel-args.h
> new file mode 100644
> index 0000000..a88e34e
> --- /dev/null
> +++ b/lib/s390x/kernel-args.h
> @@ -0,0 +1,18 @@
> +/*
> + * Kernel argument
> + *
> + * 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.
> + */
> +
> +#ifndef KERNEL_ARGS_H
> +#define KERNEL_ARGS_H
> +
> +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val);
> +
> +#endif
> diff --git a/s390x/Makefile b/s390x/Makefile
> index ddb4b48..47a94cc 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/kernel-args.o
>  
>  OBJDIRS += lib/s390x
>  
> 



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

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

[Janosch Frank]

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

On 6/15/20 11:31 AM, Pierre Morel wrote:
> Provide some definitions and library routines that can be used by
> tests targeting the channel subsystem.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

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

> ---
>  lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
>  s390x/Makefile       |   1 +
>  3 files changed, 410 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..0ddceb1
> --- /dev/null
> +++ b/lib/s390x/css.h
> @@ -0,0 +1,256 @@
> +/*
> + * CSS definitions
> + *
> + * Copyright IBM, Corp. 2020
> + * Author: 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 CSS_H
> +#define CSS_H
> +
> +/* subchannel ID bit 16 must always be one */
> +#define SCHID_ONE	0x00010000
> +
> +#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 {
> +	uint8_t code;
> +	uint8_t flags;
> +	uint16_t count;
> +	uint32_t data_address;
> +} __attribute__ ((aligned(8)));
> +
> +#define ORB_CTRL_KEY	0xf0000000
> +#define ORB_CTRL_SPND	0x08000000
> +#define ORB_CTRL_STR	0x04000000
> +#define ORB_CTRL_MOD	0x02000000
> +#define ORB_CTRL_SYNC	0x01000000
> +#define ORB_CTRL_FMT	0x00800000
> +#define ORB_CTRL_PFCH	0x00400000
> +#define ORB_CTRL_ISIC	0x00200000
> +#define ORB_CTRL_ALCC	0x00100000
> +#define ORB_CTRL_SSIC	0x00080000
> +#define ORB_CTRL_CPTC	0x00040000
> +#define ORB_CTRL_C64	0x00020000
> +#define ORB_CTRL_I2K	0x00010000
> +#define ORB_CTRL_LPM	0x0000ff00
> +#define ORB_CTRL_ILS	0x00000080
> +#define ORB_CTRL_MIDAW	0x00000040
> +#define ORB_CTRL_ORBX	0x00000001
> +
> +#define ORB_LPM_DFLT	0x00008000
> +
> +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)
> +		: "d" (reg1), "m" (*addr), "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);
> +
> +void dump_scsw(struct scsw *scsw);
> +void dump_irb(struct irb *irbp);
> +void dump_schib(struct schib *sch);
> +struct ccw1 *dump_ccw(struct ccw1 *cp);
> +void dump_irb(struct irb *irbp);
> +void dump_pmcw(struct pmcw *p);
> +void dump_orb(struct orb *op);
> +
> +int css_enumerate(void);
> +#define MAX_ENABLE_RETRIES      5
> +int css_enable(int schid);
> +
> +#endif
> diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
> new file mode 100644
> index 0000000..0c2b64e
> --- /dev/null
> +++ b/lib/s390x/css_dump.c
> @@ -0,0 +1,153 @@
> +/*
> + * 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 <libcflat.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include <css.h>
> +
> +/*
> + * Try to have a more human representation of the SCSW flags:
> + * each letter in the two strings represents 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 to have a more human representation of the PMCW flags
> + * each letter in the string represents 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;
> +}
> +
> +void dump_scsw(struct scsw *s)
> +{
> +	dump_scsw_flags(s->ctrl);
> +	printf("scsw->flags: %s\n", scsw_line);
> +	printf("scsw->addr : %08x\n", s->ccw_addr);
> +	printf("scsw->devs : %02x\n", s->dev_stat);
> +	printf("scsw->schs : %02x\n", s->sch_stat);
> +	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 ccw1 *dump_ccw(struct ccw1 *cp)
> +{
> +	printf("CCW: code: %02x flags: %02x count: %04x data: %08x\n", cp->code,
> +	    cp->flags, cp->count, cp->data_address);
> +
> +	if (cp->code == CCW_C_TIC)
> +		return (struct ccw1 *)(long)cp->data_address;
> +
> +	return (cp->flags & CCW_F_CC) ? cp + 1 : NULL;
> +}
> +
> +void dump_orb(struct orb *op)
> +{
> +	struct ccw1 *cp;
> +
> +	printf("ORB: intparm : %08x\n", op->intparm);
> +	printf("ORB: ctrl    : %08x\n", op->ctrl);
> +	printf("ORB: prio    : %08x\n", op->prio);
> +	cp = (struct ccw1 *)(long) (op->cpa);
> +	while (cp)
> +		cp = dump_ccw(cp);
> +}
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 47a94cc..3cb97da 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -52,6 +52,7 @@ cflatobjs += lib/s390x/interrupt.o
>  cflatobjs += lib/s390x/mmu.o
>  cflatobjs += lib/s390x/smp.o
>  cflatobjs += lib/s390x/kernel-args.o
> +cflatobjs += lib/s390x/css_dump.o
>  
>  OBJDIRS += lib/s390x
>  
> 



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

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Andrew Jones]

On Mon, Jun 22, 2020 at 11:33:24AM +0200, Janosch Frank wrote:
> On 6/15/20 11:31 AM, Pierre Morel wrote:
> > We often need to retrieve hexadecimal kernel parameters.
> > Let's implement a shared utility to do it.
> 
> Often?
> 
> My main problem with this patch is that it doesn't belong into the s390
> library. atol() is already in string.c so htol() can be next to it.
> 
> util.c already has parse_keyval() so you should be able to extend it a
> bit for hex values and add a function below that goes through argv[].
> 
> CCing Andrew as he wrote most of the common library

I'd prefer we add strtol(), rather than htol(), as we try to add
common libc functions when possible. It could live in the same
files at atol (string.c/libcflat.h), but we should considering
adding a stdlib.c file some day.

Also, if we had strtol(), than parse_key() could use it with base=0
instead of atol(). That would get pretty close to the implementation
of kernel_arg(). We'd just need to add a

  char *find_key(const char *key, char **array, int array_len)

type of a function to do the command line iterating. Then,

  ret = parse_keyval(find_key("foo", argv, argc), &val);

should be the same as kernel_arg() if find_key returns NULL when
the key isn't found and parse_keyval learns to return -2 when s
is NULL.

Thanks,
drew


> 
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
> >  lib/s390x/kernel-args.h | 18 +++++++++++++
> >  s390x/Makefile          |  1 +
> >  3 files changed, 79 insertions(+)
> >  create mode 100644 lib/s390x/kernel-args.c
> >  create mode 100644 lib/s390x/kernel-args.h
> > 
> > diff --git a/lib/s390x/kernel-args.c b/lib/s390x/kernel-args.c
> > new file mode 100644
> > index 0000000..2d3b2c2
> > --- /dev/null
> > +++ b/lib/s390x/kernel-args.c
> > @@ -0,0 +1,60 @@
> > +/*
> > + * Retrieving kernel arguments
> > + *
> > + * 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 <string.h>
> > +#include <asm/arch_def.h>
> > +#include <kernel-args.h>
> > +
> > +static const char *hex_digit = "0123456789abcdef";
> > +
> > +static unsigned long htol(char *s)
> > +{
> > +	unsigned long v = 0, shift = 0, value = 0;
> > +	int i, digit, len = strlen(s);
> > +
> > +	for (shift = 0, i = len - 1; i >= 0; i--, shift += 4) {
> > +		digit = s[i] | 0x20;	/* Set lowercase */
> > +		if (!strchr(hex_digit, digit))
> > +			return 0;	/* this is not a digit ! */
> > +
> > +		if (digit <= '9')
> > +			v = digit - '0';
> > +		else
> > +			v = digit - 'a' + 10;
> > +		value += (v << shift);
> > +	}
> > +
> > +	return value;
> > +}
> > +
> > +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val)
> > +{
> > +	int i, ret;
> > +	char *p, *q;
> > +
> > +	for (i = 0; i < argc; i++) {
> > +		ret = strncmp(argv[i], str, strlen(str));
> > +		if (ret)
> > +			continue;
> > +		p = strchr(argv[i], '=');
> > +		if (!p)
> > +			return -1;
> > +		q = strchr(p, 'x');
> > +		if (!q)
> > +			*val = atol(p + 1);
> > +		else
> > +			*val = htol(q + 1);
> > +		return 0;
> > +	}
> > +	return -2;
> > +}
> > diff --git a/lib/s390x/kernel-args.h b/lib/s390x/kernel-args.h
> > new file mode 100644
> > index 0000000..a88e34e
> > --- /dev/null
> > +++ b/lib/s390x/kernel-args.h
> > @@ -0,0 +1,18 @@
> > +/*
> > + * Kernel argument
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef KERNEL_ARGS_H
> > +#define KERNEL_ARGS_H
> > +
> > +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val);
> > +
> > +#endif
> > diff --git a/s390x/Makefile b/s390x/Makefile
> > index ddb4b48..47a94cc 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/kernel-args.o
> >  
> >  OBJDIRS += lib/s390x
> >  
> > 
> 
> 

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

[Thomas Huth]

On 15/06/2020 11.31, Pierre Morel wrote:
> Provide some definitions and library routines that can be used by
> tests targeting the channel subsystem.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
>   s390x/Makefile       |   1 +
>   3 files changed, 410 insertions(+)
>   create mode 100644 lib/s390x/css.h
>   create mode 100644 lib/s390x/css_dump.c
[...]
> diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
> new file mode 100644
> index 0000000..0c2b64e
> --- /dev/null
> +++ b/lib/s390x/css_dump.c
> @@ -0,0 +1,153 @@
> +/*
> + * 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 <libcflat.h>
> +#include <unistd.h>

Please don't use unistd.h in kvm-unit-tests - this header is not usable 
in cross-compilation environments:

  https://travis-ci.com/github/huth/kvm-unit-tests/jobs/353089278#L536

Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v9 06/12] s390x: clock and delays caluculations

[Pierre Morel]

On 2020-06-22 11:09, Janosch Frank wrote:
> On 6/15/20 11:31 AM, Pierre Morel wrote:
>> The hardware gives us a good definition of the microsecond,
>> let's keep this information and let the routine accessing
>> the hardware keep all the information and return microseconds.
>>
>> Calculate delays in microseconds and take care about wrapping
>> around zero.
>>
>> Define values with macros and use inlines to keep the
>> milliseconds interface.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Small nit below.

seen, I move it up.

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

Thanks,
Pierre


> 
>> ---
>>   lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
>> index 1791380..7f1d891 100644
>> --- a/lib/s390x/asm/time.h
>> +++ b/lib/s390x/asm/time.h
>> @@ -13,14 +13,39 @@
>>   #ifndef ASM_S390X_TIME_H
>>   #define ASM_S390X_TIME_H
>>   
>> -static inline uint64_t get_clock_ms(void)
>> +#define STCK_SHIFT_US	(63 - 51)
>> +#define STCK_MAX	((1UL << 52) - 1)
>> +
>> +static inline uint64_t get_clock_us(void)
>>   {
>>   	uint64_t clk;
>>   
>>   	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
>>   
>>   	/* Bit 51 is incrememented each microsecond */
>> -	return (clk >> (63 - 51)) / 1000;
>> +	return clk >> STCK_SHIFT_US;
>> +}
>> +
>> +static inline void udelay(unsigned long us)
>> +{
>> +	unsigned long startclk = get_clock_us();
>> +	unsigned long c;
>> +
>> +	do {
>> +		c = get_clock_us();
>> +		if (c < startclk)
>> +			c += STCK_MAX;
>> +	} while (c < startclk + us);
>> +}
>> +
>> +static inline void mdelay(unsigned long ms)
>> +{
>> +	udelay(ms * 1000);
>> +}
>> +
>> +static inline uint64_t get_clock_ms(void)
>> +{
>> +	return get_clock_us() / 1000;
>>   }
> 
> Why don't you put that below to the get_clock_us()?

right, better.


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-24 14:26, Thomas Huth wrote:
> On 15/06/2020 11.31, Pierre Morel wrote:
>> Provide some definitions and library routines that can be used by
>> tests targeting the channel subsystem.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
>>   s390x/Makefile       |   1 +
>>   3 files changed, 410 insertions(+)
>>   create mode 100644 lib/s390x/css.h
>>   create mode 100644 lib/s390x/css_dump.c
> [...]
>> diff --git a/lib/s390x/css_dump.c b/lib/s390x/css_dump.c
>> new file mode 100644
>> index 0000000..0c2b64e
>> --- /dev/null
>> +++ b/lib/s390x/css_dump.c
>> @@ -0,0 +1,153 @@
>> +/*
>> + * 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 <libcflat.h>
>> +#include <unistd.h>
> 
> Please don't use unistd.h in kvm-unit-tests - this header is not usable 
> in cross-compilation environments:
> 
>   https://travis-ci.com/github/huth/kvm-unit-tests/jobs/353089278#L536
> 
> Thanks,
>   Thomas
> 

Yes, no problem, it does not belong here anyway.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v9 08/12] s390x: retrieve decimal and hexadecimal kernel parameters

[Pierre Morel]

On 2020-06-22 12:57, Andrew Jones wrote:
> On Mon, Jun 22, 2020 at 11:33:24AM +0200, Janosch Frank wrote:
>> On 6/15/20 11:31 AM, Pierre Morel wrote:
>>> We often need to retrieve hexadecimal kernel parameters.
>>> Let's implement a shared utility to do it.
>>
>> Often?
>>
>> My main problem with this patch is that it doesn't belong into the s390
>> library. atol() is already in string.c so htol() can be next to it.
>>
>> util.c already has parse_keyval() so you should be able to extend it a
>> bit for hex values and add a function below that goes through argv[].
>>
>> CCing Andrew as he wrote most of the common library
> 
> I'd prefer we add strtol(), rather than htol(), as we try to add
> common libc functions when possible. It could live in the same
> files at atol (string.c/libcflat.h), but we should considering
> adding a stdlib.c file some day.
> 
> Also, if we had strtol(), than parse_key() could use it with base=0
> instead of atol(). That would get pretty close to the implementation
> of kernel_arg(). We'd just need to add a
> 
>    char *find_key(const char *key, char **array, int array_len)
> 
> type of a function to do the command line iterating. Then,
> 
>    ret = parse_keyval(find_key("foo", argv, argc), &val);
> 
> should be the same as kernel_arg() if find_key returns NULL when
> the key isn't found and parse_keyval learns to return -2 when s
> is NULL.
> 
> Thanks,
> drew

Yes it is generally better, but it looks like a much bigger patch to me 
so I will treat it separately from this series.

Thanks,
Pierre

> 
> 
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/kernel-args.h | 18 +++++++++++++
>>>   s390x/Makefile          |  1 +
>>>   3 files changed, 79 insertions(+)
>>>   create mode 100644 lib/s390x/kernel-args.c
>>>   create mode 100644 lib/s390x/kernel-args.h
>>>
>>> diff --git a/lib/s390x/kernel-args.c b/lib/s390x/kernel-args.c
>>> new file mode 100644
>>> index 0000000..2d3b2c2
>>> --- /dev/null
>>> +++ b/lib/s390x/kernel-args.c
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> + * Retrieving kernel arguments
>>> + *
>>> + * 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 <string.h>
>>> +#include <asm/arch_def.h>
>>> +#include <kernel-args.h>
>>> +
>>> +static const char *hex_digit = "0123456789abcdef";
>>> +
>>> +static unsigned long htol(char *s)
>>> +{
>>> +	unsigned long v = 0, shift = 0, value = 0;
>>> +	int i, digit, len = strlen(s);
>>> +
>>> +	for (shift = 0, i = len - 1; i >= 0; i--, shift += 4) {
>>> +		digit = s[i] | 0x20;	/* Set lowercase */
>>> +		if (!strchr(hex_digit, digit))
>>> +			return 0;	/* this is not a digit ! */
>>> +
>>> +		if (digit <= '9')
>>> +			v = digit - '0';
>>> +		else
>>> +			v = digit - 'a' + 10;
>>> +		value += (v << shift);
>>> +	}
>>> +
>>> +	return value;
>>> +}
>>> +
>>> +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val)
>>> +{
>>> +	int i, ret;
>>> +	char *p, *q;
>>> +
>>> +	for (i = 0; i < argc; i++) {
>>> +		ret = strncmp(argv[i], str, strlen(str));
>>> +		if (ret)
>>> +			continue;
>>> +		p = strchr(argv[i], '=');
>>> +		if (!p)
>>> +			return -1;
>>> +		q = strchr(p, 'x');
>>> +		if (!q)
>>> +			*val = atol(p + 1);
>>> +		else
>>> +			*val = htol(q + 1);
>>> +		return 0;
>>> +	}
>>> +	return -2;
>>> +}
>>> diff --git a/lib/s390x/kernel-args.h b/lib/s390x/kernel-args.h
>>> new file mode 100644
>>> index 0000000..a88e34e
>>> --- /dev/null
>>> +++ b/lib/s390x/kernel-args.h
>>> @@ -0,0 +1,18 @@
>>> +/*
>>> + * Kernel argument
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#ifndef KERNEL_ARGS_H
>>> +#define KERNEL_ARGS_H
>>> +
>>> +int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val);
>>> +
>>> +#endif
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index ddb4b48..47a94cc 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/kernel-args.o
>>>   
>>>   OBJDIRS += lib/s390x
>>>   
>>>
>>
>>
> 
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2020-06-19 08:57, Cornelia Huck wrote:
> On Wed, 17 Jun 2020 13:55:52 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-06-17 11:54, Cornelia Huck wrote:
>>> On Mon, 15 Jun 2020 11:32:01 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> (...)
> 
>>>> +int start_subchannel(unsigned int sid, int code, void *data, int count,
>>>> +		     unsigned char flags)
>>>> +{
>>>> +	int cc;
>>>> +	struct ccw1 *ccw = &unique_ccw;
>>>
>>> Hm... it might better to call this function "start_single_ccw" or
>>> something like that.
>>
>> You are right.
>> I will rework this.
>> What about differentiating this badly named "start_subchannel()" into:
>>
>> ccw_setup_ccw(ccw, data, cnt, flgs);
>> ccw_setup_orb(orb, ccw, flgs)
>> ccw_start_request(schid, orb);
>>
>> would be much clearer I think.
> 
> Not sure about ccw_setup_ccw; might get a bit non-obvious if you're
> trying to build a chain.
> 
> Let's see how this turns out.
> 
> (...)
> 
>> I will rework this.
>>
>> - rework the start_subchannel()
>> - rework the read_len() if we ever need this
> 
> I think checking the count after the request concluded is actually a
> good idea. In the future, we could also add a check that it matches the
> requested length for a request where SLI was not specified.
> 
>>
>> Also thinking to put the irq_io routine inside the library, it will be
>> reused by other tests.
> 
> Yes, that probably makes sense as well.
> 

Thanks,
I respin soon.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen