s390x: Testing the Channel Subsystem I/O

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 <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
 lib/s390x/asm/arch_def.h | 15 +++++++++++----
 s390x/cstart64.S         | 15 ++++++++-------
 2 files changed, 19 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +0 -0

index 1b3bb0c..5388114 100644
@@ -10,15 +10,21 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_

Quoted 23 lines · 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 <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
  lib/s390x/asm/arch_def.h | 15 +++++++++++----
  s390x/cstart64.S         | 15 ++++++++-------
  2 files changed, 19 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +0 -0

index 1b3bb0c..5388114 100644
@@ -10,15 +10,21 @@
  #ifndef _ASM_S390X_ARCH_DEF_H_
  #define _ASM_S390X_ARCH_DEF_H_
 

lib/s390x/asm/arch_def.h line 20

Quoted 8 lines

lib/s390x/asm/arch_def.h +8 -0

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

PSW_EXCEPTION_MASK sounds a little bit unfortunate - that term rather reminds me of something that disables some interrupts
... in case you
respin, maybe rather use something like "PSW_EXC_ADDR_MODE" ?

Well, since nobody else complained yet, and the rest of the patch looks fine:

Acked-by Thomas Huth <[email protected]>

Quoted 28 lines · Thomas Huth

Quoted 27 lines · 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 <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
  lib/s390x/asm/arch_def.h | 15 +++++++++++----
  s390x/cstart64.S         | 15 ++++++++-------

  2 files changed, 19 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +0 -0

index 1b3bb0c..5388114 100644
@@ -10,15 +10,21 @@
   #ifndef _ASM_S390X_ARCH_DEF_H_
   #define _ASM_S390X_ARCH_DEF_H_

  +#define PSW_MASK_EXT            0x0100000000000000UL

lib/s390x/asm/arch_def.h +4 -0

+#define PSW_MASK_DAT            0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW        0x0008000000000000UL
+#define PSW_MASK_PSTATE            0x0001000000000000UL
+#define PSW_MASK_BA            0x0000000080000000UL

Quoted 8 lines

lib/s390x/asm/arch_def.h line 20

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+#define PSW_MASK_EA            0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK    (PSW_MASK_EA | PSW_MASK_BA)

PSW_EXCEPTION_MASK sounds a little bit unfortunate - that term rather reminds me of something that disables some interrupts
... in case you
respin, maybe rather use something like "PSW_EXC_ADDR_MODE" ?

EXCEPTIONS_PSW_MASK ?

Quoted 40 lines · Pierre Morel

Quoted 28 lines · Thomas Huth

Quoted 27 lines · 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 <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
   lib/s390x/asm/arch_def.h | 15 +++++++++++----
   s390x/cstart64.S         | 15 ++++++++-------
   2 files changed, 19 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +4 -0

index 1b3bb0c..5388114 100644
@@ -10,15 +10,21 @@
   #ifndef _ASM_S390X_ARCH_DEF_H_
   #define _ASM_S390X_ARCH_DEF_H_
   +#define PSW_MASK_EXT            0x0100000000000000UL
+#define PSW_MASK_DAT            0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW        0x0008000000000000UL
+#define PSW_MASK_PSTATE            0x0001000000000000UL
+#define PSW_MASK_BA            0x0000000080000000UL

Quoted 8 lines

lib/s390x/asm/arch_def.h line 20

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+#define PSW_MASK_EA            0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK    (PSW_MASK_EA | PSW_MASK_BA)

PSW_EXCEPTION_MASK sounds a little bit unfortunate - that term rather reminds me of something that disables some interrupts
... in case you
respin, maybe rather use something like "PSW_EXC_ADDR_MODE" ?

Quoted 1 line

EXCEPTIONS_PSW_MASK ?

I think it is the _MASK suffix that mainly bugs me here, since this is not a define that you normally use for extracting the bits from a PSW... so EXCEPTIONS_PSW without _MASK would be fine for me... but as long as I'm the only one who has a strange feeling about this, it's also ok if you keep the current name.

Thomas

Quoted 45 lines · Thomas Huth

Quoted 42 lines · Pierre Morel

Quoted 30 lines · Thomas Huth

Quoted 29 lines · 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 <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

---
   lib/s390x/asm/arch_def.h | 15 +++++++++++----
   s390x/cstart64.S         | 15 ++++++++-------

   2 files changed, 19 insertions(+), 11 deletions(-)

lib/s390x/asm/arch_def.h +0 -0

index 1b3bb0c..5388114 100644
@@ -10,15 +10,21 @@
    #ifndef _ASM_S390X_ARCH_DEF_H_
    #define _ASM_S390X_ARCH_DEF_H_

   +#define PSW_MASK_EXT            0x0100000000000000UL

lib/s390x/asm/arch_def.h +4 -0

+#define PSW_MASK_DAT            0x0400000000000000UL
+#define PSW_MASK_SHORT_PSW        0x0008000000000000UL
+#define PSW_MASK_PSTATE            0x0001000000000000UL
+#define PSW_MASK_BA            0x0000000080000000UL

Quoted 8 lines

lib/s390x/asm/arch_def.h line 20

Quoted 3 lines

lib/s390x/asm/arch_def.h +3 -0

+#define PSW_MASK_EA            0x0000000100000000UL
+
+#define PSW_EXCEPTION_MASK    (PSW_MASK_EA | PSW_MASK_BA)

PSW_EXCEPTION_MASK sounds a little bit unfortunate - that term rather reminds me of something that disables some interrupts
... in case you
respin, maybe rather use something like "PSW_EXC_ADDR_MODE" ?

Quoted 1 line

EXCEPTIONS_PSW_MASK ?

Quoted 8 lines

I think it is the _MASK suffix that mainly bugs me here, since this is not a define that you normally use for extracting the bits from a PSW... so EXCEPTIONS_PSW without _MASK would be fine for me... but as long as I'm the only one who has a strange feeling about this, it's also ok if you keep the current name.

  Thomas

The _MASK is because it is applied to the psw.mask and not to the psw.addr part.

But I agree that the name is not good, to keep the naming convention, may be it should be:

PSW_MASK_ON_EXCEPTION

beginning with PSW_MASK_ like all other psw.mask definitions and ON_EXCEPTION clearly define when it is used.

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 <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 lib/s390x/asm/arch_def.h | 11 ++++++-----
 s390x/cstart64.S         |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

lib/s390x/asm/arch_def.h +6 -5

index 5388114..12045ff 100644
@@ -19,17 +19,18 @@
 
 #define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
 
+#define CR0_EXTM_SCLP			0x0000000000000200UL
+#define CR0_EXTM_EXTC			0x0000000000002000UL
+#define CR0_EXTM_EMGC			0x0000000000004000UL
+#define CR0_EXTM_MASK			0x0000000000006200UL
+#define CR0_AFP_REG_CRTL		0x0000000000040000UL
+
 #ifndef __ASSEMBLER__
 struct psw {
 	uint64_t	mask;
 	uint64_t	addr;
 };
 
-#define CR0_EXTM_SCLP			0x0000000000000200UL
-#define CR0_EXTM_EXTC			0x0000000000002000UL
-#define CR0_EXTM_EMGC			0x0000000000004000UL
-#define CR0_EXTM_MASK			0x0000000000006200UL
-
 struct lowcore {
 	uint8_t		pad_0x0000[0x0080 - 0x0000];	/* 0x0000 */
 	uint32_t	ext_int_param;			/* 0x0080 */

s390x/cstart64.S +0 -0

index 6e85635..b50c42c 100644

Quoted 46 lines · 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 <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
  lib/s390x/asm/arch_def.h | 11 ++++++-----
  s390x/cstart64.S         |  2 +-
  2 files changed, 7 insertions(+), 6 deletions(-)

lib/s390x/asm/arch_def.h +6 -5

index 5388114..12045ff 100644
@@ -19,17 +19,18 @@
 
  #define PSW_EXCEPTION_MASK	(PSW_MASK_EA | PSW_MASK_BA)
 
+#define CR0_EXTM_SCLP			0x0000000000000200UL
+#define CR0_EXTM_EXTC			0x0000000000002000UL
+#define CR0_EXTM_EMGC			0x0000000000004000UL
+#define CR0_EXTM_MASK			0x0000000000006200UL
+#define CR0_AFP_REG_CRTL		0x0000000000040000UL
+
  #ifndef __ASSEMBLER__
  struct psw {
  	uint64_t	mask;
  	uint64_t	addr;
  };
 
-#define CR0_EXTM_SCLP			0x0000000000000200UL
-#define CR0_EXTM_EXTC			0x0000000000002000UL
-#define CR0_EXTM_EMGC			0x0000000000004000UL
-#define CR0_EXTM_MASK			0x0000000000006200UL
-
  struct lowcore {
  	uint8_t		pad_0x0000[0x0080 - 0x0000];	/* 0x0000 */
  	uint32_t	ext_int_param;			/* 0x0080 */

s390x/cstart64.S +0 -0

index 6e85635..b50c42c 100644
@@ -214,4 +214,4 @@ svc_int_psw:

s390x/cstart64.S svc_int_psw: · line 217

Quoted 6 lines

s390x/cstart64.S +1 -1

  	.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

Acked-by Thomas Huth <[email protected]>

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

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

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

Signed-off-by Pierre Morel <[email protected]>

Acked-by Janosch Frank <[email protected]>

---
 s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

s390x/cstart64.S +8 -0

index b50c42c..a9d8223 100644
@@ -119,6 +119,43 @@ memsetxc:
 	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	.endm
 
+/* Save registers on the stack (r15), so we can have stacked interrupts. */
+	.macro SAVE_REGS_STACK
+	/* Allocate a stack frame for 15 general registers */
+	slgfi   %r15, 15 * 8
+	/* Store registers r0 to r14 on the stack */
+	stmg    %r0, %r14, 0(%r15)
+	/* Allocate a stack frame for 16 floating point registers */
+	/* The size of a FP register is the size of an double word */

Quoted 40 lines · Pierre Morel

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

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

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

Signed-off-by Pierre Morel <[email protected]>

Acked-by Janosch Frank <[email protected]>

---
  s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 39 insertions(+), 2 deletions(-)

s390x/cstart64.S +15 -0

index b50c42c..a9d8223 100644
@@ -119,6 +119,43 @@ memsetxc:
  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
  	.endm
 
+/* Save registers on the stack (r15), so we can have stacked interrupts. */
+	.macro SAVE_REGS_STACK
+	/* Allocate a stack frame for 15 general registers */
+	slgfi   %r15, 15 * 8
+	/* Store registers r0 to r14 on the stack */
+	stmg    %r0, %r14, 0(%r15)
+	/* Allocate a stack frame for 16 floating point registers */
+	/* The size of a FP register is the size of an double word */
+	slgfi   %r15, 16 * 8
+	/* Save fp register on stack: offset to SP is multiple of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8(%r15)
+	.endr
+	/* Save fpc, but keep stack aligned on 64bits */
+	slgfi   %r15, 8

s390x/cstart64.S memsetxc: · line 139

Quoted 3 lines

s390x/cstart64.S +3 -0

+	efpc	%r0
+	stg	%r0, 0(%r15)
+	.endm

I wonder whether it would be sufficient to only save the registers here that are "volatile" according to the ELF ABI? ... that would save quite some space on the stack, I think... OTOH, the old code was also saving all registers, so maybe that's something for a separate patch later...

Acked-by Thomas Huth <[email protected]>

Quoted 42 lines · Thomas Huth

Quoted 41 lines · Pierre Morel

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

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

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

Signed-off-by Pierre Morel <[email protected]>

Acked-by Janosch Frank <[email protected]>

---
  s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--

  1 file changed, 39 insertions(+), 2 deletions(-)

s390x/cstart64.S +0 -0

index b50c42c..a9d8223 100644
@@ -119,6 +119,43 @@ memsetxc:

      lmg    %r0, %r15, GEN_LC_SW_INT_GRS
      .endm
  +/* Save registers on the stack (r15), so we can have stacked interrupts. */
+    .macro SAVE_REGS_STACK
+    /* Allocate a stack frame for 15 general registers */ +    slgfi   %r15, 15 * 8
+    /* Store registers r0 to r14 on the stack */
+    stmg    %r0, %r14, 0(%r15)
+    /* Allocate a stack frame for 16 floating point registers */ +    /* The size of a FP register is the size of an double word */ +    slgfi   %r15, 16 * 8
+    /* Save fp register on stack: offset to SP is multiple of reg number */
+    .irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+    std    \i, \i * 8(%r15)
+    .endr
+    /* Save fpc, but keep stack aligned on 64bits */
+    slgfi   %r15, 8

Quoted 8 lines

Quoted 3 lines

+    efpc    %r0
+    stg    %r0, 0(%r15)
+    .endm

I wonder whether it would be sufficient to only save the registers here that are "volatile" according to the ELF ABI? ... that would save quite some space on the stack, I think... OTOH, the old code was also saving all registers, so maybe that's something for a separate patch later...

I don't think so for the general registers
The "volatile" registers are lost during a C call, so it is the duty of the caller to save them before the call, if he wants, and this is possible for the programmer or the compiler to arrange that.

For interruptions, we steal the CPU with all the registers from the program without warning, the program has no possibility to save them. So we must save all registers for him.

For the FP registers, we surely can do something if we establish a usage convention on the floating point.
A few tests need hardware floating point.

Quoted 54 lines · Pierre Morel

Quoted 42 lines · Thomas Huth

Quoted 41 lines · Pierre Morel

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

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

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

Signed-off-by Pierre Morel <[email protected]>

Acked-by Janosch Frank <[email protected]>

---
   s390x/cstart64.S | 41 +++++++++++++++++++++++++++++++++++++++--
   1 file changed, 39 insertions(+), 2 deletions(-)

s390x/cstart64.S +0 -0

index b50c42c..a9d8223 100644
@@ -119,6 +119,43 @@ memsetxc:
       lmg    %r0, %r15, GEN_LC_SW_INT_GRS
       .endm
   +/* Save registers on the stack (r15), so we can have stacked

interrupts. */
+    .macro SAVE_REGS_STACK
+    /* Allocate a stack frame for 15 general registers */ +    slgfi   %r15, 15 * 8
+    /* Store registers r0 to r14 on the stack */
+    stmg    %r0, %r14, 0(%r15)
+    /* Allocate a stack frame for 16 floating point registers */ +    /* The size of a FP register is the size of an double word */ +    slgfi   %r15, 16 * 8
+    /* Save fp register on stack: offset to SP is multiple of reg number */
+    .irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+    std    \i, \i * 8(%r15)
+    .endr
+    /* Save fpc, but keep stack aligned on 64bits */
+    slgfi   %r15, 8

Quoted 8 lines

Quoted 3 lines

+    efpc    %r0
+    stg    %r0, 0(%r15)
+    .endm

I wonder whether it would be sufficient to only save the registers here that are "volatile" according to the ELF ABI? ... that would save quite some space on the stack, I think... OTOH, the old code was also saving all registers, so maybe that's something for a separate patch later...

Quoted 8 lines

I don't think so for the general registers
The "volatile" registers are lost during a C call, so it is the duty of the caller to save them before the call, if he wants, and this is possible for the programmer or the compiler to arrange that.

For interruptions, we steal the CPU with all the registers from the program without warning, the program has no possibility to save them. So we must save all registers for him.

We certainly have to save the registers that are marked as "volatile" in the ELF ABI, no discussion. But what about the others? If we do not touch them in the assembler code, and just jump to a C function, the C function will save them before changing them, and restore the old value before returning. So when the interrupt is done, the registers should contain their original values again, shouldn't they?

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

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

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

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

lib/s390x/asm/time.h +0 -0

index 1791380..eb15941 100644

lib/s390x/asm/time.h +4 -1

+static inline uint64_t get_clock_us(void)
 {
 	uint64_t clk;
 
 	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
 
 	/* Bit 51 is incrememented each microsecond */
-	return (clk >> (63 - 51)) / 1000;
+	return clk >> STCK_SHIFT;
+}
+

lib/s390x/asm/time.h +3 -0

+}
+
+static inline void mdelay(unsigned long ms)

Quoted 20 lines · Pierre Morel

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

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

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

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
  1 file changed, 27 insertions(+), 2 deletions(-)

lib/s390x/asm/time.h +0 -0

index 1791380..eb15941 100644

lib/s390x/asm/time.h line 16

Quoted 5 lines

lib/s390x/asm/time.h +1 -1

@@ -13,14 +13,39 @@
  #ifndef ASM_S390X_TIME_H
  #define ASM_S390X_TIME_H
 
-static inline uint64_t get_clock_ms(void)
+#define STCK_SHIFT	(63 - 51)

Could you maybe rather call this STCK_SHIFT_US instead, so that it is clear that we refer to the microsecond bit here?

lib/s390x/asm/time.h line 17

Quoted 1 line

lib/s390x/asm/time.h +1 -0

+#define STCK_MAX	((1 << (STCK_SHIFT + 1)) - 1)

Hmm, is that really right? Shouldn't that rather be

((1 << (51 + 1)) - 1)

instead?

(you want to have a max. value for the 52 bits that count the microseconds, not a max value for the remainder bits, do you?)

Quoted 15 lines

lib/s390x/asm/time.h +8 -1

+static inline uint64_t get_clock_us(void)
  {
  	uint64_t clk;
 
  	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
 
  	/* Bit 51 is incrememented each microsecond */
-	return (clk >> (63 - 51)) / 1000;
+	return clk >> STCK_SHIFT;
+}
+
+static inline void udelay(unsigned long us)
+{
+	unsigned long startclk = get_clock_us();
+	unsigned long c;

lib/s390x/asm/time.h line 38

Quoted 6 lines

lib/s390x/asm/time.h +6 -0

+
+	do {
+		c = get_clock_us();
+		if (c < startclk)
+			c += STCK_MAX;
+	} while (c < (startclk + us));

You could omit the parentheses around "startclk + us" here.

Allow the program to wait for an interrupt.

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

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 lib/s390x/asm/arch_def.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

lib/s390x/asm/arch_def.h +7 -0

index 12045ff..e0ced12 100644
@@ -10,9 +10,11 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_IO			0x0200000000000000UL
 #define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_WAIT			0x0002000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
 #define PSW_MASK_BA			0x0000000080000000UL
 #define PSW_MASK_EA			0x0000000100000000UL

@@ -253,6 +255,18 @@ static inline void load_psw_mask(uint64_t mask)
 		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
 }
 
+static inline void wait_for_interrupt(uint64_t irq_mask)
+{
+	uint64_t psw_mask = extract_psw_mask();
+
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);

Quoted 42 lines · Pierre Morel

Allow the program to wait for an interrupt.

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

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Janosch Frank <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
  lib/s390x/asm/arch_def.h | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

lib/s390x/asm/arch_def.h +12 -0

index 12045ff..e0ced12 100644
@@ -10,9 +10,11 @@
  #ifndef _ASM_S390X_ARCH_DEF_H_
  #define _ASM_S390X_ARCH_DEF_H_
 
+#define PSW_MASK_IO			0x0200000000000000UL
  #define PSW_MASK_EXT			0x0100000000000000UL
  #define PSW_MASK_DAT			0x0400000000000000UL
  #define PSW_MASK_SHORT_PSW		0x0008000000000000UL
+#define PSW_MASK_WAIT			0x0002000000000000UL
  #define PSW_MASK_PSTATE			0x0001000000000000UL
  #define PSW_MASK_BA			0x0000000080000000UL
  #define PSW_MASK_EA			0x0000000100000000UL

@@ -253,6 +255,18 @@ static inline void load_psw_mask(uint64_t mask)
  		: "+r" (tmp) :  "a" (&psw) : "memory", "cc" );
  }
 
+static inline void wait_for_interrupt(uint64_t irq_mask)
+{
+	uint64_t psw_mask = extract_psw_mask();
+
+	load_psw_mask(psw_mask | irq_mask | PSW_MASK_WAIT);
+	/*
+	 * After being woken and having processed the interrupt, let's restore
+	 * the PSW mask.
+	 */
+	load_psw_mask(psw_mask);

lib/s390x/asm/arch_def.h static inline void load_psw_mask(uint64_t mask) · line 272

Quoted 6 lines

lib/s390x/asm/arch_def.h +2 -0

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

Reviewed-by Thomas Huth <[email protected]>

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

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
 lib/s390x/kernel-args.h | 18 +++++++++++++
 s390x/Makefile          |  1 +
 3 files changed, 79 insertions(+)
 create mode 100644 lib/s390x/kernel-args.c
 create mode 100644 lib/s390x/kernel-args.h

lib/s390x/kernel-args.c new file +5 -0

+int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val)
+{
+	int i, ret;
+	char *p;
+

Quoted 11 lines · Pierre Morel

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

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/kernel-args.c | 60 +++++++++++++++++++++++++++++++++++++++++
  lib/s390x/kernel-args.h | 18 +++++++++++++
  s390x/Makefile          |  1 +
  3 files changed, 79 insertions(+)

create mode 100644 lib/s390x/kernel-args.c
create mode 100644 lib/s390x/kernel-args.h

[...]

Quoted 10 lines

lib/s390x/kernel-args.c new file +10 -0

+int kernel_arg(int argc, char *argv[], const char *str, unsigned long *val)
+{
+	int i, ret;
+	char *p;
+
+	for (i = 0; i < argc; i++) {
+		ret = strncmp(argv[i], str, strlen(str));
+		if (ret)
+			continue;
+		p = strchr(argv[i], '=');

lib/s390x/kernel-args.c line 54

Quoted 5 lines

lib/s390x/kernel-args.c new file +5 -0

+		if (!p)
+			return -1;
+		p = strchr(p, 'x');
+		if (!p)
+			*val = atol(p + 1);

If p is NULL, then you call atol(NULL + 1) ... I think you need another temporary variable here instead to hold the new pointer / NULL value?

Thomas

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 <[email protected]>

---
 lib/s390x/css.h      | 257 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 153 ++++++++++++++++++++++++++
 s390x/Makefile       |   1 +
 3 files changed, 411 insertions(+)
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c

lib/s390x/css.h new file +106 -0

new file mode 100644
index 0000000..33caaa0
@@ -0,0 +1,257 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2020
+ * Author: Pierre Morel <[email protected]>
+ *
+ * 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
+
+/* subchannel ID bit 16 must always be one */
+#define SCHID_ONE	0x00010000
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20
+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw1 {
+	uint8_t code;
+	uint8_t flags;
+	uint16_t count;
+	uint32_t data_address;
+} __attribute__ ((aligned(8)));
+
+#define ORB_CTRL_KEY	0xf0000000
+#define ORB_CTRL_SPND	0x08000000
+#define ORB_CTRL_STR	0x04000000
+#define ORB_CTRL_MOD	0x02000000
+#define ORB_CTRL_SYNC	0x01000000
+#define ORB_CTRL_FMT	0x00800000
+#define ORB_CTRL_PFCH	0x00400000
+#define ORB_CTRL_ISIC	0x00200000
+#define ORB_CTRL_ALCC	0x00100000
+#define ORB_CTRL_SSIC	0x00080000
+#define ORB_CTRL_CPTC	0x00040000
+#define ORB_CTRL_C64	0x00020000
+#define ORB_CTRL_I2K	0x00010000
+#define ORB_CTRL_LPM	0x0000ff00
+#define ORB_CTRL_ILS	0x00000080
+#define ORB_CTRL_MIDAW	0x00000040
+#define ORB_CTRL_ORBX	0x00000001
+
+#define ORB_LPM_DFLT	0x00008000
+
+struct orb {
+	uint32_t intparm;
+	uint32_t ctrl;
+	uint32_t cpa;
+	uint32_t prio;
+	uint32_t reserved[4];
+} __attribute__ ((aligned(4)));
+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t ccw_addr;
+	uint8_t  dev_stat;
+	uint8_t  sch_stat;
+	uint16_t count;
+};
+
+struct pmcw {
+	uint32_t intparm;
+#define PMCW_DNV        0x0001
+#define PMCW_ENABLE     0x0080
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	uint32_t flags2;
+};
+#define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
+
+struct schib {
+	struct pmcw pmcw;
+	struct scsw scsw;
+	uint8_t  md[12];
+} __attribute__ ((aligned(4)));
+
+struct irb {
+	struct scsw scsw;
+	uint32_t esw[5];
+	uint32_t ecw[8];
+	uint32_t emw[8];
+} __attribute__ ((aligned(4)));
+
+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)

lib/s390x/css.h new file +5 -0

+	return cc;
+}
+
+static inline int stsch(unsigned long schid, struct schib *addr)
+{

lib/s390x/css.h new file +4 -0

+	return cc;
+}
+
+static inline int msch(unsigned long schid, struct schib *addr)

lib/s390x/css.h new file +3 -0

+		: "cc");
+	return cc;
+}

lib/s390x/css.h new file +16 -0

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

lib/s390x/css.h new file +5 -0

+int css_enumerate(void);
+#define MAX_ENABLE_RETRIES      5
+int css_enable(int schid);
+
+#endif

lib/s390x/css_dump.c new file +0 -0

new file mode 100644
index 0000000..0c2b64e

Quoted 1 line · Pierre Morel

Provide some definitions and library routines that can be used by

Quoted 6 lines

lib/s390x/css.h new file +6 -0

+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int cc;
+
+	asm volatile(

lib/s390x/css.h line 116

Quoted 5 lines

lib/s390x/css.h new file +5 -0

+		"	ssch	0(%2)\n"
+		"	ipm	%0\n"
+		"	srl	%0,28\n"
+		: "=d" (cc)
+		: "d" (reg1), "a" (addr), "m" (*addr)

Hmm... What's the "m" (*addr) here good for? %3 is not used in the assembly code?

lib/s390x/css.h line 117

Quoted 2 lines

lib/s390x/css.h new file +1 -0

+		: "cc", "memory");

Why "memory" ? Can this instruction also change the orb?

Quoted 11 lines

lib/s390x/css.h new file +10 -0

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

lib/s390x/css.h line 132

Quoted 5 lines

lib/s390x/css.h new file +5 -0

+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)
+		: "cc");

I'm surprised that this does not use "memory" in the clobber list ... I guess that's what the "=m" (*addr) is good for?

Quoted 10 lines

lib/s390x/css.h new file +9 -0

+	return cc;
+}
+
+static inline int msch(unsigned long schid, struct schib *addr)
+{
+	register unsigned long reg1 asm ("1") = schid;
+	int cc;
+
+	asm volatile(

lib/s390x/css.h line 146

Quoted 5 lines

lib/s390x/css.h new file +5 -0

+		"	msch	0(%3)\n"
+		"	ipm	%0\n"
+		"	srl	%0,28"
+		: "=d" (cc), "=m" (*addr)
+		: "d" (reg1), "a" (addr)

I'm not an expert with these IO instructions, but this looks wrong to me ... Is MSCH reading or writing the SCHIB data?

Quoted 7 lines

lib/s390x/css.h new file +7 -0

+/* Debug functions */
+char *dump_pmcw_flags(uint16_t f);
+char *dump_scsw_flags(uint32_t f);
+
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);

lib/s390x/css.h line 251

Quoted 4 lines

lib/s390x/css.h new file +4 -0

+struct ccw1 *dump_ccw(struct ccw1 *cp);
+void dump_irb(struct irb *irbp);
+void dump_pmcw(struct pmcw *p);
+void dump_orb(struct orb *op);

In the patch description, you said that DEBUG_CSS needs to be defined for these - but now DEBUG_CSS is not used in this header... does the patch description need to be changed?

Quoted 17 lines

lib/s390x/css.h new file +5 -0

+int css_enumerate(void);
+#define MAX_ENABLE_RETRIES      5
+int css_enable(int schid);
+
+#endif

lib/s390x/css_dump.c new file +5 -0

new file mode 100644
index 0000000..0c2b64e
@@ -0,0 +1,153 @@
+/*
+ * Channel subsystem structures dumping
+ *
+ * Copyright (c) 2020 IBM Corp.
+ *

lib/s390x/css_dump.c line 10

Quoted 5 lines

lib/s390x/css_dump.c new file +5 -0

+ * Authors:
+ *  Pierre Morel <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.

In the header you used "or any later version" - here it's version 2 only. Maybe you want to standardize one one of the two flavors?

Quoted 2 lines · Thomas Huth

Quoted 1 line · Pierre Morel

Provide some definitions and library routines that can be used by

...snip...

Quoted 6 lines

Quoted 6 lines

lib/s390x/css.h new file +6 -0

+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+    register long long reg1 asm("1") = schid;
+    int cc;
+
+    asm volatile(

Quoted 8 lines

Quoted 5 lines

+        "    ssch    0(%2)\n"
+        "    ipm    %0\n"
+        "    srl    %0,28\n"
+        : "=d" (cc)
+        : "d" (reg1), "a" (addr), "m" (*addr)

Hmm... What's the "m" (*addr) here good for? %3 is not used in the assembly code?

addr is %2
"m" (*addr) means memory pointed by addr is read

Quoted 4 lines

Quoted 2 lines

+        : "cc", "memory");

Why "memory" ? Can this instruction also change the orb?

The orb not but this instruction modifies memory as follow:
orb -> ccw -> data

The CCW can be a READ or a WRITE instruction and the data my be anywhere in memory (<2G)

A compiler memory barrier is need to avoid write instructions started before the SSCH instruction to occur after for a write
and memory read made after the instruction to be executed before for a read.

Quoted 6 lines

Quoted 6 lines

lib/s390x/css.h new file +6 -0

+static inline int msch(unsigned long schid, struct schib *addr)
+{
+    register unsigned long reg1 asm ("1") = schid;
+    int cc;
+
+    asm volatile(

Quoted 8 lines

Quoted 5 lines

+        "    msch    0(%3)\n"
+        "    ipm    %0\n"
+        "    srl    %0,28"
+        : "=d" (cc), "=m" (*addr)
+        : "d" (reg1), "a" (addr)

I'm not an expert with these IO instructions, but this looks wrong to me ... Is MSCH reading or writing the SCHIB data?

MSCH is reading the SCHIB data in memory.

Quoted 34 lines · Pierre Morel

Quoted 2 lines · Thomas Huth

Quoted 1 line · Pierre Morel

Provide some definitions and library routines that can be used by

...snip...

Quoted 6 lines

Quoted 6 lines

lib/s390x/css.h new file +6 -0

+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+    register long long reg1 asm("1") = schid;
+    int cc;
+
+    asm volatile(

Quoted 8 lines

Quoted 5 lines

+        "    ssch    0(%2)\n"
+        "    ipm    %0\n"
+        "    srl    %0,28\n"
+        : "=d" (cc)
+        : "d" (reg1), "a" (addr), "m" (*addr)

Hmm... What's the "m" (*addr) here good for? %3 is not used in the assembly code?

addr is %2
"m" (*addr) means memory pointed by addr is read

Quoted 4 lines

Quoted 2 lines

+        : "cc", "memory");

Why "memory" ? Can this instruction also change the orb?

The orb not but this instruction modifies memory as follow:
orb -> ccw -> data

Quoted 7 lines

The CCW can be a READ or a WRITE instruction and the data my be anywhere in memory (<2G)

A compiler memory barrier is need to avoid write instructions started before the SSCH instruction to occur after for a write
and memory read made after the instruction to be executed before for a read.

Ok, makes sense now, thanks!

Quoted 15 lines

Quoted 14 lines

lib/s390x/css.h line 146

Quoted 11 lines

lib/s390x/css.h new file +11 -0

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

I'm not an expert with these IO instructions, but this looks wrong to me ... Is MSCH reading or writing the SCHIB data?

Quoted 1 line

MSCH is reading the SCHIB data in memory.

So if it is reading, you don't need the "=m" (*addr) in the output list, do you? You should rather use "m" (*addr) in the input list instead?

Thomas

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 <[email protected]>

---
 lib/s390x/css_lib.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/Makefile      |  2 ++
 s390x/css.c         | 64 +++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 4 files changed, 140 insertions(+)
 create mode 100644 lib/s390x/css_lib.c
 create mode 100644 s390x/css.c

lib/s390x/css_lib.c new file +36 -0

new file mode 100644
index 0000000..dc5a512
@@ -0,0 +1,70 @@
+/*
+ * Channel Subsystem tests library
+ *
+ * Copyright (c) 2020 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+
+#include <css.h>
+
+static struct schib schib;
+
+/*
+ * css_enumerate:
+ * On success return the first subchannel ID found.
+ * On error return an invalid subchannel ID containing cc
+ */
+int css_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int scn_found = 0;
+	int dev_found = 0;
+	int schid = 0;
+	int cc;
+	int scn;
+

lib/s390x/css_lib.c new file +24 -0

+			return cc;
+		}
+
+		/* We currently only support type 0, a.k.a. I/O channels */
+		if (PMCW_CHANNEL_TYPE(pmcw) != 0)
+			continue;
+
+		/* We ignore I/O channels without valid devices */
+		scn_found++;
+		if (!(pmcw->flags & PMCW_DNV))
+			continue;
+
+		/* We keep track of the first device as our test device */
+		if (!schid)
+			schid = scn | SCHID_ONE;
+		report_info("Found subchannel %08x", scn | SCHID_ONE);
+		dev_found++;
+	}
+
+out:
+	report_info("Tested subchannels: %d, I/O subchannels: %d, I/O devices: %d",
+		    scn, scn_found, dev_found);
+	return schid;
+}

s390x/Makefile +2 -0

index 3cb97da..afd2c9b 100644
@@ -17,6 +17,7 @@ tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
+tests += $(TEST_DIR)/css.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary

@@ -53,6 +54,7 @@ cflatobjs += lib/s390x/mmu.o
 cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/kernel-args.o
 cflatobjs += lib/s390x/css_dump.o
+cflatobjs += lib/s390x/css_lib.o
 
 OBJDIRS += lib/s390x
 

s390x/css.c new file +30 -0

new file mode 100644
index 0000000..f0e8f47
@@ -0,0 +1,64 @@
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2020 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+
+#include <css.h>
+
+static int test_device_sid;
+
+static void test_enumerate(void)
+{
+	test_device_sid = css_enumerate();
+	if (test_device_sid & SCHID_ONE) {
+		report(1, "First device schid: 0x%08x", test_device_sid);
+		return;
+	}

On 08/06/2020 10.12, Pierre Morel wrote:

Quoted 14 lines

lib/s390x/css_lib.c new file +14 -0

+int css_enumerate(void)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int scn_found = 0;
+	int dev_found = 0;
+	int schid = 0;
+	int cc;
+	int scn;
+
+	for (scn = 0; scn < 0xffff; scn++) {
+		cc = stsch(scn | SCHID_ONE, &schib);
+		switch (cc) {
+		case 0:		/* 0 means SCHIB stored */
+			break;

lib/s390x/css_lib.c line 46

Quoted 5 lines

lib/s390x/css_lib.c new file +5 -0

+		case 3:		/* 3 means no more channels */
+			goto out;
+		default:	/* 1 or 2 should never happened for STSCH */
+			report_info("Unexpected error %d on subchannel %08x",
+				    cc, scn | SCHID_ONE);

Should this maybe even be a report_abort() instead? Or leave the error reporting to the caller...

Quoted 3 lines

(quoted diff) +2 -0

+			return cc;
+		}

Quoted 7 lines

s390x/css.c new file +7 -0

+static void test_enumerate(void)
+{
+	test_device_sid = css_enumerate();
+	if (test_device_sid & SCHID_ONE) {
+		report(1, "First device schid: 0x%08x", test_device_sid);
+		return;
+	}

s390x/css.c line 40

Quoted 10 lines

s390x/css.c new file +10 -0

+
+	switch (test_device_sid) {
+	case 0:
+		report (0, "No I/O device found");
+		break;
+	default:	/* 1 or 2 should never happened for STSCH */
+		report(0, "Unexpected cc=%d during enumeration",
+		       test_device_sid);
+			return;
+	}

Ok, so here is now the test failure for the cc=1 or 2 that should never happen. That means currently you print out the CC for this error twice. One time should be enough, either here, or use an report_abort() in the css_enumerate(), I'd say.

Anyway, can you please replace this switch statement with a "if (!test_device_sid)" instead? Or do you plan to add more "case" statements later?

Quoted 25 lines · Thomas Huth

Quoted 7 lines · Pierre Morel

s390x/css.c new file +7 -0

+static void test_enumerate(void)
+{
+	test_device_sid = css_enumerate();
+	if (test_device_sid & SCHID_ONE) {
+		report(1, "First device schid: 0x%08x", test_device_sid);
+		return;
+	}

s390x/css.c line 40

Quoted 10 lines

s390x/css.c new file +10 -0

+
+	switch (test_device_sid) {
+	case 0:
+		report (0, "No I/O device found");
+		break;
+	default:	/* 1 or 2 should never happened for STSCH */
+		report(0, "Unexpected cc=%d during enumeration",
+		       test_device_sid);
+			return;
+	}  

Ok, so here is now the test failure for the cc=1 or 2 that should never happen. That means currently you print out the CC for this error twice. One time should be enough, either here, or use an report_abort() in the css_enumerate(), I'd say.

Quoted 3 lines

Anyway, can you please replace this switch statement with a "if (!test_device_sid)" instead? Or do you plan to add more "case" statements later?

I will use the repor_abort() in the css_enumerate() so there is only two case, I find a channel or not, so I don't even need the second if :) .

A second step when testing the channel subsystem is to prepare a channel for use.
This includes:
- Get the current subchannel Information Block (SCHIB) using STSCH
- Update it in memory to set the ENABLE bit
- Tell the CSS that the SCHIB has been modified using MSCH
- Get the SCHIB from the CSS again to verify that the subchannel is enabled.
- If the command succeeds but subchannel is not enabled retry a predefined retries count.
- If the command fails, report the failure and do not retry, even if cc indicates a busy/status pending as we do not expect this.

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

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/css_lib.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/css.c         | 18 ++++++++++++++
 2 files changed, 78 insertions(+)

lib/s390x/css_lib.c +60 -0

index dc5a512..831a116 100644
@@ -15,6 +15,7 @@
 #include <string.h>
 #include <interrupt.h>
 #include <asm/arch_def.h>
+#include <asm/time.h>
 
 #include <css.h>
 

@@ -68,3 +69,62 @@ out:
 		    scn, scn_found, dev_found);
 	return schid;
 }
+
+int css_enable(int schid)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int retry_count = 0;
+	int cc;
+
+	/* Read the SCHIB for this subchannel */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch failed with cc=%d", cc);
+		return cc;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report_info("stsch: sch %08x already enabled", schid);
+		return 0;
+	}
+
+retry:
+	/* Update the SCHIB to enable the channel */
+	pmcw->flags |= PMCW_ENABLE;
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(schid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report_info("msch failed with cc=%d", cc);
+		return cc;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch failed with cc=%d", cc);
+		return cc;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report_info("Subchannel %08x enabled after %d retries",
+			    schid, retry_count);
+		return 0;
+	}
+
+	if (retry_count++ < MAX_ENABLE_RETRIES) {
+		mdelay(10); /* the hardware was not ready, give it some time */
+		goto retry;
+	}
+
+	report_info("msch: enabling sch %08x failed after %d retries. pmcw flags: %x",
+		    schid, retry_count, pmcw->flags);
+	return -1;
+}

s390x/css.c +5 -0

index f0e8f47..6f58d4a 100644
@@ -40,11 +40,29 @@ static void test_enumerate(void)
 	}
 }
 
+static void test_enable(void)
+{
+	int cc;
+
+	if (!test_device_sid) {

Quoted 111 lines · Pierre Morel

A second step when testing the channel subsystem is to prepare a channel for use.
This includes:
- Get the current subchannel Information Block (SCHIB) using STSCH
- Update it in memory to set the ENABLE bit
- Tell the CSS that the SCHIB has been modified using MSCH
- Get the SCHIB from the CSS again to verify that the subchannel is enabled.
- If the command succeeds but subchannel is not enabled retry a predefined retries count.
- If the command fails, report the failure and do not retry, even if cc indicates a busy/status pending as we do not expect this.

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

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/css_lib.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
  s390x/css.c         | 18 ++++++++++++++
  2 files changed, 78 insertions(+)

lib/s390x/css_lib.c +60 -0

index dc5a512..831a116 100644
@@ -15,6 +15,7 @@
  #include <string.h>
  #include <interrupt.h>
  #include <asm/arch_def.h>
+#include <asm/time.h>
 
  #include <css.h>
 

@@ -68,3 +69,62 @@ out:
  		    scn, scn_found, dev_found);
  	return schid;
  }
+
+int css_enable(int schid)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int retry_count = 0;
+	int cc;
+
+	/* Read the SCHIB for this subchannel */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch failed with cc=%d", cc);
+		return cc;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report_info("stsch: sch %08x already enabled", schid);
+		return 0;
+	}
+
+retry:
+	/* Update the SCHIB to enable the channel */
+	pmcw->flags |= PMCW_ENABLE;
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(schid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report_info("msch failed with cc=%d", cc);
+		return cc;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch failed with cc=%d", cc);
+		return cc;
+	}
+
+	if (pmcw->flags & PMCW_ENABLE) {
+		report_info("Subchannel %08x enabled after %d retries",
+			    schid, retry_count);
+		return 0;
+	}
+
+	if (retry_count++ < MAX_ENABLE_RETRIES) {
+		mdelay(10); /* the hardware was not ready, give it some time */
+		goto retry;
+	}
+
+	report_info("msch: enabling sch %08x failed after %d retries. pmcw flags: %x",
+		    schid, retry_count, pmcw->flags);
+	return -1;
+}

s390x/css.c +5 -0

index f0e8f47..6f58d4a 100644
@@ -40,11 +40,29 @@ static void test_enumerate(void)
  	}
  }
 
+static void test_enable(void)
+{
+	int cc;
+
+	if (!test_device_sid) {

s390x/css.c static void test_enumerate(void) · line 57

Quoted 10 lines

s390x/css.c +10 -0

+		report_skip("No device");
+		return;
+	}
+
+	cc = css_enable(test_device_sid);
+
+	if (cc)
+		report(0, "Failed to enable subchannel %08x", test_device_sid);
+	else
+		report(1, "Subchannel %08x enabled", test_device_sid);

Could you please write this as:

	report(cc == 1, "Enable subchannel %08x", test_device_sid);

... checking for a right value is the whole point of the first parameter of report() :-)

Thanks,
Thomas

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

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

The SENSE_ID command response is tested to report 0xff inside its reserved field and to report the same control unit type as the cu_type kernel argument.

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

Signed-off-by Pierre Morel <[email protected]>

---
 lib/s390x/css.h     |  20 ++++++
 lib/s390x/css_lib.c |  46 ++++++++++++++
 s390x/css.c         | 149 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 214 insertions(+), 1 deletion(-)

(quoted diff) +1 -0

+}

s390x/css.c +6 -0

index 6f58d4a..79c997d 100644
@@ -16,10 +16,26 @@
 #include <string.h>
 #include <interrupt.h>
 #include <asm/arch_def.h>
+#include <kernel-args.h>
 
 #include <css.h>
 
+#define DEFAULT_CU_TYPE		0x3832
+static unsigned long cu_type = DEFAULT_CU_TYPE;
+
+struct lowcore *lowcore = (void *)0x0;
+

Quoted 19 lines · Pierre Morel

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

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

The SENSE_ID command response is tested to report 0xff inside its reserved field and to report the same control unit type as the cu_type kernel argument.

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

Signed-off-by Pierre Morel <[email protected]>

---
  lib/s390x/css.h     |  20 ++++++
  lib/s390x/css_lib.c |  46 ++++++++++++++
  s390x/css.c         | 149 +++++++++++++++++++++++++++++++++++++++++++-
  3 files changed, 214 insertions(+), 1 deletion(-)

[...]

Quoted 24 lines

(quoted diff) +1 -0

+}

s390x/css.c +11 -0

index 6f58d4a..79c997d 100644
@@ -16,10 +16,26 @@
  #include <string.h>
  #include <interrupt.h>
  #include <asm/arch_def.h>
+#include <kernel-args.h>
 
  #include <css.h>
 
+#define DEFAULT_CU_TYPE		0x3832
+static unsigned long cu_type = DEFAULT_CU_TYPE;
+
+struct lowcore *lowcore = (void *)0x0;
+
  static int test_device_sid;
+static struct irb irb;
+static struct senseid senseid;
+
+static void set_io_irq_subclass_mask(uint64_t const new_mask)
+{

s390x/css.c line 37

Quoted 4 lines

s390x/css.c +4 -0

+	asm volatile (
+		"lctlg %%c6, %%c6, %[source]\n"
+		: /* No outputs */
+		: [source] "R" (new_mask));

I think the "R" constraint is wrong here - this instruction does not use an index register. "Q" is likely the better choice. But it might be easier to use the lctlg() wrapper from lib/s390x/asm/arch_def.h instead.

[...]

s390x/css.c

Quoted 2 lines

s390x/css.c +2 -0

+
+	report((senseid.cu_type == cu_type),

Please drop the innermost parentheses here.