[kvm-unit-tests PATCH v1 0/6] Testing SSCH, CSCH and HSCH for errors

[kvm-unit-tests PATCH v1 0/6] Testing SSCH, CSCH and HSCH for errors

[Pierre Morel]

The goal of this series is to test some of the I/O instructions,
SSCH, CSCH and HSCH for errors like invalid parameters, addressing,
timing etc.
Testing about timing in QEMU/KVM is truncated to sending an instruction
before the status of the preceding instruction is cleared due ton
the QEMU serialization.

To be able to achieve these tests we we need to enhance the testing
environment with:

- new definitions for the SCSW control bits
- a new function to disable a subchannel
- handling multiple interrupts
- checking the reason of the interrupts
- deferring tsch outside of the interrupt routine

regards,
Pierre

Pierre Morel (6):
  s390x: lib: css: disabling a subchannel
  s390x: lib: css: SCSW bit definitions
  s390x: lib: css: upgrading IRQ handling
  s390x: lib: css: add expectations to wait for interrupt
  s390x: css: testing ssch error response
  s390x: css: testing clear and halt subchannel

 lib/s390x/css.h     |  55 ++++++++-
 lib/s390x/css_lib.c | 197 ++++++++++++++++++++++++++-----
 s390x/css.c         | 276 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 498 insertions(+), 30 deletions(-)

-- 
2.17.1

[kvm-unit-tests PATCH v1 1/6] s390x: lib: css: disabling a subchannel

[Pierre Morel]

Some tests require to disable a subchannel.
Let's implement the css_disable() function.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 7e3d261..b0de3a3 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -284,6 +284,7 @@ int css_enumerate(void);
 #define IO_SCH_ISC      3
 int css_enable(int schid, int isc);
 bool css_enabled(int schid);
+int css_disable(int schid);
 
 /* Library functions */
 int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index efc7057..f8db205 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -186,6 +186,75 @@ bool css_enabled(int schid)
 	}
 	return true;
 }
+
+/*
+ * css_disable: disable the subchannel
+ * @schid: Subchannel Identifier
+ * Return value:
+ *   On success: 0
+ *   On error the CC of the faulty instruction
+ *      or -1 if the retry count is exceeded.
+ */
+int css_disable(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: sch %08x failed with cc=%d", schid, cc);
+		return cc;
+	}
+
+	if (!(pmcw->flags & PMCW_ENABLE)) {
+		report_info("stsch: sch %08x already disabled", schid);
+		return 0;
+	}
+
+retry:
+	/* Update the SCHIB to disable the subchannel */
+	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: sch %08x failed with cc=%d", schid, cc);
+		return cc;
+	}
+
+	/*
+	 * Read the SCHIB again to verify the enablement
+	 */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch: updating sch %08x failed with cc=%d",
+			    schid, cc);
+		return cc;
+	}
+
+	if (!(pmcw->flags & PMCW_ENABLE)) {
+		if (retry_count)
+			report_info("stsch: sch %08x successfully disabled 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: modifying sch %08x failed after %d retries. pmcw flags: %04x",
+		    schid, retry_count, pmcw->flags);
+	return -1;
+}
 /*
  * css_enable: enable the subchannel with the specified ISC
  * @schid: Subchannel Identifier
-- 
2.17.1

[kvm-unit-tests PATCH v1 2/6] s390x: lib: css: SCSW bit definitions

[Pierre Morel]

We need the SCSW definitions to test clear and halt subchannel.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index b0de3a3..460b0bd 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -67,6 +67,29 @@ struct scsw {
 #define SCSW_SC_PRIMARY		0x00000004
 #define SCSW_SC_INTERMEDIATE	0x00000008
 #define SCSW_SC_ALERT		0x00000010
+#define SCSW_AC_SUSPEND_PEND	0x00000020
+#define SCSW_AC_DEVICE_PEND	0x00000040
+#define SCSW_AC_SUBCHANNEL_PEND	0x00000080
+#define SCSW_AC_CLEAR_PEND	0x00000100
+#define SCSW_AC_HALT_PEND	0x00000200
+#define SCSW_AC_START_PEND	0x00000400
+#define SCSW_AC_RESUME_PEND	0x00000800
+#define SCSW_FC_CLEAR		0x00001000
+#define SCSW_FC_HALT		0x00002000
+#define SCSW_FC_START		0x00004000
+#define SCSW_QDIO_RESERVED	0x00008000
+#define SCSW_PATH_NON_OP	0x00010000
+#define SCSW_EXTENDED_CTRL	0x00020000
+#define SCSW_ZERO_COND		0x00040000
+#define SCSW_SUPPRESS_SUSP_INT	0x00080000
+#define SCSW_IRB_FMT_CTRL	0x00100000
+#define SCSW_INITIAL_IRQ_STATUS	0x00200000
+#define SCSW_PREFETCH		0x00400000
+#define SCSW_CCW_FORMAT		0x00800000
+#define SCSW_DEFERED_CC		0x03000000
+#define SCSW_ESW_FORMAT		0x04000000
+#define SCSW_SUSPEND_CTRL	0x08000000
+#define SCSW_KEY		0xf0000000
 	uint32_t ctrl;
 	uint32_t ccw_addr;
 #define SCSW_DEVS_DEV_END	0x04
-- 
2.17.1

[kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

[Pierre Morel]

Until now we had very few usage of interrupts, to be able to handle
several interrupts coming up asynchronously we need to take care
to save the previous interrupt before handling the next one.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     |  29 +++++++++++
 lib/s390x/css_lib.c | 117 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 120 insertions(+), 26 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 460b0bd..65fc335 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -425,4 +425,33 @@ struct measurement_block_format1 {
 	uint32_t irq_prio_delay_time;
 };
 
+struct irq_entry {
+	struct irq_entry *next;
+	struct irb irb;
+	uint32_t sid;
+};
+
+struct irq_entry *alloc_irq(void);
+struct irq_entry *get_irq(void);
+void put_irq(struct irq_entry *irq);
+
+#include <asm/arch_def.h>
+static inline void disable_io_irq(void)
+{
+	uint64_t mask;
+
+	mask = extract_psw_mask();
+	mask &= ~PSW_MASK_IO;
+	load_psw_mask(mask);
+}
+
+static inline void enable_io_irq(void)
+{
+	uint64_t mask;
+
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_IO;
+	load_psw_mask(mask);
+}
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index f8db205..211c73c 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -9,6 +9,8 @@
  */
 #include <libcflat.h>
 #include <alloc_phys.h>
+#include <util.h>
+#include <alloc.h>
 #include <asm/page.h>
 #include <string.h>
 #include <interrupt.h>
@@ -22,6 +24,46 @@
 struct schib schib;
 struct chsc_scsc *chsc_scsc;
 
+static struct irq_entry *irqs;
+
+struct irq_entry *get_irq(void)
+{
+	struct irq_entry *irq = NULL;
+
+	if (irqs) {
+		irq = irqs;
+		irqs = irq->next;
+	}
+	return irq;
+}
+
+void put_irq(struct irq_entry *irq)
+{
+	free(irq);
+}
+
+static void save_irq(struct irq_entry *irq)
+{
+	struct irq_entry *e;
+
+	if (!irqs) {
+		irqs = irq;
+	} else {
+		e = irqs;
+		while (e && e->next)
+			e = e->next;
+		e->next = irq;
+	}
+}
+
+struct irq_entry *alloc_irq(void)
+{
+	struct irq_entry *irq;
+
+	irq = calloc(1, sizeof(*irq));
+	return irq;
+}
+
 static const char * const chsc_rsp_description[] = {
 	"CHSC unknown error",
 	"Command executed",
@@ -422,38 +464,38 @@ static struct irb irb;
 void css_irq_io(void)
 {
 	int ret = 0;
-	char *flags;
-	int sid;
+	struct irq_entry *irq;
 
 	report_prefix_push("Interrupt");
-	sid = lowcore_ptr->subsys_id_word;
+	irq = alloc_irq();
+	assert(irq);
+
+	irq->sid = lowcore_ptr->subsys_id_word;
 	/* Lowlevel set the SID as interrupt parameter. */
-	if (lowcore_ptr->io_int_param != sid) {
+	if (lowcore_ptr->io_int_param != irq->sid) {
 		report(0,
 		       "io_int_param: %x differs from subsys_id_word: %x",
-		       lowcore_ptr->io_int_param, sid);
+		       lowcore_ptr->io_int_param, irq->sid);
 		goto pop;
 	}
 	report_prefix_pop();
 
 	report_prefix_push("tsch");
-	ret = tsch(sid, &irb);
+	ret = tsch(irq->sid, &irq->irb);
 	switch (ret) {
 	case 1:
-		dump_irb(&irb);
-		flags = dump_scsw_flags(irb.scsw.ctrl);
-		report(0,
-		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
-		       sid, flags);
+		report_info("no status pending on %08x : %s", irq->sid,
+			    dump_scsw_flags(irq->irb.scsw.ctrl));
 		break;
 	case 2:
 		report(0, "tsch returns unexpected CC 2");
 		break;
 	case 3:
-		report(0, "tsch reporting sch %08x as not operational", sid);
+		report(0, "tsch reporting sch %08x as not operational", irq->sid);
 		break;
 	case 0:
 		/* Stay humble on success */
+		save_irq(irq);
 		break;
 	}
 pop:
@@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
 int wait_and_check_io_completion(int schid)
 {
 	int ret = 0;
-
-	wait_for_interrupt(PSW_MASK_IO);
+	struct irq_entry *irq = NULL;
 
 	report_prefix_push("check I/O completion");
 
-	if (lowcore_ptr->io_int_param != schid) {
+	disable_io_irq();
+	irq = get_irq();
+	while (!irq) {
+		wait_for_interrupt(PSW_MASK_IO);
+		disable_io_irq();
+		irq = get_irq();
+		report_info("next try");
+	}
+	enable_io_irq();
+
+	assert(irq);
+
+	if (irq->sid != schid) {
 		report(0, "interrupt parameter: expected %08x got %08x",
-		       schid, lowcore_ptr->io_int_param);
+		       schid, irq->sid);
 		ret = -1;
 		goto end;
 	}
 
 	/* Verify that device status is valid */
-	if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) {
-		report(0, "No status pending after interrupt. Subch Ctrl: %08x",
-		       irb.scsw.ctrl);
-		ret = -1;
+	if (!(irq->irb.scsw.ctrl & SCSW_SC_PENDING)) {
+		ret = 0;
 		goto end;
 	}
 
-	if (!(irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
+	/* clear and halt pending are valid even without secondary or primary status */
+	if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) {
+		ret = 0;
+		goto end;
+	}
+
+	/* For start pending we need at least one of primary or secondary status */
+	if (!(irq->irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
 		report(0, "Primary or secondary status missing. Subch Ctrl: %08x",
-		       irb.scsw.ctrl);
+		       irq->irb.scsw.ctrl);
 		ret = -1;
 		goto end;
 	}
 
-	if (!(irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
+	/* For start pending we also need to have device or channel end information */
+	if (!(irq->irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
 		report(0, "No device end or sch end. Dev. status: %02x",
-		       irb.scsw.dev_stat);
+		       irq->irb.scsw.dev_stat);
 		ret = -1;
 		goto end;
 	}
 
-	if (irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
-		report_info("Unexpected Subch. status %02x", irb.scsw.sch_stat);
+	/* We only accept the SubCHannel Status for Illegal Length */
+	if (irq->irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
+		report_info("Unexpected Subch. status %02x",
+			    irq->irb.scsw.sch_stat);
 		ret = -1;
 		goto end;
 	}
 
 end:
+	if (ret)
+		dump_irb(&irq->irb);
+
+	put_irq(irq);
 	report_prefix_pop();
 	return ret;
 }
-- 
2.17.1

[kvm-unit-tests PATCH v1 4/6] s390x: lib: css: add expectations to wait for interrupt

[Pierre Morel]

When waiting for an interrupt we may need to check the cause of
the interrupt depending on the test case.

Let's provide the tests the possibility to check if the last valid
IRQ received is for the expected instruction.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     |  2 +-
 lib/s390x/css_lib.c | 11 ++++++++++-
 s390x/css.c         |  4 ++--
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 65fc335..add3b4e 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -316,7 +316,7 @@ void css_irq_io(void);
 int css_residual_count(unsigned int schid);
 
 void enable_io_isc(uint8_t isc);
-int wait_and_check_io_completion(int schid);
+int wait_and_check_io_completion(int schid, uint32_t pending);
 
 /*
  * CHSC definitions
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 211c73c..4cdd7ad 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -537,7 +537,7 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
  * completion.
  * Only report failures.
  */
-int wait_and_check_io_completion(int schid)
+int wait_and_check_io_completion(int schid, uint32_t pending)
 {
 	int ret = 0;
 	struct irq_entry *irq = NULL;
@@ -569,6 +569,15 @@ int wait_and_check_io_completion(int schid)
 		goto end;
 	}
 
+	if (pending) {
+		if (!(pending & irq->irb.scsw.ctrl)) {
+			report_info("%08x : %s", schid, dump_scsw_flags(irq->irb.scsw.ctrl));
+			report_info("expect   : %s", dump_scsw_flags(pending));
+			ret = -1;
+			goto end;
+		}
+	}
+
 	/* clear and halt pending are valid even without secondary or primary status */
 	if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) {
 		ret = 0;
diff --git a/s390x/css.c b/s390x/css.c
index c340c53..a6a9773 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -94,7 +94,7 @@ static void test_sense(void)
 		goto error;
 	}
 
-	if (wait_and_check_io_completion(test_device_sid) < 0)
+	if (wait_and_check_io_completion(test_device_sid, SCSW_FC_START) < 0)
 		goto error;
 
 	/* Test transfer completion */
@@ -137,7 +137,7 @@ static void sense_id(void)
 {
 	assert(!start_ccw1_chain(test_device_sid, ccw));
 
-	assert(wait_and_check_io_completion(test_device_sid) >= 0);
+	assert(wait_and_check_io_completion(test_device_sid, SCSW_FC_START) >= 0);
 }
 
 static void css_init(void)
-- 
2.17.1

[kvm-unit-tests PATCH v1 5/6] s390x: css: testing ssch error response

[Pierre Morel]

Checking error response on various eroneous SSCH instructions:
- ORB alignment
- ORB above 2G
- CCW above 2G
- bad ORB flags

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

diff --git a/s390x/css.c b/s390x/css.c
index a6a9773..1c891f8 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -51,6 +51,107 @@ static void test_enable(void)
 	report(cc == 0, "Enable subchannel %08x", test_device_sid);
 }
 
+static void test_ssch(void)
+{
+	struct orb orb = {
+		.intparm = test_device_sid,
+		.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT,
+	};
+	int i;
+	phys_addr_t base, top;
+
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(register_io_int_func(css_irq_io) == 0);
+
+	/* ORB address should be aligned on 32 bits */
+	report_prefix_push("ORB alignment");
+	expect_pgm_int();
+	ssch(test_device_sid, (void *)0x110002);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	/* ORB address should be lower than 2G */
+	report_prefix_push("ORB Address above 2G");
+	expect_pgm_int();
+	ssch(test_device_sid, (void *)0x80000000);
+	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
+	report_prefix_pop();
+
+	phys_alloc_get_unused(&base, &top);
+	report_info("base %08lx, top %08lx", base, top);
+
+	/* ORB address should be available we check 1G*/
+	report_prefix_push("ORB Address must be available");
+	if (top < 0x40000000) {
+		expect_pgm_int();
+		ssch(test_device_sid, (void *)0x40000000);
+		check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
+	} else {
+		report_skip("guest started with more than 1G memory");
+	}
+	report_prefix_pop();
+
+	report_prefix_push("CCW address above 2G");
+	orb.cpa = 0x80000000;
+	expect_pgm_int();
+	ssch(test_device_sid, &orb);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+	report_prefix_pop();
+
+	senseid = alloc_io_mem(sizeof(*senseid), 0);
+	assert(senseid);
+	orb.cpa = (uint64_t)ccw_alloc(CCW_CMD_SENSE_ID, senseid,
+				      sizeof(*senseid), CCW_F_SLI);
+	assert(orb.cpa);
+
+	report_prefix_push("Disabled subchannel");
+	assert(css_disable(test_device_sid) == 0);
+	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	report_prefix_pop();
+
+	/*
+	 * Check sending a second SSCH before clearing the status with TSCH
+	 * the subchannel is left disabled.
+	 */
+	report_prefix_push("SSCH on channel with status pending");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(ssch(test_device_sid, &orb) == 0);
+	report(ssch(test_device_sid, &orb) == 1, "CC = 1");
+	/* now we clear the status */
+	assert(wait_and_check_io_completion(test_device_sid, SCSW_FC_START) == 0);
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	report_prefix_push("ORB MIDAW unsupported");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	orb.ctrl |= ORB_CTRL_MIDAW;
+	expect_pgm_int();
+	ssch(test_device_sid, &orb);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+	report_prefix_pop();
+	orb.ctrl = 0;
+
+	for (i = 0; i < 5; i++) {
+		char buffer[30];
+
+		orb.ctrl = (0x02 << i);
+		snprintf(buffer, 30, "ORB reserved ctrl flags %02x", orb.ctrl);
+		report_prefix_push(buffer);
+		expect_pgm_int();
+		ssch(test_device_sid, &orb);
+		check_pgm_int_code(PGM_INT_CODE_OPERAND);
+		report_prefix_pop();
+	}
+
+	report_prefix_push("ORB wrong ctrl flags");
+	orb.ctrl |= 0x040000;
+	expect_pgm_int();
+	ssch(test_device_sid, &orb);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+	report_prefix_pop();
+}
+
 /*
  * test_sense
  * Pre-requisites:
@@ -339,6 +440,7 @@ static struct {
 	{ "initialize CSS (chsc)", css_init },
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
+	{ "start subchannel", test_ssch },
 	{ "sense (ssch/tsch)", test_sense },
 	{ "measurement block (schm)", test_schm },
 	{ "measurement block format0", test_schm_fmt0 },
-- 
2.17.1

[kvm-unit-tests PATCH v1 6/6] s390x: css: testing clear and halt subchannel

[Pierre Morel]

checking return values for CSCH and HSCH for various configurations.

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

diff --git a/s390x/css.c b/s390x/css.c
index 1c891f8..ee02cdd 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -51,6 +51,175 @@ static void test_enable(void)
 	report(cc == 0, "Enable subchannel %08x", test_device_sid);
 }
 
+static void dummy_irq(void)
+{
+}
+
+static int check_io_completion(int sid, uint32_t expected)
+{
+	struct irb irb;
+	int ret;
+
+	report_prefix_push("IRQ flags");
+
+	ret = tsch(sid, &irb);
+	if (ret)
+		goto end;
+
+	if (!expected)
+		goto end;
+
+	if (!(expected & irb.scsw.ctrl)) {
+		report_info("expect: %s got: %s",
+			    dump_scsw_flags(expected),
+			    dump_scsw_flags(irb.scsw.ctrl));
+		ret = -1;
+	}
+
+end:
+	report(!ret, "expectations");
+	report_prefix_pop();
+	return ret;
+}
+
+static void test_csch(void)
+{
+	struct orb orb = {
+		.intparm = test_device_sid,
+		.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT,
+	};
+	struct ccw1 *ccw;
+
+	senseid = alloc_io_mem(sizeof(*senseid), 0);
+	assert(senseid);
+	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
+	assert(ccw);
+	orb.cpa = (uint64_t)ccw;
+
+	/* 1- Basic check for CSCH */
+	report_prefix_push("CSCH on a quiet subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	report(csch(test_device_sid) == 0, "subchannel clear");
+
+	/* now we check the flags */
+	report_prefix_push("IRQ flags");
+	report(wait_and_check_io_completion(test_device_sid, SCSW_FC_CLEAR) == 0, "expected");
+	report_prefix_pop();
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* For the following checks we need to execute tsch synchronously */
+	assert(unregister_io_int_func(css_irq_io) == 0);
+	assert(register_io_int_func(dummy_irq) == 0);
+
+	/* 2- We want to check if the IRQ flags of SSCH are erased by clear */
+	report_prefix_push("CSCH on SSCH status pending subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(ssch(test_device_sid, &orb) == 0);
+	report(csch(test_device_sid) == 0, "subchannel cleared");
+	check_io_completion(test_device_sid, SCSW_FC_CLEAR);
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* 3- Checking CSCH after HSCH */
+	report_prefix_push("CSCH on a halted subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(hsch(test_device_sid) == 0);
+	report(csch(test_device_sid) == 0, "subchannel cleared");
+	check_io_completion(test_device_sid, SCSW_FC_CLEAR);
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* 4- Checking CSCH after CSCH */
+	report_prefix_push("CSCH on a cleared subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(csch(test_device_sid) == 0);
+	report(csch(test_device_sid) == 0, "subchannel cleared");
+	check_io_completion(test_device_sid, SCSW_FC_CLEAR);
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* Reset the IRQ handler */
+	assert(unregister_io_int_func(dummy_irq) == 0);
+	assert(register_io_int_func(css_irq_io) == 0);
+
+	free_io_mem(senseid, sizeof(*senseid));
+	free_io_mem(ccw, sizeof(*ccw));
+}
+
+static void test_hsch(void)
+{
+	struct orb orb = {
+		.intparm = test_device_sid,
+		.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT,
+	};
+	struct ccw1 *ccw;
+
+	senseid = alloc_io_mem(sizeof(*senseid), 0);
+	assert(senseid);
+	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
+	assert(ccw);
+	orb.cpa = (uint64_t)ccw;
+
+	/* 1- Basic HSCH */
+	report_prefix_push("HSCH on a quiet subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	report(hsch(test_device_sid) == 0, "subchannel halted");
+
+	/* now we check the flags */
+	report_prefix_push("IRQ flags");
+	report(wait_and_check_io_completion(test_device_sid, SCSW_FC_HALT) == 0, "expected");
+	report_prefix_pop();
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* For the following checks we need to execute tsch synchronously */
+	assert(unregister_io_int_func(css_irq_io) == 0);
+	assert(register_io_int_func(dummy_irq) == 0);
+
+	/* 2- Check HSCH after SSCH */
+	report_prefix_push("HSCH on status pending subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(ssch(test_device_sid, &orb) == 0);
+	report(hsch(test_device_sid) == 1, "Halt subchannel should fail with CC 1");
+	check_io_completion(test_device_sid, SCSW_FC_START);
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* 3- Check HSCH after CSCH */
+	report_prefix_push("HSCH on busy on CSCH subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(csch(test_device_sid) == 0);
+	report(hsch(test_device_sid) == 1, "Halt subchannel should fail with CC 1");
+	check_io_completion(test_device_sid, SCSW_FC_CLEAR);
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* 4- Check HSCH after HSCH */
+	report_prefix_push("HSCH on busy on HSCH subchannel");
+	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
+	assert(hsch(test_device_sid) == 0);
+	report(hsch(test_device_sid) == 1, "Halt subchannel should fail with CC 1");
+	check_io_completion(test_device_sid, SCSW_FC_HALT);
+
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+
+	/* Reset the IRQ handler */
+	assert(unregister_io_int_func(dummy_irq) == 0);
+	assert(register_io_int_func(css_irq_io) == 0);
+
+	free_io_mem(senseid, sizeof(*senseid));
+	free_io_mem(ccw, sizeof(*ccw));
+}
+
 static void test_ssch(void)
 {
 	struct orb orb = {
@@ -61,7 +230,6 @@ static void test_ssch(void)
 	phys_addr_t base, top;
 
 	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
-	assert(register_io_int_func(css_irq_io) == 0);
 
 	/* ORB address should be aligned on 32 bits */
 	report_prefix_push("ORB alignment");
@@ -441,6 +609,8 @@ static struct {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "start subchannel", test_ssch },
+	{ "halt subchannel", test_hsch },
+	{ "clear subchannel", test_csch },
 	{ "sense (ssch/tsch)", test_sense },
 	{ "measurement block (schm)", test_schm },
 	{ "measurement block format0", test_schm_fmt0 },
-- 
2.17.1

Re: [kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

[Pierre Morel]

hum, I just saw I forgot to upgrade the report of the residual count.
This is not really relevant for this series.
Of course I will adapt it for the v2.


On 3/18/21 2:26 PM, Pierre Morel wrote:
> Until now we had very few usage of interrupts, to be able to handle
> several interrupts coming up asynchronously we need to take care
> to save the previous interrupt before handling the next one.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     |  29 +++++++++++
>   lib/s390x/css_lib.c | 117 ++++++++++++++++++++++++++++++++++----------
>   2 files changed, 120 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 460b0bd..65fc335 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -425,4 +425,33 @@ struct measurement_block_format1 {
>   	uint32_t irq_prio_delay_time;
>   };
>   
> +struct irq_entry {
> +	struct irq_entry *next;
> +	struct irb irb;
> +	uint32_t sid;
> +};
> +
> +struct irq_entry *alloc_irq(void);
> +struct irq_entry *get_irq(void);
> +void put_irq(struct irq_entry *irq);
> +
> +#include <asm/arch_def.h>
> +static inline void disable_io_irq(void)
> +{
> +	uint64_t mask;
> +
> +	mask = extract_psw_mask();
> +	mask &= ~PSW_MASK_IO;
> +	load_psw_mask(mask);
> +}
> +
> +static inline void enable_io_irq(void)
> +{
> +	uint64_t mask;
> +
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_IO;
> +	load_psw_mask(mask);
> +}
> +
>   #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index f8db205..211c73c 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -9,6 +9,8 @@
>    */
>   #include <libcflat.h>
>   #include <alloc_phys.h>
> +#include <util.h>
> +#include <alloc.h>
>   #include <asm/page.h>
>   #include <string.h>
>   #include <interrupt.h>
> @@ -22,6 +24,46 @@
>   struct schib schib;
>   struct chsc_scsc *chsc_scsc;
>   
> +static struct irq_entry *irqs;
> +
> +struct irq_entry *get_irq(void)
> +{
> +	struct irq_entry *irq = NULL;
> +
> +	if (irqs) {
> +		irq = irqs;
> +		irqs = irq->next;
> +	}
> +	return irq;
> +}
> +
> +void put_irq(struct irq_entry *irq)
> +{
> +	free(irq);
> +}
> +
> +static void save_irq(struct irq_entry *irq)
> +{
> +	struct irq_entry *e;
> +
> +	if (!irqs) {
> +		irqs = irq;
> +	} else {
> +		e = irqs;
> +		while (e && e->next)
> +			e = e->next;
> +		e->next = irq;
> +	}
> +}
> +
> +struct irq_entry *alloc_irq(void)
> +{
> +	struct irq_entry *irq;
> +
> +	irq = calloc(1, sizeof(*irq));
> +	return irq;
> +}
> +
>   static const char * const chsc_rsp_description[] = {
>   	"CHSC unknown error",
>   	"Command executed",
> @@ -422,38 +464,38 @@ static struct irb irb;
>   void css_irq_io(void)
>   {
>   	int ret = 0;
> -	char *flags;
> -	int sid;
> +	struct irq_entry *irq;
>   
>   	report_prefix_push("Interrupt");
> -	sid = lowcore_ptr->subsys_id_word;
> +	irq = alloc_irq();
> +	assert(irq);
> +
> +	irq->sid = lowcore_ptr->subsys_id_word;
>   	/* Lowlevel set the SID as interrupt parameter. */
> -	if (lowcore_ptr->io_int_param != sid) {
> +	if (lowcore_ptr->io_int_param != irq->sid) {
>   		report(0,
>   		       "io_int_param: %x differs from subsys_id_word: %x",
> -		       lowcore_ptr->io_int_param, sid);
> +		       lowcore_ptr->io_int_param, irq->sid);
>   		goto pop;
>   	}
>   	report_prefix_pop();
>   
>   	report_prefix_push("tsch");
> -	ret = tsch(sid, &irb);
> +	ret = tsch(irq->sid, &irq->irb);
>   	switch (ret) {
>   	case 1:
> -		dump_irb(&irb);
> -		flags = dump_scsw_flags(irb.scsw.ctrl);
> -		report(0,
> -		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
> -		       sid, flags);
> +		report_info("no status pending on %08x : %s", irq->sid,
> +			    dump_scsw_flags(irq->irb.scsw.ctrl));
>   		break;
>   	case 2:
>   		report(0, "tsch returns unexpected CC 2");
>   		break;
>   	case 3:
> -		report(0, "tsch reporting sch %08x as not operational", sid);
> +		report(0, "tsch reporting sch %08x as not operational", irq->sid);
>   		break;
>   	case 0:
>   		/* Stay humble on success */
> +		save_irq(irq);
>   		break;
>   	}
>   pop:
> @@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>   int wait_and_check_io_completion(int schid)
>   {
>   	int ret = 0;
> -
> -	wait_for_interrupt(PSW_MASK_IO);
> +	struct irq_entry *irq = NULL;
>   
>   	report_prefix_push("check I/O completion");
>   
> -	if (lowcore_ptr->io_int_param != schid) {
> +	disable_io_irq();
> +	irq = get_irq();
> +	while (!irq) {
> +		wait_for_interrupt(PSW_MASK_IO);
> +		disable_io_irq();
> +		irq = get_irq();
> +		report_info("next try");
> +	}
> +	enable_io_irq();
> +
> +	assert(irq);
> +
> +	if (irq->sid != schid) {
>   		report(0, "interrupt parameter: expected %08x got %08x",
> -		       schid, lowcore_ptr->io_int_param);
> +		       schid, irq->sid);
>   		ret = -1;
>   		goto end;
>   	}
>   
>   	/* Verify that device status is valid */
> -	if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) {
> -		report(0, "No status pending after interrupt. Subch Ctrl: %08x",
> -		       irb.scsw.ctrl);
> -		ret = -1;
> +	if (!(irq->irb.scsw.ctrl & SCSW_SC_PENDING)) {
> +		ret = 0;
>   		goto end;
>   	}
>   
> -	if (!(irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
> +	/* clear and halt pending are valid even without secondary or primary status */
> +	if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) {
> +		ret = 0;
> +		goto end;
> +	}
> +
> +	/* For start pending we need at least one of primary or secondary status */
> +	if (!(irq->irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
>   		report(0, "Primary or secondary status missing. Subch Ctrl: %08x",
> -		       irb.scsw.ctrl);
> +		       irq->irb.scsw.ctrl);
>   		ret = -1;
>   		goto end;
>   	}
>   
> -	if (!(irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
> +	/* For start pending we also need to have device or channel end information */
> +	if (!(irq->irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
>   		report(0, "No device end or sch end. Dev. status: %02x",
> -		       irb.scsw.dev_stat);
> +		       irq->irb.scsw.dev_stat);
>   		ret = -1;
>   		goto end;
>   	}
>   
> -	if (irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
> -		report_info("Unexpected Subch. status %02x", irb.scsw.sch_stat);
> +	/* We only accept the SubCHannel Status for Illegal Length */
> +	if (irq->irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
> +		report_info("Unexpected Subch. status %02x",
> +			    irq->irb.scsw.sch_stat);
>   		ret = -1;
>   		goto end;
>   	}
>   
>   end:
> +	if (ret)
> +		dump_irb(&irq->irb);
> +
> +	put_irq(irq);
>   	report_prefix_pop();
>   	return ret;
>   }
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 1/6] s390x: lib: css: disabling a subchannel

[Janosch Frank]

On 3/18/21 2:26 PM, Pierre Morel wrote:
> Some tests require to disable a subchannel.
> Let's implement the css_disable() function.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Looks ok, minor nits below.

> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 7e3d261..b0de3a3 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -284,6 +284,7 @@ int css_enumerate(void);
>  #define IO_SCH_ISC      3
>  int css_enable(int schid, int isc);
>  bool css_enabled(int schid);
> +int css_disable(int schid);
>  
>  /* Library functions */
>  int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index efc7057..f8db205 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -186,6 +186,75 @@ bool css_enabled(int schid)
>  	}
>  	return true;
>  }
> +
> +/*
> + * css_disable: disable the subchannel
> + * @schid: Subchannel Identifier
> + * Return value:
> + *   On success: 0
> + *   On error the CC of the faulty instruction
> + *      or -1 if the retry count is exceeded.
> + */
> +int css_disable(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: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +	if (!(pmcw->flags & PMCW_ENABLE)) {
> +		report_info("stsch: sch %08x already disabled", schid);
> +		return 0;
> +	}
> +
> +retry:
> +	/* Update the SCHIB to disable the subchannel */
> +	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.
> +		 */

Weird indentation, the lines should be longer, no?

> +		report_info("msch: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +	/*
> +	 * Read the SCHIB again to verify the enablement
> +	 */

Can be one line

> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return cc;
> +	}
> +
> +	if (!(pmcw->flags & PMCW_ENABLE)) {
> +		if (retry_count)
> +			report_info("stsch: sch %08x successfully disabled 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 */

Personally I dislike comments at the end of lines except for
constant/variable comments. Just put it before the delay.

> +		goto retry;
> +	}
> +
> +	report_info("msch: modifying sch %08x failed after %d retries. pmcw flags: %04x",
> +		    schid, retry_count, pmcw->flags);
> +	return -1;
> +}
>  /*
>   * css_enable: enable the subchannel with the specified ISC
>   * @schid: Subchannel Identifier
> 

Re: [kvm-unit-tests PATCH v1 4/6] s390x: lib: css: add expectations to wait for interrupt

[Janosch Frank]

On 3/18/21 2:26 PM, Pierre Morel wrote:
> When waiting for an interrupt we may need to check the cause of
> the interrupt depending on the test case.
> 
> Let's provide the tests the possibility to check if the last valid
> IRQ received is for the expected instruction.

s/instruction/command/?

We're checking for some value in an IO structure, right?
Instruction makes me expect an actual processor instruction.

Is there another word that can be used to describe what we're checking
here? If yes please also add it to the "pending" variable. "pending_fc"
or "pending_scsw_fc" for example.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  2 +-
>  lib/s390x/css_lib.c | 11 ++++++++++-
>  s390x/css.c         |  4 ++--
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 65fc335..add3b4e 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -316,7 +316,7 @@ void css_irq_io(void);
>  int css_residual_count(unsigned int schid);
>  
>  void enable_io_isc(uint8_t isc);
> -int wait_and_check_io_completion(int schid);
> +int wait_and_check_io_completion(int schid, uint32_t pending);
>  
>  /*
>   * CHSC definitions
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 211c73c..4cdd7ad 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -537,7 +537,7 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>   * completion.
>   * Only report failures.
>   */
> -int wait_and_check_io_completion(int schid)
> +int wait_and_check_io_completion(int schid, uint32_t pending)
>  {
>  	int ret = 0;
>  	struct irq_entry *irq = NULL;
> @@ -569,6 +569,15 @@ int wait_and_check_io_completion(int schid)
>  		goto end;
>  	}
>  
> +	if (pending) {
> +		if (!(pending & irq->irb.scsw.ctrl)) {
> +			report_info("%08x : %s", schid, dump_scsw_flags(irq->irb.scsw.ctrl));
> +			report_info("expect   : %s", dump_scsw_flags(pending));
> +			ret = -1;
> +			goto end;
> +		}
> +	}
> +
>  	/* clear and halt pending are valid even without secondary or primary status */
>  	if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) {
>  		ret = 0;
> diff --git a/s390x/css.c b/s390x/css.c
> index c340c53..a6a9773 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -94,7 +94,7 @@ static void test_sense(void)
>  		goto error;
>  	}
>  
> -	if (wait_and_check_io_completion(test_device_sid) < 0)
> +	if (wait_and_check_io_completion(test_device_sid, SCSW_FC_START) < 0)
>  		goto error;
>  
>  	/* Test transfer completion */
> @@ -137,7 +137,7 @@ static void sense_id(void)
>  {
>  	assert(!start_ccw1_chain(test_device_sid, ccw));
>  
> -	assert(wait_and_check_io_completion(test_device_sid) >= 0);
> +	assert(wait_and_check_io_completion(test_device_sid, SCSW_FC_START) >= 0);
>  }
>  
>  static void css_init(void)
> 

Re: [kvm-unit-tests PATCH v1 1/6] s390x: lib: css: disabling a subchannel

[Pierre Morel]

On 3/19/21 10:02 AM, Janosch Frank wrote:
> On 3/18/21 2:26 PM, Pierre Morel wrote:
>> Some tests require to disable a subchannel.
>> Let's implement the css_disable() function.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Looks ok, minor nits below.
> 
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 70 insertions(+)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 7e3d261..b0de3a3 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -284,6 +284,7 @@ int css_enumerate(void);
>>   #define IO_SCH_ISC      3
>>   int css_enable(int schid, int isc);
>>   bool css_enabled(int schid);
>> +int css_disable(int schid);
>>   
>>   /* Library functions */
>>   int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index efc7057..f8db205 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -186,6 +186,75 @@ bool css_enabled(int schid)
>>   	}
>>   	return true;
>>   }
>> +
>> +/*
>> + * css_disable: disable the subchannel
>> + * @schid: Subchannel Identifier
>> + * Return value:
>> + *   On success: 0
>> + *   On error the CC of the faulty instruction
>> + *      or -1 if the retry count is exceeded.
>> + */
>> +int css_disable(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: sch %08x failed with cc=%d", schid, cc);
>> +		return cc;
>> +	}
>> +
>> +	if (!(pmcw->flags & PMCW_ENABLE)) {
>> +		report_info("stsch: sch %08x already disabled", schid);
>> +		return 0;
>> +	}
>> +
>> +retry:
>> +	/* Update the SCHIB to disable the subchannel */
>> +	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.
>> +		 */
> 
> Weird indentation, the lines should be longer, no?
> 
>> +		report_info("msch: sch %08x failed with cc=%d", schid, cc);
>> +		return cc;
>> +	}
>> +
>> +	/*
>> +	 * Read the SCHIB again to verify the enablement
>> +	 */
> 
> Can be one line
> 
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: updating sch %08x failed with cc=%d",
>> +			    schid, cc);
>> +		return cc;
>> +	}
>> +
>> +	if (!(pmcw->flags & PMCW_ENABLE)) {
>> +		if (retry_count)
>> +			report_info("stsch: sch %08x successfully disabled 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 */
> 
> Personally I dislike comments at the end of lines except for
> constant/variable comments. Just put it before the delay.
> 
>> +		goto retry;
>> +	}
>> +
>> +	report_info("msch: modifying sch %08x failed after %d retries. pmcw flags: %04x",
>> +		    schid, retry_count, pmcw->flags);
>> +	return -1;
>> +}
>>   /*
>>    * css_enable: enable the subchannel with the specified ISC
>>    * @schid: Subchannel Identifier
>>
> 

Thanks, will do the changes
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 5/6] s390x: css: testing ssch error response

[Janosch Frank]

On 3/18/21 2:26 PM, Pierre Morel wrote:
> Checking error response on various eroneous SSCH instructions:
> - ORB alignment
> - ORB above 2G
> - CCW above 2G
> - bad ORB flags
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index a6a9773..1c891f8 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -51,6 +51,107 @@ static void test_enable(void)
>  	report(cc == 0, "Enable subchannel %08x", test_device_sid);
>  }
>  
> +static void test_ssch(void)
> +{
> +	struct orb orb = {
> +		.intparm = test_device_sid,
> +		.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT,
> +	};
> +	int i;
> +	phys_addr_t base, top;
> +
> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
> +	assert(register_io_int_func(css_irq_io) == 0);
> +
> +	/* ORB address should be aligned on 32 bits */
> +	report_prefix_push("ORB alignment");
> +	expect_pgm_int();
> +	ssch(test_device_sid, (void *)0x110002);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	/* ORB address should be lower than 2G */
> +	report_prefix_push("ORB Address above 2G");
> +	expect_pgm_int();
> +	ssch(test_device_sid, (void *)0x80000000);
> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
> +	report_prefix_pop();
> +
> +	phys_alloc_get_unused(&base, &top);
> +	report_info("base %08lx, top %08lx", base, top);

Please use this function from lib/s390x/sclp.c

uint64_t get_ram_size(void)
{
        return ram_size;
}

> +
> +	/* ORB address should be available we check 1G*/
> +	report_prefix_push("ORB Address must be available");
> +	if (top < 0x40000000) {
> +		expect_pgm_int();
> +		ssch(test_device_sid, (void *)0x40000000);
> +		check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
> +	} else {
> +		report_skip("guest started with more than 1G memory");
> +	}
> +	report_prefix_pop();
> +
> +	report_prefix_push("CCW address above 2G");
> +	orb.cpa = 0x80000000;
> +	expect_pgm_int();
> +	ssch(test_device_sid, &orb);
> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +	report_prefix_pop();
> +
> +	senseid = alloc_io_mem(sizeof(*senseid), 0);
> +	assert(senseid);
> +	orb.cpa = (uint64_t)ccw_alloc(CCW_CMD_SENSE_ID, senseid,
> +				      sizeof(*senseid), CCW_F_SLI);
> +	assert(orb.cpa);
> +
> +	report_prefix_push("Disabled subchannel");
> +	assert(css_disable(test_device_sid) == 0);
> +	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
> +	report_prefix_pop();
> +
> +	/*
> +	 * Check sending a second SSCH before clearing the status with TSCH
> +	 * the subchannel is left disabled.

If a second SSCH is sent before clearing via TSCH the subchannel is
disabled by firmware? Did I get that right?

> +	 */
> +	report_prefix_push("SSCH on channel with status pending");
> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
> +	assert(ssch(test_device_sid, &orb) == 0);
> +	report(ssch(test_device_sid, &orb) == 1, "CC = 1");
> +	/* now we clear the status */
> +	assert(wait_and_check_io_completion(test_device_sid, SCSW_FC_START) == 0);
> +	assert(css_disable(test_device_sid) == 0);
> +	report_prefix_pop();
> +
> +	report_prefix_push("ORB MIDAW unsupported");
> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
> +	orb.ctrl |= ORB_CTRL_MIDAW;
> +	expect_pgm_int();
> +	ssch(test_device_sid, &orb);
> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +	report_prefix_pop();
> +	orb.ctrl = 0;
> +
> +	for (i = 0; i < 5; i++) {
> +		char buffer[30];
> +
> +		orb.ctrl = (0x02 << i);
> +		snprintf(buffer, 30, "ORB reserved ctrl flags %02x", orb.ctrl);
> +		report_prefix_push(buffer);
> +		expect_pgm_int();
> +		ssch(test_device_sid, &orb);
> +		check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +		report_prefix_pop();
> +	}
> +
> +	report_prefix_push("ORB wrong ctrl flags");
> +	orb.ctrl |= 0x040000;
> +	expect_pgm_int();
> +	ssch(test_device_sid, &orb);
> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +	report_prefix_pop();
> +}
> +
>  /*
>   * test_sense
>   * Pre-requisites:
> @@ -339,6 +440,7 @@ static struct {
>  	{ "initialize CSS (chsc)", css_init },
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
> +	{ "start subchannel", test_ssch },
>  	{ "sense (ssch/tsch)", test_sense },
>  	{ "measurement block (schm)", test_schm },
>  	{ "measurement block format0", test_schm_fmt0 },
> 

Re: [kvm-unit-tests PATCH v1 4/6] s390x: lib: css: add expectations to wait for interrupt

[Pierre Morel]

On 3/19/21 10:09 AM, Janosch Frank wrote:
> On 3/18/21 2:26 PM, Pierre Morel wrote:
>> When waiting for an interrupt we may need to check the cause of
>> the interrupt depending on the test case.
>>
>> Let's provide the tests the possibility to check if the last valid
>> IRQ received is for the expected instruction.
> 
> s/instruction/command/?

Right, instruction may not be the optimal wording.
I/O architecture description have some strange (for me) wording, the 
best is certainly to stick on this.

Then I will use "the expected function" here.

> 
> We're checking for some value in an IO structure, right?
> Instruction makes me expect an actual processor instruction.
> 
> Is there another word that can be used to describe what we're checking
> here? If yes please also add it to the "pending" variable. "pending_fc"
> or "pending_scsw_fc" for example.

Pending is used to specify that the instruction has been accepted but 
the according function is still pending, i.e. not finished and will stay 
pending for a normal operation until the device active bit is set.

So pending is not the right word, what we check here is the function 
control, indicating the function the status refers too.

> 
>>
...snip...

>>    * Only report failures.
>>    */
>> -int wait_and_check_io_completion(int schid)
>> +int wait_and_check_io_completion(int schid, uint32_t pending)


Consequently I will change "pending" with "function_ctrl"

Thanks for forcing clarification
I hope Connie will agree with this :)

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 5/6] s390x: css: testing ssch error response

[Pierre Morel]

On 3/19/21 10:18 AM, Janosch Frank wrote:
> On 3/18/21 2:26 PM, Pierre Morel wrote:
>> Checking error response on various eroneous SSCH instructions:
>> - ORB alignment
>> - ORB above 2G
>> - CCW above 2G
>> - bad ORB flags
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 102 insertions(+)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index a6a9773..1c891f8 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -51,6 +51,107 @@ static void test_enable(void)
>>   	report(cc == 0, "Enable subchannel %08x", test_device_sid);
>>   }
>>   
>> +static void test_ssch(void)
>> +{
>> +	struct orb orb = {
>> +		.intparm = test_device_sid,
>> +		.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT,
>> +	};
>> +	int i;
>> +	phys_addr_t base, top;
>> +
>> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
>> +	assert(register_io_int_func(css_irq_io) == 0);
>> +
>> +	/* ORB address should be aligned on 32 bits */
>> +	report_prefix_push("ORB alignment");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x110002);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	/* ORB address should be lower than 2G */
>> +	report_prefix_push("ORB Address above 2G");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x80000000);
>> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>> +	report_prefix_pop();
>> +
>> +	phys_alloc_get_unused(&base, &top);
>> +	report_info("base %08lx, top %08lx", base, top);
> 
> Please use this function from lib/s390x/sclp.c
> 
> uint64_t get_ram_size(void)
> {
>          return ram_size;
> }
> 

thanks, I was not aware of this function.


>> +
>> +	/* ORB address should be available we check 1G*/
>> +	report_prefix_push("ORB Address must be available");
>> +	if (top < 0x40000000) {
>> +		expect_pgm_int();
>> +		ssch(test_device_sid, (void *)0x40000000);
>> +		check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>> +	} else {
>> +		report_skip("guest started with more than 1G memory");
>> +	}
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("CCW address above 2G");
>> +	orb.cpa = 0x80000000;
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, &orb);
>> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
>> +	report_prefix_pop();
>> +
>> +	senseid = alloc_io_mem(sizeof(*senseid), 0);
>> +	assert(senseid);
>> +	orb.cpa = (uint64_t)ccw_alloc(CCW_CMD_SENSE_ID, senseid,
>> +				      sizeof(*senseid), CCW_F_SLI);
>> +	assert(orb.cpa);
>> +
>> +	report_prefix_push("Disabled subchannel");
>> +	assert(css_disable(test_device_sid) == 0);
>> +	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
>> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
>> +	report_prefix_pop();
>> +
>> +	/*
>> +	 * Check sending a second SSCH before clearing the status with TSCH >> +	 * the subchannel is left disabled.
> 
> If a second SSCH is sent before clearing via TSCH the subchannel is
> disabled by firmware? Did I get that right?


Oh, no, sorry, the comment is not good, no the firmware does not disable 
the subchannel, the comment is not at the right place.

> 
>> +	 */
>> +	report_prefix_push("SSCH on channel with status pending");
>> +	assert(css_enable(test_device_sid, IO_SCH_ISC) == 0);
>> +	assert(ssch(test_device_sid, &orb) == 0);
>> +	report(ssch(test_device_sid, &orb) == 1, "CC = 1");
>> +	/* now we clear the status */
>> +	assert(wait_and_check_io_completion(test_device_sid, SCSW_FC_START) == 0);

The comment about leaving the channel disabled should be here  should be 
here... :(
The idea about disabling the subchannel is to make sure to have a clean 
subchannel for the next test.
However I am not so sure it really bring something.


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 1/6] s390x: lib: css: disabling a subchannel

[Cornelia Huck]

On Thu, 18 Mar 2021 14:26:23 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Some tests require to disable a subchannel.
> Let's implement the css_disable() function.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)

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

Re: [kvm-unit-tests PATCH v1 1/6] s390x: lib: css: disabling a subchannel

[Pierre Morel]

On 3/19/21 11:03 AM, Cornelia Huck wrote:
> On Thu, 18 Mar 2021 14:26:23 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Some tests require to disable a subchannel.
>> Let's implement the css_disable() function.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 70 insertions(+)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 2/6] s390x: lib: css: SCSW bit definitions

[Cornelia Huck]

On Thu, 18 Mar 2021 14:26:24 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We need the SCSW definitions to test clear and halt subchannel.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index b0de3a3..460b0bd 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -67,6 +67,29 @@ struct scsw {
>  #define SCSW_SC_PRIMARY		0x00000004
>  #define SCSW_SC_INTERMEDIATE	0x00000008
>  #define SCSW_SC_ALERT		0x00000010
> +#define SCSW_AC_SUSPEND_PEND	0x00000020
> +#define SCSW_AC_DEVICE_PEND	0x00000040
> +#define SCSW_AC_SUBCHANNEL_PEND	0x00000080

Naming: aren't these two rather "active", not "pending"? So maybe
SCSW_AC_DEVICE_ACTIVE and SCSW_AC_SUBCH_ACTIVE?

> +#define SCSW_AC_CLEAR_PEND	0x00000100
> +#define SCSW_AC_HALT_PEND	0x00000200
> +#define SCSW_AC_START_PEND	0x00000400
> +#define SCSW_AC_RESUME_PEND	0x00000800
> +#define SCSW_FC_CLEAR		0x00001000
> +#define SCSW_FC_HALT		0x00002000
> +#define SCSW_FC_START		0x00004000
> +#define SCSW_QDIO_RESERVED	0x00008000
> +#define SCSW_PATH_NON_OP	0x00010000
> +#define SCSW_EXTENDED_CTRL	0x00020000
> +#define SCSW_ZERO_COND		0x00040000
> +#define SCSW_SUPPRESS_SUSP_INT	0x00080000
> +#define SCSW_IRB_FMT_CTRL	0x00100000
> +#define SCSW_INITIAL_IRQ_STATUS	0x00200000
> +#define SCSW_PREFETCH		0x00400000
> +#define SCSW_CCW_FORMAT		0x00800000
> +#define SCSW_DEFERED_CC		0x03000000
> +#define SCSW_ESW_FORMAT		0x04000000
> +#define SCSW_SUSPEND_CTRL	0x08000000
> +#define SCSW_KEY		0xf0000000
>  	uint32_t ctrl;
>  	uint32_t ccw_addr;
>  #define SCSW_DEVS_DEV_END	0x04

Re: [kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

[Cornelia Huck]

On Thu, 18 Mar 2021 14:26:25 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Until now we had very few usage of interrupts, to be able to handle
> several interrupts coming up asynchronously we need to take care
> to save the previous interrupt before handling the next one.

An alternative would be to keep I/O interrupts disabled until you are
done with processing any information that might be overwritten.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  29 +++++++++++
>  lib/s390x/css_lib.c | 117 ++++++++++++++++++++++++++++++++++----------
>  2 files changed, 120 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 460b0bd..65fc335 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -425,4 +425,33 @@ struct measurement_block_format1 {
>  	uint32_t irq_prio_delay_time;
>  };
>  
> +struct irq_entry {
> +	struct irq_entry *next;
> +	struct irb irb;
> +	uint32_t sid;

I'm wondering whether that set of information make sense for saving.

We basically have some things in the lowcore that get overwritten by
subsequent I/O interrupts (in addition to the sid the intparm and the
interrupt identification word which contains the isc), and the irb,
which only gets overwritten if you do a tsch into the same memory area.
So, if you need to save some things, I'd suggest to add the intparm and
the interrupt identification word to it. Not sure whether the irb can
be handled independently? Need to read code first :)

> +};

(...)

> @@ -422,38 +464,38 @@ static struct irb irb;
>  void css_irq_io(void)
>  {
>  	int ret = 0;
> -	char *flags;
> -	int sid;
> +	struct irq_entry *irq;
>  
>  	report_prefix_push("Interrupt");
> -	sid = lowcore_ptr->subsys_id_word;
> +	irq = alloc_irq();
> +	assert(irq);
> +
> +	irq->sid = lowcore_ptr->subsys_id_word;
>  	/* Lowlevel set the SID as interrupt parameter. */
> -	if (lowcore_ptr->io_int_param != sid) {
> +	if (lowcore_ptr->io_int_param != irq->sid) {
>  		report(0,
>  		       "io_int_param: %x differs from subsys_id_word: %x",
> -		       lowcore_ptr->io_int_param, sid);
> +		       lowcore_ptr->io_int_param, irq->sid);
>  		goto pop;
>  	}
>  	report_prefix_pop();
>  
>  	report_prefix_push("tsch");
> -	ret = tsch(sid, &irb);
> +	ret = tsch(irq->sid, &irq->irb);
>  	switch (ret) {
>  	case 1:
> -		dump_irb(&irb);
> -		flags = dump_scsw_flags(irb.scsw.ctrl);
> -		report(0,
> -		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
> -		       sid, flags);
> +		report_info("no status pending on %08x : %s", irq->sid,
> +			    dump_scsw_flags(irq->irb.scsw.ctrl));

This is not what you are looking at here, though?

The problem is that the hypervisor gave you cc 1 (stored, not status
pending) while you just got an interrupt; the previous message logged
that, while the new one does not. (The scsw flags are still
interesting, as it gives further information about the mismatch.)

>  		break;
>  	case 2:
>  		report(0, "tsch returns unexpected CC 2");
>  		break;
>  	case 3:
> -		report(0, "tsch reporting sch %08x as not operational", sid);
> +		report(0, "tsch reporting sch %08x as not operational", irq->sid);
>  		break;
>  	case 0:
>  		/* Stay humble on success */
> +		save_irq(irq);
>  		break;
>  	}
>  pop:
> @@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>  int wait_and_check_io_completion(int schid)
>  {
>  	int ret = 0;
> -
> -	wait_for_interrupt(PSW_MASK_IO);
> +	struct irq_entry *irq = NULL;
>  
>  	report_prefix_push("check I/O completion");
>  
> -	if (lowcore_ptr->io_int_param != schid) {
> +	disable_io_irq();
> +	irq = get_irq();
> +	while (!irq) {
> +		wait_for_interrupt(PSW_MASK_IO);
> +		disable_io_irq();

Isn't the disable_io_irq() redundant here?

(In general, I'm a bit confused about the I/O interrupt handling here.
Might need to read through the whole thing again.)

> +		irq = get_irq();
> +		report_info("next try");
> +	}
> +	enable_io_irq();
> +
> +	assert(irq);
> +
> +	if (irq->sid != schid) {
>  		report(0, "interrupt parameter: expected %08x got %08x",
> -		       schid, lowcore_ptr->io_int_param);
> +		       schid, irq->sid);
>  		ret = -1;
>  		goto end;

You're still expecting that there's only one subchannel enabled for I/O
interrupts at the same time, right?

>  	}
>  
>  	/* Verify that device status is valid */
> -	if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) {
> -		report(0, "No status pending after interrupt. Subch Ctrl: %08x",
> -		       irb.scsw.ctrl);
> -		ret = -1;
> +	if (!(irq->irb.scsw.ctrl & SCSW_SC_PENDING)) {

Confused. An I/O interrupt for a subchannel that is not status pending
is surely an issue?

> +		ret = 0;
>  		goto end;
>  	}
>  
> -	if (!(irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
> +	/* clear and halt pending are valid even without secondary or primary status */
> +	if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) {

Can you factor out the new/changed checks here into a separate patch?
Would make the change easier to follow.

Also, you might want to check other things for halt/clear as well?

> +		ret = 0;
> +		goto end;
> +	}
> +
> +	/* For start pending we need at least one of primary or secondary status */
> +	if (!(irq->irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
>  		report(0, "Primary or secondary status missing. Subch Ctrl: %08x",
> -		       irb.scsw.ctrl);
> +		       irq->irb.scsw.ctrl);

I'm wondering whether that is actually true. Maybe need to double check
what happens with deferred ccs etc.

>  		ret = -1;
>  		goto end;
>  	}
>  
> -	if (!(irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
> +	/* For start pending we also need to have device or channel end information */
> +	if (!(irq->irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
>  		report(0, "No device end or sch end. Dev. status: %02x",
> -		       irb.scsw.dev_stat);
> +		       irq->irb.scsw.dev_stat);

Again, not sure whether that is true in any case (surely for the good
path, and I think for unit check as well; but ISTR that there can be
error conditions where we won't get another interrupt for the same I/O,
but device end is not set, because the error occurred before we even
reached the device... should those be logged?)

>  		ret = -1;
>  		goto end;
>  	}
>  
> -	if (irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
> -		report_info("Unexpected Subch. status %02x", irb.scsw.sch_stat);
> +	/* We only accept the SubCHannel Status for Illegal Length */

It's more like that we just don't deal with any of the other subchannel
status flags, right?

> +	if (irq->irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
> +		report_info("Unexpected Subch. status %02x",
> +			    irq->irb.scsw.sch_stat);
>  		ret = -1;
>  		goto end;
>  	}
>  
>  end:
> +	if (ret)
> +		dump_irb(&irq->irb);
> +
> +	put_irq(irq);
>  	report_prefix_pop();
>  	return ret;
>  }

Re: [kvm-unit-tests PATCH v1 4/6] s390x: lib: css: add expectations to wait for interrupt

[Cornelia Huck]

On Fri, 19 Mar 2021 10:50:09 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 3/19/21 10:09 AM, Janosch Frank wrote:
> > On 3/18/21 2:26 PM, Pierre Morel wrote:  
> >> When waiting for an interrupt we may need to check the cause of
> >> the interrupt depending on the test case.
> >>
> >> Let's provide the tests the possibility to check if the last valid
> >> IRQ received is for the expected instruction.  
> > 
> > s/instruction/command/?  
> 
> Right, instruction may not be the optimal wording.
> I/O architecture description have some strange (for me) wording, the 
> best is certainly to stick on this.
> 
> Then I will use "the expected function" here.
> 
> > 
> > We're checking for some value in an IO structure, right?
> > Instruction makes me expect an actual processor instruction.
> > 
> > Is there another word that can be used to describe what we're checking
> > here? If yes please also add it to the "pending" variable. "pending_fc"
> > or "pending_scsw_fc" for example.  
> 
> Pending is used to specify that the instruction has been accepted but 
> the according function is still pending, i.e. not finished and will stay 
> pending for a normal operation until the device active bit is set.
> 
> So pending is not the right word, what we check here is the function 
> control, indicating the function the status refers too.
> 
> >   
> >>  
> ...snip...
> 
> >>    * Only report failures.
> >>    */
> >> -int wait_and_check_io_completion(int schid)
> >> +int wait_and_check_io_completion(int schid, uint32_t pending)  
> 
> 
> Consequently I will change "pending" with "function_ctrl"
> 
> Thanks for forcing clarification
> I hope Connie will agree with this :)

I'm not quite sure yet :)

I/O wording and operation can be complicated... we basically have:

- various instructions: ssch, hsch, csch
- invoking one of those instructions may initiate a function at the
  subchannel
- if an instruction lead to a function being initiated (but not
  necessarily actually being performed!), the matching bit is set in
  the fctl
- the fctl bits are accumulative (e.g. if you do a hsch on a subchannel
  where a start function is still in progress, you'll have both the
  start and the halt function indicated) and only cleared after
  collecting final status

So, setting the function is a direct consequence of executing an I/O
instruction -- but the interrupt may not be directly related to *all*
of the functions that are indicated (e.g. in the example above, where
we may get an interrupt for the hsch, but none directly for the ssch;
you can also add a csch on top of this; fortunately, we only stack in
the start->halt->clear direction.)

Regarding wording:

Maybe

"if the last valid IRQ received is for the function expected
after executing an instruction or sequence of instructions."

and

int wait_and_check_io_completion(int schid, uint32_t expected_fctl)

?

Re: [kvm-unit-tests PATCH v1 1/6] s390x: lib: css: disabling a subchannel

[Pierre Morel]

On 3/19/21 11:03 AM, Cornelia Huck wrote:
> On Thu, 18 Mar 2021 14:26:23 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Some tests require to disable a subchannel.
>> Let's implement the css_disable() function.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 70 insertions(+)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 2/6] s390x: lib: css: SCSW bit definitions

[Pierre Morel]

On 3/19/21 11:16 AM, Cornelia Huck wrote:
> On Thu, 18 Mar 2021 14:26:24 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We need the SCSW definitions to test clear and halt subchannel.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index b0de3a3..460b0bd 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -67,6 +67,29 @@ struct scsw {
>>   #define SCSW_SC_PRIMARY		0x00000004
>>   #define SCSW_SC_INTERMEDIATE	0x00000008
>>   #define SCSW_SC_ALERT		0x00000010
>> +#define SCSW_AC_SUSPEND_PEND	0x00000020
>> +#define SCSW_AC_DEVICE_PEND	0x00000040
>> +#define SCSW_AC_SUBCHANNEL_PEND	0x00000080
> 
> Naming: aren't these two rather "active", not "pending"? So maybe
> SCSW_AC_DEVICE_ACTIVE and SCSW_AC_SUBCH_ACTIVE?

right, I modify this

> 
>> +#define SCSW_AC_CLEAR_PEND	0x00000100
>> +#define SCSW_AC_HALT_PEND	0x00000200
>> +#define SCSW_AC_START_PEND	0x00000400
>> +#define SCSW_AC_RESUME_PEND	0x00000800
>> +#define SCSW_FC_CLEAR		0x00001000
>> +#define SCSW_FC_HALT		0x00002000
>> +#define SCSW_FC_START		0x00004000
>> +#define SCSW_QDIO_RESERVED	0x00008000
>> +#define SCSW_PATH_NON_OP	0x00010000
>> +#define SCSW_EXTENDED_CTRL	0x00020000
>> +#define SCSW_ZERO_COND		0x00040000
>> +#define SCSW_SUPPRESS_SUSP_INT	0x00080000
>> +#define SCSW_IRB_FMT_CTRL	0x00100000
>> +#define SCSW_INITIAL_IRQ_STATUS	0x00200000
>> +#define SCSW_PREFETCH		0x00400000
>> +#define SCSW_CCW_FORMAT		0x00800000
>> +#define SCSW_DEFERED_CC		0x03000000
>> +#define SCSW_ESW_FORMAT		0x04000000
>> +#define SCSW_SUSPEND_CTRL	0x08000000
>> +#define SCSW_KEY		0xf0000000
>>   	uint32_t ctrl;
>>   	uint32_t ccw_addr;
>>   #define SCSW_DEVS_DEV_END	0x04
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

[Pierre Morel]

On 3/19/21 12:01 PM, Cornelia Huck wrote:
> On Thu, 18 Mar 2021 14:26:25 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Until now we had very few usage of interrupts, to be able to handle
>> several interrupts coming up asynchronously we need to take care
>> to save the previous interrupt before handling the next one.
> 
> An alternative would be to keep I/O interrupts disabled until you are
> done with processing any information that might be overwritten.
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  29 +++++++++++
>>   lib/s390x/css_lib.c | 117 ++++++++++++++++++++++++++++++++++----------
>>   2 files changed, 120 insertions(+), 26 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 460b0bd..65fc335 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -425,4 +425,33 @@ struct measurement_block_format1 {
>>   	uint32_t irq_prio_delay_time;
>>   };
>>   
>> +struct irq_entry {
>> +	struct irq_entry *next;
>> +	struct irb irb;
>> +	uint32_t sid;
> 
> I'm wondering whether that set of information make sense for saving.
> 
> We basically have some things in the lowcore that get overwritten by
> subsequent I/O interrupts (in addition to the sid the intparm and the
> interrupt identification word which contains the isc), and the irb,
> which only gets overwritten if you do a tsch into the same memory area.
> So, if you need to save some things, I'd suggest to add the intparm and
> the interrupt identification word to it. Not sure whether the irb can
> be handled independently? Need to read code first :)

That is right.
I only saved what I needed for the moment.

> 
>> +};
> 
> (...)
> 
>> @@ -422,38 +464,38 @@ static struct irb irb;
>>   void css_irq_io(void)
>>   {
>>   	int ret = 0;
>> -	char *flags;
>> -	int sid;
>> +	struct irq_entry *irq;
>>   
>>   	report_prefix_push("Interrupt");
>> -	sid = lowcore_ptr->subsys_id_word;
>> +	irq = alloc_irq();
>> +	assert(irq);
>> +
>> +	irq->sid = lowcore_ptr->subsys_id_word;
>>   	/* Lowlevel set the SID as interrupt parameter. */
>> -	if (lowcore_ptr->io_int_param != sid) {
>> +	if (lowcore_ptr->io_int_param != irq->sid) {
>>   		report(0,
>>   		       "io_int_param: %x differs from subsys_id_word: %x",
>> -		       lowcore_ptr->io_int_param, sid);
>> +		       lowcore_ptr->io_int_param, irq->sid);
>>   		goto pop;
>>   	}
>>   	report_prefix_pop();
>>   
>>   	report_prefix_push("tsch");
>> -	ret = tsch(sid, &irb);
>> +	ret = tsch(irq->sid, &irq->irb);
>>   	switch (ret) {
>>   	case 1:
>> -		dump_irb(&irb);
>> -		flags = dump_scsw_flags(irb.scsw.ctrl);
>> -		report(0,
>> -		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
>> -		       sid, flags);
>> +		report_info("no status pending on %08x : %s", irq->sid,
>> +			    dump_scsw_flags(irq->irb.scsw.ctrl));
> 
> This is not what you are looking at here, though?
> 
> The problem is that the hypervisor gave you cc 1 (stored, not status
> pending) while you just got an interrupt; the previous message logged
> that, while the new one does not. (The scsw flags are still
> interesting, as it gives further information about the mismatch.)

I can keep the old message.
How ever I do not think it is a reason to report a failure.
Do you agree with replaacing report(0,) with report_info()

> 
>>   		break;
>>   	case 2:
>>   		report(0, "tsch returns unexpected CC 2");
>>   		break;
>>   	case 3:
>> -		report(0, "tsch reporting sch %08x as not operational", sid);
>> +		report(0, "tsch reporting sch %08x as not operational", irq->sid);
>>   		break;
>>   	case 0:
>>   		/* Stay humble on success */
>> +		save_irq(irq);
>>   		break;
>>   	}
>>   pop:
>> @@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>>   int wait_and_check_io_completion(int schid)
>>   {
>>   	int ret = 0;
>> -
>> -	wait_for_interrupt(PSW_MASK_IO);
>> +	struct irq_entry *irq = NULL;
>>   
>>   	report_prefix_push("check I/O completion");
>>   
>> -	if (lowcore_ptr->io_int_param != schid) {
>> +	disable_io_irq();
>> +	irq = get_irq();
>> +	while (!irq) {
>> +		wait_for_interrupt(PSW_MASK_IO);
>> +		disable_io_irq();
> 
> Isn't the disable_io_irq() redundant here?

No because wait for interrupt re-enable the interrupts
I will add a comment

> 
> (In general, I'm a bit confused about the I/O interrupt handling here.
> Might need to read through the whole thing again.)
> 
>> +		irq = get_irq();
>> +		report_info("next try");
>> +	}
>> +	enable_io_irq();
>> +
>> +	assert(irq);
>> +
>> +	if (irq->sid != schid) {
>>   		report(0, "interrupt parameter: expected %08x got %08x",
>> -		       schid, lowcore_ptr->io_int_param);
>> +		       schid, irq->sid);
>>   		ret = -1;
>>   		goto end;
> 
> You're still expecting that there's only one subchannel enabled for I/O
> interrupts at the same time, right?

Yes, I plan to introduce multiple channels later.

> 
>>   	}
>>   
>>   	/* Verify that device status is valid */
>> -	if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) {
>> -		report(0, "No status pending after interrupt. Subch Ctrl: %08x",
>> -		       irb.scsw.ctrl);
>> -		ret = -1;
>> +	if (!(irq->irb.scsw.ctrl & SCSW_SC_PENDING)) {
> 
> Confused. An I/O interrupt for a subchannel that is not status pending
> is surely an issue?
> 
>> +		ret = 0;
>>   		goto end;
>>   	}
>>   
>> -	if (!(irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
>> +	/* clear and halt pending are valid even without secondary or primary status */
>> +	if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) {
> 
> Can you factor out the new/changed checks here into a separate patch?
> Would make the change easier to follow.

yes, these changes should not belong here.
I will rewrite this all

> 
> Also, you might want to check other things for halt/clear as well?

Yes in a further patch

> 
>> +		ret = 0;
>> +		goto end;
>> +	}
>> +
>> +	/* For start pending we need at least one of primary or secondary status */
>> +	if (!(irq->irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
>>   		report(0, "Primary or secondary status missing. Subch Ctrl: %08x",
>> -		       irb.scsw.ctrl);
>> +		       irq->irb.scsw.ctrl);
> 
> I'm wondering whether that is actually true. Maybe need to double check
> what happens with deferred ccs etc.

Yes,

> 
>>   		ret = -1;
>>   		goto end;
>>   	}
>>   
>> -	if (!(irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
>> +	/* For start pending we also need to have device or channel end information */
>> +	if (!(irq->irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
>>   		report(0, "No device end or sch end. Dev. status: %02x",
>> -		       irb.scsw.dev_stat);
>> +		       irq->irb.scsw.dev_stat);
> 
> Again, not sure whether that is true in any case (surely for the good
> path, and I think for unit check as well; but ISTR that there can be
> error conditions where we won't get another interrupt for the same I/O,
> but device end is not set, because the error occurred before we even
> reached the device... should those be logged?)

surely

> 
>>   		ret = -1;
>>   		goto end;
>>   	}
>>   
>> -	if (irb.scsw.sch_stat & ~SCSW_SCHS_IL) {
>> -		report_info("Unexpected Subch. status %02x", irb.scsw.sch_stat);
>> +	/* We only accept the SubCHannel Status for Illegal Length */
> 
> It's more like that we just don't deal with any of the other subchannel
> status flags, right?

OK, I will rework this completely
Thanks for the comment,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

[Cornelia Huck]

On Fri, 19 Mar 2021 16:55:15 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 3/19/21 12:01 PM, Cornelia Huck wrote:
> > On Thu, 18 Mar 2021 14:26:25 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> @@ -422,38 +464,38 @@ static struct irb irb;
> >>   void css_irq_io(void)
> >>   {
> >>   	int ret = 0;
> >> -	char *flags;
> >> -	int sid;
> >> +	struct irq_entry *irq;
> >>   
> >>   	report_prefix_push("Interrupt");
> >> -	sid = lowcore_ptr->subsys_id_word;
> >> +	irq = alloc_irq();
> >> +	assert(irq);
> >> +
> >> +	irq->sid = lowcore_ptr->subsys_id_word;
> >>   	/* Lowlevel set the SID as interrupt parameter. */
> >> -	if (lowcore_ptr->io_int_param != sid) {
> >> +	if (lowcore_ptr->io_int_param != irq->sid) {
> >>   		report(0,
> >>   		       "io_int_param: %x differs from subsys_id_word: %x",
> >> -		       lowcore_ptr->io_int_param, sid);
> >> +		       lowcore_ptr->io_int_param, irq->sid);
> >>   		goto pop;
> >>   	}
> >>   	report_prefix_pop();
> >>   
> >>   	report_prefix_push("tsch");
> >> -	ret = tsch(sid, &irb);
> >> +	ret = tsch(irq->sid, &irq->irb);
> >>   	switch (ret) {
> >>   	case 1:
> >> -		dump_irb(&irb);
> >> -		flags = dump_scsw_flags(irb.scsw.ctrl);
> >> -		report(0,
> >> -		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
> >> -		       sid, flags);
> >> +		report_info("no status pending on %08x : %s", irq->sid,
> >> +			    dump_scsw_flags(irq->irb.scsw.ctrl));  
> > 
> > This is not what you are looking at here, though?
> > 
> > The problem is that the hypervisor gave you cc 1 (stored, not status
> > pending) while you just got an interrupt; the previous message logged
> > that, while the new one does not. (The scsw flags are still
> > interesting, as it gives further information about the mismatch.)  
> 
> I can keep the old message.
> How ever I do not think it is a reason to report a failure.
> Do you agree with replaacing report(0,) with report_info()

I don't really see how we could get an I/O interrupt for a subchannel
that is not status pending, unless we have other code racing with this
one that cleared the status pending already (and that would be a bug in
our test program.) Or are you aware in anything in the architecture
that could make the status pending go away again (other than the
subchannel becoming not operational?)

> 
> >   
> >>   		break;
> >>   	case 2:
> >>   		report(0, "tsch returns unexpected CC 2");
> >>   		break;
> >>   	case 3:
> >> -		report(0, "tsch reporting sch %08x as not operational", sid);
> >> +		report(0, "tsch reporting sch %08x as not operational", irq->sid);
> >>   		break;
> >>   	case 0:
> >>   		/* Stay humble on success */
> >> +		save_irq(irq);
> >>   		break;
> >>   	}
> >>   pop:
> >> @@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
> >>   int wait_and_check_io_completion(int schid)
> >>   {
> >>   	int ret = 0;
> >> -
> >> -	wait_for_interrupt(PSW_MASK_IO);
> >> +	struct irq_entry *irq = NULL;
> >>   
> >>   	report_prefix_push("check I/O completion");
> >>   
> >> -	if (lowcore_ptr->io_int_param != schid) {
> >> +	disable_io_irq();
> >> +	irq = get_irq();
> >> +	while (!irq) {
> >> +		wait_for_interrupt(PSW_MASK_IO);
> >> +		disable_io_irq();  
> > 
> > Isn't the disable_io_irq() redundant here?  
> 
> No because wait for interrupt re-enable the interrupts
> I will add a comment

Hm, I thought it restored the previous status.

> 
> > 
> > (In general, I'm a bit confused about the I/O interrupt handling here.
> > Might need to read through the whole thing again.)

But also see this comment :)

> >   
> >> +		irq = get_irq();
> >> +		report_info("next try");
> >> +	}
> >> +	enable_io_irq();

Re: [kvm-unit-tests PATCH v1 4/6] s390x: lib: css: add expectations to wait for interrupt

[Pierre Morel]

On 3/19/21 12:23 PM, Cornelia Huck wrote:
> On Fri, 19 Mar 2021 10:50:09 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 3/19/21 10:09 AM, Janosch Frank wrote:
>>> On 3/18/21 2:26 PM, Pierre Morel wrote:
>>>> When waiting for an interrupt we may need to check the cause of
>>>> the interrupt depending on the test case.
>>>>
>>>> Let's provide the tests the possibility to check if the last valid
>>>> IRQ received is for the expected instruction.
>>>
>>> s/instruction/command/?
>>
>> Right, instruction may not be the optimal wording.
>> I/O architecture description have some strange (for me) wording, the
>> best is certainly to stick on this.
>>
>> Then I will use "the expected function" here.
>>
>>>
>>> We're checking for some value in an IO structure, right?
>>> Instruction makes me expect an actual processor instruction.
>>>
>>> Is there another word that can be used to describe what we're checking
>>> here? If yes please also add it to the "pending" variable. "pending_fc"
>>> or "pending_scsw_fc" for example.
>>
>> Pending is used to specify that the instruction has been accepted but
>> the according function is still pending, i.e. not finished and will stay
>> pending for a normal operation until the device active bit is set.
>>
>> So pending is not the right word, what we check here is the function
>> control, indicating the function the status refers too.
>>
>>>    
>>>>   
>> ...snip...
>>
>>>>     * Only report failures.
>>>>     */
>>>> -int wait_and_check_io_completion(int schid)
>>>> +int wait_and_check_io_completion(int schid, uint32_t pending)
>>
>>
>> Consequently I will change "pending" with "function_ctrl"
>>
>> Thanks for forcing clarification
>> I hope Connie will agree with this :)
> 
> I'm not quite sure yet :)
> 
> I/O wording and operation can be complicated... we basically have:
> 
> - various instructions: ssch, hsch, csch
> - invoking one of those instructions may initiate a function at the
>    subchannel
> - if an instruction lead to a function being initiated (but not
>    necessarily actually being performed!), the matching bit is set in
>    the fctl
> - the fctl bits are accumulative (e.g. if you do a hsch on a subchannel
>    where a start function is still in progress, you'll have both the
>    start and the halt function indicated) and only cleared after
>    collecting final status
> 
> So, setting the function is a direct consequence of executing an I/O
> instruction -- but the interrupt may not be directly related to *all*
> of the functions that are indicated (e.g. in the example above, where
> we may get an interrupt for the hsch, but none directly for the ssch;
> you can also add a csch on top of this; fortunately, we only stack in
> the start->halt->clear direction.)

For the real machine but QEMU serialize every I/O instruction so we 
never get 2 activities indicated at the same time.
That is something I tried to check with the last 2 patches.

> 
> Regarding wording:
> 
> Maybe
> 
> "if the last valid IRQ received is for the function expected
> after executing an instruction or sequence of instructions."
> 
> and
> 
> int wait_and_check_io_completion(int schid, uint32_t expected_fctl)
> 
> ?
> 

Yes better.

Thanks for the comments,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

[Pierre Morel]

On 3/19/21 5:09 PM, Cornelia Huck wrote:
> On Fri, 19 Mar 2021 16:55:15 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 3/19/21 12:01 PM, Cornelia Huck wrote:
>>> On Thu, 18 Mar 2021 14:26:25 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> @@ -422,38 +464,38 @@ static struct irb irb;
>>>>    void css_irq_io(void)
>>>>    {
>>>>    	int ret = 0;
>>>> -	char *flags;
>>>> -	int sid;
>>>> +	struct irq_entry *irq;
>>>>    
>>>>    	report_prefix_push("Interrupt");
>>>> -	sid = lowcore_ptr->subsys_id_word;
>>>> +	irq = alloc_irq();
>>>> +	assert(irq);
>>>> +
>>>> +	irq->sid = lowcore_ptr->subsys_id_word;
>>>>    	/* Lowlevel set the SID as interrupt parameter. */
>>>> -	if (lowcore_ptr->io_int_param != sid) {
>>>> +	if (lowcore_ptr->io_int_param != irq->sid) {
>>>>    		report(0,
>>>>    		       "io_int_param: %x differs from subsys_id_word: %x",
>>>> -		       lowcore_ptr->io_int_param, sid);
>>>> +		       lowcore_ptr->io_int_param, irq->sid);
>>>>    		goto pop;
>>>>    	}
>>>>    	report_prefix_pop();
>>>>    
>>>>    	report_prefix_push("tsch");
>>>> -	ret = tsch(sid, &irb);
>>>> +	ret = tsch(irq->sid, &irq->irb);
>>>>    	switch (ret) {
>>>>    	case 1:
>>>> -		dump_irb(&irb);
>>>> -		flags = dump_scsw_flags(irb.scsw.ctrl);
>>>> -		report(0,
>>>> -		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
>>>> -		       sid, flags);
>>>> +		report_info("no status pending on %08x : %s", irq->sid,
>>>> +			    dump_scsw_flags(irq->irb.scsw.ctrl));
>>>
>>> This is not what you are looking at here, though?
>>>
>>> The problem is that the hypervisor gave you cc 1 (stored, not status
>>> pending) while you just got an interrupt; the previous message logged
>>> that, while the new one does not. (The scsw flags are still
>>> interesting, as it gives further information about the mismatch.)
>>
>> I can keep the old message.
>> How ever I do not think it is a reason to report a failure.
>> Do you agree with replaacing report(0,) with report_info()
> 
> I don't really see how we could get an I/O interrupt for a subchannel
> that is not status pending, unless we have other code racing with this
> one that cleared the status pending already (and that would be a bug in
> our test program.) Or are you aware in anything in the architecture
> that could make the status pending go away again (other than the
> subchannel becoming not operational?)

:) no
I really messed up with this patch.
sorry, can only do better


> 
>>
>>>    
>>>>    		break;
>>>>    	case 2:
>>>>    		report(0, "tsch returns unexpected CC 2");
>>>>    		break;
>>>>    	case 3:
>>>> -		report(0, "tsch reporting sch %08x as not operational", sid);
>>>> +		report(0, "tsch reporting sch %08x as not operational", irq->sid);
>>>>    		break;
>>>>    	case 0:
>>>>    		/* Stay humble on success */
>>>> +		save_irq(irq);
>>>>    		break;
>>>>    	}
>>>>    pop:
>>>> @@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>>>>    int wait_and_check_io_completion(int schid)
>>>>    {
>>>>    	int ret = 0;
>>>> -
>>>> -	wait_for_interrupt(PSW_MASK_IO);
>>>> +	struct irq_entry *irq = NULL;
>>>>    
>>>>    	report_prefix_push("check I/O completion");
>>>>    
>>>> -	if (lowcore_ptr->io_int_param != schid) {
>>>> +	disable_io_irq();
>>>> +	irq = get_irq();
>>>> +	while (!irq) {
>>>> +		wait_for_interrupt(PSW_MASK_IO);
>>>> +		disable_io_irq();
>>>
>>> Isn't the disable_io_irq() redundant here?
>>
>> No because wait for interrupt re-enable the interrupts
>> I will add a comment
> 
> Hm, I thought it restored the previous status.
> 
>>
>>>
>>> (In general, I'm a bit confused about the I/O interrupt handling here.
>>> Might need to read through the whole thing again.)
> 
> But also see this comment :)
> 

Oh you mean the comment were it restores the psw mask.
yes,I see it now.
hum
yes, this patch is awful. really sorry

please do not lose more time I must really rework the all series.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen