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

[kvm-unit-tests PATCH v2 0/8] 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.
We can not test the sending of an instruction before the last instruction
has been proceeded by QEMU due to the QEMU serialization but we can 
check the behavior of an instruction if it is started before the status
of the last instruction is read.

To do this we first separate the waiting for the interruption and the
checking of the IRB and enable the subchannel without an I/O ISC to
avoid interruptions at this subchannel and second, we add an argument
to the routine in charge to check the IRB representing the expected
SCSW control field of the IRB.

We also need several other enhancements to the testing environment:

- definitions for the SCSW control bits
- a new function to disable a subchannel
- a macro to simplify skiping tests when no device is present
  (I know the warning about return in macro, can we accept it?)

In the new tests we assume that all the test preparation is working and
use asserts for all function for which we do not expect a failure.

regards,
Pierre


Pierre Morel (8):
  s390x: lib: css: disabling a subchannel
  s390x: lib: css: SCSW bit definitions
  s390x: css: simplify skipping tests on no device
  s390x: lib: css: separate wait for IRQ and check I/O completion
  s390x: lib: css: add SCSW ctrl expectations to check I/O completion
  s390x: css: testing ssch error response
  s390x: css: testing halt subchannel
  s390x: css: testing clear subchannel

 lib/s390x/css.h     |  35 +++++-
 lib/s390x/css_lib.c | 104 ++++++++++++++++--
 s390x/css.c         | 251 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 363 insertions(+), 27 deletions(-)

-- 
2.17.1

log:

from v1:

- rework the buggy interrupt handling
  (Connie)

- identation and comments changes in "disabling subchannel"
  (Janosch)

- Bit definition naming
  (Connie)

- use get_ram_size() to get the maximal address
  (Janosch)

- better comments for SSCH (hopefully)
  (Pierre)

[kvm-unit-tests PATCH v2 1/8] 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/css.h     |  1 +
 lib/s390x/css_lib.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 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..f5c4f37 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -186,6 +186,73 @@ 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 the subchannel is status pending or if a function is in progress,
+	 * we consider both cases as errors.
+	 */
+	if (cc) {
+		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) {
+		/* the hardware was not ready, give it some time */
+		mdelay(10);
+		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 v2 2/8] 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..0058355 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_SUSPENDED	0x00000020
+#define SCSW_AC_DEVICE_ACTIVE	0x00000040
+#define SCSW_AC_SUBCH_ACTIVE	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 v2 3/8] s390x: css: simplify skipping tests on no device

[Pierre Morel]

We will lhave to test if a device is present for every tests
in the future.
Let's provide a macro to check if the device is present and
to skip the tests if it is not.

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

diff --git a/s390x/css.c b/s390x/css.c
index c340c53..16723f6 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -27,6 +27,13 @@ static int test_device_sid;
 static struct senseid *senseid;
 struct ccw1 *ccw;
 
+#define NODEV_SKIP(dev) do {						\
+				if (!(dev)) {				\
+					report_skip("No device");	\
+					return;				\
+				}					\
+			} while (0)
+
 static void test_enumerate(void)
 {
 	test_device_sid = css_enumerate();
@@ -41,10 +48,7 @@ static void test_enable(void)
 {
 	int cc;
 
-	if (!test_device_sid) {
-		report_skip("No device");
-		return;
-	}
+	NODEV_SKIP(test_device_sid);
 
 	cc = css_enable(test_device_sid, IO_SCH_ISC);
 
@@ -62,10 +66,7 @@ static void test_sense(void)
 	int ret;
 	int len;
 
-	if (!test_device_sid) {
-		report_skip("No device");
-		return;
-	}
+	NODEV_SKIP(test_device_sid);
 
 	ret = css_enable(test_device_sid, IO_SCH_ISC);
 	if (ret) {
@@ -218,10 +219,7 @@ static void test_schm_fmt0(void)
 	struct measurement_block_format0 *mb0;
 	int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
 
-	if (!test_device_sid) {
-		report_skip("No device");
-		return;
-	}
+	NODEV_SKIP(test_device_sid);
 
 	/* Allocate zeroed Measurement block */
 	mb0 = alloc_io_mem(shared_mb_size, 0);
@@ -289,10 +287,7 @@ static void test_schm_fmt1(void)
 {
 	struct measurement_block_format1 *mb1;
 
-	if (!test_device_sid) {
-		report_skip("No device");
-		return;
-	}
+	NODEV_SKIP(test_device_sid);
 
 	if (!css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
 		report_skip("Extended measurement block not available");
-- 
2.17.1

[kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Pierre Morel]

We will may want to check the result of an I/O without waiting
for an interrupt.
For example because we do not handle interrupt.
Let's separate waiting for interrupt and the I/O complretion check.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 0058355..5d1e1f0 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -317,6 +317,7 @@ int css_residual_count(unsigned int schid);
 
 void enable_io_isc(uint8_t isc);
 int wait_and_check_io_completion(int schid);
+int check_io_completion(int schid);
 
 /*
  * CHSC definitions
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index f5c4f37..1e5c409 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -487,18 +487,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
 }
 
 /* wait_and_check_io_completion:
+ * @schid: the subchannel ID
+ */
+int wait_and_check_io_completion(int schid)
+{
+	wait_for_interrupt(PSW_MASK_IO);
+	return check_io_completion(schid);
+}
+
+/* check_io_completion:
  * @schid: the subchannel ID
  *
  * Makes the most common check to validate a successful I/O
  * completion.
  * Only report failures.
  */
-int wait_and_check_io_completion(int schid)
+int check_io_completion(int schid)
 {
 	int ret = 0;
 
-	wait_for_interrupt(PSW_MASK_IO);
-
 	report_prefix_push("check I/O completion");
 
 	if (lowcore_ptr->io_int_param != schid) {
-- 
2.17.1

[kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Pierre Morel]

When checking for an I/O completion 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 function expected after executing
an instruction or sequence of instructions and if all ctrl flags
of the SCSW are set as expected.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 5d1e1f0..1603781 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -316,8 +316,8 @@ 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 check_io_completion(int schid);
+int wait_and_check_io_completion(int schid, uint32_t ctrl);
+int check_io_completion(int schid, uint32_t ctrl);
 
 /*
  * CHSC definitions
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 1e5c409..55e70e6 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
 
 /* wait_and_check_io_completion:
  * @schid: the subchannel ID
+ * @ctrl : expected SCSW control flags
  */
-int wait_and_check_io_completion(int schid)
+int wait_and_check_io_completion(int schid, uint32_t ctrl)
 {
 	wait_for_interrupt(PSW_MASK_IO);
-	return check_io_completion(schid);
+	return check_io_completion(schid, ctrl);
 }
 
 /* check_io_completion:
  * @schid: the subchannel ID
+ * @ctrl : expected SCSW control flags
  *
- * Makes the most common check to validate a successful I/O
- * completion.
+ * If the ctrl parameter is not null check the IRB SCSW ctrl
+ * against the ctrl parameter.
+ * Otherwise, makes the most common check to validate a successful
+ * I/O completion.
  * Only report failures.
  */
-int check_io_completion(int schid)
+int check_io_completion(int schid, uint32_t ctrl)
 {
 	int ret = 0;
 
@@ -515,6 +519,13 @@ int check_io_completion(int schid)
 		goto end;
 	}
 
+	if (ctrl && (ctrl ^ irb.scsw.ctrl)) {
+		report_info("%08x : %s", schid, dump_scsw_flags(irb.scsw.ctrl));
+		report_info("expected : %s", dump_scsw_flags(ctrl));
+		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",
diff --git a/s390x/css.c b/s390x/css.c
index 16723f6..57dc340 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -95,7 +95,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, 0) < 0)
 		goto error;
 
 	/* Test transfer completion */
@@ -138,7 +138,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, 0) >= 0);
 }
 
 static void css_init(void)
-- 
2.17.1

[kvm-unit-tests PATCH v2 6/8] 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>
---
 lib/s390x/css.h     |   4 ++
 lib/s390x/css_lib.c |   5 +--
 s390x/css.c         | 105 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 1603781..e1e9264 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -90,6 +90,9 @@ struct scsw {
 #define SCSW_ESW_FORMAT		0x04000000
 #define SCSW_SUSPEND_CTRL	0x08000000
 #define SCSW_KEY		0xf0000000
+#define SCSW_SSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_START | \
+				 SCSW_SC_PENDING | SCSW_SC_SECONDARY | \
+				 SCSW_SC_PRIMARY)
 	uint32_t ctrl;
 	uint32_t ccw_addr;
 #define SCSW_DEVS_DEV_END	0x04
@@ -138,6 +141,7 @@ struct irb {
 	uint32_t ecw[8];
 	uint32_t emw[8];
 } __attribute__ ((aligned(4)));
+extern struct irb irb;
 
 #define CCW_CMD_SENSE_ID	0xe4
 #define CSS_SENSEID_COMMON_LEN	8
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 55e70e6..7c93e94 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -21,6 +21,7 @@
 
 struct schib schib;
 struct chsc_scsc *chsc_scsc;
+struct irb irb;
 
 static const char * const chsc_rsp_description[] = {
 	"CHSC unknown error",
@@ -415,8 +416,6 @@ bool css_disable_mb(int schid)
 	return retry_count > 0;
 }
 
-static struct irb irb;
-
 void css_irq_io(void)
 {
 	int ret = 0;
@@ -512,7 +511,7 @@ int check_io_completion(int schid, uint32_t ctrl)
 
 	report_prefix_push("check I/O completion");
 
-	if (lowcore_ptr->io_int_param != schid) {
+	if (!ctrl && lowcore_ptr->io_int_param != schid) {
 		report(0, "interrupt parameter: expected %08x got %08x",
 		       schid, lowcore_ptr->io_int_param);
 		ret = -1;
diff --git a/s390x/css.c b/s390x/css.c
index 57dc340..f6890f2 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -15,6 +15,7 @@
 #include <interrupt.h>
 #include <asm/arch_def.h>
 #include <alloc_page.h>
+#include <sclp.h>
 
 #include <malloc_io.h>
 #include <css.h>
@@ -55,6 +56,109 @@ 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 top;
+
+	NODEV_SKIP(test_device_sid);
+
+	assert(css_enable(test_device_sid, 0) == 0);
+
+	/* 1- 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();
+
+	/* 2- 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();
+
+	/* 3- ORB address should be available we check 1G*/
+	top = get_ram_size();
+	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();
+
+	/* 3- ORB address should not be equal or above 2G */
+	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);
+
+	/* 4- Start on a disabled subchannel */
+	report_prefix_push("Disabled subchannel");
+	assert(css_disable(test_device_sid) == 0);
+	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
+	report_prefix_pop();
+
+	/* 5- MIDAW is not supported by the firmware */
+	report_prefix_push("ORB MIDAW unsupported");
+	assert(css_enable(test_device_sid, 0) == 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;
+
+	/* 6-12- Check the reserved bits of the ORB CTRL field */
+	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();
+	}
+
+	/* 13- check the reserved bits of the ORB flags */
+	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();
+
+	/* 14- Check sending a second SSCH before clearing the status.  */
+	orb.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT;
+	report_prefix_push("SSCH on channel with status pending");
+	assert(css_enable(test_device_sid, 0) == 0);
+	assert(ssch(test_device_sid, &orb) == 0);
+	report(ssch(test_device_sid, &orb) == 1, "CC = 1");
+	/* now we clear the status */
+	assert(tsch(test_device_sid, &irb) == 0);
+	assert(check_io_completion(test_device_sid, SCSW_SSCH_COMPLETED) == 0);
+	assert(css_disable(test_device_sid) == 0);
+	report_prefix_pop();
+}
+
 /*
  * test_sense
  * Pre-requisites:
@@ -334,6 +438,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 v2 7/8] s390x: css: testing halt subchannel

[Pierre Morel]

checking return values for HSCH for various configurations.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index e1e9264..cf48abc 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -93,6 +93,10 @@ struct scsw {
 #define SCSW_SSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_START | \
 				 SCSW_SC_PENDING | SCSW_SC_SECONDARY | \
 				 SCSW_SC_PRIMARY)
+#define SCSW_HSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_HALT | \
+				 SCSW_SC_PENDING)
+#define SCSW_CSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_CLEAR | \
+				 SCSW_SC_PENDING)
 	uint32_t ctrl;
 	uint32_t ccw_addr;
 #define SCSW_DEVS_DEV_END	0x04
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 7c93e94..d9ed2c3 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -533,6 +533,10 @@ int check_io_completion(int schid, uint32_t ctrl)
 		goto end;
 	}
 
+	/* We do not need more check for HSCH or CSCH */
+	if (irb.scsw.ctrl & (SCSW_FC_HALT | SCSW_FC_CLEAR))
+		goto end;
+
 	if (!(irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
 		report(0, "Primary or secondary status missing. Subch Ctrl: %08x",
 		       irb.scsw.ctrl);
diff --git a/s390x/css.c b/s390x/css.c
index f6890f2..ffc067e 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -56,6 +56,62 @@ static void test_enable(void)
 	report(cc == 0, "Enable subchannel %08x", test_device_sid);
 }
 
+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;
+
+	NODEV_SKIP(test_device_sid);
+
+	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, 0) == 0);
+	report(hsch(test_device_sid) == 0, "subchannel halted");
+	report_prefix_pop();
+
+	/* now we check the flags */
+	report_prefix_push("Ctrl flags");
+	assert(tsch(test_device_sid, &irb) == 0);
+	report(check_io_completion(test_device_sid, SCSW_HSCH_COMPLETED) == 0, "expected");
+	report_prefix_pop();
+
+	/* 2- Check HSCH after SSCH */
+	report_prefix_push("HSCH on status pending subchannel");
+	assert(ssch(test_device_sid, &orb) == 0);
+	report(hsch(test_device_sid) == 1, "Halt subchannel should fail with CC 1");
+	assert(tsch(test_device_sid, &irb) == 0);
+	check_io_completion(test_device_sid, SCSW_SSCH_COMPLETED);
+	report_prefix_pop();
+
+	/* 3- Check HSCH after CSCH */
+	report_prefix_push("HSCH on busy on CSCH subchannel");
+	assert(csch(test_device_sid) == 0);
+	report(hsch(test_device_sid) == 1, "Halt subchannel should fail with CC 1");
+	assert(tsch(test_device_sid, &irb) == 0);
+	check_io_completion(test_device_sid, SCSW_CSCH_COMPLETED);
+	report_prefix_pop();
+
+	/* 4- Check HSCH after HSCH */
+	report_prefix_push("HSCH on busy on HSCH subchannel");
+	assert(hsch(test_device_sid) == 0);
+	report(hsch(test_device_sid) == 1, "Halt subchannel should fail with CC 1");
+	assert(tsch(test_device_sid, &irb) == 0);
+	check_io_completion(test_device_sid, SCSW_HSCH_COMPLETED);
+	report_prefix_pop();
+
+	free_io_mem(senseid, sizeof(*senseid));
+	free_io_mem(ccw, sizeof(*ccw));
+}
+
 static void test_ssch(void)
 {
 	struct orb orb = {
@@ -439,6 +495,7 @@ static struct {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "start subchannel", test_ssch },
+	{ "halt subchannel", test_hsch },
 	{ "sense (ssch/tsch)", test_sense },
 	{ "measurement block (schm)", test_schm },
 	{ "measurement block format0", test_schm_fmt0 },
-- 
2.17.1

[kvm-unit-tests PATCH v2 8/8] s390x: css: testing clear subchannel

[Pierre Morel]

Checking return values for CSCH for various configurations.

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

diff --git a/s390x/css.c b/s390x/css.c
index ffc067e..d5b7b00 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -56,6 +56,63 @@ static void test_enable(void)
 	report(cc == 0, "Enable subchannel %08x", test_device_sid);
 }
 
+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;
+
+	NODEV_SKIP(test_device_sid);
+
+	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, 0) == 0);
+	report(csch(test_device_sid) == 0, "subchannel clear");
+	report_prefix_pop();
+
+	/* now we check the flags */
+	report_prefix_push("IRQ flags");
+	assert(tsch(test_device_sid, &irb) == 0);
+	report(check_io_completion(test_device_sid, SCSW_CSCH_COMPLETED) == 0, "expected");
+	report_prefix_pop();
+
+	/* 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(ssch(test_device_sid, &orb) == 0);
+	report(csch(test_device_sid) == 0, "subchannel cleared");
+	assert(tsch(test_device_sid, &irb) == 0);
+	check_io_completion(test_device_sid, SCSW_CSCH_COMPLETED |
+			    SCSW_SC_SECONDARY | SCSW_SC_PRIMARY);
+	report_prefix_pop();
+
+	/* 3- Checking CSCH after HSCH */
+	report_prefix_push("CSCH on a halted subchannel");
+	assert(hsch(test_device_sid) == 0);
+	report(csch(test_device_sid) == 0, "subchannel cleared");
+	assert(tsch(test_device_sid, &irb) == 0);
+	check_io_completion(test_device_sid, SCSW_CSCH_COMPLETED);
+	report_prefix_pop();
+
+	/* 4- Checking CSCH after CSCH */
+	report_prefix_push("CSCH on a cleared subchannel");
+	assert(csch(test_device_sid) == 0);
+	report(csch(test_device_sid) == 0, "subchannel cleared");
+	assert(tsch(test_device_sid, &irb) == 0);
+	check_io_completion(test_device_sid, SCSW_CSCH_COMPLETED);
+	report_prefix_pop();
+
+	free_io_mem(senseid, sizeof(*senseid));
+	free_io_mem(ccw, sizeof(*ccw));
+}
+
 static void test_hsch(void)
 {
 	struct orb orb = {
@@ -496,6 +553,7 @@ static struct {
 	{ "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 v2 1/8] s390x: lib: css: disabling a subchannel

[Claudio Imbrenda]

On Thu, 25 Mar 2021 10:39:00 +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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68
> 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..f5c4f37 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -186,6 +186,73 @@ 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);

KVM unit tests allow up to 120 columns per line, please use them, the
code will be more readable; this applies for all broken lines, not just
in this patch.

> +		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 the subchannel is status pending or if a function is
> in progress,
> +	 * we consider both cases as errors.
> +	 */
> +	if (cc) {
> +		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) {
> +		/* the hardware was not ready, give it some time */
> +		mdelay(10);
> +		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 v2 2/8] s390x: lib: css: SCSW bit definitions

[Claudio Imbrenda]

On Thu, 25 Mar 2021 10:39:01 +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>

Reviewed-by: Claudio Imbrenda <imbrenda@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..0058355 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_SUSPENDED	0x00000020
> +#define SCSW_AC_DEVICE_ACTIVE	0x00000040
> +#define SCSW_AC_SUBCH_ACTIVE	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

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Claudio Imbrenda]

On Thu, 25 Mar 2021 10:39:02 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We will lhave to test if a device is present for every tests
> in the future.
> Let's provide a macro to check if the device is present and
> to skip the tests if it is not.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/css.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index c340c53..16723f6 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -27,6 +27,13 @@ static int test_device_sid;
>  static struct senseid *senseid;
>  struct ccw1 *ccw;
>  
> +#define NODEV_SKIP(dev) do {
> 	\
> +				if (!(dev)) {
> 	\
> +					report_skip("No
> device");	\
> +					return;
> 		\
> +				}
> 	\
> +			} while (0)

I wonder if you can add for example which device is not present (might
help with debugging)

in any case:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

>  static void test_enumerate(void)
>  {
>  	test_device_sid = css_enumerate();
> @@ -41,10 +48,7 @@ static void test_enable(void)
>  {
>  	int cc;
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	cc = css_enable(test_device_sid, IO_SCH_ISC);
>  
> @@ -62,10 +66,7 @@ static void test_sense(void)
>  	int ret;
>  	int len;
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	ret = css_enable(test_device_sid, IO_SCH_ISC);
>  	if (ret) {
> @@ -218,10 +219,7 @@ static void test_schm_fmt0(void)
>  	struct measurement_block_format0 *mb0;
>  	int shared_mb_size = 2 * sizeof(struct
> measurement_block_format0); 
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	/* Allocate zeroed Measurement block */
>  	mb0 = alloc_io_mem(shared_mb_size, 0);
> @@ -289,10 +287,7 @@ static void test_schm_fmt1(void)
>  {
>  	struct measurement_block_format1 *mb1;
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	if
> (!css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
> report_skip("Extended measurement block not available");

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Claudio Imbrenda]

On Thu, 25 Mar 2021 10:39:03 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We will may want to check the result of an I/O without waiting
> for an interrupt.
> For example because we do not handle interrupt.
> Let's separate waiting for interrupt and the I/O complretion check.
                                                   ^^^^^^^^^^^
                                                   completion

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 0058355..5d1e1f0 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -317,6 +317,7 @@ int css_residual_count(unsigned int schid);
>  
>  void enable_io_isc(uint8_t isc);
>  int wait_and_check_io_completion(int schid);
> +int check_io_completion(int schid);
>  
>  /*
>   * CHSC definitions
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index f5c4f37..1e5c409 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -487,18 +487,25 @@ struct ccw1 *ccw_alloc(int code, void *data,
> int count, unsigned char flags) }
>  
>  /* wait_and_check_io_completion:
> + * @schid: the subchannel ID
> + */
> +int wait_and_check_io_completion(int schid)
> +{
> +	wait_for_interrupt(PSW_MASK_IO);
> +	return check_io_completion(schid);
> +}
> +
> +/* check_io_completion:
>   * @schid: the subchannel ID
>   *
>   * Makes the most common check to validate a successful I/O
>   * completion.
>   * Only report failures.
>   */
> -int wait_and_check_io_completion(int schid)
> +int check_io_completion(int schid)
>  {
>  	int ret = 0;
>  
> -	wait_for_interrupt(PSW_MASK_IO);
> -
>  	report_prefix_push("check I/O completion");
>  
>  	if (lowcore_ptr->io_int_param != schid) {

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Claudio Imbrenda]

On Thu, 25 Mar 2021 10:39:04 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> When checking for an I/O completion 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 function expected after executing
> an instruction or sequence of instructions and if all ctrl flags
> of the SCSW are set as expected.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  4 ++--
>  lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>  s390x/css.c         |  4 ++--
>  3 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 5d1e1f0..1603781 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -316,8 +316,8 @@ 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 check_io_completion(int schid);
> +int wait_and_check_io_completion(int schid, uint32_t ctrl);
> +int check_io_completion(int schid, uint32_t ctrl);
>  
>  /*
>   * CHSC definitions
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 1e5c409..55e70e6 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data,
> int count, unsigned char flags) 
>  /* wait_and_check_io_completion:
>   * @schid: the subchannel ID
> + * @ctrl : expected SCSW control flags
>   */
> -int wait_and_check_io_completion(int schid)
> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>  {
>  	wait_for_interrupt(PSW_MASK_IO);
> -	return check_io_completion(schid);
> +	return check_io_completion(schid, ctrl);
>  }
>  
>  /* check_io_completion:
>   * @schid: the subchannel ID
> + * @ctrl : expected SCSW control flags
>   *
> - * Makes the most common check to validate a successful I/O
> - * completion.
> + * If the ctrl parameter is not null check the IRB SCSW ctrl

I would say "not zero" instead of null, since it's an integer and not a
pointer.

with that fixed:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> + * against the ctrl parameter.
> + * Otherwise, makes the most common check to validate a successful
> + * I/O completion.
>   * Only report failures.
>   */
> -int check_io_completion(int schid)
> +int check_io_completion(int schid, uint32_t ctrl)
>  {
>  	int ret = 0;
>  
> @@ -515,6 +519,13 @@ int check_io_completion(int schid)
>  		goto end;
>  	}
>  
> +	if (ctrl && (ctrl ^ irb.scsw.ctrl)) {
> +		report_info("%08x : %s", schid,
> dump_scsw_flags(irb.scsw.ctrl));
> +		report_info("expected : %s", dump_scsw_flags(ctrl));
> +		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", diff --git a/s390x/css.c b/s390x/css.c
> index 16723f6..57dc340 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -95,7 +95,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, 0) < 0)
>  		goto error;
>  
>  	/* Test transfer completion */
> @@ -138,7 +138,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, 0) >=
> 0); }
>  
>  static void css_init(void)

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

[Claudio Imbrenda]

On Thu, 25 Mar 2021 10:39:05 +0100
Pierre Morel <pmorel@linux.ibm.com> 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>
> ---
>  lib/s390x/css.h     |   4 ++
>  lib/s390x/css_lib.c |   5 +--
>  s390x/css.c         | 105
> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111
> insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 1603781..e1e9264 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -90,6 +90,9 @@ struct scsw {
>  #define SCSW_ESW_FORMAT		0x04000000
>  #define SCSW_SUSPEND_CTRL	0x08000000
>  #define SCSW_KEY		0xf0000000
> +#define SCSW_SSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_START
> | \
> +				 SCSW_SC_PENDING | SCSW_SC_SECONDARY
> | \
> +				 SCSW_SC_PRIMARY)
>  	uint32_t ctrl;
>  	uint32_t ccw_addr;
>  #define SCSW_DEVS_DEV_END	0x04
> @@ -138,6 +141,7 @@ struct irb {
>  	uint32_t ecw[8];
>  	uint32_t emw[8];
>  } __attribute__ ((aligned(4)));
> +extern struct irb irb;
>  
>  #define CCW_CMD_SENSE_ID	0xe4
>  #define CSS_SENSEID_COMMON_LEN	8
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 55e70e6..7c93e94 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -21,6 +21,7 @@
>  
>  struct schib schib;
>  struct chsc_scsc *chsc_scsc;
> +struct irb irb;
>  
>  static const char * const chsc_rsp_description[] = {
>  	"CHSC unknown error",
> @@ -415,8 +416,6 @@ bool css_disable_mb(int schid)
>  	return retry_count > 0;
>  }
>  
> -static struct irb irb;
> -
>  void css_irq_io(void)
>  {
>  	int ret = 0;
> @@ -512,7 +511,7 @@ int check_io_completion(int schid, uint32_t ctrl)
>  
>  	report_prefix_push("check I/O completion");
>  
> -	if (lowcore_ptr->io_int_param != schid) {
> +	if (!ctrl && lowcore_ptr->io_int_param != schid) {
>  		report(0, "interrupt parameter: expected %08x got
> %08x", schid, lowcore_ptr->io_int_param);
>  		ret = -1;
> diff --git a/s390x/css.c b/s390x/css.c
> index 57dc340..f6890f2 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -15,6 +15,7 @@
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
>  #include <alloc_page.h>
> +#include <sclp.h>
>  
>  #include <malloc_io.h>
>  #include <css.h>
> @@ -55,6 +56,109 @@ 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 top;
> +
> +	NODEV_SKIP(test_device_sid);
> +
> +	assert(css_enable(test_device_sid, 0) == 0);
> +
> +	/* 1- ORB address should be aligned on 32 bits */
> +	report_prefix_push("ORB alignment");
> +	expect_pgm_int();
> +	ssch(test_device_sid, (void *)0x110002);

I don't like using random hardcoded addresses. can you use a valid
address for it? either allocate it or use a static buffer.

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	/* 2- ORB address should be lower than 2G */
> +	report_prefix_push("ORB Address above 2G");
> +	expect_pgm_int();
> +	ssch(test_device_sid, (void *)0x80000000);

another hardcoded address... you should try allocating memory over 2G,
and try to use it. put a check if there is enough memory, and skip if
you do not have enough memory, like you did below

> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
> +	report_prefix_pop();
> +
> +	/* 3- ORB address should be available we check 1G*/
> +	top = get_ram_size();
> +	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");

this is what I meant above. you will need to run this test both with 1G
and with 3G of ram (look at the SCLP test, it has the same issue)

> +	}
> +	report_prefix_pop();
> +
> +	/* 3- ORB address should not be equal or above 2G */

the comment seems the same as the one above, maybe clarify that here
you mean the address inside the ORB, instead of the address of the ORB
itself

> +	report_prefix_push("CCW address above 2G");
> +	orb.cpa = 0x80000000;

and again, please no hardcoded values; put a check that enough memory
is available, and allocate from >2GB

> +	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);
> +
> +	/* 4- Start on a disabled subchannel */
> +	report_prefix_push("Disabled subchannel");
> +	assert(css_disable(test_device_sid) == 0);
> +	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
> +	report_prefix_pop();
> +
> +	/* 5- MIDAW is not supported by the firmware */
> +	report_prefix_push("ORB MIDAW unsupported");
> +	assert(css_enable(test_device_sid, 0) == 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;
> +
> +	/* 6-12- Check the reserved bits of the ORB CTRL field */
> +	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();
> +	}
> +
> +	/* 13- check the reserved bits of the ORB flags */
> +	report_prefix_push("ORB wrong ctrl flags");
> +	orb.ctrl |= 0x040000;

do you need the magic constant, or can you define a name for it? (or is
even a name already defined?)

> +	expect_pgm_int();
> +	ssch(test_device_sid, &orb);
> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +	report_prefix_pop();
> +
> +	/* 14- Check sending a second SSCH before clearing the
> status.  */
> +	orb.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT;
> +	report_prefix_push("SSCH on channel with status pending");
> +	assert(css_enable(test_device_sid, 0) == 0);
> +	assert(ssch(test_device_sid, &orb) == 0);
> +	report(ssch(test_device_sid, &orb) == 1, "CC = 1");
> +	/* now we clear the status */
> +	assert(tsch(test_device_sid, &irb) == 0);
> +	assert(check_io_completion(test_device_sid,
> SCSW_SSCH_COMPLETED) == 0);
> +	assert(css_disable(test_device_sid) == 0);
> +	report_prefix_pop();
> +}
> +
>  /*
>   * test_sense
>   * Pre-requisites:
> @@ -334,6 +438,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 v2 1/8] s390x: lib: css: disabling a subchannel

[Pierre Morel]

On 3/25/21 3:52 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:00 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Some tests require to disable a subchannel.
>> Let's implement the css_disable() function.
>>
...snip...
>> +	/* Read the SCHIB for this subchannel */
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: sch %08x failed with cc=%d",
>> schid, cc);
> 
> KVM unit tests allow up to 120 columns per line, please use them, the
> code will be more readable; this applies for all broken lines, not just
> in this patch.

Oh someone did not read the README!
thanks.


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/25/21 4:00 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:01 +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>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
>> ---

Thanks,
Pierre


> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Pierre Morel]

On 3/25/21 4:21 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:02 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We will lhave to test if a device is present for every tests
>> in the future.
>> Let's provide a macro to check if the device is present and
>> to skip the tests if it is not.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index c340c53..16723f6 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -27,6 +27,13 @@ static int test_device_sid;
>>   static struct senseid *senseid;
>>   struct ccw1 *ccw;
>>   
>> +#define NODEV_SKIP(dev) do {
>> 	\
>> +				if (!(dev)) {
>> 	\
>> +					report_skip("No
>> device");	\
>> +					return;
>> 		\
>> +				}
>> 	\
>> +			} while (0)
> 
> I wonder if you can add for example which device is not present (might
> help with debugging)

potentially any CSS device would be OK for most of the tests.
For simplicity we use virtio-net-ccw because it does not require any 
argument for allowing us to sense it.

When we need a more specific device I will add information.

> 
> in any case:
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Pierre Morel]

On 3/25/21 4:24 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:03 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We will may want to check the result of an I/O without waiting
>> for an interrupt.
>> For example because we do not handle interrupt.
>> Let's separate waiting for interrupt and the I/O complretion check.
>                                                     ^^^^^^^^^^^
>                                                     completion

completed :)

> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/25/21 5:02 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:05 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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>
>> ---
>>   lib/s390x/css.h     |   4 ++
>>   lib/s390x/css_lib.c |   5 +--
>>   s390x/css.c         | 105
>> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111
>> insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 1603781..e1e9264 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -90,6 +90,9 @@ struct scsw {
>>   #define SCSW_ESW_FORMAT		0x04000000
>>   #define SCSW_SUSPEND_CTRL	0x08000000
>>   #define SCSW_KEY		0xf0000000
>> +#define SCSW_SSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_START
>> | \
>> +				 SCSW_SC_PENDING | SCSW_SC_SECONDARY
>> | \
>> +				 SCSW_SC_PRIMARY)
>>   	uint32_t ctrl;
>>   	uint32_t ccw_addr;
>>   #define SCSW_DEVS_DEV_END	0x04
>> @@ -138,6 +141,7 @@ struct irb {
>>   	uint32_t ecw[8];
>>   	uint32_t emw[8];
>>   } __attribute__ ((aligned(4)));
>> +extern struct irb irb;
>>   
>>   #define CCW_CMD_SENSE_ID	0xe4
>>   #define CSS_SENSEID_COMMON_LEN	8
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 55e70e6..7c93e94 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -21,6 +21,7 @@
>>   
>>   struct schib schib;
>>   struct chsc_scsc *chsc_scsc;
>> +struct irb irb;
>>   
>>   static const char * const chsc_rsp_description[] = {
>>   	"CHSC unknown error",
>> @@ -415,8 +416,6 @@ bool css_disable_mb(int schid)
>>   	return retry_count > 0;
>>   }
>>   
>> -static struct irb irb;
>> -
>>   void css_irq_io(void)
>>   {
>>   	int ret = 0;
>> @@ -512,7 +511,7 @@ int check_io_completion(int schid, uint32_t ctrl)
>>   
>>   	report_prefix_push("check I/O completion");
>>   
>> -	if (lowcore_ptr->io_int_param != schid) {
>> +	if (!ctrl && lowcore_ptr->io_int_param != schid) {
>>   		report(0, "interrupt parameter: expected %08x got
>> %08x", schid, lowcore_ptr->io_int_param);
>>   		ret = -1;
>> diff --git a/s390x/css.c b/s390x/css.c
>> index 57dc340..f6890f2 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -15,6 +15,7 @@
>>   #include <interrupt.h>
>>   #include <asm/arch_def.h>
>>   #include <alloc_page.h>
>> +#include <sclp.h>
>>   
>>   #include <malloc_io.h>
>>   #include <css.h>
>> @@ -55,6 +56,109 @@ 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 top;
>> +
>> +	NODEV_SKIP(test_device_sid);
>> +
>> +	assert(css_enable(test_device_sid, 0) == 0);
>> +
>> +	/* 1- ORB address should be aligned on 32 bits */
>> +	report_prefix_push("ORB alignment");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x110002);
> 
> I don't like using random hardcoded addresses. can you use a valid
> address for it? either allocate it or use a static buffer.

OK, I can make it differently.

> 
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	/* 2- ORB address should be lower than 2G */
>> +	report_prefix_push("ORB Address above 2G");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x80000000);
> 
> another hardcoded address... you should try allocating memory over 2G,
> and try to use it. put a check if there is enough memory, and skip if
> you do not have enough memory, like you did below

OK, you are right, here I can not know if the memory is available at 
this address.

> 
>> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>> +	report_prefix_pop();
>> +
>> +	/* 3- ORB address should be available we check 1G*/
>> +	top = get_ram_size();
>> +	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");
> 
> this is what I meant above. you will need to run this test both with 1G
> and with 3G of ram (look at the SCLP test, it has the same issue)

OK thanks, will take example from it.

> 
>> +	}
>> +	report_prefix_pop();
>> +
>> +	/* 3- ORB address should not be equal or above 2G */
> 
> the comment seems the same as the one above, maybe clarify that here
> you mean the address inside the ORB, instead of the address of the ORB
> itself

cut and past and forgot to adapt: here I check the CCW address not the 
ORB address

> 
>> +	report_prefix_push("CCW address above 2G");
>> +	orb.cpa = 0x80000000;
> 
> and again, please no hardcoded values; put a check that enough memory
> is available, and allocate from >2GB

and again.. yes

> 
>> +	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);
>> +
>> +	/* 4- Start on a disabled subchannel */
>> +	report_prefix_push("Disabled subchannel");
>> +	assert(css_disable(test_device_sid) == 0);
>> +	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
>> +	report_prefix_pop();
>> +
>> +	/* 5- MIDAW is not supported by the firmware */
>> +	report_prefix_push("ORB MIDAW unsupported");
>> +	assert(css_enable(test_device_sid, 0) == 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;
>> +
>> +	/* 6-12- Check the reserved bits of the ORB CTRL field */
>> +	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();
>> +	}
>> +
>> +	/* 13- check the reserved bits of the ORB flags */
>> +	report_prefix_push("ORB wrong ctrl flags");
>> +	orb.ctrl |= 0x040000;
> 
> do you need the magic constant, or can you define a name for it? (or is
> even a name already defined?)

I can define this

...snip...

Thanks for the comments,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Janosch Frank]

On 3/25/21 10:39 AM, 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 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..f5c4f37 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -186,6 +186,73 @@ 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 the subchannel is status pending or if a function is in progress,
> +	 * we consider both cases as errors.
> +	 */
> +	if (cc) {
> +		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) {
> +		/* the hardware was not ready, give it some time */
> +		mdelay(10);
> +		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 v2 3/8] s390x: css: simplify skipping tests on no device

[Janosch Frank]

On 3/25/21 10:39 AM, Pierre Morel wrote:
> We will lhave to test if a device is present for every tests

s/lhave/have/

> in the future.
> Let's provide a macro to check if the device is present and
> to skip the tests if it is not.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

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

> ---
>  s390x/css.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index c340c53..16723f6 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -27,6 +27,13 @@ static int test_device_sid;
>  static struct senseid *senseid;
>  struct ccw1 *ccw;
>  
> +#define NODEV_SKIP(dev) do {						\

s/device/schid/ ?
I have no strong opinions either way so choose what you like best.

Also, since you use report(0, "") so often, maybe you want to introduce
report_fail() into the library in the future? The x86 vmx tests also use
report(0, "") a lot so you're not completely alone.

Could you please move the "do {" one line down so we start with a zero
indent?

> +				if (!(dev)) {				\
> +					report_skip("No device");	\
> +					return;				\
> +				}					\
> +			} while (0)
> +
>  static void test_enumerate(void)
>  {
>  	test_device_sid = css_enumerate();
> @@ -41,10 +48,7 @@ static void test_enable(void)
>  {
>  	int cc;
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	cc = css_enable(test_device_sid, IO_SCH_ISC);
>  
> @@ -62,10 +66,7 @@ static void test_sense(void)
>  	int ret;
>  	int len;
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	ret = css_enable(test_device_sid, IO_SCH_ISC);
>  	if (ret) {
> @@ -218,10 +219,7 @@ static void test_schm_fmt0(void)
>  	struct measurement_block_format0 *mb0;
>  	int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	/* Allocate zeroed Measurement block */
>  	mb0 = alloc_io_mem(shared_mb_size, 0);
> @@ -289,10 +287,7 @@ static void test_schm_fmt1(void)
>  {
>  	struct measurement_block_format1 *mb1;
>  
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>  
>  	if (!css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
>  		report_skip("Extended measurement block not available");
> 

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

[Pierre Morel]

On 3/26/21 9:26 AM, Janosch Frank wrote:
> On 3/25/21 10:39 AM, 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>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>

thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Pierre Morel]

On 3/26/21 9:41 AM, Janosch Frank wrote:
> On 3/25/21 10:39 AM, Pierre Morel wrote:
>> We will lhave to test if a device is present for every tests
> 
> s/lhave/have/

yes

> 
>> in the future.
>> Let's provide a macro to check if the device is present and
>> to skip the tests if it is not.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Thanks

> 
>> ---
>>   s390x/css.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index c340c53..16723f6 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -27,6 +27,13 @@ static int test_device_sid;
>>   static struct senseid *senseid;
>>   struct ccw1 *ccw;
>>   
>> +#define NODEV_SKIP(dev) do {						\
> 
> s/device/schid/ ?
> I have no strong opinions either way so choose what you like best.

can do it.

> 
> Also, since you use report(0, "") so often, maybe you want to introduce
> report_fail() into the library in the future? The x86 vmx tests also use
> report(0, "") a lot so you're not completely alone.
> 
> Could you please move the "do {" one line down so we start with a zero
> indent?

OK


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/25/21 5:02 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:05 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 

...snip...


Trying to follow your comment, I have some questions:


>> +	/* 2- ORB address should be lower than 2G */
>> +	report_prefix_push("ORB Address above 2G");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x80000000);
> 
> another hardcoded address... you should try allocating memory over 2G,
> and try to use it. put a check if there is enough memory, and skip if
> you do not have enough memory, like you did below

How can I allocate memory above 2G?

> 
>> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>> +	report_prefix_pop();
>> +
>> +	/* 3- ORB address should be available we check 1G*/
>> +	top = get_ram_size();
>> +	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");
> 
> this is what I meant above. you will need to run this test both with 1G
> and with 3G of ram (look at the SCLP test, it has the same issue)

I do not understand, if I test with 3G RAM, I suppose that the framework 
works right and I have my 3G RAM available.
Then I can check with an address under 1G and recheck with an address 
above 1G.

What is the purpose to check with only 1G memory?


Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Claudio Imbrenda]

On Fri, 26 Mar 2021 11:41:34 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 3/25/21 5:02 PM, Claudio Imbrenda wrote:
> > On Thu, 25 Mar 2021 10:39:05 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> 
> ...snip...
> 
> 
> Trying to follow your comment, I have some questions:
> 
> 
> >> +	/* 2- ORB address should be lower than 2G */
> >> +	report_prefix_push("ORB Address above 2G");
> >> +	expect_pgm_int();
> >> +	ssch(test_device_sid, (void *)0x80000000);  
> > 
> > another hardcoded address... you should try allocating memory over
> > 2G, and try to use it. put a check if there is enough memory, and
> > skip if you do not have enough memory, like you did below  
> 
> How can I allocate memory above 2G?

alloc_pages_flags(order, AREA_NORMAL)

btw that allocation will fail if there is no free memory available
above 2G

> >   
> >> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
> >> +	report_prefix_pop();
> >> +
> >> +	/* 3- ORB address should be available we check 1G*/
> >> +	top = get_ram_size();
> >> +	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");  
> > 
> > this is what I meant above. you will need to run this test both
> > with 1G and with 3G of ram (look at the SCLP test, it has the same
> > issue)  
> 
> I do not understand, if I test with 3G RAM, I suppose that the
> framework works right and I have my 3G RAM available.
> Then I can check with an address under 1G and recheck with an address 
> above 1G.
> 
> What is the purpose to check with only 1G memory?

you need to run this test twice, once with 1G and once with 3G.
it's the same test, so it can't know if it is being run with 1G or
3G, so you have to test for it.

when you need a valid address above 2G, you need to make sure you have
that much memory, and when you want an invalid address between 1G and
2G, you have to make sure you have no more than 1G.

> 
> Regards,
> Pierre
> 

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

[Pierre Morel]

On 3/26/21 11:58 AM, Claudio Imbrenda wrote:
> On Fri, 26 Mar 2021 11:41:34 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 3/25/21 5:02 PM, Claudio Imbrenda wrote:
>>> On Thu, 25 Mar 2021 10:39:05 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>
>> ...snip...
>>
>>
>> Trying to follow your comment, I have some questions:
>>
>>
>>>> +	/* 2- ORB address should be lower than 2G */
>>>> +	report_prefix_push("ORB Address above 2G");
>>>> +	expect_pgm_int();
>>>> +	ssch(test_device_sid, (void *)0x80000000);
>>>
>>> another hardcoded address... you should try allocating memory over
>>> 2G, and try to use it. put a check if there is enough memory, and
>>> skip if you do not have enough memory, like you did below
>>
>> How can I allocate memory above 2G?
> 
> alloc_pages_flags(order, AREA_NORMAL)
> 
> btw that allocation will fail if there is no free memory available
> above 2G
> 
>>>    
>>>> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>>>> +	report_prefix_pop();
>>>> +
>>>> +	/* 3- ORB address should be available we check 1G*/
>>>> +	top = get_ram_size();
>>>> +	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");
>>>
>>> this is what I meant above. you will need to run this test both
>>> with 1G and with 3G of ram (look at the SCLP test, it has the same
>>> issue)
>>
>> I do not understand, if I test with 3G RAM, I suppose that the
>> framework works right and I have my 3G RAM available.
>> Then I can check with an address under 1G and recheck with an address
>> above 1G.
>>
>> What is the purpose to check with only 1G memory?
> 
> you need to run this test twice, once with 1G and once with 3G.
> it's the same test, so it can't know if it is being run with 1G or
> 3G, so you have to test for it.
> 
> when you need a valid address above 2G, you need to make sure you have
> that much memory, and when you want an invalid address between 1G and
> 2G, you have to make sure you have no more than 1G.

OK, thanks




-- 
Pierre Morel
IBM Lab Boeblingen

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

[Thomas Huth]

On 25/03/2021 10.39, 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   lib/s390x/css.h     |  1 +
>   lib/s390x/css_lib.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 68 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..f5c4f37 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -186,6 +186,73 @@ 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:

I have to saythat I really dislike writing loops with gotos if it can be 
avoided easily. What about:

for (retry_count = 0; retry_count < MAX_ENABLE_RETRIES; retry_count++) ?

(and maybe rename that variable to "retries" to keep it short?)

> +	/* 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 the subchannel is status pending or if a function is in progress,
> +	 * we consider both cases as errors.
> +	 */
> +	if (cc) {
> +		report_info("msch: sch %08x failed with cc=%d", schid, cc);
> +		return cc;
> +	}
> +
> +	/* Read the SCHIB again to verify the enablement */

"verify the disablement" ?

> +	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) {
> +		/* the hardware was not ready, give it some time */
> +		mdelay(10);
> +		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
> 

  Thomas

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

[Thomas Huth]

On 25/03/2021 10.39, Pierre Morel 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(+)

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

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Thomas Huth]

On 25/03/2021 10.39, Pierre Morel wrote:
> We will lhave to test if a device is present for every tests
> in the future.
> Let's provide a macro to check if the device is present and
> to skip the tests if it is not.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   s390x/css.c | 27 +++++++++++----------------
>   1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index c340c53..16723f6 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -27,6 +27,13 @@ static int test_device_sid;
>   static struct senseid *senseid;
>   struct ccw1 *ccw;
>   
> +#define NODEV_SKIP(dev) do {						\
> +				if (!(dev)) {				\
> +					report_skip("No device");	\
> +					return;				\
> +				}					\
> +			} while (0)
> +
>   static void test_enumerate(void)
>   {
>   	test_device_sid = css_enumerate();
> @@ -41,10 +48,7 @@ static void test_enable(void)
>   {
>   	int cc;
>   
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>   
>   	cc = css_enable(test_device_sid, IO_SCH_ISC);
>   
> @@ -62,10 +66,7 @@ static void test_sense(void)
>   	int ret;
>   	int len;
>   
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>   
>   	ret = css_enable(test_device_sid, IO_SCH_ISC);
>   	if (ret) {
> @@ -218,10 +219,7 @@ static void test_schm_fmt0(void)
>   	struct measurement_block_format0 *mb0;
>   	int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
>   
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>   
>   	/* Allocate zeroed Measurement block */
>   	mb0 = alloc_io_mem(shared_mb_size, 0);
> @@ -289,10 +287,7 @@ static void test_schm_fmt1(void)
>   {
>   	struct measurement_block_format1 *mb1;
>   
> -	if (!test_device_sid) {
> -		report_skip("No device");
> -		return;
> -	}
> +	NODEV_SKIP(test_device_sid);
>   
>   	if (!css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
>   		report_skip("Extended measurement block not available");
> 

I wonder whether it would be easier to simply skip all tests in main() if 
the test device is not available, instead of checking it again and again and 
again...?

  Thomas

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Thomas Huth]

On 25/03/2021 10.39, Pierre Morel wrote:
> We will may want to check the result of an I/O without waiting
> for an interrupt.
> For example because we do not handle interrupt.
> Let's separate waiting for interrupt and the I/O complretion check.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     |  1 +
>   lib/s390x/css_lib.c | 13 ++++++++++---
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 0058355..5d1e1f0 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -317,6 +317,7 @@ int css_residual_count(unsigned int schid);
>   
>   void enable_io_isc(uint8_t isc);
>   int wait_and_check_io_completion(int schid);
> +int check_io_completion(int schid);
>   
>   /*
>    * CHSC definitions
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index f5c4f37..1e5c409 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -487,18 +487,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>   }
>   
>   /* wait_and_check_io_completion:
> + * @schid: the subchannel ID
> + */
> +int wait_and_check_io_completion(int schid)
> +{
> +	wait_for_interrupt(PSW_MASK_IO);
> +	return check_io_completion(schid);
> +}
> +
> +/* check_io_completion:
>    * @schid: the subchannel ID
>    *
>    * Makes the most common check to validate a successful I/O
>    * completion.
>    * Only report failures.
>    */
> -int wait_and_check_io_completion(int schid)
> +int check_io_completion(int schid)
>   {
>   	int ret = 0;
>   
> -	wait_for_interrupt(PSW_MASK_IO);
> -
>   	report_prefix_push("check I/O completion");
>   
>   	if (lowcore_ptr->io_int_param != schid) {
> 

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

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Thomas Huth]

On 25/03/2021 10.39, Pierre Morel wrote:
> When checking for an I/O completion 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 function expected after executing
> an instruction or sequence of instructions and if all ctrl flags
> of the SCSW are set as expected.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     |  4 ++--
>   lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>   s390x/css.c         |  4 ++--
>   3 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 5d1e1f0..1603781 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -316,8 +316,8 @@ 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 check_io_completion(int schid);
> +int wait_and_check_io_completion(int schid, uint32_t ctrl);
> +int check_io_completion(int schid, uint32_t ctrl);
>   
>   /*
>    * CHSC definitions
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 1e5c409..55e70e6 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>   
>   /* wait_and_check_io_completion:
>    * @schid: the subchannel ID
> + * @ctrl : expected SCSW control flags
>    */
> -int wait_and_check_io_completion(int schid)
> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>   {
>   	wait_for_interrupt(PSW_MASK_IO);
> -	return check_io_completion(schid);
> +	return check_io_completion(schid, ctrl);
>   }
>   
>   /* check_io_completion:
>    * @schid: the subchannel ID
> + * @ctrl : expected SCSW control flags
>    *
> - * Makes the most common check to validate a successful I/O
> - * completion.
> + * If the ctrl parameter is not null check the IRB SCSW ctrl
> + * against the ctrl parameter.
> + * Otherwise, makes the most common check to validate a successful
> + * I/O completion.
>    * Only report failures.
>    */
> -int check_io_completion(int schid)
> +int check_io_completion(int schid, uint32_t ctrl)
>   {
>   	int ret = 0;
>   
> @@ -515,6 +519,13 @@ int check_io_completion(int schid)
>   		goto end;
>   	}
>   
> +	if (ctrl && (ctrl ^ irb.scsw.ctrl)) {

With the xor, you can only check for enabled bits ... do we also want to 
check for disabled bits, or is this always out of scope?

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

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Thomas Huth]

On 29/03/2021 10.27, Thomas Huth wrote:
> On 25/03/2021 10.39, Pierre Morel wrote:
>> When checking for an I/O completion 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 function expected after executing
>> an instruction or sequence of instructions and if all ctrl flags
>> of the SCSW are set as expected.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  4 ++--
>>   lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>>   s390x/css.c         |  4 ++--
>>   3 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 5d1e1f0..1603781 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -316,8 +316,8 @@ 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 check_io_completion(int schid);
>> +int wait_and_check_io_completion(int schid, uint32_t ctrl);
>> +int check_io_completion(int schid, uint32_t ctrl);
>>   /*
>>    * CHSC definitions
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 1e5c409..55e70e6 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int 
>> count, unsigned char flags)
>>   /* wait_and_check_io_completion:
>>    * @schid: the subchannel ID
>> + * @ctrl : expected SCSW control flags
>>    */
>> -int wait_and_check_io_completion(int schid)
>> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>>   {
>>       wait_for_interrupt(PSW_MASK_IO);
>> -    return check_io_completion(schid);
>> +    return check_io_completion(schid, ctrl);
>>   }
>>   /* check_io_completion:
>>    * @schid: the subchannel ID
>> + * @ctrl : expected SCSW control flags
>>    *
>> - * Makes the most common check to validate a successful I/O
>> - * completion.
>> + * If the ctrl parameter is not null check the IRB SCSW ctrl
>> + * against the ctrl parameter.
>> + * Otherwise, makes the most common check to validate a successful
>> + * I/O completion.
>>    * Only report failures.
>>    */
>> -int check_io_completion(int schid)
>> +int check_io_completion(int schid, uint32_t ctrl)
>>   {
>>       int ret = 0;
>> @@ -515,6 +519,13 @@ int check_io_completion(int schid)
>>           goto end;
>>       }
>> +    if (ctrl && (ctrl ^ irb.scsw.ctrl)) {
> 
> With the xor, you can only check for enabled bits ... do we also want to 
> check for disabled bits, or is this always out of scope?

Never mind, I think I just did not have enough coffee yet, the check should 
be fine. But couldn't you simply use "!=" instead of "^" here?

  Thomas

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

[Pierre Morel]

On 3/25/21 10:39 AM, Pierre Morel wrote:
> Checking error response on various eroneous SSCH instructions:
> - ORB alignment
> - ORB above 2G

! seems I made an error here, I do not find from where I got that ORB 
must be under 2G.
...



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Pierre Morel]

On 3/29/21 10:21 AM, Thomas Huth wrote:
> On 25/03/2021 10.39, Pierre Morel wrote:
>> We will may want to check the result of an I/O without waiting
>> for an interrupt.
>> For example because we do not handle interrupt.
>> Let's separate waiting for interrupt and the I/O complretion check.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 13 ++++++++++---
>>   2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 0058355..5d1e1f0 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -317,6 +317,7 @@ int css_residual_count(unsigned int schid);
>>   void enable_io_isc(uint8_t isc);
>>   int wait_and_check_io_completion(int schid);
>> +int check_io_completion(int schid);
>>   /*
>>    * CHSC definitions
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index f5c4f37..1e5c409 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -487,18 +487,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int 
>> count, unsigned char flags)
>>   }
>>   /* wait_and_check_io_completion:
>> + * @schid: the subchannel ID
>> + */
>> +int wait_and_check_io_completion(int schid)
>> +{
>> +    wait_for_interrupt(PSW_MASK_IO);
>> +    return check_io_completion(schid);
>> +}
>> +
>> +/* check_io_completion:
>>    * @schid: the subchannel ID
>>    *
>>    * Makes the most common check to validate a successful I/O
>>    * completion.
>>    * Only report failures.
>>    */
>> -int wait_and_check_io_completion(int schid)
>> +int check_io_completion(int schid)
>>   {
>>       int ret = 0;
>> -    wait_for_interrupt(PSW_MASK_IO);
>> -
>>       report_prefix_push("check I/O completion");
>>       if (lowcore_ptr->io_int_param != schid) {
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Pierre Morel]

On 3/29/21 10:32 AM, Thomas Huth wrote:
> On 29/03/2021 10.27, Thomas Huth wrote:
>> On 25/03/2021 10.39, Pierre Morel wrote:
>>> When checking for an I/O completion 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 function expected after executing
>>> an instruction or sequence of instructions and if all ctrl flags
>>> of the SCSW are set as expected.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/css.h     |  4 ++--
>>>   lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>>>   s390x/css.c         |  4 ++--
>>>   3 files changed, 20 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>>> index 5d1e1f0..1603781 100644
>>> --- a/lib/s390x/css.h
>>> +++ b/lib/s390x/css.h
>>> @@ -316,8 +316,8 @@ 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 check_io_completion(int schid);
>>> +int wait_and_check_io_completion(int schid, uint32_t ctrl);
>>> +int check_io_completion(int schid, uint32_t ctrl);
>>>   /*
>>>    * CHSC definitions
>>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>>> index 1e5c409..55e70e6 100644
>>> --- a/lib/s390x/css_lib.c
>>> +++ b/lib/s390x/css_lib.c
>>> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, 
>>> int count, unsigned char flags)
>>>   /* wait_and_check_io_completion:
>>>    * @schid: the subchannel ID
>>> + * @ctrl : expected SCSW control flags
>>>    */
>>> -int wait_and_check_io_completion(int schid)
>>> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>>>   {
>>>       wait_for_interrupt(PSW_MASK_IO);
>>> -    return check_io_completion(schid);
>>> +    return check_io_completion(schid, ctrl);
>>>   }
>>>   /* check_io_completion:
>>>    * @schid: the subchannel ID
>>> + * @ctrl : expected SCSW control flags
>>>    *
>>> - * Makes the most common check to validate a successful I/O
>>> - * completion.
>>> + * If the ctrl parameter is not null check the IRB SCSW ctrl
>>> + * against the ctrl parameter.
>>> + * Otherwise, makes the most common check to validate a successful
>>> + * I/O completion.
>>>    * Only report failures.
>>>    */
>>> -int check_io_completion(int schid)
>>> +int check_io_completion(int schid, uint32_t ctrl)
>>>   {
>>>       int ret = 0;
>>> @@ -515,6 +519,13 @@ int check_io_completion(int schid)
>>>           goto end;
>>>       }
>>> +    if (ctrl && (ctrl ^ irb.scsw.ctrl)) {
>>
>> With the xor, you can only check for enabled bits ... do we also want 
>> to check for disabled bits, or is this always out of scope?
> 
> Never mind, I think I just did not have enough coffee yet, the check 
> should be fine. But couldn't you simply use "!=" instead of "^" here?
> 
>   Thomas
> 


OK, yes I can


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Pierre Morel]

On 3/29/21 10:27 AM, Thomas Huth wrote:
> On 25/03/2021 10.39, Pierre Morel wrote:
>> When checking for an I/O completion 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 function expected after executing
>> an instruction or sequence of instructions and if all ctrl flags
>> of the SCSW are set as expected.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  4 ++--
>>   lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>>   s390x/css.c         |  4 ++--
>>   3 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 5d1e1f0..1603781 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -316,8 +316,8 @@ 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 check_io_completion(int schid);
>> +int wait_and_check_io_completion(int schid, uint32_t ctrl);
>> +int check_io_completion(int schid, uint32_t ctrl);
>>   /*
>>    * CHSC definitions
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 1e5c409..55e70e6 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int 
>> count, unsigned char flags)
>>   /* wait_and_check_io_completion:
>>    * @schid: the subchannel ID
>> + * @ctrl : expected SCSW control flags
>>    */
>> -int wait_and_check_io_completion(int schid)
>> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>>   {
>>       wait_for_interrupt(PSW_MASK_IO);
>> -    return check_io_completion(schid);
>> +    return check_io_completion(schid, ctrl);
>>   }
>>   /* check_io_completion:
>>    * @schid: the subchannel ID
>> + * @ctrl : expected SCSW control flags
>>    *
>> - * Makes the most common check to validate a successful I/O
>> - * completion.
>> + * If the ctrl parameter is not null check the IRB SCSW ctrl
>> + * against the ctrl parameter.
>> + * Otherwise, makes the most common check to validate a successful
>> + * I/O completion.
>>    * Only report failures.
>>    */
>> -int check_io_completion(int schid)
>> +int check_io_completion(int schid, uint32_t ctrl)
>>   {
>>       int ret = 0;
>> @@ -515,6 +519,13 @@ int check_io_completion(int schid)
>>           goto end;
>>       }
>> +    if (ctrl && (ctrl ^ irb.scsw.ctrl)) {
> 
> With the xor, you can only check for enabled bits ... do we also want to 
> check for disabled bits, or is this always out of scope?
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/29/21 10:09 AM, Thomas Huth wrote:
> On 25/03/2021 10.39, Pierre Morel 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(+)
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/29/21 10:00 AM, Thomas Huth wrote:
> On 25/03/2021 10.39, 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>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 68 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..f5c4f37 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -186,6 +186,73 @@ 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:
> 
> I have to saythat I really dislike writing loops with gotos if it can be 
> avoided easily. What about:
> 
> for (retry_count = 0; retry_count < MAX_ENABLE_RETRIES; retry_count++) ?
> 
> (and maybe rename that variable to "retries" to keep it short?)

hum, you already said that.
Sorry, I forgot and duplicated css_enable()

done.


...


>> +    /* Read the SCHIB again to verify the enablement */
> 
> "verify the disablement" ?

:) yes

> 
>> +    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) {
>> +        /* the hardware was not ready, give it some time */
>> +        mdelay(10);
>> +        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
>>
> 
>   Thomas
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Pierre Morel]

On 3/29/21 10:19 AM, Thomas Huth wrote:
> On 25/03/2021 10.39, Pierre Morel wrote:
>> We will lhave to test if a device is present for every tests
>> in the future.
>> Let's provide a macro to check if the device is present and
>> to skip the tests if it is not.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index c340c53..16723f6 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -27,6 +27,13 @@ static int test_device_sid;
>>   static struct senseid *senseid;
>>   struct ccw1 *ccw;
>> +#define NODEV_SKIP(dev) do {                        \
>> +                if (!(dev)) {                \
>> +                    report_skip("No device");    \
>> +                    return;                \
>> +                }                    \
>> +            } while (0)
>> +
>>   static void test_enumerate(void)
>>   {
>>       test_device_sid = css_enumerate();
>> @@ -41,10 +48,7 @@ static void test_enable(void)
>>   {
>>       int cc;
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       cc = css_enable(test_device_sid, IO_SCH_ISC);
>> @@ -62,10 +66,7 @@ static void test_sense(void)
>>       int ret;
>>       int len;
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       ret = css_enable(test_device_sid, IO_SCH_ISC);
>>       if (ret) {
>> @@ -218,10 +219,7 @@ static void test_schm_fmt0(void)
>>       struct measurement_block_format0 *mb0;
>>       int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       /* Allocate zeroed Measurement block */
>>       mb0 = alloc_io_mem(shared_mb_size, 0);
>> @@ -289,10 +287,7 @@ static void test_schm_fmt1(void)
>>   {
>>       struct measurement_block_format1 *mb1;
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       if (!css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
>>           report_skip("Extended measurement block not available");
>>
> 
> I wonder whether it would be easier to simply skip all tests in main() 
> if the test device is not available, instead of checking it again and 
> again and again...?
> 
>   Thomas
> 

Yes I can do this.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Pierre Morel]

On 3/29/21 10:19 AM, Thomas Huth wrote:
> On 25/03/2021 10.39, Pierre Morel wrote:
>> We will lhave to test if a device is present for every tests
>> in the future.
>> Let's provide a macro to check if the device is present and
>> to skip the tests if it is not.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/css.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index c340c53..16723f6 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -27,6 +27,13 @@ static int test_device_sid;
>>   static struct senseid *senseid;
>>   struct ccw1 *ccw;
>> +#define NODEV_SKIP(dev) do {                        \
>> +                if (!(dev)) {                \
>> +                    report_skip("No device");    \
>> +                    return;                \
>> +                }                    \
>> +            } while (0)
>> +
>>   static void test_enumerate(void)
>>   {
>>       test_device_sid = css_enumerate();
>> @@ -41,10 +48,7 @@ static void test_enable(void)
>>   {
>>       int cc;
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       cc = css_enable(test_device_sid, IO_SCH_ISC);
>> @@ -62,10 +66,7 @@ static void test_sense(void)
>>       int ret;
>>       int len;
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       ret = css_enable(test_device_sid, IO_SCH_ISC);
>>       if (ret) {
>> @@ -218,10 +219,7 @@ static void test_schm_fmt0(void)
>>       struct measurement_block_format0 *mb0;
>>       int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       /* Allocate zeroed Measurement block */
>>       mb0 = alloc_io_mem(shared_mb_size, 0);
>> @@ -289,10 +287,7 @@ static void test_schm_fmt1(void)
>>   {
>>       struct measurement_block_format1 *mb1;
>> -    if (!test_device_sid) {
>> -        report_skip("No device");
>> -        return;
>> -    }
>> +    NODEV_SKIP(test_device_sid);
>>       if (!css_test_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
>>           report_skip("Extended measurement block not available");
>>
> 
> I wonder whether it would be easier to simply skip all tests in main() 
> if the test device is not available, instead of checking it again and 
> again and again...?
> 
>   Thomas
> 

I will silently skip the remaining tests when the enumeration fails or 
do you want that we see other information?
It seems obvious enough that finding no device we do not continue testing.

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Thu, 25 Mar 2021 10:39:01 +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(+)

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

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Cornelia Huck]

On Mon, 29 Mar 2021 14:50:22 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 3/29/21 10:19 AM, Thomas Huth wrote:
> > On 25/03/2021 10.39, Pierre Morel wrote:  
> >> We will lhave to test if a device is present for every tests
> >> in the future.
> >> Let's provide a macro to check if the device is present and
> >> to skip the tests if it is not.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >>   s390x/css.c | 27 +++++++++++----------------
> >>   1 file changed, 11 insertions(+), 16 deletions(-)

> > I wonder whether it would be easier to simply skip all tests in main() 
> > if the test device is not available, instead of checking it again and 
> > again and again...?
> > 
> >   Thomas
> >   
> 
> I will silently skip the remaining tests when the enumeration fails or 
> do you want that we see other information?
> It seems obvious enough that finding no device we do not continue testing.

Logging that the device enumeration failed should be enough info.

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Cornelia Huck]

On Thu, 25 Mar 2021 10:39:03 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We will may want to check the result of an I/O without waiting
> for an interrupt.
> For example because we do not handle interrupt.

It's more because we may poll the subchannel state without enabling I/O
interrupts, no?

> Let's separate waiting for interrupt and the I/O complretion check.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)

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

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Cornelia Huck]

On Thu, 25 Mar 2021 10:39:04 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> When checking for an I/O completion may need to check the cause of
> the interrupt depending on the test case.

"When we check for the completion of an I/O, we may need to check..." ?

> 
> Let's provide the tests the possibility to check if the last
> valid IRQ received is for the function expected after executing

"Let's make it possible for the tests to check whether the last valid
IRB received indicates the expected functions..." ?

> an instruction or sequence of instructions and if all ctrl flags
> of the SCSW are set as expected.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h     |  4 ++--
>  lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>  s390x/css.c         |  4 ++--
>  3 files changed, 20 insertions(+), 9 deletions(-)
> 

(...)

> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 1e5c409..55e70e6 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>  
>  /* wait_and_check_io_completion:
>   * @schid: the subchannel ID
> + * @ctrl : expected SCSW control flags
>   */
> -int wait_and_check_io_completion(int schid)
> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>  {
>  	wait_for_interrupt(PSW_MASK_IO);
> -	return check_io_completion(schid);
> +	return check_io_completion(schid, ctrl);
>  }
>  
>  /* check_io_completion:
>   * @schid: the subchannel ID
> + * @ctrl : expected SCSW control flags
>   *
> - * Makes the most common check to validate a successful I/O
> - * completion.
> + * If the ctrl parameter is not null check the IRB SCSW ctrl
> + * against the ctrl parameter.
> + * Otherwise, makes the most common check to validate a successful
> + * I/O completion.

What about:

"Perform some standard checks to validate a successful I/O completion.
If the ctrl parameter is not zero, additionally verify that the
specified bits are indicated in the IRB SCSW ctrl flags."

>   * Only report failures.
>   */
> -int check_io_completion(int schid)
> +int check_io_completion(int schid, uint32_t ctrl)
>  {
>  	int ret = 0;
>  

With Thomas' suggested change,

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

Re: [kvm-unit-tests PATCH v2 5/8] s390x: lib: css: add SCSW ctrl expectations to check I/O completion

[Pierre Morel]

On 3/30/21 2:10 PM, Cornelia Huck wrote:
> On Thu, 25 Mar 2021 10:39:04 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> When checking for an I/O completion may need to check the cause of
>> the interrupt depending on the test case.
> 
> "When we check for the completion of an I/O, we may need to check..." ?
> 

yes, thanks

>>
>> Let's provide the tests the possibility to check if the last
>> valid IRQ received is for the function expected after executing
> 
> "Let's make it possible for the tests to check whether the last valid
> IRB received indicates the expected functions..." ?


better too :)

> 
>> an instruction or sequence of instructions and if all ctrl flags
>> of the SCSW are set as expected.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  4 ++--
>>   lib/s390x/css_lib.c | 21 ++++++++++++++++-----
>>   s390x/css.c         |  4 ++--
>>   3 files changed, 20 insertions(+), 9 deletions(-)
>>
> 
> (...)
> 
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 1e5c409..55e70e6 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>>   
>>   /* wait_and_check_io_completion:
>>    * @schid: the subchannel ID
>> + * @ctrl : expected SCSW control flags
>>    */
>> -int wait_and_check_io_completion(int schid)
>> +int wait_and_check_io_completion(int schid, uint32_t ctrl)
>>   {
>>   	wait_for_interrupt(PSW_MASK_IO);
>> -	return check_io_completion(schid);
>> +	return check_io_completion(schid, ctrl);
>>   }
>>   
>>   /* check_io_completion:
>>    * @schid: the subchannel ID
>> + * @ctrl : expected SCSW control flags
>>    *
>> - * Makes the most common check to validate a successful I/O
>> - * completion.
>> + * If the ctrl parameter is not null check the IRB SCSW ctrl
>> + * against the ctrl parameter.
>> + * Otherwise, makes the most common check to validate a successful
>> + * I/O completion.
> 
> What about:
> 
> "Perform some standard checks to validate a successful I/O completion.
> If the ctrl parameter is not zero, additionally verify that the
> specified bits are indicated in the IRB SCSW ctrl flags."

Yes, looks better, thanks

> 
>>    * Only report failures.
>>    */
>> -int check_io_completion(int schid)
>> +int check_io_completion(int schid, uint32_t ctrl)
>>   {
>>   	int ret = 0;
>>   
> 
> With Thomas' suggested change,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Pierre Morel]

On 3/30/21 1:57 PM, Cornelia Huck wrote:
> On Thu, 25 Mar 2021 10:39:03 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We will may want to check the result of an I/O without waiting
>> for an interrupt.
>> For example because we do not handle interrupt.
> 
> It's more because we may poll the subchannel state without enabling I/O
> interrupts, no?

absolutely.
may be I just replace with your rewording :)

"
We will may want to check the result of an I/O without waiting
for an interrupt because we may poll the subchannel state without 
enabling I/O interrupts.
"

> 
>> Let's separate waiting for interrupt and the I/O complretion check.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 13 ++++++++++---
>>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/8] s390x: css: simplify skipping tests on no device

[Pierre Morel]

On 3/30/21 1:52 PM, Cornelia Huck wrote:
> On Mon, 29 Mar 2021 14:50:22 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 3/29/21 10:19 AM, Thomas Huth wrote:
>>> On 25/03/2021 10.39, Pierre Morel wrote:
>>>> We will lhave to test if a device is present for every tests
>>>> in the future.
>>>> Let's provide a macro to check if the device is present and
>>>> to skip the tests if it is not.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    s390x/css.c | 27 +++++++++++----------------
>>>>    1 file changed, 11 insertions(+), 16 deletions(-)
> 
>>> I wonder whether it would be easier to simply skip all tests in main()
>>> if the test device is not available, instead of checking it again and
>>> again and again...?
>>>
>>>    Thomas
>>>    
>>
>> I will silently skip the remaining tests when the enumeration fails or
>> do you want that we see other information?
>> It seems obvious enough that finding no device we do not continue testing.
> 
> Logging that the device enumeration failed should be enough info.
> 

OK then I do so.
Thanks
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/30/21 1:49 PM, Cornelia Huck wrote:
> On Thu, 25 Mar 2021 10:39:01 +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(+)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 3/29/21 9:42 AM, Pierre Morel wrote:
> 
> 
> On 3/26/21 11:58 AM, Claudio Imbrenda wrote:
>> On Fri, 26 Mar 2021 11:41:34 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:

...snip...

>>> What is the purpose to check with only 1G memory?
>>
>> you need to run this test twice, once with 1G and once with 3G.
>> it's the same test, so it can't know if it is being run with 1G or
>> 3G, so you have to test for it.
>>
>> when you need a valid address above 2G, you need to make sure you have
>> that much memory, and when you want an invalid address between 1G and
>> 2G, you have to make sure you have no more than 1G.
> 
> OK, thanks
> 
> 
> 
> 

To handle the access errors I will need to extend the checking I 
currently do on the SCSW to the status fields for subchannel and device.

So no need to review this part for now because I will reorganize and 
extend it.

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 4/8] s390x: lib: css: separate wait for IRQ and check I/O completion

[Pierre Morel]

On 3/25/21 10:39 AM, Pierre Morel wrote:
> We will may want to check the result of an I/O without waiting
> for an interrupt.
> For example because we do not handle interrupt.
> Let's separate waiting for interrupt and the I/O complretion check.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     |  1 +
>   lib/s390x/css_lib.c | 13 ++++++++++---
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 0058355..5d1e1f0 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -317,6 +317,7 @@ int css_residual_count(unsigned int schid);
>   
>   void enable_io_isc(uint8_t isc);
>   int wait_and_check_io_completion(int schid);
> +int check_io_completion(int schid);
>   
>   /*
>    * CHSC definitions
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index f5c4f37..1e5c409 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -487,18 +487,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>   }
>   
>   /* wait_and_check_io_completion:
> + * @schid: the subchannel ID
> + */
> +int wait_and_check_io_completion(int schid)
> +{
> +	wait_for_interrupt(PSW_MASK_IO);
> +	return check_io_completion(schid);
> +}
> +
> +/* check_io_completion:
>    * @schid: the subchannel ID
>    *
>    * Makes the most common check to validate a successful I/O
>    * completion.
>    * Only report failures.
>    */
> -int wait_and_check_io_completion(int schid)
> +int check_io_completion(int schid)
>   {
>   	int ret = 0;
>   
> -	wait_for_interrupt(PSW_MASK_IO);
> -
>   	report_prefix_push("check I/O completion");
>   
>   	if (lowcore_ptr->io_int_param != schid) {
> 

Hum, sorry, it seems I forgot here --^ to move the check on io_int_param 
inside the interrupt routine.
I change this for the next spin

regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen