s390x: Testing the Channel Subsystem I/O

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

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

---
 lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
 lib/s390x/asm/arch_def.h  |  6 ++----

Quoted 7 lines · Pierre Morel

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

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

---
  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
  lib/s390x/asm/arch_def.h  |  6 ++----

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

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

Am 28.11.2019 um 20:16 schrieb Pierre Morel <[email protected]>:



Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Pierre Morel

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

Quoted 6 lines

Quoted 4 lines

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

---
 lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
 lib/s390x/asm/arch_def.h  |  6 ++----

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

no because arch_def.h contains C structures and inline.

Quoted 10 lines

Am 28.11.2019 um 20:16 schrieb Pierre Morel <[email protected]>:



Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Pierre Morel

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

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

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

---
  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
  lib/s390x/asm/arch_def.h  |  6 ++----

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

no because arch_def.h contains C structures and inline.

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

You‘re looking for

#ifndef __ASSEMBLER__
...

See lib/s390x/asm/sigp.h

Quoted 7 lines · Pierre Morel

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

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

---
 lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
 lib/s390x/asm/arch_def.h  |  6 ++----

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

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

Quoted 9 lines · David Hildenbrand

Quoted 7 lines · Pierre Morel

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

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

---
 lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
 lib/s390x/asm/arch_def.h  |  6 ++----

Quoted 7 lines

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

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

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

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

Quoted 18 lines · Janosch Frank

Quoted 16 lines · David Hildenbrand

Quoted 7 lines · Pierre Morel

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

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

---
  lib/s390x/asm/arch_bits.h | 20 ++++++++++++++++++++
  lib/s390x/asm/arch_def.h  |  6 ++----

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

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

Quoted 5 lines

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

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

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

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

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

Reviewed-by Thomas Huth <[email protected]>

---
 lib/s390x/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/s390x/interrupt.c +0 -0

index 3e07867..d70fde3 100644

Quoted 19 lines · Pierre Morel

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

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

Reviewed-by Thomas Huth <[email protected]>

---
 lib/s390x/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/s390x/interrupt.c +0 -0

index 3e07867..d70fde3 100644
@@ -140,7 +140,7 @@ void handle_mcck_int(void)
 		     lc->mcck_old_psw.addr);
 }
 

lib/s390x/interrupt.c void handle_mcck_int(void) · line 146

Quoted 6 lines

lib/s390x/interrupt.c +1 -1

-void handle_io_int(void)
+__attribute__((weak)) void handle_io_int(void)
 {
 	report_abort("Unexpected io interrupt: at %#lx",
 		     lc->io_old_psw.addr);

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

Quoted 20 lines · David Hildenbrand

Quoted 19 lines · Pierre Morel

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

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

Reviewed-by Thomas Huth <[email protected]>

---
  lib/s390x/interrupt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

lib/s390x/interrupt.c +0 -0

index 3e07867..d70fde3 100644
@@ -140,7 +140,7 @@ void handle_mcck_int(void)
  		     lc->mcck_old_psw.addr);
  }
 

Quoted 10 lines

lib/s390x/interrupt.c void handle_mcck_int(void) · line 146

Quoted 6 lines

lib/s390x/interrupt.c +1 -1

-void handle_io_int(void)
+__attribute__((weak)) void handle_io_int(void)
  {
  	report_abort("Unexpected io interrupt: at %#lx",
  		     lc->io_old_psw.addr);

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

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

Thomas

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

Quoted 2 lines · Pierre Morel

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

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

Reviewed-by David Hildenbrand <[email protected]>

Quoted 2 lines · Pierre Morel

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

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

Reviewed-by David Hildenbrand <[email protected]>

Quoted 5 lines · David Hildenbrand

Quoted 2 lines · Pierre Morel

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

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

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

Quoted 5 lines

Reviewed-by David Hildenbrand <[email protected]>

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

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

---
 lib/s390x/css.h      | 269 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/css_dump.c | 147 +++++++++++++++++++++++
 2 files changed, 416 insertions(+)
 create mode 100644 lib/s390x/css.h
 create mode 100644 lib/s390x/css_dump.c

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

new file mode 100644
index 0000000..95dec72
@@ -0,0 +1,269 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * 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
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20

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

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

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

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

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

+} __attribute__ ((aligned(4)));
+
+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int cc = -1;
+

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

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

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

+static inline int rchp(unsigned long chpid)
+{
+	register unsigned long reg1 asm("1") = chpid;
+	int cc;

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

+
+/* Debug functions */
+char *dump_pmcw_flags(uint16_t f);
+char *dump_scsw_flags(uint32_t f);
+#undef DEBUG
+#ifdef DEBUG
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);
+struct ccw *dump_ccw(struct ccw *cp);
+#else
+static inline void dump_scsw(struct scsw *scsw){}
+static inline void dump_irb(struct irb *irbp){}
+static inline void dump_pmcw(struct pmcw *p){}
+static inline void dump_schib(struct schib *sch){}
+static inline void dump_orb(struct orb *op){}
+static inline struct ccw *dump_ccw(struct ccw *cp)
+{
+	return NULL;
+}
+#endif
+
+extern unsigned long stacktop;
+#endif

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

new file mode 100644
index 0000000..5356df2

Quoted 34 lines · On Thu, 28 Nov 2019 13:46:03 +0100 Pierre Morel <[email protected]> wrote:

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

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

---
  lib/s390x/css.h      | 269 +++++++++++++++++++++++++++++++++++++++++++
  lib/s390x/css_dump.c | 147 +++++++++++++++++++++++
  2 files changed, 416 insertions(+)

create mode 100644 lib/s390x/css.h
create mode 100644 lib/s390x/css_dump.c

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

new file mode 100644
index 0000000..95dec72
@@ -0,0 +1,269 @@
+/*
+ * CSS definitions
+ *
+ * Copyright IBM, Corp. 2019
+ * 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
+
+#define CCW_F_CD	0x80
+#define CCW_F_CC	0x40
+#define CCW_F_SLI	0x20

lib/s390x/css.h line 27

Quoted 10 lines

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

+#define CCW_F_SKP	0x10
+#define CCW_F_PCI	0x08
+#define CCW_F_IDA	0x04
+#define CCW_F_S		0x02
+#define CCW_F_MIDA	0x01
+
+#define CCW_C_NOP	0x03
+#define CCW_C_TIC	0x08
+
+struct ccw {

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

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

lib/s390x/css.h line 31

Quoted 5 lines

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

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

data_address?

Quoted 27 lines

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

+} __attribute__ ((aligned(4)));
+
+#define ORB_M_KEY	0xf0000000
+#define ORB_F_SUSPEND	0x08000000
+#define ORB_F_STREAMING	0x04000000
+#define ORB_F_MODIFCTRL	0x02000000
+#define ORB_F_SYNC	0x01000000
+#define ORB_F_FORMAT	0x00800000
+#define ORB_F_PREFETCH	0x00400000
+#define ORB_F_INIT_IRQ	0x00200000
+#define ORB_F_ADDRLIMIT	0x00100000
+#define ORB_F_SUSP_IRQ	0x00080000
+#define ORB_F_TRANSPORT	0x00040000
+#define ORB_F_IDAW2	0x00020000
+#define ORB_F_IDAW_2K	0x00010000
+#define ORB_M_LPM	0x0000ff00
+#define ORB_F_LPM_DFLT	0x00008000
+#define ORB_F_ILSM	0x00000080
+#define ORB_F_CCW_IND	0x00000040
+#define ORB_F_ORB_EXT	0x00000001
+
+struct orb {
+	unsigned int	intparm;
+	unsigned int	ctrl;
+	unsigned int	cpa;
+	unsigned int	prio;

lib/s390x/css.h line 63

Quoted 6 lines

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

+	unsigned int	reserved[4];
+} __attribute__ ((aligned(4)));
+
+struct scsw {
+	uint32_t ctrl;
+	uint32_t addr;

ccw_addr?

lib/s390x/css.h line 65

Quoted 3 lines

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

+	uint8_t  devs;
+	uint8_t  schs;

Maybe dev_stat/sch_stat?

lib/s390x/css.h line 67

Quoted 3 lines

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

+	uint16_t count;
+};

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

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

Quoted 19 lines

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

+
+struct pmcw {
+	uint32_t intparm;
+#define PMCW_DNV        0x0001
+#define PMCW_ENABLE     0x0080
+	uint16_t flags;
+	uint16_t devnum;
+	uint8_t  lpm;
+	uint8_t  pnom;
+	uint8_t  lpum;
+	uint8_t  pim;
+	uint16_t mbi;
+	uint8_t  pom;
+	uint8_t  pam;
+	uint8_t  chpid[8];
+	struct {
+		uint8_t res0;
+		uint8_t st:3;

lib/s390x/css.h line 91

Quoted 6 lines

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

+		uint8_t :5;
+		uint16_t :13;
+		uint16_t f:1;
+		uint16_t x:1;
+		uint16_t s:1;
+	};

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

lib/s390x/css.h line 99

Quoted 9 lines

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

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

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

lib/s390x/css.h line 106

Quoted 8 lines

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

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

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

Quoted 17 lines

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

+} __attribute__ ((aligned(4)));
+
+/* CSS low level access functions */
+
+static inline int ssch(unsigned long schid, struct orb *addr)
+{
+	register long long reg1 asm("1") = schid;
+	int cc = -1;
+
+	asm volatile(
+		"	   ssch	0(%2)\n"
+		"0:	 ipm	 %0\n"
+		"	   srl	 %0,28\n"
+		"1:\n"
+		: "+d" (cc)
+		: "d" (reg1), "a" (addr), "m" (*addr)

lib/s390x/css.h line 125

Quoted 3 lines

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

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

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

Quoted 31 lines

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

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

(...)

Quoted 9 lines

lib/s390x/css.h new file +9 -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"

lib/s390x/css.h line 185

Quoted 5 lines

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

+		: "=d" (cc)
+		: "d" (reg1)
+		: "cc");
+	return cc;
+}

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

Quoted 27 lines

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

+
+/* Debug functions */
+char *dump_pmcw_flags(uint16_t f);
+char *dump_scsw_flags(uint32_t f);
+#undef DEBUG
+#ifdef DEBUG
+void dump_scsw(struct scsw *);
+void dump_irb(struct irb *irbp);
+void dump_schib(struct schib *sch);
+struct ccw *dump_ccw(struct ccw *cp);
+#else
+static inline void dump_scsw(struct scsw *scsw){}
+static inline void dump_irb(struct irb *irbp){}
+static inline void dump_pmcw(struct pmcw *p){}
+static inline void dump_schib(struct schib *sch){}
+static inline void dump_orb(struct orb *op){}
+static inline struct ccw *dump_ccw(struct ccw *cp)
+{
+	return NULL;
+}
+#endif
+
+extern unsigned long stacktop;
+#endif

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

new file mode 100644

lib/s390x/css_dump.c line 2

Quoted 6 lines

index 0000000..5356df2

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

@@ -0,0 +1,147 @@
+/*
+ * Channel Sub-System structures dumping

I think "subsystem" is the more usual spelling.

lib/s390x/css_dump.c line 14

Quoted 13 lines

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

+ *
+ * Copyright (c) 2019 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 Library General Public License version 2.
+ *
+ * Description:
+ * Provides the dumping functions for various structures used by subchannels:
+ * - ORB  : Operation request block, describe the I/O operation and point to

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

lib/s390x/css_dump.c line 16

Quoted 3 lines

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

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

s/describe/describes/

but maybe better:

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

?

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

lib/s390x/css_dump.c line 17

Quoted 2 lines

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

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

s/describe/describes/

lib/s390x/css_dump.c line 18

Quoted 2 lines

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

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

s/hold/holds/

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

lib/s390x/css_dump.c line 19

Quoted 2 lines

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

+ * - SCHIB: SubChannel Information Block composed of:

s/SubChannel/Subchannel/

lib/s390x/css_dump.c line 20

Quoted 1 line

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

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

s/SubChannel/Subchannel/

lib/s390x/css_dump.c line 21

Quoted 2 lines

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

+ *           operation when in IRB.

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

Quoted 4 lines

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

+ *   - PMCW: Path Management Control Word
+ * You need the QEMU ccw-pong device in QEMU to answer the I/O transfers.
+ */

lib/s390x/css_dump.c line 34

Quoted 10 lines

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

+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <css.h>
+
+static const char *scsw_str = "kkkkslccfpixuzen";
+static const char *scsw_str2 = "1SHCrshcsdsAIPSs";

Nice, random strings? :)

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

retrieve the css devices.

This test the success of STSCH I/O instruction.

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

---
 s390x/Makefile      |  4 ++-
 s390x/css.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 s390x/css.c

s390x/css.c new file +10 -0

+ *
+ * Copyright (c) 2019 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.
+ */
+

s390x/css.c new file +5 -0

+}
+
+static struct {
+	const char *name;
+	void (*func)(void);

s390x/css.c new file +9 -0

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

Quoted 1 line · On Thu, 28 Nov 2019 13:46:04 +0100 Pierre Morel <[email protected]> wrote:

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

s/by/for/

Quoted 4 lines

retrieve the css devices.

This test the success of STSCH I/O instruction.

s/test/tests/

Quoted 10 lines

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

---
  s390x/Makefile      |  4 ++-
  s390x/css.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++
  s390x/unittests.cfg |  4 +++
  3 files changed, 93 insertions(+), 1 deletion(-)

create mode 100644 s390x/css.c

(...)

s390x/css.c line 2

Quoted 8 lines

s390x/css.c new file +2 -0

new file mode 100644
index 0000000..8186f55
@@ -0,0 +1,86 @@
+/*
+ * Channel Sub-System tests

s/Sub-System/Subsystem/

Quoted 15 lines

s390x/css.c new file +14 -0

+ *
+ * Copyright (c) 2019 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 <css.h>
+

s390x/css.c line 22

Quoted 6 lines

s390x/css.c new file +6 -0

+#define SID_ONE		0x00010000
+
+static struct schib schib;
+
+static const char *Channel_type[3] = {
+	"I/O", "CHSC", "MSG"

No EADM? :)

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

Quoted 4 lines

s390x/css.c new file +3 -0

+};
+
+static int test_device_sid;

s390x/css.c line 35

Quoted 10 lines

s390x/css.c new file +10 -0

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

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

s390x/css.c line 37

Quoted 2 lines

s390x/css.c new file +2 -0

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

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

s390x/css.c line 48

Quoted 11 lines

s390x/css.c new file +11 -0

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

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

s390x/css.c line 50

Quoted 3 lines

s390x/css.c new file +2 -0

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

You set test_device_sid to the last valid subchannel? Why?

s390x/css.c line 56

Quoted 7 lines

s390x/css.c new file +6 -0

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

Why do you do a stsch() again?

s390x/css.c line 62

Quoted 7 lines

s390x/css.c new file +6 -0

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

I don't think you need this return statement.

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

Quoted 10 lines

s390x/css.c new file +9 -0

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

s390x/css.c line 77

Quoted 6 lines

s390x/css.c new file +6 -0

+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("Channel Sub-System");

s/Sub-System/Subsystem/

Quoted 24 lines · Cornelia Huck

s390x/css.c line 35

Quoted 11 lines · On Thu, 28 Nov 2019 13:46:04 +0100 Pierre Morel <[email protected]> wrote:

s390x/css.c new file +11 -0

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

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

s390x/css.c line 37

Quoted 2 lines

s390x/css.c new file +2 -0

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

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

Quoted 15 lines

s390x/css.c line 48

Quoted 11 lines

s390x/css.c new file +11 -0

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

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

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

Quoted 6 lines

s390x/css.c line 50

Quoted 4 lines

s390x/css.c new file +2 -0

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

You set test_device_sid to the last valid subchannel? Why?

The last ? I wanted the first one

I wanted something easy but I should have explain.

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

Quoted 5 lines

s390x/css.c line 53

Quoted 5 lines

s390x/css.c new file +3 -0

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

Quoted 5 lines

s390x/css.c line 56

Quoted 3 lines

s390x/css.c new file +3 -0

+		return;
+	}
+	ret = stsch(test_device_sid, &schib);  

Why do you do a stsch() again?

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

Quoted 10 lines

s390x/css.c line 62

Quoted 8 lines

s390x/css.c new file +6 -0

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

I don't think you need this return statement.

right I have enough work. :)

Quoted 4 lines

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

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

Quoted 41 lines · On Mon, 2 Dec 2019 18:53:16 +0100 Pierre Morel <[email protected]> wrote:

Quoted 39 lines · Cornelia Huck

s390x/css.c line 35

Quoted 11 lines · On Thu, 28 Nov 2019 13:46:04 +0100 Pierre Morel <[email protected]> wrote:

s390x/css.c new file +11 -0

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

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

s390x/css.c line 37

Quoted 2 lines

s390x/css.c new file +2 -0

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

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

s390x/css.c line 48

Quoted 11 lines

s390x/css.c new file +11 -0

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

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

Quoted 4 lines

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

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

Quoted 9 lines

Quoted 7 lines

s390x/css.c line 50

Quoted 5 lines

s390x/css.c new file +2 -0

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

You set test_device_sid to the last valid subchannel? Why?

The last ? I wanted the first one

It is indeed the first one, -ENOCOFFEE.

Quoted 7 lines

I wanted something easy but I should have explain.

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

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

Quoted 6 lines

Quoted 6 lines

s390x/css.c line 53

Quoted 6 lines

s390x/css.c new file +3 -0

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

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

Quoted 11 lines

Quoted 6 lines

s390x/css.c line 56

Quoted 4 lines

s390x/css.c new file +3 -0

+		return;
+	}
+	ret = stsch(test_device_sid, &schib);

Why do you do a stsch() again?

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

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

Quoted 14 lines

Quoted 10 lines

s390x/css.c line 62

Quoted 8 lines

s390x/css.c new file +6 -0

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

I don't think you need this return statement.

right I have enough work. :)

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

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

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

Sure, just asked out of interest :)

Quoted 49 lines · Cornelia Huck

Quoted 45 lines · On Mon, 2 Dec 2019 18:53:16 +0100 Pierre Morel <[email protected]> wrote:

Quoted 24 lines · Cornelia Huck

s390x/css.c line 35

Quoted 11 lines · On Thu, 28 Nov 2019 13:46:04 +0100 Pierre Morel <[email protected]> wrote:

s390x/css.c new file +11 -0

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

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

s390x/css.c line 37

Quoted 2 lines

s390x/css.c new file +2 -0

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

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

Quoted 15 lines

s390x/css.c line 48

Quoted 11 lines

s390x/css.c new file +11 -0

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

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

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

Quoted 3 lines

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

So the the subchannel are always one after the other?

Quoted 54 lines · On Mon, 2 Dec 2019 19:33:59 +0100 Pierre Morel <[email protected]> wrote:

Quoted 52 lines · Cornelia Huck

Quoted 41 lines · On Mon, 2 Dec 2019 18:53:16 +0100 Pierre Morel <[email protected]> wrote:

Quoted 24 lines · Cornelia Huck

s390x/css.c line 35

Quoted 11 lines · On Thu, 28 Nov 2019 13:46:04 +0100 Pierre Morel <[email protected]> wrote:

s390x/css.c new file +11 -0

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

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

s390x/css.c line 37

Quoted 2 lines

s390x/css.c new file +2 -0

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

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

Quoted 15 lines

s390x/css.c line 48

Quoted 11 lines

s390x/css.c new file +11 -0

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

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

Quoted 4 lines

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

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

Quoted 1 line

So the the subchannel are always one after the other?

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

A second step when testing the channel subsystem is to prepare a channel for use.

This tests the success of the MSCH instruction by enabling a channel.

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

---
 s390x/css.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

s390x/css.c +12 -0

index 8186f55..e42dc2f 100644
@@ -62,11 +62,38 @@ static void test_enumerate(void)
 	return;
 }
 
+static void set_schib(void)
+{
+	struct pmcw *p = &schib.pmcw;
+
+	p->intparm = 0xdeadbeef;
+	p->flags |= PMCW_ENABLE;
+}
+
+static void test_enable(void)
+{
+	int ret;
+

s390x/css.c +5 -0

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

Quoted 30 lines · On Thu, 28 Nov 2019 13:46:05 +0100 Pierre Morel <[email protected]> wrote:

A second step when testing the channel subsystem is to prepare a channel for use.

This tests the success of the MSCH instruction by enabling a channel.

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

---
  s390x/css.c | 27 +++++++++++++++++++++++++++
  1 file changed, 27 insertions(+)

s390x/css.c +12 -0

index 8186f55..e42dc2f 100644
@@ -62,11 +62,38 @@ static void test_enumerate(void)
  	return;
  }
 
+static void set_schib(void)
+{
+	struct pmcw *p = &schib.pmcw;
+
+	p->intparm = 0xdeadbeef;
+	p->flags |= PMCW_ENABLE;
+}
+
+static void test_enable(void)
+{
+	int ret;
+

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

Quoted 10 lines

s390x/css.c +10 -0

+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+	set_schib();
+	dump_schib(&schib);
+
+	ret = msch(test_device_sid, &schib);
+	if (ret)
+		report("msch cc=%d", 0, ret);

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

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

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

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

The test expect a device with a control unit type of 0xC0CA.

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

---
 lib/s390x/css.h |  13 +++++
 s390x/css.c     | 137 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

s390x/css.c +6 -0

index e42dc2f..534864f 100644
@@ -11,12 +11,28 @@
  */
 
 #include <libcflat.h>
+#include <alloc_phys.h>
+#include <asm/page.h>
+#include <string.h>
+#include <asm/interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/clock.h>

s390x/css.c +6 -0

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

@@ -24,6 +40,34 @@ static const char *Channel_type[3] = {
 
 static int test_device_sid;
 

s390x/css.c +15 -0

+	report("io_int_param: %x", 1, lowcore->io_int_param);
+	report_prefix_pop();
+
+	ret = tsch(lowcore->subsys_id_word, &irb);
+	dump_irb(&irb);
+	flags = dump_scsw_flags(irb.scsw.ctrl);
+
+	if (ret)
+		report("IRB scsw flags: %s", 0, flags);
+	else
+		report("IRB scsw flags: %s", 1, flags);
+	report_prefix_pop();
+}
+
+static int start_subchannel(int code, char *data, int count)

s390x/css.c +26 -0

+	orb_p->intparm = 0xcafec0ca;
+	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
+
+	report_prefix_push("Start Subchannel");
+	ret = ssch(test_device_sid, orb_p);
+	if (ret) {
+		report("ssch cc=%d", 0, ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+static void test_sense(void)
+{
+	int success;
+
+	enable_io_irq();
+
+	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
+	if (!success) {
+		report("start_subchannel failed", 0);
+		return;
+	}

s390x/css.c +2 -0

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

Quoted 3 lines · On Thu, 28 Nov 2019 13:46:06 +0100 Pierre Morel <[email protected]> wrote:

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

Quoted 4 lines

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

The test expect a device with a control unit type of 0xC0CA.

s/expect/expects/

Quoted 8 lines

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

---
  lib/s390x/css.h |  13 +++++
  s390x/css.c     | 137 ++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 150 insertions(+)

(...)

Quoted 22 lines

s390x/css.c +9 -0

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

s390x/css.c line 29

Quoted 2 lines

s390x/css.c +1 -0

  static struct schib schib;
+#define NB_CCW  100

s/NB_CCW/NUM_CCWS/ ?

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

Quoted 14 lines

s390x/css.c +6 -0

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

@@ -24,6 +40,34 @@ static const char *Channel_type[3] = {
 
  static int test_device_sid;
 

(...)

s390x/css.c static void test_enable(void) · line 152

Quoted 11 lines

s390x/css.c +11 -0

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

Should you accommodate unsolicited interrupts as well?

Quoted 20 lines

s390x/css.c +19 -0

+	report("io_int_param: %x", 1, lowcore->io_int_param);
+	report_prefix_pop();
+
+	ret = tsch(lowcore->subsys_id_word, &irb);
+	dump_irb(&irb);
+	flags = dump_scsw_flags(irb.scsw.ctrl);
+
+	if (ret)
+		report("IRB scsw flags: %s", 0, flags);
+	else
+		report("IRB scsw flags: %s", 1, flags);
+	report_prefix_pop();
+}
+
+static int start_subchannel(int code, char *data, int count)
+{
+	int ret;
+	struct pmcw *p = &schib.pmcw;
+	struct orb *orb_p = &orb[0];

s390x/css.c static void test_enable(void) · line 177

Quoted 6 lines

s390x/css.c +6 -0

+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return 0;
+	}
+	ret = stsch(test_device_sid, &schib);

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

s390x/css.c static void test_enable(void) · line 186

Quoted 10 lines

s390x/css.c +9 -0

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

Extra ' ' before ';'

s390x/css.c static void test_enable(void) · line 187

Quoted 2 lines

s390x/css.c +1 -0

+	ccw[0].flags = CCW_F_PCI;

Huh, what's that PCI for?

s390x/css.c static void test_enable(void) · line 189

Quoted 3 lines

s390x/css.c +2 -0

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

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

Quoted 33 lines

s390x/css.c +32 -0

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

s390x/css.c static void test_enable(void) · line 225

Quoted 4 lines

s390x/css.c +4 -0

+		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
+	else
+		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
+}

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

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

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

Quoted 6 lines · Cornelia Huck

s390x/css.c static void test_enable(void) · line 186

Quoted 1 line · On Thu, 28 Nov 2019 13:46:06 +0100 Pierre Morel <[email protected]> wrote:

s390x/css.c +1 -0

+	ccw[0].code = code ;  

Extra ' ' before ';'

yes, thanks

Quoted 4 lines

s390x/css.c static void test_enable(void) · line 187

Quoted 2 lines

s390x/css.c +1 -0

+	ccw[0].flags = CCW_F_PCI;  

Huh, what's that PCI for?

Program Control Interruption

I will add a comment :)

Quoted 6 lines

s390x/css.c static void test_enable(void) · line 189

Quoted 4 lines

s390x/css.c +2 -0

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

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

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

Quoted 34 lines

Quoted 34 lines

s390x/css.c +32 -0

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

Quoted 8 lines

s390x/css.c static void test_enable(void) · line 225

Quoted 4 lines

s390x/css.c +4 -0

+		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
+	else
+		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
+}  

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

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

Quoted 9 lines

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

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

I can do it.

Thanks for the comments,

Best regards,
Pierre

Quoted 15 lines · On Mon, 2 Dec 2019 19:18:20 +0100 Pierre Morel <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

s390x/css.c static void test_enable(void) · line 186

Quoted 1 line · On Thu, 28 Nov 2019 13:46:06 +0100 Pierre Morel <[email protected]> wrote:

s390x/css.c +1 -0

+	ccw[0].code = code ;

Extra ' ' before ';'

yes, thanks

Quoted 4 lines

s390x/css.c static void test_enable(void) · line 187

Quoted 2 lines

s390x/css.c +1 -0

+	ccw[0].flags = CCW_F_PCI;

Huh, what's that PCI for?

Quoted 1 line

Program Control Interruption

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

Quoted 3 lines

I will add a comment :)

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

Quoted 7 lines

Quoted 6 lines

s390x/css.c static void test_enable(void) · line 189

Quoted 4 lines

s390x/css.c +2 -0

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

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

Quoted 3 lines

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

Nod.

Quoted 44 lines

Quoted 43 lines

Quoted 29 lines

s390x/css.c +26 -0

+	orb_p->intparm = 0xcafec0ca;
+	orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
+	orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
+
+	report_prefix_push("Start Subchannel");
+	ret = ssch(test_device_sid, orb_p);
+	if (ret) {
+		report("ssch cc=%d", 0, ret);
+		report_prefix_pop();
+		return 0;
+	}
+	report_prefix_pop();
+	return 1;
+}
+
+static void test_sense(void)
+{
+	int success;
+
+	enable_io_irq();
+
+	success = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
+	if (!success) {
+		report("start_subchannel failed", 0);
+		return;
+	}

s390x/css.c static void test_enable(void) · line 225

Quoted 10 lines

s390x/css.c +10 -0

+
+	senseid.cu_type = buffer[2] | (buffer[1] << 8);
+	delay(1000);
+
+	/* Sense ID is non packed cut_type is at offset +1 byte */
+	if (senseid.cu_type == PONG_CU)
+		report("cu_type: expect c0ca, got %04x", 1, senseid.cu_type);
+	else
+		report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
+}

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

Quoted 2 lines

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

Yes, please :)