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

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

[Pierre Morel]

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

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

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


Note:
- The following patches may be pulled first:
  s390x: saving regs for interrupts
  s390x: Use PSW bits definitions in cstart
  s390x: Move control register bit definitions and add AFP to them

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

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

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

Pierre Morel (10):
  s390x: saving regs for interrupts
  s390x: Use PSW bits definitions in cstart
  s390x: Move control register bit definitions and add AFP to them
  s390x: interrupt registration
  s390x: Library resources for CSS tests
  s390x: css: stsch, enumeration test
  s390x: css: msch, enable test
  s390x: define wfi: wait for interrupt
  s390x: css: ssch/tsch with sense and interrupt
  s390x: css: ping pong

 lib/s390x/asm/arch_def.h |  31 +++-
 lib/s390x/css.h          | 277 +++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c     | 157 ++++++++++++++++++
 lib/s390x/css_lib.c      |  55 +++++++
 lib/s390x/interrupt.c    |  23 ++-
 lib/s390x/interrupt.h    |   8 +
 s390x/Makefile           |   3 +
 s390x/css.c              | 343 +++++++++++++++++++++++++++++++++++++++
 s390x/cstart64.S         |  51 ++++--
 s390x/unittests.cfg      |   4 +
 10 files changed, 936 insertions(+), 16 deletions(-)
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c
 create mode 100644 lib/s390x/css_lib.c
 create mode 100644 lib/s390x/interrupt.h
 create mode 100644 s390x/css.c

-- 
2.25.1

Changelog:

from 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 v6 01/10] 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 and the floating point registers on the stack.

Note that we keep the static register saving to recover from the
RESET tests.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/cstart64.S | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 9af6bb3..ba2e67c 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -118,6 +118,36 @@ memsetxc:
 	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	.endm
 
+/* Save registers on the stack (r15), so we can have stacked interrupts. */
+	.macro SAVE_IRQ_REGS
+	/* Allocate a stack frame for 15 integer registers */
+	slgfi   %r15, 15 * 8
+	/* Store all registers from r0 to r14 on the stack */
+	stmg    %r0, %r14, 0(%r15)
+	/* Allocate a stack frame for 16 floating point registers */
+	/* The size of a FP register is the size of an integer */
+	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
+	.endm
+
+/* Restore the register in reverse order */
+	.macro RESTORE_IRQ_REGS
+	/* Restore fp register from stack: SP still where it was left */
+	/* and offset to SP is a multile of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8(%r15)
+	.endr
+	/* Now it is done, rewind the stack pointer by 16 integers */
+	algfi   %r15, 16 * 8
+	/* Load the registers from stack */
+	lmg     %r0, %r14, 0(%r15)
+	/* Rewind the stack by 15 integers */
+	algfi   %r15, 15 * 8
+	.endm
+
 .section .text
 /*
  * load_reset calling convention:
@@ -182,9 +212,9 @@ mcck_int:
 	lpswe	GEN_LC_MCCK_OLD_PSW
 
 io_int:
-	SAVE_REGS
+	SAVE_IRQ_REGS
 	brasl	%r14, handle_io_int
-	RESTORE_REGS
+	RESTORE_IRQ_REGS
 	lpswe	GEN_LC_IO_OLD_PSW
 
 svc_int:
-- 
2.25.1

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

[David Hildenbrand]

On 24.04.20 12:45, Pierre Morel wrote:
> If we use multiple source of interrupts, for example, using SCLP
> console to print information while using I/O interrupts, we need
> to have a re-entrant register saving interruption handling.

So the primary reason is to print during I/O interrupts (which we
already do, but usually never trigger - handle_io_int())

> 
> Instead of saving at a static memory address, let's save the base
> registers and the floating point registers on the stack.

".. in case of I/O interrupts".

> 
> Note that we keep the static register saving to recover from the
> RESET tests.

and for all other types of interrupts.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/cstart64.S | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 9af6bb3..ba2e67c 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -118,6 +118,36 @@ memsetxc:
>  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
>  	.endm
>  
> +/* Save registers on the stack (r15), so we can have stacked interrupts. */
> +	.macro SAVE_IRQ_REGS

s/SAVE_IRQ_REGS/SAVE_REGS_STACK/ ?

Same for the other macro.

> +	/* Allocate a stack frame for 15 integer registers */

not integers. sizeof(int) == 4.

"doublewords" should be the right s390x speak.

> +	slgfi   %r15, 15 * 8
> +	/* Store all registers from r0 to r14 on the stack */
> +	stmg    %r0, %r14, 0(%r15)
> +	/* Allocate a stack frame for 16 floating point registers */
> +	/* The size of a FP register is the size of an integer */
> +	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
> +	.endm

What about the FPC?

> +
> +/* Restore the register in reverse order */
> +	.macro RESTORE_IRQ_REGS
> +	/* Restore fp register from stack: SP still where it was left */
> +	/* and offset to SP is a multile of reg number */
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	ld	\i, \i * 8(%r15)
> +	.endr
> +	/* Now it is done, rewind the stack pointer by 16 integers */
> +	algfi   %r15, 16 * 8
> +	/* Load the registers from stack */
> +	lmg     %r0, %r14, 0(%r15)
> +	/* Rewind the stack by 15 integers */
> +	algfi   %r15, 15 * 8
> +	.endm

Dito, FPC?

> +
>  .section .text
>  /*
>   * load_reset calling convention:
> @@ -182,9 +212,9 @@ mcck_int:
>  	lpswe	GEN_LC_MCCK_OLD_PSW
>  
>  io_int:
> -	SAVE_REGS
> +	SAVE_IRQ_REGS
>  	brasl	%r14, handle_io_int
> -	RESTORE_REGS
> +	RESTORE_IRQ_REGS
>  	lpswe	GEN_LC_IO_OLD_PSW
>  
>  svc_int:
> 


-- 
Thanks,

David / dhildenb

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

[Pierre Morel]

On 2020-04-27 10:59, David Hildenbrand wrote:
> On 24.04.20 12:45, Pierre Morel wrote:
>> If we use multiple source of interrupts, for example, using SCLP
>> console to print information while using I/O interrupts, we need
>> to have a re-entrant register saving interruption handling.
> 
> So the primary reason is to print during I/O interrupts (which we
> already do, but usually never trigger - handle_io_int())
> 
>>
>> Instead of saving at a static memory address, let's save the base
>> registers and the floating point registers on the stack.
> 
> ".. in case of I/O interrupts".

OK

> 
>>
>> Note that we keep the static register saving to recover from the
>> RESET tests.
> 
> and for all other types of interrupts.

OK

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/cstart64.S | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index 9af6bb3..ba2e67c 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -118,6 +118,36 @@ memsetxc:
>>   	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
>>   	.endm
>>   
>> +/* Save registers on the stack (r15), so we can have stacked interrupts. */
>> +	.macro SAVE_IRQ_REGS
> 
> s/SAVE_IRQ_REGS/SAVE_REGS_STACK/ ?
> 
> Same for the other macro.

OK

> 
>> +	/* Allocate a stack frame for 15 integer registers */
> 
> not integers. sizeof(int) == 4.

hum, yes, I just do not know from where I took the word "integer" here!.

> 
> "doublewords" should be the right s390x speak.

in fact they are named general registers in the POP.
(which indeed are doublewords :) )


> 
>> +	slgfi   %r15, 15 * 8
>> +	/* Store all registers from r0 to r14 on the stack */
>> +	stmg    %r0, %r14, 0(%r15)
>> +	/* Allocate a stack frame for 16 floating point registers */
>> +	/* The size of a FP register is the size of an integer */
>> +	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
>> +	.endm
> 
> What about the FPC?

Seems I forgot it.
I will update.
Thanks.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

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

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

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

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 15a4d49..c54409a 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,15 +10,22 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_EXT			0x0100000000000000UL
+#define PSW_MASK_DAT			0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_PSTATE			0x0001000000000000UL
+#define PSW_MASK_BA			0x0000000080000000UL
+#define PSW_MASK_EA			0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
+#define PSW_RESET_MASK		(PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW)
+
+#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 +304,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 ba2e67c..e394b3a 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
 
@@ -225,19 +226,19 @@ svc_int:
 
 	.align	8
 reset_psw:
-	.quad	0x0008000180000000
+	.quad	PSW_RESET_MASK
 initial_psw:
-	.quad	0x0000000180000000, clear_bss_start
+	.quad	PSW_EXCEPTION_MASK, clear_bss_start
 pgm_int_psw:
-	.quad	0x0000000180000000, pgm_int
+	.quad	PSW_EXCEPTION_MASK, pgm_int
 ext_int_psw:
-	.quad	0x0000000180000000, ext_int
+	.quad	PSW_EXCEPTION_MASK, ext_int
 mcck_int_psw:
-	.quad	0x0000000180000000, mcck_int
+	.quad	PSW_EXCEPTION_MASK, mcck_int
 io_int_psw:
-	.quad	0x0000000180000000, io_int
+	.quad	PSW_EXCEPTION_MASK, io_int
 svc_int_psw:
-	.quad	0x0000000180000000, svc_int
+	.quad	PSW_EXCEPTION_MASK, svc_int
 initial_cr0:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
 	.quad	0x0000000000040000
-- 
2.25.1

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

[David Hildenbrand]

On 24.04.20 12:45, Pierre Morel wrote:
> This patch defines the PSW bits EA/BA used to initialize the PSW masks
> for exceptions.
> 
> Since some PSW mask definitions exist already in arch_def.h we add these
> definitions there.
> We move all PSW definitions together and protect assembler code against
> C syntax.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 16 ++++++++++++----
>  s390x/cstart64.S         | 15 ++++++++-------
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 15a4d49..c54409a 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -10,15 +10,22 @@
>  #ifndef _ASM_S390X_ARCH_DEF_H_
>  #define _ASM_S390X_ARCH_DEF_H_
>  
> +#define PSW_MASK_EXT			0x0100000000000000UL
> +#define PSW_MASK_DAT			0x0400000000000000UL
> +#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
> +#define PSW_MASK_PSTATE			0x0001000000000000UL
> +#define PSW_MASK_BA			0x0000000080000000UL
> +#define PSW_MASK_EA			0x0000000100000000UL
> +
> +#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
> +#define PSW_RESET_MASK		(PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW)
> +
> +#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 +304,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 ba2e67c..e394b3a 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
>  
> @@ -225,19 +226,19 @@ svc_int:
>  
>  	.align	8
>  reset_psw:
> -	.quad	0x0008000180000000
> +	.quad	PSW_RESET_MASK

I'd really prefer

.quad	PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW

here instead and drop PSW_RESET_MASK. Makes it clearer that we are
talking about a special short psw here.

Apart from that, looks good to me.


-- 
Thanks,

David / dhildenb

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

[Pierre Morel]

On 2020-04-27 11:01, David Hildenbrand wrote:
> On 24.04.20 12:45, Pierre Morel wrote:
>> This patch defines the PSW bits EA/BA used to initialize the PSW masks
>> for exceptions.
>>
>> Since some PSW mask definitions exist already in arch_def.h we add these
>> definitions there.
>> We move all PSW definitions together and protect assembler code against
>> C syntax.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h | 16 ++++++++++++----
>>   s390x/cstart64.S         | 15 ++++++++-------
>>   2 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 15a4d49..c54409a 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -10,15 +10,22 @@
>>   #ifndef _ASM_S390X_ARCH_DEF_H_
>>   #define _ASM_S390X_ARCH_DEF_H_
>>   
>> +#define PSW_MASK_EXT			0x0100000000000000UL
>> +#define PSW_MASK_DAT			0x0400000000000000UL
>> +#define PSW_MASK_SHORT_PSW		0x0008000000000000UL
>> +#define PSW_MASK_PSTATE			0x0001000000000000UL
>> +#define PSW_MASK_BA			0x0000000080000000UL
>> +#define PSW_MASK_EA			0x0000000100000000UL
>> +
>> +#define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
>> +#define PSW_RESET_MASK		(PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW)
>> +
>> +#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 +304,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 ba2e67c..e394b3a 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
>>   
>> @@ -225,19 +226,19 @@ svc_int:
>>   
>>   	.align	8
>>   reset_psw:
>> -	.quad	0x0008000180000000
>> +	.quad	PSW_RESET_MASK
> 
> I'd really prefer
> 
> .quad	PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW
> 
> here instead and drop PSW_RESET_MASK. Makes it clearer that we are
> talking about a special short psw here.
> 
> Apart from that, looks good to me.
> 
> 

Thanks, I will do as you propose,

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

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

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

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index c54409a..a0d2362 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -20,17 +20,18 @@
 #define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
 #define PSW_RESET_MASK		(PSW_EXCEPTION_MASK | PSW_MASK_SHORT_PSW)
 
+#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 e394b3a..2906e39 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -241,4 +241,4 @@ svc_int_psw:
 	.quad	PSW_EXCEPTION_MASK, svc_int
 initial_cr0:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
-	.quad	0x0000000000040000
+	.quad	CR0_AFP_REG_CRTL
-- 
2.25.1

[kvm-unit-tests PATCH v6 04/10] s390x: interrupt registration

[Pierre Morel]

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

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

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

Re: [kvm-unit-tests PATCH v6 04/10] s390x: interrupt registration

[Cornelia Huck]

On Fri, 24 Apr 2020 12:45:46 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Let's make it possible to add and remove a custom io interrupt handler,
> that can be used instead of the normal one.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
>  lib/s390x/interrupt.h |  8 ++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>  create mode 100644 lib/s390x/interrupt.h

As the "normal one" means "no handler, just abort", is there any reason
not simply to always provide one? What is the use case for multiple I/O
interrupt handlers?

Re: [kvm-unit-tests PATCH v6 04/10] s390x: interrupt registration

[Pierre Morel]

On 2020-05-14 13:58, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 12:45:46 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Let's make it possible to add and remove a custom io interrupt handler,
>> that can be used instead of the normal one.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
>>   lib/s390x/interrupt.h |  8 ++++++++
>>   2 files changed, 30 insertions(+), 1 deletion(-)
>>   create mode 100644 lib/s390x/interrupt.h
> 
> As the "normal one" means "no handler, just abort", is there any reason
> not simply to always provide one? What is the use case for multiple I/O
> interrupt handlers?
> 

I can only agree, I proposed this initially.
David asked for a registration.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v6 04/10] s390x: interrupt registration

[David Hildenbrand]

On 15.05.20 08:57, Pierre Morel wrote:
> 
> 
> On 2020-05-14 13:58, Cornelia Huck wrote:
>> On Fri, 24 Apr 2020 12:45:46 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> Let's make it possible to add and remove a custom io interrupt handler,
>>> that can be used instead of the normal one.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>   lib/s390x/interrupt.c | 23 ++++++++++++++++++++++-
>>>   lib/s390x/interrupt.h |  8 ++++++++
>>>   2 files changed, 30 insertions(+), 1 deletion(-)
>>>   create mode 100644 lib/s390x/interrupt.h
>>
>> As the "normal one" means "no handler, just abort", is there any reason
>> not simply to always provide one? What is the use case for multiple I/O
>> interrupt handlers?
>>
> 
> I can only agree, I proposed this initially.
> David asked for a registration.
> 

I don't understand "not simply to always provide one" or "multiple I/O
interrupt handlers".

There is always exactly *one* handler

void handle_io_int(void)
{
...
}

All we do here, is to allow to register a callback from the handler,
e.g., to verify in a test case that a specific I/O interrupt was received.

-- 
Thanks,

David / dhildenb

[kvm-unit-tests PATCH v6 05/10] s390x: Library resources for CSS tests

[Pierre Morel]

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

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

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

Re: [kvm-unit-tests PATCH v6 05/10] s390x: Library resources for CSS tests

[Cornelia Huck]

On Fri, 24 Apr 2020 12:45:47 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

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

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

?

> 
> Debug function can be activated by defining DEBUG_CSS before the
> inclusion of the css.h header file.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
>  2 files changed, 413 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..2f33fab
> --- /dev/null
> +++ b/lib/s390x/css_dump.c
> @@ -0,0 +1,157 @@
> +/*
> + * Channel subsystem structures dumping
> + *
> + * Copyright (c) 2020 IBM Corp.
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + *
> + * Description:
> + * Provides the dumping functions for various structures used by subchannels:
> + * - ORB  : Operation request block, describes the I/O operation and points to
> + *          a CCW chain
> + * - CCW  : Channel Command Word, describes the data and flow control

"describes the command, data, and flow control" ?

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

s/operation/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.
> + */

(...)

Otherwise, looks good.

Re: [kvm-unit-tests PATCH v6 05/10] s390x: Library resources for CSS tests

[Pierre Morel]

On 2020-05-14 14:03, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 12:45:47 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> These are the include and library utilities for the css tests patch
>> series.
> 
> "Provide some definitions and library routines that can be used by
> tests targeting the channel subsystem."
> 
> ?

Definitively better. Thanks, I will update.

> 
>>
>> Debug function can be activated by defining DEBUG_CSS before the
>> inclusion of the css.h header file.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h      | 256 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/css_dump.c | 157 ++++++++++++++++++++++++++
>>   2 files changed, 413 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..2f33fab
>> --- /dev/null
>> +++ b/lib/s390x/css_dump.c
>> @@ -0,0 +1,157 @@
>> +/*
>> + * Channel subsystem structures dumping
>> + *
>> + * Copyright (c) 2020 IBM Corp.
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2.
>> + *
>> + * Description:
>> + * Provides the dumping functions for various structures used by subchannels:
>> + * - ORB  : Operation request block, describes the I/O operation and points to
>> + *          a CCW chain
>> + * - CCW  : Channel Command Word, describes the data and flow control
> 
> "describes the command, data, and flow control" ?

OK, thanks

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

? I do not understand, do you want a ";" at the end of "operation"
Isn't it a typo error?

> 
>> + *          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.
>> + */
> 
> (...)
> 
> Otherwise, looks good.
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v6 05/10] s390x: Library resources for CSS tests

[Cornelia Huck]

On Fri, 15 May 2020 09:02:37 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-05-14 14:03, Cornelia Huck wrote:
> > On Fri, 24 Apr 2020 12:45:47 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> + * - IRB  : Interuption response Block, describes the result of an operation  
> > 
> > s/operation/operation;/  
> 
> ? I do not understand, do you want a ";" at the end of "operation"

Yes, I think that makes the description read more like a proper
sentence (describe what its purpose is; describe its contents).

> Isn't it a typo error?
> 
> >   
> >> + *          holds a SCSW and model-dependent data.

Re: [kvm-unit-tests PATCH v6 05/10] s390x: Library resources for CSS tests

[Pierre Morel]

On 2020-05-15 09:11, Cornelia Huck wrote:
> On Fri, 15 May 2020 09:02:37 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-05-14 14:03, Cornelia Huck wrote:
>>> On Fri, 24 Apr 2020 12:45:47 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> + * - IRB  : Interuption response Block, describes the result of an operation
>>>
>>> s/operation/operation;/
>>
>> ? I do not understand, do you want a ";" at the end of "operation"
> 
> Yes, I think that makes the description read more like a proper
> sentence (describe what its purpose is; describe its contents).
> 

You are right, thanks.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH v6 06/10] s390x: css: stsch, enumeration test

[Pierre Morel]

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

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

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index bab0dd5..9417541 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -82,6 +82,7 @@ struct pmcw {
 	uint8_t  chpid[8];
 	uint32_t flags2;
 };
+#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
 
 struct schib {
 	struct pmcw pmcw;
diff --git a/s390x/Makefile b/s390x/Makefile
index ddb4b48..baebf18 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -17,6 +17,7 @@ tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
@@ -51,6 +52,7 @@ cflatobjs += lib/s390x/sclp-console.o
 cflatobjs += lib/s390x/interrupt.o
 cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
+cflatobjs += lib/s390x/css_dump.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/css.c b/s390x/css.c
new file mode 100644
index 0000000..cc97e79
--- /dev/null
+++ b/s390x/css.c
@@ -0,0 +1,92 @@
+/*
+ * 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>
+
+#define SID_ONE		0x00010000
+
+static struct schib schib;
+static int test_device_sid;
+
+static void test_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+	int scn;
+	int scn_found = 0;
+	int dev_found = 0;
+
+	for (scn = 0; scn < 0xffff; scn++) {
+		cc = stsch(scn|SID_ONE, &schib);
+		switch (cc) {
+		case 0:		/* 0 means SCHIB stored */
+			break;
+		case 3:		/* 3 means no more channels */
+			goto out;
+		default:	/* 1 or 2 should never happened for STSCH */
+			report(0, "Unexpected cc=%d on subchannel number 0x%x",
+			       cc, scn);
+			return;
+		}
+		/* We currently only support type 0, a.k.a. I/O channels */
+		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
+			continue;
+		/* We ignore I/O channels without valid devices */
+		scn_found++;
+		if (!(pmcw->flags & PMCW_DNV))
+			continue;
+		/* We keep track of the first device as our test device */
+		if (!test_device_sid)
+			test_device_sid = scn|SID_ONE;
+		dev_found++;
+	}
+out:
+	if (!dev_found) {
+		report(0,
+		       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+		       scn, scn_found, dev_found);
+		return;
+	}
+	report(1,
+	       "Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+	       scn, scn_found, dev_found);
+}
+
+static struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "enumerate (stsch)", test_enumerate },
+	{ NULL, NULL }
+};
+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("Channel Subsystem");
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		tests[i].func();
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 07013b2..a436ec0 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -83,3 +83,7 @@ extra_params = -m 1G
 [sclp-3g]
 file = sclp.elf
 extra_params = -m 3G
+
+[css]
+file = css.elf
+extra_params =-device ccw-pong
-- 
2.25.1

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

[Janosch Frank]

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

On 4/24/20 12:45 PM, 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.h     |  1 +
>  s390x/Makefile      |  2 +
>  s390x/css.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 ++
>  4 files changed, 99 insertions(+)
>  create mode 100644 s390x/css.c
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index bab0dd5..9417541 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -82,6 +82,7 @@ struct pmcw {
>  	uint8_t  chpid[8];
>  	uint32_t flags2;
>  };
> +#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)

Why isn't this in the library patch?

>  
>  struct schib {
>  	struct pmcw pmcw;
> diff --git a/s390x/Makefile b/s390x/Makefile
> index ddb4b48..baebf18 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -17,6 +17,7 @@ tests += $(TEST_DIR)/stsi.elf
>  tests += $(TEST_DIR)/skrf.elf
>  tests += $(TEST_DIR)/smp.elf
>  tests += $(TEST_DIR)/sclp.elf
> +tests += $(TEST_DIR)/css.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> @@ -51,6 +52,7 @@ cflatobjs += lib/s390x/sclp-console.o
>  cflatobjs += lib/s390x/interrupt.o
>  cflatobjs += lib/s390x/mmu.o
>  cflatobjs += lib/s390x/smp.o
> +cflatobjs += lib/s390x/css_dump.o

Why isn't this in the library patch?

>  
>  OBJDIRS += lib/s390x
>  
> diff --git a/s390x/css.c b/s390x/css.c
> new file mode 100644
> index 0000000..cc97e79
> --- /dev/null
> +++ b/s390x/css.c
> @@ -0,0 +1,92 @@
> +/*
> + * 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>
> +
> +#define SID_ONE		0x00010000

Why isn't this in the library patch?

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

Give the pipe some space :)

> +		dev_found++;

Newlines would make this more readable.

> +	}
> +out:

We can report dev_found instead of 0/1 and a if/else

report(dev_found,
	"Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
                    scn, scn_found, dev_found);	

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



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

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

[Pierre Morel]

On 2020-04-27 15:06, Janosch Frank wrote:
> On 4/24/20 12:45 PM, Pierre Morel wrote:
>> First step for testing the channel subsystem is to enumerate the css and
>> retrieve the css devices.
>>
...snip...
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index bab0dd5..9417541 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -82,6 +82,7 @@ struct pmcw {
>>   	uint8_t  chpid[8];
>>   	uint32_t flags2;
>>   };
>> +#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
> 
> Why isn't this in the library patch?

OK


>> @@ -51,6 +52,7 @@ cflatobjs += lib/s390x/sclp-console.o
>>   cflatobjs += lib/s390x/interrupt.o
>>   cflatobjs += lib/s390x/mmu.o
>>   cflatobjs += lib/s390x/smp.o
>> +cflatobjs += lib/s390x/css_dump.o
> 
> Why isn't this in the library patch?

OK

> 
...snip...
>> +#define SID_ONE		0x00010000
> 
> Why isn't this in the library patch?

OK

> 
...snip...
>> +		if (!test_device_sid)
>> +			test_device_sid = scn|SID_ONE;
> 
> Give the pipe some space :)
> 
>> +		dev_found++;
> 
> Newlines would make this more readable.

OK and yes.

> 
>> +	}
>> +out:
> 
> We can report dev_found instead of 0/1 and a if/else
> 
> report(dev_found,
> 	"Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
>                      scn, scn_found, dev_found);	

Yes, :)


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Fri, 24 Apr 2020 12:45:48 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> First step for testing the channel subsystem is to enumerate the css and
> retrieve the css devices.
> 
> This tests the success of STSCH I/O instruction, we do not test the

s/of/of the/

> reaction of the VM for an instruction with wrong parameters.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  1 +
>  s390x/Makefile      |  2 +
>  s390x/css.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  4 ++
>  4 files changed, 99 insertions(+)
>  create mode 100644 s390x/css.c
> 

(...)

> +static void test_enumerate(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int cc;
> +	int scn;
> +	int scn_found = 0;
> +	int dev_found = 0;
> +
> +	for (scn = 0; scn < 0xffff; scn++) {
> +		cc = stsch(scn|SID_ONE, &schib);
> +		switch (cc) {
> +		case 0:		/* 0 means SCHIB stored */
> +			break;
> +		case 3:		/* 3 means no more channels */
> +			goto out;
> +		default:	/* 1 or 2 should never happened for STSCH */

s/happened/happen/

> +			report(0, "Unexpected cc=%d on subchannel number 0x%x",
> +			       cc, scn);
> +			return;
> +		}

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

[Pierre Morel]

On 2020-05-14 14:05, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 12:45:48 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> First step for testing the channel subsystem is to enumerate the css and
>> retrieve the css devices.
>>
>> This tests the success of STSCH I/O instruction, we do not test the
> 
> s/of/of the/

Yes, zhanks.

> 
>> reaction of the VM for an instruction with wrong parameters.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   s390x/Makefile      |  2 +
>>   s390x/css.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |  4 ++
>>   4 files changed, 99 insertions(+)
>>   create mode 100644 s390x/css.c
>>
> 
> (...)
> 
>> +static void test_enumerate(void)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
...snip...
			goto out;
>> +		default:	/* 1 or 2 should never happened for STSCH */
> 
> s/happened/happen/

Yes, thanks.

> 
>> +			report(0, "Unexpected cc=%d on subchannel number 0x%x",
>> +			       cc, scn);
>> +			return;
>> +		}
> 

Thanks,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Pierre Morel]

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

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

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

diff --git a/s390x/css.c b/s390x/css.c
index cc97e79..fa068bf 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -68,11 +68,59 @@ out:
 	       scn, scn_found, dev_found);
 }
 
+static void test_enable(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+	/* Read the SCHIB for this subchannel */
+	cc = stsch(test_device_sid, &schib);
+	if (cc) {
+		report(0, "stsch cc=%d", cc);
+		return;
+	}
+
+	/* Update the SCHIB to enable the channel */
+	pmcw->flags |= PMCW_ENABLE;
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(test_device_sid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report(0, "msch cc=%d", cc);
+		return;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(test_device_sid, &schib);
+	if (cc) {
+		report(0, "stsch cc=%d", cc);
+		return;
+	}
+
+	if (!(pmcw->flags & PMCW_ENABLE)) {
+		report(0, "Enable failed. pmcw: %x", pmcw->flags);
+		return;
+	}
+	report(1, "Tested");
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
+	{ "enable (msch)", test_enable },
 	{ NULL, NULL }
 };
 
-- 
2.25.1

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Janosch Frank]

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

On 4/24/20 12:45 PM, 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.
> 
> This tests the MSCH instruction to enable a channel succesfuly.

successfully

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

Would we expect needing retries for the pong device?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index cc97e79..fa068bf 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -68,11 +68,59 @@ out:
>  	       scn, scn_found, dev_found);
>  }
>  
> +static void test_enable(void)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int cc;
> +
> +	if (!test_device_sid) {
> +		report_skip("No device");
> +		return;
> +	}

If these tests are layered on top of each other and need a device to
work, we should abort or skip and exit the test if the enumeration
doesn't bring up devices

> +	/* Read the SCHIB for this subchannel */
> +	cc = stsch(test_device_sid, &schib);
> +	if (cc) {
> +		report(0, "stsch cc=%d", cc);
> +		return;
> +	}
> +
> +	/* Update the SCHIB to enable the channel */
> +	pmcw->flags |= PMCW_ENABLE;
> +
> +	/* Tell the CSS we want to modify the subchannel */
> +	cc = msch(test_device_sid, &schib);
> +	if (cc) {
> +		/*
> +		 * If the subchannel is status pending or
> +		 * if a function is in progress,
> +		 * we consider both cases as errors.
> +		 */
> +		report(0, "msch cc=%d", cc);
> +		return;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again to verify the enablement
> +	 */
> +	cc = stsch(test_device_sid, &schib);
> +	if (cc) {
> +		report(0, "stsch cc=%d", cc);
> +		return;
> +	}
> +
> +	if (!(pmcw->flags & PMCW_ENABLE)) {
> +		report(0, "Enable failed. pmcw: %x", pmcw->flags);
> +		return;
> +	}
> +	report(1, "Tested");

s/Tested/Enabled/

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



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

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Pierre Morel]

On 2020-04-27 15:11, Janosch Frank wrote:
> On 4/24/20 12:45 PM, 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.
>>
>> This tests the MSCH instruction to enable a channel succesfuly.
> 
> successfully

Thx

> 
>> This is NOT a routine to really enable the channel, no retry is done,
>> in case of error, a report is made.
> 
> Would we expect needing retries for the pong device?

Yes it can be that we need to retry some instructions if we want them to 
succeed.
This is the case for example if we develop a driver for an operating system.
When working with firmware, sometime, things do not work at the first 
time. Mostly due to races in silicium, firmware or hypervisor or between 
them all.

Since our purpose is to detect such problems we do not retry 
instructions but report the error.

If we detect such problem we may in the future enhance the tests.

> 
>>


>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
> 
> If these tests are layered on top of each other and need a device to
> work, we should abort or skip and exit the test if the enumeration
> doesn't bring up devices

OK, we can abort instead of skipping

>> +	report(1, "Tested");
> 
> s/Tested/Enabled/

OK

Thanks,
Regards,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Cornelia Huck]

On Tue, 28 Apr 2020 10:27:36 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-04-27 15:11, Janosch Frank wrote:
> > On 4/24/20 12:45 PM, Pierre Morel wrote:  

> >> This is NOT a routine to really enable the channel, no retry is done,
> >> in case of error, a report is made.  
> > 
> > Would we expect needing retries for the pong device?  
> 
> Yes it can be that we need to retry some instructions if we want them to 
> succeed.
> This is the case for example if we develop a driver for an operating system.
> When working with firmware, sometime, things do not work at the first 
> time. Mostly due to races in silicium, firmware or hypervisor or between 
> them all.
> 
> Since our purpose is to detect such problems we do not retry 
> instructions but report the error.
> 
> If we detect such problem we may in the future enhance the tests.

I think I've seen retries needed on z/VM in the past; do you know if
that still happens?

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Pierre Morel]

On 2020-05-14 14:08, Cornelia Huck wrote:
> On Tue, 28 Apr 2020 10:27:36 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-04-27 15:11, Janosch Frank wrote:
>>> On 4/24/20 12:45 PM, Pierre Morel wrote:
> 
>>>> This is NOT a routine to really enable the channel, no retry is done,
>>>> in case of error, a report is made.
>>>
>>> Would we expect needing retries for the pong device?
>>
>> Yes it can be that we need to retry some instructions if we want them to
>> succeed.
>> This is the case for example if we develop a driver for an operating system.
>> When working with firmware, sometime, things do not work at the first
>> time. Mostly due to races in silicium, firmware or hypervisor or between
>> them all.
>>
>> Since our purpose is to detect such problems we do not retry
>> instructions but report the error.
>>
>> If we detect such problem we may in the future enhance the tests.
> 
> I think I've seen retries needed on z/VM in the past; do you know if
> that still happens?
> 

I did not try the tests under z/VM, nor direct on an LPAR, only under 
QEMU/KVM.
Under QEMU/KVM, I did not encounter any need for retry, 100% of the 
enabled succeeded on first try.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Cornelia Huck]

On Fri, 15 May 2020 09:11:52 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-05-14 14:08, Cornelia Huck wrote:
> > On Tue, 28 Apr 2020 10:27:36 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> On 2020-04-27 15:11, Janosch Frank wrote:  
> >>> On 4/24/20 12:45 PM, Pierre Morel wrote:  
> >   
> >>>> This is NOT a routine to really enable the channel, no retry is done,
> >>>> in case of error, a report is made.  
> >>>
> >>> Would we expect needing retries for the pong device?  
> >>
> >> Yes it can be that we need to retry some instructions if we want them to
> >> succeed.
> >> This is the case for example if we develop a driver for an operating system.
> >> When working with firmware, sometime, things do not work at the first
> >> time. Mostly due to races in silicium, firmware or hypervisor or between
> >> them all.
> >>
> >> Since our purpose is to detect such problems we do not retry
> >> instructions but report the error.
> >>
> >> If we detect such problem we may in the future enhance the tests.  
> > 
> > I think I've seen retries needed on z/VM in the past; do you know if
> > that still happens?
> >   
> 
> I did not try the tests under z/VM, nor direct on an LPAR, only under 
> QEMU/KVM.
> Under QEMU/KVM, I did not encounter any need for retry, 100% of the 
> enabled succeeded on first try.

Yep, QEMU/KVM should be fine. Do you plan to run this on anything else?

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Janosch Frank]

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

On 5/15/20 10:25 AM, Cornelia Huck wrote:
> On Fri, 15 May 2020 09:11:52 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-05-14 14:08, Cornelia Huck wrote:
>>> On Tue, 28 Apr 2020 10:27:36 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>   
>>>> On 2020-04-27 15:11, Janosch Frank wrote:  
>>>>> On 4/24/20 12:45 PM, Pierre Morel wrote:  
>>>   
>>>>>> This is NOT a routine to really enable the channel, no retry is done,
>>>>>> in case of error, a report is made.  
>>>>>
>>>>> Would we expect needing retries for the pong device?  
>>>>
>>>> Yes it can be that we need to retry some instructions if we want them to
>>>> succeed.
>>>> This is the case for example if we develop a driver for an operating system.
>>>> When working with firmware, sometime, things do not work at the first
>>>> time. Mostly due to races in silicium, firmware or hypervisor or between
>>>> them all.
>>>>
>>>> Since our purpose is to detect such problems we do not retry
>>>> instructions but report the error.
>>>>
>>>> If we detect such problem we may in the future enhance the tests.  
>>>
>>> I think I've seen retries needed on z/VM in the past; do you know if
>>> that still happens?
>>>   
>>
>> I did not try the tests under z/VM, nor direct on an LPAR, only under 
>> QEMU/KVM.
>> Under QEMU/KVM, I did not encounter any need for retry, 100% of the 
>> enabled succeeded on first try.
> 
> Yep, QEMU/KVM should be fine. Do you plan to run this on anything else?
> 

I'd like to have it compatible with z/VM / LPAR as well if it isn't too
much work. You never know when you need it and having tests for all
hypervisors has been quite a help in the past.


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

Re: [kvm-unit-tests PATCH v6 07/10] s390x: css: msch, enable test

[Pierre Morel]

On 2020-05-15 10:53, Janosch Frank wrote:
> On 5/15/20 10:25 AM, Cornelia Huck wrote:
>> On Fri, 15 May 2020 09:11:52 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 2020-05-14 14:08, Cornelia Huck wrote:
>>>> On Tue, 28 Apr 2020 10:27:36 +0200
>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>    
>>>>> On 2020-04-27 15:11, Janosch Frank wrote:
>>>>>> On 4/24/20 12:45 PM, Pierre Morel wrote:
>>>>    
>>>>>>> This is NOT a routine to really enable the channel, no retry is done,
>>>>>>> in case of error, a report is made.
>>>>>>
>>>>>> Would we expect needing retries for the pong device?
>>>>>
>>>>> Yes it can be that we need to retry some instructions if we want them to
>>>>> succeed.
>>>>> This is the case for example if we develop a driver for an operating system.
>>>>> When working with firmware, sometime, things do not work at the first
>>>>> time. Mostly due to races in silicium, firmware or hypervisor or between
>>>>> them all.
>>>>>
>>>>> Since our purpose is to detect such problems we do not retry
>>>>> instructions but report the error.
>>>>>
>>>>> If we detect such problem we may in the future enhance the tests.
>>>>
>>>> I think I've seen retries needed on z/VM in the past; do you know if
>>>> that still happens?
>>>>    
>>>
>>> I did not try the tests under z/VM, nor direct on an LPAR, only under
>>> QEMU/KVM.
>>> Under QEMU/KVM, I did not encounter any need for retry, 100% of the
>>> enabled succeeded on first try.
>>
>> Yep, QEMU/KVM should be fine. Do you plan to run this on anything else?
>>
> 
> I'd like to have it compatible with z/VM / LPAR as well if it isn't too
> much work. You never know when you need it and having tests for all
> hypervisors has been quite a help in the past.
> 

OK, then I can add some retries in prevision.



-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH v6 08/10] s390x: define wfi: wait for interrupt

[Pierre Morel]

wfi(irq_mask) allows the programm to wait for an interrupt.
The interrupt handler is in charge to remove the WAIT bit
when it finished handling interrupt.

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

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index a0d2362..e04866c 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -13,6 +13,7 @@
 #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
@@ -254,6 +255,16 @@ static inline void load_psw_mask(uint64_t mask)
 		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
 }
 
+static inline void wfi(uint64_t irq_mask)
+{
+	uint64_t psw_mask;
+
+	psw_mask = extract_psw_mask();
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
+	load_psw_mask(psw_mask);
+}
+
+
 static inline void enter_pstate(void)
 {
 	uint64_t mask;
-- 
2.25.1

Re: [kvm-unit-tests PATCH v6 08/10] s390x: define wfi: wait for interrupt

[Janosch Frank]

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

On 4/24/20 12:45 PM, Pierre Morel wrote:
> wfi(irq_mask) allows the programm to wait for an interrupt.

s/programm/program/

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

...finished handling the interrupt.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index a0d2362..e04866c 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -13,6 +13,7 @@
>  #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
> @@ -254,6 +255,16 @@ static inline void load_psw_mask(uint64_t mask)
>  		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
>  }
>  
> +static inline void wfi(uint64_t irq_mask)

enabled_wait()

> +{
> +	uint64_t psw_mask;

You can directly initialize this variable.

> +
> +	psw_mask = extract_psw_mask();
> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);

Maybe add a comment here:

/*
 * After being woken and having processed the interrupt, let's restore
the PSW mask.
*/

> +	load_psw_mask(psw_mask);
> +}
> +
> +
>  static inline void enter_pstate(void)
>  {
>  	uint64_t mask;
> 



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

Re: [kvm-unit-tests PATCH v6 08/10] s390x: define wfi: wait for interrupt

[Pierre Morel]

On 2020-04-27 14:59, Janosch Frank wrote:
> On 4/24/20 12:45 PM, Pierre Morel wrote:
>> wfi(irq_mask) allows the programm to wait for an interrupt.
> 
> s/programm/program/

Thx,

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

OK, thx

> 

>>   }
>>   
>> +static inline void wfi(uint64_t irq_mask)
> 
> enabled_wait()


I do not like enabled_wait(), we do not know what is enabled and we do 
not know what we are waiting for.

What about wait_for_interrupt()

> 
>> +{
>> +	uint64_t psw_mask;
> 
> You can directly initialize this variable.
> 
>> +
>> +	psw_mask = extract_psw_mask();
>> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
> 
> Maybe add a comment here:
> 
> /*
>   * After being woken and having processed the interrupt, let's restore
> the PSW mask.
> */
> 
>> +	load_psw_mask(psw_mask);
>> +}
>> +

I can do this, but wasn't it obvious?


Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v6 08/10] s390x: define wfi: wait for interrupt

[Janosch Frank]

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

On 4/28/20 10:44 AM, Pierre Morel wrote:
> 
> 
> On 2020-04-27 14:59, Janosch Frank wrote:
>> On 4/24/20 12:45 PM, Pierre Morel wrote:
>>> wfi(irq_mask) allows the programm to wait for an interrupt.
>>
>> s/programm/program/
> 
> Thx,
> 
>>
>>> The interrupt handler is in charge to remove the WAIT bit
>>> when it finished handling interrupt.
>>
>> ...finished handling the interrupt.
> 
> OK, thx
> 
>>
> 
>>>   }
>>>   
>>> +static inline void wfi(uint64_t irq_mask)
>>
>> enabled_wait()
> 
> 
> I do not like enabled_wait(), we do not know what is enabled and we do 
> not know what we are waiting for.
> 
> What about wait_for_interrupt()

As long as it's not called wfi...

> 
>>
>>> +{
>>> +	uint64_t psw_mask;
>>
>> You can directly initialize this variable.
>>
>>> +
>>> +	psw_mask = extract_psw_mask();
>>> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
>>
>> Maybe add a comment here:
>>
>> /*
>>   * After being woken and having processed the interrupt, let's restore
>> the PSW mask.
>> */
>>
>>> +	load_psw_mask(psw_mask);
>>> +}
>>> +
> 
> I can do this, but wasn't it obvious?

It took me a minute, so it will take even longer for developers that are
not yet familiar with s390 kernel development.

> 
> 
> Regards,
> Pierre
> 



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

Re: [kvm-unit-tests PATCH v6 08/10] s390x: define wfi: wait for interrupt

[Pierre Morel]

On 2020-04-28 11:20, Janosch Frank wrote:
> On 4/28/20 10:44 AM, Pierre Morel wrote:
>>
>>
>> On 2020-04-27 14:59, Janosch Frank wrote:
>>> On 4/24/20 12:45 PM, Pierre Morel wrote:
>>>> wfi(irq_mask) allows the programm to wait for an interrupt.
>>>
>>> s/programm/program/
>>
>> Thx,
>>
>>>
>>>> The interrupt handler is in charge to remove the WAIT bit
>>>> when it finished handling interrupt.
>>>
>>> ...finished handling the interrupt.
>>
>> OK, thx
>>
>>>
>>
>>>>    }
>>>>    
>>>> +static inline void wfi(uint64_t irq_mask)
>>>
>>> enabled_wait()
>>
>>
>> I do not like enabled_wait(), we do not know what is enabled and we do
>> not know what we are waiting for.
>>
>> What about wait_for_interrupt()
> 
> As long as it's not called wfi...
> 
>>
>>>
>>>> +{
>>>> +	uint64_t psw_mask;
>>>
>>> You can directly initialize this variable.
>>>
>>>> +
>>>> +	psw_mask = extract_psw_mask();
>>>> +	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
>>>
>>> Maybe add a comment here:
>>>
>>> /*
>>>    * After being woken and having processed the interrupt, let's restore
>>> the PSW mask.
>>> */
>>>
>>>> +	load_psw_mask(psw_mask);
>>>> +}
>>>> +
>>
>> I can do this, but wasn't it obvious?
> 
> It took me a minute, so it will take even longer for developers that are
> not yet familiar with s390 kernel development.
> 
>>
>>
>> Regards,
>> Pierre
>>
> 
> 

OK, will do
Thx

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH v6 09/10] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

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

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

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

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

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

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index e04866c..7a63ab7 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,6 +10,7 @@
 #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
diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 9417541..f278d0b 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -97,6 +97,19 @@ struct irb {
 	uint32_t emw[8];
 } __attribute__ ((aligned(4)));
 
+#define CCW_CMD_SENSE_ID	0xe4
+#define PONG_CU			0xc0ca
+struct senseid {
+	/* common part */
+	uint8_t reserved;        /* always 0x'FF' */
+	uint16_t cu_type;        /* control unit type */
+	uint8_t cu_model;        /* control unit model */
+	uint16_t dev_type;       /* device type */
+	uint8_t dev_model;       /* device model */
+	uint8_t unused;          /* padding byte */
+	uint8_t padding[256 - 10]; /* Extra padding for CCW */
+} __attribute__ ((aligned(4))) __attribute__ ((packed));
+
 /* CSS low level access functions */
 
 static inline int ssch(unsigned long schid, struct orb *addr)
@@ -254,4 +267,11 @@ static inline struct ccw *dump_ccw(struct ccw *cp)
 	return NULL;
 }
 #endif /* DEBUG_CSS */
+
+#define SID_ONE         0x00010000
+
+/* Library functions */
+int enable_subchannel(unsigned int sid);
+int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
new file mode 100644
index 0000000..80b9359
--- /dev/null
+++ b/lib/s390x/css_lib.c
@@ -0,0 +1,55 @@
+/*
+ * Channel subsystem library functions
+ *
+ * Copyright (c) 2020 IBM Corp.
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+#include <stdint.h>
+#include <stddef.h>
+#include <css.h>
+
+int enable_subchannel(unsigned int sid)
+{
+	struct schib schib;
+	struct pmcw *pmcw = &schib.pmcw;
+	int try_count = 5;
+	int cc;
+
+	if (!(sid & SID_ONE))
+		return -1;
+
+	cc = stsch(sid, &schib);
+	if (cc)
+		return -cc;
+
+	do {
+		pmcw->flags |= PMCW_ENABLE;
+
+		cc = msch(sid, &schib);
+		if (cc)
+			return -cc;
+
+		cc = stsch(sid, &schib);
+		if (cc)
+			return -cc;
+
+	} while (!(pmcw->flags & PMCW_ENABLE) && --try_count);
+
+	return try_count;
+}
+
+int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
+{
+	struct orb orb;
+
+	orb.intparm = sid;
+	orb.ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	orb.cpa = (unsigned int) (unsigned long)ccw;
+
+	return ssch(sid, &orb);
+}
diff --git a/s390x/Makefile b/s390x/Makefile
index baebf18..166cb5c 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -53,6 +53,7 @@ cflatobjs += lib/s390x/interrupt.o
 cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/css_dump.o
+cflatobjs += lib/s390x/css_lib.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/css.c b/s390x/css.c
index fa068bf..b9dbf01 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -20,9 +20,26 @@
 #include <css.h>
 
 #define SID_ONE		0x00010000
+#define PSW_PRG_MASK (PSW_MASK_EA | PSW_MASK_BA)
+
+#define PONG_CU_TYPE		0xc0ca
+
+struct lowcore *lowcore = (void *)0x0;
 
 static struct schib schib;
 static int test_device_sid;
+#define NUM_CCW  100
+static struct ccw1 ccw[NUM_CCW];
+static struct irb irb;
+static struct senseid senseid;
+
+static void set_io_irq_subclass_mask(uint64_t const new_mask)
+{
+	asm volatile (
+		"lctlg %%c6, %%c6, %[source]\n"
+		: /* No outputs */
+		: [source] "R" (new_mask));
+}
 
 static void test_enumerate(void)
 {
@@ -115,12 +132,143 @@ static void test_enable(void)
 	report(1, "Tested");
 }
 
+static void enable_io_isc(void)
+{
+	/* Let's enable all ISCs for I/O interrupt */
+	set_io_irq_subclass_mask(0x00000000ff000000);
+}
+
+static void irq_io(void)
+{
+	int ret = 0;
+	char *flags;
+	int sid;
+
+	report_prefix_push("Interrupt");
+	/* Lowlevel set the SID as interrupt parameter. */
+	if (lowcore->io_int_param != test_device_sid) {
+		report(0,
+		       "Bad io_int_param: %x expected %x",
+		       lowcore->io_int_param, test_device_sid);
+		goto pop;
+	}
+	report_prefix_pop();
+
+	report_prefix_push("tsch");
+	sid = lowcore->subsys_id_word;
+	ret = tsch(sid, &irb);
+	switch (ret) {
+	case 1:
+		dump_irb(&irb);
+		flags = dump_scsw_flags(irb.scsw.ctrl);
+		report(0,
+		       "I/O interrupt, but sch not status pending: %s", flags);
+		break;
+	case 2:
+		report(0, "TSCH returns unexpected CC 2");
+		break;
+	case 3:
+		report(0, "Subchannel %08x not operational", sid);
+		break;
+	case 0:
+		/* Stay humble on success */
+		break;
+	}
+pop:
+	report_prefix_pop();
+	lowcore->io_old_psw.mask &= ~PSW_MASK_WAIT;
+}
+
+static int start_subchannel(int code, void *data, int count)
+{
+	int ret;
+
+	report_prefix_push("start_senseid");
+	/* Build the CCW chain with a single CCW */
+	ccw[0].code = code;
+	ccw[0].flags = 0; /* No flags need to be set */
+	ccw[0].count = count;
+	ccw[0].data_address = (int)(unsigned long)data;
+
+	ret = start_ccw1_chain(test_device_sid, ccw);
+	if (ret) {
+		report(0, "start_ccw_chain failed ret=%d", ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+/*
+ * test_sense
+ * Pre-requisits:
+ * - We need the QEMU PONG device as the first recognized
+ *   device by the enumeration.
+ * - ./s390x-run s390x/css.elf -device ccw-pong,cu_type=0xc0ca
+ */
+static void test_sense(void)
+{
+	int ret;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	ret = enable_subchannel(test_device_sid);
+	if (ret < 0) {
+		report(0,
+		       "Could not enable the subchannel: %08x",
+		       test_device_sid);
+		return;
+	}
+
+	ret = register_io_int_func(irq_io);
+	if (ret) {
+		report(0, "Could not register IRQ handler");
+		goto unreg_cb;
+	}
+
+	lowcore->io_int_param = 0;
+
+	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid));
+	if (!ret) {
+		report(0, "start_senseid failed");
+		goto unreg_cb;
+	}
+
+	wfi(PSW_MASK_IO);
+
+	if (lowcore->io_int_param != test_device_sid) {
+		report(0,
+		       "No interrupts. io_int_param: expect 0x%08x, got 0x%08x",
+		       test_device_sid, lowcore->io_int_param);
+		goto unreg_cb;
+	}
+
+	report_info("reserved %02x cu_type %04x cu_model %02x dev_type %04x dev_model %02x",
+		    senseid.reserved, senseid.cu_type, senseid.cu_model,
+		    senseid.dev_type, senseid.dev_model);
+
+	if (senseid.cu_type == PONG_CU)
+		report(1, "cu_type: expect 0x%04x got 0x%04x",
+		       PONG_CU_TYPE, senseid.cu_type);
+	else
+		report(0, "cu_type: expect 0x%04x got 0x%04x",
+		       PONG_CU_TYPE, senseid.cu_type);
+
+unreg_cb:
+	unregister_io_int_func(irq_io);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
+	{ "sense (ssch/tsch)", test_sense },
 	{ NULL, NULL }
 };
 
@@ -129,6 +277,7 @@ int main(int argc, char *argv[])
 	int i;
 
 	report_prefix_push("Channel Subsystem");
+	enable_io_isc();
 	for (i = 0; tests[i].name; i++) {
 		report_prefix_push(tests[i].name);
 		tests[i].func();
-- 
2.25.1

Re: [kvm-unit-tests PATCH v6 09/10] s390x: css: ssch/tsch with sense and interrupt

[Cornelia Huck]

On Fri, 24 Apr 2020 12:45:51 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We add a new css_lib file to contain the I/O function we may

s/function/functions/

> share with different tests.
> First function is the subchannel_enable() function.
> 
> When a channel is enabled we can start a SENSE_ID command using
> the SSCH instruction to recognize the control unit and device.
> 
> This tests the success of SSCH, the I/O interruption and the TSCH
> instructions.
> 
> The test expects a device with a control unit type of 0xC0CA as the
> first subchannel of the CSS.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h |   1 +
>  lib/s390x/css.h          |  20 ++++++
>  lib/s390x/css_lib.c      |  55 +++++++++++++++
>  s390x/Makefile           |   1 +
>  s390x/css.c              | 149 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 226 insertions(+)
>  create mode 100644 lib/s390x/css_lib.c
> 

(...)

> +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, but sch not status pending: %s", flags);

"...but tsch reporting sch as not status pending" ?

A buggy implementation might give the wrong cc for tsch, but still
indicate status pending in the control block.

> +		break;
> +	case 2:
> +		report(0, "TSCH returns unexpected CC 2");
> +		break;
> +	case 3:
> +		report(0, "Subchannel %08x not operational", sid);

"tsch reporting subchannel %08x as not operational" ?

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

You're always send the full (extended) sense id block. What if the the
machine you're running on doesn't support extended sense id? Would the
SLI ccw flag help?

> +	if (!ret) {
> +		report(0, "start_senseid failed");

"ssch failed for SENSE ID on sch <sch>" ?

> +		goto unreg_cb;
> +	}
> +
> +	wfi(PSW_MASK_IO);
> +
> +	if (lowcore->io_int_param != test_device_sid) {
> +		report(0,
> +		       "No interrupts. io_int_param: expect 0x%08x, got 0x%08x",
> +		       test_device_sid, lowcore->io_int_param);

Doesn't irq_io() already moan here?

> +		goto unreg_cb;
> +	}
> +
> +	report_info("reserved %02x cu_type %04x cu_model %02x dev_type %04x dev_model %02x",
> +		    senseid.reserved, senseid.cu_type, senseid.cu_model,
> +		    senseid.dev_type, senseid.dev_model);
> +
> +	if (senseid.cu_type == PONG_CU)
> +		report(1, "cu_type: expect 0x%04x got 0x%04x",
> +		       PONG_CU_TYPE, senseid.cu_type);
> +	else
> +		report(0, "cu_type: expect 0x%04x got 0x%04x",
> +		       PONG_CU_TYPE, senseid.cu_type);
> +
> +unreg_cb:
> +	unregister_io_int_func(irq_io);
> +}
> +

Re: [kvm-unit-tests PATCH v6 09/10] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel]

On 2020-05-14 14:24, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 12:45:51 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We add a new css_lib file to contain the I/O function we may
> 
> s/function/functions/

Yes, thanks.


>> +	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, but sch not status pending: %s", flags);
> 
> "...but tsch reporting sch as not status pending" ?

Yes, better, Thx

> 
> A buggy implementation might give the wrong cc for tsch, but still
> indicate status pending in the control block.

OK, I will write the status for other error cases too.
> 
>> +		break;
>> +	case 2:
>> +		report(0, "TSCH returns unexpected CC 2");

will also s/TSCH/tsch/ here

>> +		break;
>> +	case 3:
>> +		report(0, "Subchannel %08x not operational", sid);
> 
> "tsch reporting subchannel %08x as not operational" ?

Yes, better.
and I will standardize these three reports.

> 
>> +		break;
>> +	case 0:
>> +		/* Stay humble on success */
>> +		break;
>> +	}
>> +pop:
>> +	report_prefix_pop();
>> +	lowcore->io_old_psw.mask &= ~PSW_MASK_WAIT;
>> +}
> 
...snip...

>> +	lowcore->io_int_param = 0;
>> +
>> +	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid));
> 
> You're always send the full (extended) sense id block. What if the the
> machine you're running on doesn't support extended sense id? Would the
> SLI ccw flag help?

Yes, you are right, since I only use senseid for recognition of the PONG 
subchannel, I can accept non extended senseid

> 
>> +	if (!ret) {
>> +		report(0, "start_senseid failed");
> 
> "ssch failed for SENSE ID on sch <sch>" ?

Better. Thx

> 
>> +		goto unreg_cb;
>> +	}
>> +
>> +	wfi(PSW_MASK_IO);
>> +
>> +	if (lowcore->io_int_param != test_device_sid) {
>> +		report(0,
>> +		       "No interrupts. io_int_param: expect 0x%08x, got 0x%08x",
>> +		       test_device_sid, lowcore->io_int_param);
> 
> Doesn't irq_io() already moan here?

Yes, right, I kept this from last version with delays.
It has no sense here:
- We are the only user of the CSS
- If the interrupt did not fire we are stuck in wfi()

Thanks.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH v6 10/10] s390x: css: ping pong

[Pierre Morel]

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

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

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

diff --git a/s390x/css.c b/s390x/css.c
index b9dbf01..7cd8731 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -23,6 +23,12 @@
 #define PSW_PRG_MASK (PSW_MASK_EA | PSW_MASK_BA)
 
 #define PONG_CU_TYPE		0xc0ca
+/* Channel Commands for PONG device */
+#define PONG_WRITE	0x21 /* Write */
+#define PONG_READ	0x22 /* Read buffer */
+
+#define BUFSZ	9
+static char buffer[BUFSZ];
 
 struct lowcore *lowcore = (void *)0x0;
 
@@ -262,6 +268,53 @@ unreg_cb:
 	unregister_io_int_func(irq_io);
 }
 
+static void test_ping(void)
+{
+	int success, result;
+	int cnt = 0, max = 4;
+
+	if (senseid.cu_type != PONG_CU) {
+		report_skip("No PONG, no ping-pong");
+		return;
+	}
+
+	result = register_io_int_func(irq_io);
+	if (result) {
+		report(0, "Could not register IRQ handler");
+		return;
+	}
+
+	while (cnt++ < max) {
+		snprintf(buffer, BUFSZ, "%08x\n", cnt);
+		success = start_subchannel(PONG_WRITE, buffer, BUFSZ);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+
+		wfi(PSW_MASK_IO);
+
+		success = start_subchannel(PONG_READ, buffer, BUFSZ);
+		if (!success) {
+			report(0, "start_subchannel failed");
+			goto unreg_cb;
+		}
+
+		wfi(PSW_MASK_IO);
+
+		result = atol(buffer);
+		if (result != (cnt + 1)) {
+			report(0, "Bad answer from pong: %08x - %08x",
+			       cnt, result);
+			goto unreg_cb;
+		}
+	}
+	report(1, "ping-pong count 0x%08x", cnt);
+
+unreg_cb:
+	unregister_io_int_func(irq_io);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -269,6 +322,7 @@ static struct {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "sense (ssch/tsch)", test_sense },
+	{ "ping-pong (ssch/tsch)", test_ping },
 	{ NULL, NULL }
 };
 
-- 
2.25.1

Re: [kvm-unit-tests PATCH v6 10/10] s390x: css: ping pong

[Janosch Frank]

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

On 4/24/20 12:45 PM, Pierre Morel wrote:
> To test a write command with the SSCH instruction we need a QEMU device,
> with control unit type 0xC0CA. The PONG device is such a device.
> 
> This type of device responds to PONG_WRITE requests by incrementing an
> integer, stored as a string at offset 0 of the CCW data.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index b9dbf01..7cd8731 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -23,6 +23,12 @@
>  #define PSW_PRG_MASK (PSW_MASK_EA | PSW_MASK_BA)
>  
>  #define PONG_CU_TYPE		0xc0ca
> +/* Channel Commands for PONG device */
> +#define PONG_WRITE	0x21 /* Write */
> +#define PONG_READ	0x22 /* Read buffer */
> +
> +#define BUFSZ	9
> +static char buffer[BUFSZ];
>  
>  struct lowcore *lowcore = (void *)0x0;
>  
> @@ -262,6 +268,53 @@ unreg_cb:
>  	unregister_io_int_func(irq_io);
>  }
>  
> +static void test_ping(void)
> +{
> +	int success, result;
> +	int cnt = 0, max = 4;
> +
> +	if (senseid.cu_type != PONG_CU) {
> +		report_skip("No PONG, no ping-pong");

"Device is not a pong device."

> +		return;
> +	}
> +
> +	result = register_io_int_func(irq_io);
> +	if (result) {
> +		report(0, "Could not register IRQ handler");
> +		return;
> +	}

I'm not sure if it's worth checking the return values or even having a
check in register_io_int_func() in the first place.

> +
> +	while (cnt++ < max) {
> +		snprintf(buffer, BUFSZ, "%08x\n", cnt);
> +		success = start_subchannel(PONG_WRITE, buffer, BUFSZ);
> +		if (!success) {
> +			report(0, "start_subchannel failed");
> +			goto unreg_cb;
> +		}
> +
> +		wfi(PSW_MASK_IO);
> +
> +		success = start_subchannel(PONG_READ, buffer, BUFSZ);
> +		if (!success) {
> +			report(0, "start_subchannel failed");
> +			goto unreg_cb;
> +		}
> +
> +		wfi(PSW_MASK_IO);
> +
> +		result = atol(buffer);
> +		if (result != (cnt + 1)) {
> +			report(0, "Bad answer from pong: %08x - %08x",
> +			       cnt, result);
> +			goto unreg_cb;
> +		}
> +	}
> +	report(1, "ping-pong count 0x%08x", cnt);
> +
> +unreg_cb:
> +	unregister_io_int_func(irq_io);
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -269,6 +322,7 @@ static struct {
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
>  	{ "sense (ssch/tsch)", test_sense },
> +	{ "ping-pong (ssch/tsch)", test_ping },
>  	{ NULL, NULL }
>  };
>  
> 



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

Re: [kvm-unit-tests PATCH v6 10/10] s390x: css: ping pong

[Pierre Morel]

On 2020-04-27 15:14, Janosch Frank wrote:
> On 4/24/20 12:45 PM, Pierre Morel wrote:
>> To test a write command with the SSCH instruction we need a QEMU device,
>> with control unit type 0xC0CA. The PONG device is such a device.
>>
>> This type of device responds to PONG_WRITE requests by incrementing an
>> integer, stored as a string at offset 0 of the CCW data.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

>> +static void test_ping(void)
>> +{
>> +	int success, result;
>> +	int cnt = 0, max = 4;
>> +
>> +	if (senseid.cu_type != PONG_CU) {
>> +		report_skip("No PONG, no ping-pong");
> 
> "Device is not a pong device."

:( so far for poetry


> 
>> +		return;
>> +	}
>> +
>> +	result = register_io_int_func(irq_io);
>> +	if (result) {
>> +		report(0, "Could not register IRQ handler");
>> +		return;
>> +	}
> 
> I'm not sure if it's worth checking the return values or even having a
> check in register_io_int_func() in the first place.

I wait for reviewers/maintainers having already given their reviewed-by 
to the interrupt registration patch to give their opinion.
Then I change it if needed.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen