[kvm-unit-tests PATCH v4 0/6] CSS Mesurement Block

[kvm-unit-tests PATCH v4 0/6] CSS Mesurement Block

[Pierre Morel]

We tests the update of the Mesurement Block (MB) format 0
and format 1 using a serie of senseid requests.

*Warning*: One of the tests for format-1 will unexpectedly fail for QEMU elf
unless the QEMU patch "css: SCHIB measurement block origin must be aligned"
is applied.
With Protected Virtualization, the PGM is correctly recognized.

The MB format 1 is only provided if the Extended mesurement Block
feature is available.

This feature is exposed by the CSS characteristics general features
stored by the Store Channel Subsystem Characteristics CHSC command,
consequently, we implement the CHSC instruction call and the SCSC CHSC
command.

In order to ease the writing of new tests using:
- interrupt
- enablement of a subchannel
- multiple I/O on a subchannel

We do the following simplifications:
- we create a CSS initialization routine
- we register the I/O interrupt handler on CSS initialization
- we do not enable or disable a subchannel in the senseid test,
  assuming this test is done after the enable test, this allows
  to create traffic using the SSCH used by senseid.
- we add a css_enabled() function to test if a subchannel is enabled.

@Connie, I restructured the patches but I did not modify the
functionalities, so I kept your R-B, I hope you are OK with this.

Regards,
Pierre

Pierre Morel (6):
  s390x: css: Store CSS Characteristics
  s390x: css: simplifications of the tests
  s390x: css: extending the subchannel modifying functions
  s390x: css: implementing Set CHannel Monitor
  s390x: css: testing measurement block format 0
  s390x: css: testing measurement block format 1

 lib/s390x/css.h     | 116 +++++++++++++++++++++-
 lib/s390x/css_lib.c | 232 +++++++++++++++++++++++++++++++++++++++++---
 s390x/css.c         | 228 +++++++++++++++++++++++++++++++++++++++----
 3 files changed, 542 insertions(+), 34 deletions(-)

-- 
2.17.1

changelog:

from v3:

- stay coherent and use uintX_t types in css.h
  (Janosch)

- reworking test versus library to leave the reports
  mostly inside the tests
  (Janosh)

- Restructured some tests to use booleans instead of int
  this simplifies testing.
  (inspired by comments from Janosch)

- rewordings and orthograph (Measurement !!!)
  (Connie)

- stop measurement at the channel too before freeing the
  MB memory.
  (Connie)

- split out the subchannel-modifying functions into a
  separate patch
  (Connie)

- move msch_with_wrong_fmt1_mbo() from the library
  into the tests
  (Janosch)

- Suppress redundancy in the prefixes of format0/1 tests
  (Janosch)

from v2:

- stop measurement before freeing memory
  (Connie)

- added a css_disable_mb()
  (Connie)

- several rewriting of comments and commits
  (Connie)

- modified eroneous test for MB index for fmt0
  (Pierre)

- modified eroneous test for unaligned MBO for fmt1
  (Pierre)

- several small coding style issues corrected
  (Pierre)

from v1:

- check the return code of CHSC
  (Connie)

- reporting in css_init
  (Connie)

- added braces when a loop contains several statement
  (Thomas)

- changed retval to success in boolean function
  (Thomas)

- suppress goto retries
  (thomas)

- rewording and use correct return types in css_enabled
  (Janosch)

[kvm-unit-tests PATCH v4 1/6] s390x: css: Store CSS Characteristics

[Pierre Morel]

CSS characteristics exposes the features of the Channel SubSystem.
Let's use Store Channel Subsystem Characteristics to retrieve
the features of the CSS.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/css.h     | 67 ++++++++++++++++++++++++++++++++
 lib/s390x/css_lib.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-
 s390x/css.c         |  8 ++++
 3 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 3e57445..4210472 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -288,4 +288,71 @@ int css_residual_count(unsigned int schid);
 void enable_io_isc(uint8_t isc);
 int wait_and_check_io_completion(int schid);
 
+/*
+ * CHSC definitions
+ */
+struct chsc_header {
+	uint16_t len;
+	uint16_t code;
+};
+
+/* Store Channel Subsystem Characteristics */
+struct chsc_scsc {
+	struct chsc_header req;
+	uint16_t req_fmt;
+	uint8_t cssid;
+	uint8_t res_03;
+	uint32_t res_04[2];
+	struct chsc_header res;
+	uint32_t res_fmt;
+	uint64_t general_char[255];
+	uint64_t chsc_char[254];
+};
+
+extern struct chsc_scsc *chsc_scsc;
+#define CHSC_SCSC	0x0010
+#define CHSC_SCSC_LEN	0x0010
+
+bool get_chsc_scsc(void);
+
+#define CSS_GENERAL_FEAT_BITLEN	(255 * 64)
+#define CSS_CHSC_FEAT_BITLEN	(254 * 64)
+
+#define CHSC_SCSC	0x0010
+#define CHSC_SCSC_LEN	0x0010
+
+#define CHSC_ERROR	0x0000
+#define CHSC_RSP_OK	0x0001
+#define CHSC_RSP_INVAL	0x0002
+#define CHSC_RSP_REQERR	0x0003
+#define CHSC_RSP_ENOCMD	0x0004
+#define CHSC_RSP_NODATA	0x0005
+#define CHSC_RSP_SUP31B	0x0006
+#define CHSC_RSP_EFRMT	0x0007
+#define CHSC_RSP_ECSSID	0x0008
+#define CHSC_RSP_ERFRMT	0x0009
+#define CHSC_RSP_ESSID	0x000A
+#define CHSC_RSP_EBUSY	0x000B
+#define CHSC_RSP_MAX	0x000B
+
+static inline int _chsc(void *p)
+{
+	int cc;
+
+	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
+		     " ipm     %0\n"
+		     " srl     %0,28\n"
+		     : "=d" (cc), "=m" (p)
+		     : "d" (p), "m" (p)
+		     : "cc");
+
+	return cc;
+}
+
+bool chsc(void *p, uint16_t code, uint16_t len);
+
+#include <bitops.h>
+#define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
+#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 3c24480..f46e871 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -15,11 +15,102 @@
 #include <asm/arch_def.h>
 #include <asm/time.h>
 #include <asm/arch_def.h>
-
+#include <alloc_page.h>
 #include <malloc_io.h>
 #include <css.h>
 
 static struct schib schib;
+struct chsc_scsc *chsc_scsc;
+
+static const char * const chsc_rsp_description[] = {
+	"CHSC unknown error",
+	"Command executed",
+	"Invalid command",
+	"Request-block error",
+	"Command not installed",
+	"Data not available",
+	"Absolute address of channel-subsystem communication block exceeds 2G - 1.",
+	"Invalid command format",
+	"Invalid channel-subsystem identification (CSSID)",
+	"The command-request block specified an invalid format for the command response block.",
+	"Invalid subchannel-set identification (SSID)",
+	"A busy condition precludes execution.",
+};
+
+static bool check_response(void *p)
+{
+	struct chsc_header *h = p;
+
+	if (h->code == CHSC_RSP_OK)
+		return true;
+
+	if (h->code > CHSC_RSP_MAX)
+		h->code = 0;
+
+	report_abort("Response code %04x: %s", h->code,
+		      chsc_rsp_description[h->code]);
+	return false;
+}
+
+bool chsc(void *p, uint16_t code, uint16_t len)
+{
+	struct chsc_header *h = p;
+
+	h->code = code;
+	h->len = len;
+
+	switch (_chsc(p)) {
+	case 3:
+		report_abort("Subchannel invalid or not enabled.");
+		break;
+	case 2:
+		report_abort("CHSC subchannel busy.");
+		break;
+	case 1:
+		report_abort("Subchannel invalid or not enabled.");
+		break;
+	case 0:
+		return check_response(p + len);
+	}
+	return false;
+}
+
+bool get_chsc_scsc(void)
+{
+	int i, n;
+	char buffer[510];
+	char *p;
+
+	if (chsc_scsc) /* chsc_scsc already initialized */
+		return true;
+
+	chsc_scsc = alloc_page();
+	if (!chsc_scsc) {
+		report_abort("could not allocate chsc_scsc page!");
+		return false;
+	}
+
+	if (!chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN))
+		return false;
+
+	for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++) {
+		if (css_general_feature(i)) {
+			n = snprintf(p, sizeof(buffer), "%d,", i);
+			p += n;
+		}
+	}
+	report_info("General features: %s", buffer);
+
+	for (i = 0, p = buffer; i < CSS_CHSC_FEAT_BITLEN; i++) {
+		if (css_chsc_feature(i)) {
+			n = snprintf(p, sizeof(buffer), "%d,", i);
+			p += n;
+		}
+	}
+	report_info("CHSC features: %s", buffer);
+
+	return true;
+}
 
 /*
  * css_enumerate:
diff --git a/s390x/css.c b/s390x/css.c
index 1a61a5c..09703c1 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -14,6 +14,7 @@
 #include <string.h>
 #include <interrupt.h>
 #include <asm/arch_def.h>
+#include <alloc_page.h>
 
 #include <malloc_io.h>
 #include <css.h>
@@ -140,10 +141,17 @@ error_senseid:
 	unregister_io_int_func(css_irq_io);
 }
 
+static void css_init(void)
+{
+	report(!!get_chsc_scsc(), "Store Channel Characteristics");
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
 } tests[] = {
+	/* The css_init test is needed to initialize the CSS Characteristics */
+	{ "initialize CSS (chsc)", css_init },
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "sense (ssch/tsch)", test_sense },
-- 
2.17.1

[kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Pierre Morel]

In order to ease the writing of tests based on:
- interrupt
- enabling a subchannel
- using multiple I/O on a channel without disabling it

We do the following simplifications:
- the I/O interrupt handler is registered on CSS initialization
- We do not enable again a subchannel in senseid if it is already
  enabled
- we add a css_enabled() function to test if a subchannel is enabled

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 | 37 ++++++++++++++++++++++++++-----------
 s390x/css.c         | 44 ++++++++++++++++++++++++--------------------
 3 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 4210472..fbfa034 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -278,6 +278,7 @@ int css_enumerate(void);
 
 #define IO_SCH_ISC      3
 int css_enable(int schid, int isc);
+bool css_enabled(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 f46e871..41134dc 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -161,6 +161,31 @@ out:
 	return schid;
 }
 
+/*
+ * css_enabled: report if the subchannel is enabled
+ * @schid: Subchannel Identifier
+ * Return value:
+ *   true if the subchannel is enabled
+ *   false otherwise
+ */
+bool css_enabled(int schid)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch: updating sch %08x failed with cc=%d",
+			    schid, cc);
+		return false;
+	}
+
+	if (!(pmcw->flags & PMCW_ENABLE)) {
+		report_info("stsch: sch %08x not enabled", schid);
+		return false;
+	}
+	return true;
+}
 /*
  * css_enable: enable the subchannel with the specified ISC
  * @schid: Subchannel Identifier
@@ -210,18 +235,8 @@ retry:
 	/*
 	 * 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 & flags) == flags) {
-		report_info("stsch: sch %08x successfully modified after %d retries",
-			    schid, retry_count);
+	if (css_enabled(schid))
 		return 0;
-	}
 
 	if (retry_count++ < MAX_ENABLE_RETRIES) {
 		mdelay(10); /* the hardware was not ready, give it some time */
diff --git a/s390x/css.c b/s390x/css.c
index 09703c1..c9e4903 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -56,36 +56,27 @@ static void test_enable(void)
  * - We need the test device as the first recognized
  *   device by the enumeration.
  */
-static void test_sense(void)
+static bool do_test_sense(void)
 {
 	struct ccw1 *ccw;
+	bool success = false;
 	int ret;
 	int len;
 
 	if (!test_device_sid) {
 		report_skip("No device");
-		return;
+		return success;
 	}
 
-	ret = css_enable(test_device_sid, IO_SCH_ISC);
-	if (ret) {
-		report(0, "Could not enable the subchannel: %08x",
-		       test_device_sid);
-		return;
+	if (!css_enabled(test_device_sid)) {
+		report(0, "enabling subchannel %08x", test_device_sid);
+		return success;
 	}
 
-	ret = register_io_int_func(css_irq_io);
-	if (ret) {
-		report(0, "Could not register IRQ handler");
-		return;
-	}
-
-	lowcore_ptr->io_int_param = 0;
-
 	senseid = alloc_io_mem(sizeof(*senseid), 0);
 	if (!senseid) {
 		report(0, "Allocation of senseid");
-		goto error_senseid;
+		return success;
 	}
 
 	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
@@ -129,21 +120,34 @@ static void test_sense(void)
 	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
 		    senseid->reserved, senseid->cu_type, senseid->cu_model,
 		    senseid->dev_type, senseid->dev_model);
+	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
+		    senseid->cu_type);
 
-	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
-	       (uint16_t)cu_type, senseid->cu_type);
+	success = senseid->cu_type == cu_type;
 
 error:
 	free_io_mem(ccw, sizeof(*ccw));
 error_ccw:
 	free_io_mem(senseid, sizeof(*senseid));
-error_senseid:
-	unregister_io_int_func(css_irq_io);
+	return success;
+}
+
+static void test_sense(void)
+{
+	report(do_test_sense(), "Got CU type expected");
 }
 
 static void css_init(void)
 {
 	report(!!get_chsc_scsc(), "Store Channel Characteristics");
+
+	if (register_io_int_func(css_irq_io)) {
+		report(0, "Could not register IRQ handler");
+		return;
+	}
+	lowcore_ptr->io_int_param = 0;
+
+	report(1, "CSS initialized");
 }
 
 static struct {
-- 
2.17.1

[kvm-unit-tests PATCH v4 3/6] s390x: css: extending the subchannel modifying functions

[Pierre Morel]

To enable or disable measurement we will need specific
modifications on the subchannel.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/css.h     |   9 +++-
 lib/s390x/css_lib.c | 100 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index fbfa034..1cb3de2 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -82,6 +82,8 @@ struct pmcw {
 	uint32_t intparm;
 #define PMCW_DNV	0x0001
 #define PMCW_ENABLE	0x0080
+#define PMCW_MBUE	0x0010
+#define PMCW_DCTME	0x0008
 #define PMCW_ISC_MASK	0x3800
 #define PMCW_ISC_SHIFT	11
 	uint16_t flags;
@@ -94,6 +96,7 @@ struct pmcw {
 	uint8_t  pom;
 	uint8_t  pam;
 	uint8_t  chpid[8];
+#define PMCW_MBF1	0x0004
 	uint32_t flags2;
 };
 #define PMCW_CHANNEL_TYPE(pmcw) (pmcw->flags2 >> 21)
@@ -101,7 +104,8 @@ struct pmcw {
 struct schib {
 	struct pmcw pmcw;
 	struct scsw scsw;
-	uint8_t  md[12];
+	uint64_t mbo;
+	uint8_t  md[4];
 } __attribute__ ((aligned(4)));
 
 struct irb {
@@ -356,4 +360,7 @@ bool chsc(void *p, uint16_t code, uint16_t len);
 #define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
 #define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
 
+bool css_enable_mb(int sid, uint64_t mb, uint16_t mbi, uint16_t flg, bool fmt1);
+bool css_disable_mb(int schid);
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 41134dc..77b39c7 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -248,6 +248,106 @@ retry:
 	return -1;
 }
 
+/*
+ * schib_update_mb: update the subchannel Measurement Block
+ * @schid: Subchannel Identifier
+ * @mb   : 64bit address of the measurement block
+ * @mbi : the measurement block offset
+ * @flags : PMCW_MBUE to enable measurement block update
+ *	    PMCW_DCTME to enable device connect time
+ *	    0 to disable measurement
+ * @format1: set if format 1 is to be used
+ */
+static bool schib_update_mb(int schid, uint64_t mb, uint16_t mbi,
+			    uint16_t flags, bool format1)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	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 false;
+	}
+
+	/* Update the SCHIB to enable the measurement block */
+	if (flags) {
+		pmcw->flags |= flags;
+
+		if (format1)
+			pmcw->flags2 |= PMCW_MBF1;
+		else
+			pmcw->flags2 &= ~PMCW_MBF1;
+
+		pmcw->mbi = mbi;
+		schib.mbo = mb & ~0x3f;
+	} else {
+		pmcw->flags &= ~(PMCW_MBUE | PMCW_DCTME);
+	}
+
+	/* Tell the CSS we want to modify the subchannel */
+	cc = msch(schid, &schib);
+	if (cc) {
+		/*
+		 * If the subchannel is status pending or
+		 * if a function is in progress,
+		 * we consider both cases as errors.
+		 */
+		report_info("msch: sch %08x failed with cc=%d", schid, cc);
+		return false;
+	}
+
+	/*
+	 * Read the SCHIB again
+	 */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report_info("stsch: updating sch %08x failed with cc=%d",
+			    schid, cc);
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * css_enable_mb: enable the subchannel Measurement Block
+ * @schid: Subchannel Identifier
+ * @mb   : 64bit address of the measurement block
+ * @format1: set if format 1 is to be used
+ * @mbi : the measurement block offset
+ * @flags : PMCW_MBUE to enable measurement block update
+ *	    PMCW_DCTME to enable device connect time
+ */
+bool css_enable_mb(int schid, uint64_t mb, uint16_t mbi, uint16_t flags,
+		   bool format1)
+{
+	int retry_count = MAX_ENABLE_RETRIES;
+	struct pmcw *pmcw = &schib.pmcw;
+
+	while (retry_count-- &&
+	       !schib_update_mb(schid, mb, mbi, flags, format1))
+		mdelay(10); /* the hardware was not ready, give it some time */
+
+	return schib.mbo == mb && pmcw->mbi == mbi;
+}
+
+/*
+ * css_disable_mb: disable the subchannel Measurement Block
+ * @schid: Subchannel Identifier
+ */
+bool css_disable_mb(int schid)
+{
+	int retry_count = MAX_ENABLE_RETRIES;
+
+	while (retry_count-- &&
+	       !schib_update_mb(schid, 0, 0, 0, 0))
+		mdelay(10); /* the hardware was not ready, give it some time */
+
+	return retry_count > 0;
+}
+
 static struct irb irb;
 
 void css_irq_io(void)
-- 
2.17.1

[kvm-unit-tests PATCH v4 4/6] s390x: css: implementing Set CHannel Monitor

[Pierre Morel]

We implement the call of the Set CHannel Monitor instruction,
starting the monitoring of the all Channel Sub System, and
initializing channel subsystem monitoring.

Initial tests report the presence of the extended measurement block
feature, and verify the error reporting of the hypervisor for SCHM.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/css.h | 12 ++++++++++++
 s390x/css.c     | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 1cb3de2..b8ac363 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -310,6 +310,7 @@ struct chsc_scsc {
 	uint32_t res_04[2];
 	struct chsc_header res;
 	uint32_t res_fmt;
+#define CSSC_EXTENDED_MEASUREMENT_BLOCK 48
 	uint64_t general_char[255];
 	uint64_t chsc_char[254];
 };
@@ -360,6 +361,17 @@ bool chsc(void *p, uint16_t code, uint16_t len);
 #define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
 #define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
 
+#define SCHM_DCTM	1 /* activate Device Connection TiMe */
+#define SCHM_MBU	2 /* activate Measurement Block Update */
+
+static inline void schm(void *mbo, unsigned int flags)
+{
+	register void *__gpr2 asm("2") = mbo;
+	register long __gpr1 asm("1") = flags;
+
+	asm("schm" : : "d" (__gpr2), "d" (__gpr1));
+}
+
 bool css_enable_mb(int sid, uint64_t mb, uint16_t mbi, uint16_t flg, bool fmt1);
 bool css_disable_mb(int schid);
 
diff --git a/s390x/css.c b/s390x/css.c
index c9e4903..e8f96f3 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -150,6 +150,40 @@ static void css_init(void)
 	report(1, "CSS initialized");
 }
 
+static void test_schm(void)
+{
+	if (css_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK))
+		report_info("Extended measurement block available");
+
+	/* bits 59-63 of MB address must be 0  if MBU is defined */
+	report_prefix_push("Unaligned operand");
+	expect_pgm_int();
+	schm((void *)0x01, SCHM_MBU);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+	report_prefix_pop();
+
+	/* bits 36-61 of register 1 (flags) must be 0 */
+	report_prefix_push("Bad flags");
+	expect_pgm_int();
+	schm(NULL, 0xfffffffc);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+	report_prefix_pop();
+
+	/* SCHM is a privilege operation */
+	report_prefix_push("Privilege");
+	enter_pstate();
+	expect_pgm_int();
+	schm(NULL, SCHM_MBU);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	/* Normal operation */
+	report_prefix_push("Normal operation");
+	schm(NULL, SCHM_MBU);
+	report(1, "SCHM call without address");
+	report_prefix_pop();
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -159,6 +193,7 @@ static struct {
 	{ "enumerate (stsch)", test_enumerate },
 	{ "enable (msch)", test_enable },
 	{ "sense (ssch/tsch)", test_sense },
+	{ "measurement block (schm)", test_schm },
 	{ NULL, NULL }
 };
 
-- 
2.17.1

[kvm-unit-tests PATCH v4 5/6] s390x: css: testing measurement block format 0

[Pierre Morel]

We test the update of the measurement block format 0, the
measurement block origin is calculated from the mbo argument
used by the SCHM instruction and the offset calculated using
the measurement block index of the SCHIB.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index b8ac363..40f9efa 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -375,4 +375,16 @@ static inline void schm(void *mbo, unsigned int flags)
 bool css_enable_mb(int sid, uint64_t mb, uint16_t mbi, uint16_t flg, bool fmt1);
 bool css_disable_mb(int schid);
 
+struct measurement_block_format0 {
+	uint16_t ssch_rsch_count;
+	uint16_t sample_count;
+	uint32_t device_connect_time;
+	uint32_t function_pending_time;
+	uint32_t device_disconnect_time;
+	uint32_t cu_queuing_time;
+	uint32_t device_active_only_time;
+	uint32_t device_busy_time;
+	uint32_t initial_cmd_resp_time;
+};
+
 #endif
diff --git a/s390x/css.c b/s390x/css.c
index e8f96f3..3915ed3 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -184,6 +184,71 @@ static void test_schm(void)
 	report_prefix_pop();
 }
 
+#define SCHM_UPDATE_CNT 10
+static bool start_measuring(uint64_t mbo, uint16_t mbi, bool fmt1)
+{
+	int i;
+
+	if (!css_enable_mb(test_device_sid, mbo, mbi, PMCW_MBUE, fmt1)) {
+		report(0, "Enabling measurement_block_format");
+		return false;
+	}
+
+	for (i = 0; i < SCHM_UPDATE_CNT; i++) {
+		if (!do_test_sense()) {
+			report(0, "Error during sense");
+			return false;
+		}
+	}
+
+	return true;
+}
+
+/*
+ * test_schm_fmt0:
+ * With measurement block format 0 a memory space is shared
+ * by all subchannels, each subchannel can provide an index
+ * for the measurement block facility to store the measurements.
+ */
+static void test_schm_fmt0(void)
+{
+	struct measurement_block_format0 *mb0;
+	int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
+
+	/* Allocate zeroed Measurement block */
+	mb0 = alloc_io_mem(shared_mb_size, 0);
+	if (!mb0) {
+		report_abort("measurement_block_format0 allocation failed");
+		return;
+	}
+
+	schm(NULL, 0); /* Stop any previous measurement */
+	schm(mb0, SCHM_MBU);
+
+	/* Expect success */
+	report_prefix_push("Valid MB address and index 0");
+	report(start_measuring(0, 0, false) &&
+	       mb0->ssch_rsch_count == SCHM_UPDATE_CNT,
+	       "SSCH measured %d", mb0->ssch_rsch_count);
+	report_prefix_pop();
+
+	/* Clear the measurement block for the next test */
+	memset(mb0, 0, shared_mb_size);
+
+	/* Expect success */
+	report_prefix_push("Valid MB address and index 1");
+	report(start_measuring(0, 1, false) &&
+	       mb0[1].ssch_rsch_count == SCHM_UPDATE_CNT,
+	       "SSCH measured %d", mb0[1].ssch_rsch_count);
+	report_prefix_pop();
+
+	/* Stop the measurement */
+	css_disable_mb(test_device_sid);
+	schm(NULL, 0);
+
+	free_io_mem(mb0, shared_mb_size);
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -194,6 +259,7 @@ static struct {
 	{ "enable (msch)", test_enable },
 	{ "sense (ssch/tsch)", test_sense },
 	{ "measurement block (schm)", test_schm },
+	{ "measurement block format0", test_schm_fmt0 },
 	{ NULL, NULL }
 };
 
-- 
2.17.1

[kvm-unit-tests PATCH v4 6/6] s390x: css: testing measurement block format 1

[Pierre Morel]

Measurement block format 1 is made available by the extended
measurement block facility and is indicated in the SCHIB by
the bit in the PMCW.

The MBO is specified in the SCHIB of each channel and the MBO
defined by the SCHM instruction is ignored.

The test of the MB format 1 is just skipped if the feature is
not available.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 40f9efa..c8c8e04 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -107,6 +107,7 @@ struct schib {
 	uint64_t mbo;
 	uint8_t  md[4];
 } __attribute__ ((aligned(4)));
+extern struct schib schib;
 
 struct irb {
 	struct scsw scsw;
@@ -387,4 +388,18 @@ struct measurement_block_format0 {
 	uint32_t initial_cmd_resp_time;
 };
 
+struct measurement_block_format1 {
+	uint32_t ssch_rsch_count;
+	uint32_t sample_count;
+	uint32_t device_connect_time;
+	uint32_t function_pending_time;
+	uint32_t device_disconnect_time;
+	uint32_t cu_queuing_time;
+	uint32_t device_active_only_time;
+	uint32_t device_busy_time;
+	uint32_t initial_cmd_resp_time;
+	uint32_t irq_delay_time;
+	uint32_t irq_prio_delay_time;
+};
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 77b39c7..a43da5c 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -19,7 +19,7 @@
 #include <malloc_io.h>
 #include <css.h>
 
-static struct schib schib;
+struct schib schib;
 struct chsc_scsc *chsc_scsc;
 
 static const char * const chsc_rsp_description[] = {
diff --git a/s390x/css.c b/s390x/css.c
index 3915ed3..5723808 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -249,6 +249,80 @@ static void test_schm_fmt0(void)
 	free_io_mem(mb0, shared_mb_size);
 }
 
+static void msch_with_wrong_fmt1_mbo(unsigned int schid, uint64_t mb)
+{
+	struct pmcw *pmcw = &schib.pmcw;
+	int cc;
+
+	/* Read the SCHIB for this subchannel */
+	cc = stsch(schid, &schib);
+	if (cc) {
+		report(0, "stsch: sch %08x failed with cc=%d", schid, cc);
+		return;
+	}
+
+	/* Update the SCHIB to enable the measurement block */
+	pmcw->flags |= PMCW_MBUE;
+	pmcw->flags2 |= PMCW_MBF1;
+	schib.mbo = mb;
+
+	/* Tell the CSS we want to modify the subchannel */
+	expect_pgm_int();
+	cc = msch(schid, &schib);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+}
+
+/*
+ * test_schm_fmt1:
+ * With measurement block format 1 the measurement block is
+ * dedicated to a subchannel.
+ */
+static void test_schm_fmt1(void)
+{
+	struct measurement_block_format1 *mb1;
+
+	if (!test_device_sid) {
+		report_skip("No device");
+		return;
+	}
+
+	if (!css_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK)) {
+		report_skip("Extended measurement block not available");
+		return;
+	}
+
+	/* Allocate zeroed Measurement block */
+	mb1 = alloc_io_mem(sizeof(struct measurement_block_format1), 0);
+	if (!mb1) {
+		report_abort("measurement_block_format1 allocation failed");
+		return;
+	}
+
+	schm(NULL, 0); /* Stop any previous measurement */
+	schm(0, SCHM_MBU);
+
+	/* Expect error for non aligned MB */
+	report_prefix_push("Unaligned MB origin");
+	msch_with_wrong_fmt1_mbo(test_device_sid, (uint64_t)mb1 + 1);
+	report_prefix_pop();
+
+	/* Clear the measurement block for the next test */
+	memset(mb1, 0, sizeof(*mb1));
+
+	/* Expect success */
+	report_prefix_push("Valid MB origin");
+	report(start_measuring((u64)mb1, 0, true) &&
+	       mb1->ssch_rsch_count == SCHM_UPDATE_CNT,
+	       "SSCH measured %d", mb1->ssch_rsch_count);
+	report_prefix_pop();
+
+	/* Stop the measurement */
+	css_disable_mb(test_device_sid);
+	schm(NULL, 0);
+
+	free_io_mem(mb1, sizeof(struct measurement_block_format1));
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -260,6 +334,7 @@ static struct {
 	{ "sense (ssch/tsch)", test_sense },
 	{ "measurement block (schm)", test_schm },
 	{ "measurement block format0", test_schm_fmt0 },
+	{ "measurement block format1", test_schm_fmt1 },
 	{ NULL, NULL }
 };
 
-- 
2.17.1

Re: [kvm-unit-tests PATCH v4 1/6] s390x: css: Store CSS Characteristics

[Janosch Frank]

On 3/1/21 12:47 PM, Pierre Morel wrote:
> CSS characteristics exposes the features of the Channel SubSystem.
> Let's use Store Channel Subsystem Characteristics to retrieve
> the features of the CSS.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

Small nits below

> ---
>  lib/s390x/css.h     | 67 ++++++++++++++++++++++++++++++++
>  lib/s390x/css_lib.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-
>  s390x/css.c         |  8 ++++
>  3 files changed, 167 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 3e57445..4210472 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -288,4 +288,71 @@ int css_residual_count(unsigned int schid);
>  void enable_io_isc(uint8_t isc);
>  int wait_and_check_io_completion(int schid);
>  
> +/*
> + * CHSC definitions
> + */
> +struct chsc_header {
> +	uint16_t len;
> +	uint16_t code;
> +};
> +
> +/* Store Channel Subsystem Characteristics */
> +struct chsc_scsc {
> +	struct chsc_header req;
> +	uint16_t req_fmt;
> +	uint8_t cssid;
> +	uint8_t res_03;
> +	uint32_t res_04[2];

I find the naming a bit weird and it could be one uint8_t field.

> +	struct chsc_header res;
> +	uint32_t res_fmt;
> +	uint64_t general_char[255];
> +	uint64_t chsc_char[254];
> +};
> +
> +extern struct chsc_scsc *chsc_scsc;
> +#define CHSC_SCSC	0x0010
> +#define CHSC_SCSC_LEN	0x0010
> +
> +bool get_chsc_scsc(void);
> +
> +#define CSS_GENERAL_FEAT_BITLEN	(255 * 64)
> +#define CSS_CHSC_FEAT_BITLEN	(254 * 64)
> +
> +#define CHSC_SCSC	0x0010
> +#define CHSC_SCSC_LEN	0x0010
> +
> +#define CHSC_ERROR	0x0000
> +#define CHSC_RSP_OK	0x0001
> +#define CHSC_RSP_INVAL	0x0002
> +#define CHSC_RSP_REQERR	0x0003
> +#define CHSC_RSP_ENOCMD	0x0004
> +#define CHSC_RSP_NODATA	0x0005
> +#define CHSC_RSP_SUP31B	0x0006
> +#define CHSC_RSP_EFRMT	0x0007
> +#define CHSC_RSP_ECSSID	0x0008
> +#define CHSC_RSP_ERFRMT	0x0009
> +#define CHSC_RSP_ESSID	0x000A
> +#define CHSC_RSP_EBUSY	0x000B
> +#define CHSC_RSP_MAX	0x000B
> +
> +static inline int _chsc(void *p)
> +{
> +	int cc;
> +
> +	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
> +		     " ipm     %0\n"
> +		     " srl     %0,28\n"
> +		     : "=d" (cc), "=m" (p)
> +		     : "d" (p), "m" (p)
> +		     : "cc");
> +
> +	return cc;
> +}
> +
> +bool chsc(void *p, uint16_t code, uint16_t len);
> +
> +#include <bitops.h>
> +#define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
> +#define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
> +
>  #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 3c24480..f46e871 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -15,11 +15,102 @@
>  #include <asm/arch_def.h>
>  #include <asm/time.h>
>  #include <asm/arch_def.h>
> -
> +#include <alloc_page.h>

Did you intend to remove the newline here?

>  #include <malloc_io.h>
>  #include <css.h>
>  
>  static struct schib schib;
> +struct chsc_scsc *chsc_scsc;
> +
> +static const char * const chsc_rsp_description[] = {
> +	"CHSC unknown error",
> +	"Command executed",
> +	"Invalid command",
> +	"Request-block error",
> +	"Command not installed",
> +	"Data not available",
> +	"Absolute address of channel-subsystem communication block exceeds 2G - 1.",
> +	"Invalid command format",
> +	"Invalid channel-subsystem identification (CSSID)",
> +	"The command-request block specified an invalid format for the command response block.",
> +	"Invalid subchannel-set identification (SSID)",
> +	"A busy condition precludes execution.",
> +};
> +
> +static bool check_response(void *p)
> +{
> +	struct chsc_header *h = p;
> +
> +	if (h->code == CHSC_RSP_OK)
> +		return true;
> +
> +	if (h->code > CHSC_RSP_MAX)
> +		h->code = 0;
> +
> +	report_abort("Response code %04x: %s", h->code,
> +		      chsc_rsp_description[h->code]);
> +	return false;
> +}
> +
> +bool chsc(void *p, uint16_t code, uint16_t len)
> +{
> +	struct chsc_header *h = p;
> +
> +	h->code = code;
> +	h->len = len;
> +
> +	switch (_chsc(p)) {
> +	case 3:
> +		report_abort("Subchannel invalid or not enabled.");
> +		break;
> +	case 2:
> +		report_abort("CHSC subchannel busy.");
> +		break;
> +	case 1:
> +		report_abort("Subchannel invalid or not enabled.");
> +		break;
> +	case 0:
> +		return check_response(p + len);
> +	}
> +	return false;
> +}
> +
> +bool get_chsc_scsc(void)
> +{
> +	int i, n;
> +	char buffer[510];
> +	char *p;
> +
> +	if (chsc_scsc) /* chsc_scsc already initialized */
> +		return true;
> +
> +	chsc_scsc = alloc_page();
> +	if (!chsc_scsc) {
> +		report_abort("could not allocate chsc_scsc page!");
> +		return false;
> +	}
> +
> +	if (!chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN))
> +		return false;
> +
> +	for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++) {
> +		if (css_general_feature(i)) {
> +			n = snprintf(p, sizeof(buffer), "%d,", i);
> +			p += n;
> +		}
> +	}
> +	report_info("General features: %s", buffer);
> +
> +	for (i = 0, p = buffer; i < CSS_CHSC_FEAT_BITLEN; i++) {
> +		if (css_chsc_feature(i)) {
> +			n = snprintf(p, sizeof(buffer), "%d,", i);
> +			p += n;
> +		}
> +	}
> +	report_info("CHSC features: %s", buffer);
> +
> +	return true;
> +}
>  
>  /*
>   * css_enumerate:
> diff --git a/s390x/css.c b/s390x/css.c
> index 1a61a5c..09703c1 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -14,6 +14,7 @@
>  #include <string.h>
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
> +#include <alloc_page.h>
>  
>  #include <malloc_io.h>
>  #include <css.h>
> @@ -140,10 +141,17 @@ error_senseid:
>  	unregister_io_int_func(css_irq_io);
>  }
>  
> +static void css_init(void)
> +{
> +	report(!!get_chsc_scsc(), "Store Channel Characteristics");

get_chsc_scsc() returns a bool, so you shouldn't need the !! here, no?

> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
>  } tests[] = {
> +	/* The css_init test is needed to initialize the CSS Characteristics */
> +	{ "initialize CSS (chsc)", css_init },
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
>  	{ "sense (ssch/tsch)", test_sense },
> 

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Janosch Frank]

On 3/1/21 12:47 PM, Pierre Morel wrote:
> In order to ease the writing of tests based on:
> - interrupt
> - enabling a subchannel
> - using multiple I/O on a channel without disabling it
> 
> We do the following simplifications:
> - the I/O interrupt handler is registered on CSS initialization
> - We do not enable again a subchannel in senseid if it is already
>   enabled
> - we add a css_enabled() function to test if a subchannel is enabled
> 
> 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 | 37 ++++++++++++++++++++++++++-----------
>  s390x/css.c         | 44 ++++++++++++++++++++++++--------------------
>  3 files changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 4210472..fbfa034 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -278,6 +278,7 @@ int css_enumerate(void);
>  
>  #define IO_SCH_ISC      3
>  int css_enable(int schid, int isc);
> +bool css_enabled(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 f46e871..41134dc 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -161,6 +161,31 @@ out:
>  	return schid;
>  }
>  
> +/*
> + * css_enabled: report if the subchannel is enabled
> + * @schid: Subchannel Identifier
> + * Return value:
> + *   true if the subchannel is enabled
> + *   false otherwise
> + */
> +bool css_enabled(int schid)
> +{
> +	struct pmcw *pmcw = &schib.pmcw;
> +	int cc;
> +
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return false;
> +	}
> +
> +	if (!(pmcw->flags & PMCW_ENABLE)) {
> +		report_info("stsch: sch %08x not enabled", schid);
> +		return false;
> +	}
> +	return true;
> +}
>  /*
>   * css_enable: enable the subchannel with the specified ISC
>   * @schid: Subchannel Identifier
> @@ -210,18 +235,8 @@ retry:
>  	/*
>  	 * 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 & flags) == flags) {
> -		report_info("stsch: sch %08x successfully modified after %d retries",
> -			    schid, retry_count);
> +	if (css_enabled(schid))
>  		return 0;
> -	}
>  
>  	if (retry_count++ < MAX_ENABLE_RETRIES) {
>  		mdelay(10); /* the hardware was not ready, give it some time */
> diff --git a/s390x/css.c b/s390x/css.c
> index 09703c1..c9e4903 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -56,36 +56,27 @@ static void test_enable(void)
>   * - We need the test device as the first recognized
>   *   device by the enumeration.
>   */
> -static void test_sense(void)
> +static bool do_test_sense(void)
>  {
>  	struct ccw1 *ccw;
> +	bool success = false;

That is a very counter-intuitive name, something like "retval" might be
better.
You're free to use the normal int returns but unfortunately you can't
use the E* error constants like ENOMEM.

>  	int ret;
>  	int len;
>  
>  	if (!test_device_sid) {
>  		report_skip("No device");
> -		return;
> +		return success;
>  	}
>  
> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
> -	if (ret) {
> -		report(0, "Could not enable the subchannel: %08x",
> -		       test_device_sid);
> -		return;
> +	if (!css_enabled(test_device_sid)) {
> +		report(0, "enabling subchannel %08x", test_device_sid);
> +		return success;
>  	}
>  
> -	ret = register_io_int_func(css_irq_io);
> -	if (ret) {
> -		report(0, "Could not register IRQ handler");
> -		return;
> -	}
> -
> -	lowcore_ptr->io_int_param = 0;
> -
>  	senseid = alloc_io_mem(sizeof(*senseid), 0);
>  	if (!senseid) {
>  		report(0, "Allocation of senseid");
> -		goto error_senseid;
> +		return success;
>  	}
>  
>  	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
> @@ -129,21 +120,34 @@ static void test_sense(void)
>  	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>  		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>  		    senseid->dev_type, senseid->dev_model);
> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
> +		    senseid->cu_type);
>  
> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
> -	       (uint16_t)cu_type, senseid->cu_type);
> +	success = senseid->cu_type == cu_type;
>  
>  error:
>  	free_io_mem(ccw, sizeof(*ccw));
>  error_ccw:
>  	free_io_mem(senseid, sizeof(*senseid));
> -error_senseid:
> -	unregister_io_int_func(css_irq_io);
> +	return success;
> +}
> +
> +static void test_sense(void)
> +{
> +	report(do_test_sense(), "Got CU type expected");
>  }
>  
>  static void css_init(void)
>  {
>  	report(!!get_chsc_scsc(), "Store Channel Characteristics");
> +
> +	if (register_io_int_func(css_irq_io)) {
> +		report(0, "Could not register IRQ handler");
> +		return;

assert() please

> +	}
> +	lowcore_ptr->io_int_param = 0> +
> +	report(1, "CSS initialized");
>  }
>  
>  static struct {
> 

Re: [kvm-unit-tests PATCH v4 4/6] s390x: css: implementing Set CHannel Monitor

[Janosch Frank]

On 3/1/21 12:47 PM, Pierre Morel wrote:
> We implement the call of the Set CHannel Monitor instruction,
> starting the monitoring of the all Channel Sub System, and
> initializing channel subsystem monitoring.
> 
> Initial tests report the presence of the extended measurement block
> feature, and verify the error reporting of the hypervisor for SCHM.

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

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/css.h | 12 ++++++++++++
>  s390x/css.c     | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 1cb3de2..b8ac363 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -310,6 +310,7 @@ struct chsc_scsc {
>  	uint32_t res_04[2];
>  	struct chsc_header res;
>  	uint32_t res_fmt;
> +#define CSSC_EXTENDED_MEASUREMENT_BLOCK 48
>  	uint64_t general_char[255];
>  	uint64_t chsc_char[254];
>  };
> @@ -360,6 +361,17 @@ bool chsc(void *p, uint16_t code, uint16_t len);
>  #define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
>  #define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)

css_test_general_feature(bit)
css_test_chsc_feature(bit)

?

>  
> +#define SCHM_DCTM	1 /* activate Device Connection TiMe */
> +#define SCHM_MBU	2 /* activate Measurement Block Update */
> +
> +static inline void schm(void *mbo, unsigned int flags)
> +{
> +	register void *__gpr2 asm("2") = mbo;
> +	register long __gpr1 asm("1") = flags;
> +
> +	asm("schm" : : "d" (__gpr2), "d" (__gpr1));
> +}
> +
>  bool css_enable_mb(int sid, uint64_t mb, uint16_t mbi, uint16_t flg, bool fmt1);
>  bool css_disable_mb(int schid);
>  
> diff --git a/s390x/css.c b/s390x/css.c
> index c9e4903..e8f96f3 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -150,6 +150,40 @@ static void css_init(void)
>  	report(1, "CSS initialized");
>  }
>  
> +static void test_schm(void)
> +{
> +	if (css_general_feature(CSSC_EXTENDED_MEASUREMENT_BLOCK))
> +		report_info("Extended measurement block available");
> +
> +	/* bits 59-63 of MB address must be 0  if MBU is defined */
> +	report_prefix_push("Unaligned operand");
> +	expect_pgm_int();
> +	schm((void *)0x01, SCHM_MBU);
> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +	report_prefix_pop();
> +
> +	/* bits 36-61 of register 1 (flags) must be 0 */
> +	report_prefix_push("Bad flags");
> +	expect_pgm_int();
> +	schm(NULL, 0xfffffffc);
> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
> +	report_prefix_pop();
> +
> +	/* SCHM is a privilege operation */
> +	report_prefix_push("Privilege");
> +	enter_pstate();
> +	expect_pgm_int();
> +	schm(NULL, SCHM_MBU);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	/* Normal operation */
> +	report_prefix_push("Normal operation");
> +	schm(NULL, SCHM_MBU);
> +	report(1, "SCHM call without address");
> +	report_prefix_pop();
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -159,6 +193,7 @@ static struct {
>  	{ "enumerate (stsch)", test_enumerate },
>  	{ "enable (msch)", test_enable },
>  	{ "sense (ssch/tsch)", test_sense },
> +	{ "measurement block (schm)", test_schm },
>  	{ NULL, NULL }
>  };
>  
> 

Re: [kvm-unit-tests PATCH v4 5/6] s390x: css: testing measurement block format 0

[Janosch Frank]

On 3/1/21 12:47 PM, Pierre Morel wrote:
> We test the update of the measurement block format 0, the
> measurement block origin is calculated from the mbo argument
> used by the SCHM instruction and the offset calculated using
> the measurement block index of the SCHIB.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/css.h | 12 +++++++++
>  s390x/css.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index b8ac363..40f9efa 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -375,4 +375,16 @@ static inline void schm(void *mbo, unsigned int flags)
>  bool css_enable_mb(int sid, uint64_t mb, uint16_t mbi, uint16_t flg, bool fmt1);
>  bool css_disable_mb(int schid);
>  
> +struct measurement_block_format0 {
> +	uint16_t ssch_rsch_count;
> +	uint16_t sample_count;
> +	uint32_t device_connect_time;
> +	uint32_t function_pending_time;
> +	uint32_t device_disconnect_time;
> +	uint32_t cu_queuing_time;
> +	uint32_t device_active_only_time;
> +	uint32_t device_busy_time;
> +	uint32_t initial_cmd_resp_time;
> +};
> +
>  #endif
> diff --git a/s390x/css.c b/s390x/css.c
> index e8f96f3..3915ed3 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -184,6 +184,71 @@ static void test_schm(void)
>  	report_prefix_pop();
>  }
>  
> +#define SCHM_UPDATE_CNT 10
> +static bool start_measuring(uint64_t mbo, uint16_t mbi, bool fmt1)
> +{
> +	int i;
> +
> +	if (!css_enable_mb(test_device_sid, mbo, mbi, PMCW_MBUE, fmt1)) {
> +		report(0, "Enabling measurement_block_format");
> +		return false;
> +	}
> +
> +	for (i = 0; i < SCHM_UPDATE_CNT; i++) {
> +		if (!do_test_sense()) {
> +			report(0, "Error during sense");
> +			return false;
Are these hard fails, i.e. would it make sense to stop testing if this
or the css_enable_mb() above fails?

> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * test_schm_fmt0:
> + * With measurement block format 0 a memory space is shared
> + * by all subchannels, each subchannel can provide an index
> + * for the measurement block facility to store the measurements.
> + */
> +static void test_schm_fmt0(void)
> +{
> +	struct measurement_block_format0 *mb0;
> +	int shared_mb_size = 2 * sizeof(struct measurement_block_format0);
> +
> +	/* Allocate zeroed Measurement block */
> +	mb0 = alloc_io_mem(shared_mb_size, 0);
> +	if (!mb0) {
> +		report_abort("measurement_block_format0 allocation failed");
> +		return;
> +	}
> +
> +	schm(NULL, 0); /* Stop any previous measurement */
> +	schm(mb0, SCHM_MBU);
> +
> +	/* Expect success */
> +	report_prefix_push("Valid MB address and index 0");
> +	report(start_measuring(0, 0, false) &&
> +	       mb0->ssch_rsch_count == SCHM_UPDATE_CNT,
> +	       "SSCH measured %d", mb0->ssch_rsch_count);
> +	report_prefix_pop();
> +
> +	/* Clear the measurement block for the next test */
> +	memset(mb0, 0, shared_mb_size);
> +
> +	/* Expect success */
> +	report_prefix_push("Valid MB address and index 1");
> +	report(start_measuring(0, 1, false) &&
> +	       mb0[1].ssch_rsch_count == SCHM_UPDATE_CNT,
> +	       "SSCH measured %d", mb0[1].ssch_rsch_count);
> +	report_prefix_pop();
> +
> +	/* Stop the measurement */
> +	css_disable_mb(test_device_sid);
> +	schm(NULL, 0);
> +
> +	free_io_mem(mb0, shared_mb_size);
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -194,6 +259,7 @@ static struct {
>  	{ "enable (msch)", test_enable },
>  	{ "sense (ssch/tsch)", test_sense },
>  	{ "measurement block (schm)", test_schm },
> +	{ "measurement block format0", test_schm_fmt0 },
>  	{ NULL, NULL }
>  };
>  
> 

Re: [kvm-unit-tests PATCH v4 5/6] s390x: css: testing measurement block format 0

[Cornelia Huck]

On Mon, 1 Mar 2021 16:54:57 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/1/21 12:47 PM, Pierre Morel wrote:
> > We test the update of the measurement block format 0, the
> > measurement block origin is calculated from the mbo argument
> > used by the SCHM instruction and the offset calculated using
> > the measurement block index of the SCHIB.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  lib/s390x/css.h | 12 +++++++++
> >  s390x/css.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 78 insertions(+)
> > 

(...)

> > diff --git a/s390x/css.c b/s390x/css.c
> > index e8f96f3..3915ed3 100644
> > --- a/s390x/css.c
> > +++ b/s390x/css.c
> > @@ -184,6 +184,71 @@ static void test_schm(void)
> >  	report_prefix_pop();
> >  }
> >  
> > +#define SCHM_UPDATE_CNT 10
> > +static bool start_measuring(uint64_t mbo, uint16_t mbi, bool fmt1)
> > +{
> > +	int i;
> > +
> > +	if (!css_enable_mb(test_device_sid, mbo, mbi, PMCW_MBUE, fmt1)) {
> > +		report(0, "Enabling measurement_block_format");
> > +		return false;
> > +	}
> > +
> > +	for (i = 0; i < SCHM_UPDATE_CNT; i++) {
> > +		if (!do_test_sense()) {
> > +			report(0, "Error during sense");
> > +			return false;  
> Are these hard fails, i.e. would it make sense to stop testing if this
> or the css_enable_mb() above fails?

I think so; if we can't even enable the mb or send a sense, there's
something really broken.

(...)

Otherwise, this looks good to me (same for the next patch.)

Re: [kvm-unit-tests PATCH v4 0/6] CSS Mesurement Block

[Cornelia Huck]

On Mon,  1 Mar 2021 12:46:59 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We tests the update of the Mesurement Block (MB) format 0
> and format 1 using a serie of senseid requests.
> 
> *Warning*: One of the tests for format-1 will unexpectedly fail for QEMU elf
> unless the QEMU patch "css: SCHIB measurement block origin must be aligned"
> is applied.
> With Protected Virtualization, the PGM is correctly recognized.
> 
> The MB format 1 is only provided if the Extended mesurement Block
> feature is available.
> 
> This feature is exposed by the CSS characteristics general features
> stored by the Store Channel Subsystem Characteristics CHSC command,
> consequently, we implement the CHSC instruction call and the SCSC CHSC
> command.
> 
> In order to ease the writing of new tests using:
> - interrupt
> - enablement of a subchannel
> - multiple I/O on a subchannel
> 
> We do the following simplifications:
> - we create a CSS initialization routine
> - we register the I/O interrupt handler on CSS initialization
> - we do not enable or disable a subchannel in the senseid test,
>   assuming this test is done after the enable test, this allows
>   to create traffic using the SSCH used by senseid.
> - we add a css_enabled() function to test if a subchannel is enabled.
> 
> @Connie, I restructured the patches but I did not modify the
> functionalities, so I kept your R-B, I hope you are OK with this.

Yes, that's fine with me.

> 
> Regards,
> Pierre
> 
> Pierre Morel (6):
>   s390x: css: Store CSS Characteristics
>   s390x: css: simplifications of the tests
>   s390x: css: extending the subchannel modifying functions
>   s390x: css: implementing Set CHannel Monitor
>   s390x: css: testing measurement block format 0
>   s390x: css: testing measurement block format 1
> 
>  lib/s390x/css.h     | 116 +++++++++++++++++++++-
>  lib/s390x/css_lib.c | 232 +++++++++++++++++++++++++++++++++++++++++---
>  s390x/css.c         | 228 +++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 542 insertions(+), 34 deletions(-)
> 

Re: [kvm-unit-tests PATCH v4 1/6] s390x: css: Store CSS Characteristics

[Pierre Morel]

On 3/1/21 3:45 PM, Janosch Frank wrote:
> On 3/1/21 12:47 PM, Pierre Morel wrote:
>> CSS characteristics exposes the features of the Channel SubSystem.
>> Let's use Store Channel Subsystem Characteristics to retrieve
>> the features of the CSS.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Small nits below
> 
>> ---
>>   lib/s390x/css.h     | 67 ++++++++++++++++++++++++++++++++
>>   lib/s390x/css_lib.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-
>>   s390x/css.c         |  8 ++++
>>   3 files changed, 167 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 3e57445..4210472 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -288,4 +288,71 @@ int css_residual_count(unsigned int schid);
>>   void enable_io_isc(uint8_t isc);
>>   int wait_and_check_io_completion(int schid);
>>   
>> +/*
>> + * CHSC definitions
>> + */
>> +struct chsc_header {
>> +	uint16_t len;
>> +	uint16_t code;
>> +};
>> +
>> +/* Store Channel Subsystem Characteristics */
>> +struct chsc_scsc {
>> +	struct chsc_header req;
>> +	uint16_t req_fmt;
>> +	uint8_t cssid;
>> +	uint8_t res_03;
>> +	uint32_t res_04[2];
> 
> I find the naming a bit weird and it could be one uint8_t field.

OK

> 
>> +	struct chsc_header res;
>> +	uint32_t res_fmt;
>> +	uint64_t general_char[255];
>> +	uint64_t chsc_char[254];
>> +};

... snip

>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 3c24480..f46e871 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -15,11 +15,102 @@
>>   #include <asm/arch_def.h>
>>   #include <asm/time.h>
>>   #include <asm/arch_def.h>
>> -
>> +#include <alloc_page.h>
> 
> Did you intend to remove the newline here?

Yes I do not see why we should have a new line here.
But I can keep it if you want.

> 
... snip...

>>   #include <asm/arch_def.h>
>> +#include <alloc_page.h>
>>   
>>   #include <malloc_io.h>
>>   #include <css.h>
>> @@ -140,10 +141,17 @@ error_senseid:
>>   	unregister_io_int_func(css_irq_io);
>>   }
>>   
>> +static void css_init(void)
>> +{
>> +	report(!!get_chsc_scsc(), "Store Channel Characteristics");
> 
> get_chsc_scsc() returns a bool, so you shouldn't need the !! here, no?

Yes, forgotten when changed get_chsc_scsc(), remove the !!


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Pierre Morel]

On 3/1/21 4:00 PM, Janosch Frank wrote:
> On 3/1/21 12:47 PM, Pierre Morel wrote:
>> In order to ease the writing of tests based on:

...snip...

>> -static void test_sense(void)
>> +static bool do_test_sense(void)
>>   {
>>   	struct ccw1 *ccw;
>> +	bool success = false;
> 
> That is a very counter-intuitive name, something like "retval" might be
> better.
> You're free to use the normal int returns but unfortunately you can't
> use the E* error constants like ENOMEM.

hum, I had retval and changed it to success on a proposition of Thomas...
I find it more intuitive as a bool since this function succeed or fail, 
no half way and is used for the reporting.

other opinion?


> 
>>   	int ret;
>>   	int len;
>>   
>>   	if (!test_device_sid) {
>>   		report_skip("No device");
>> -		return;
>> +		return success;
>>   	}
>>   
>> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
>> -	if (ret) {
>> -		report(0, "Could not enable the subchannel: %08x",
>> -		       test_device_sid);
>> -		return;
>> +	if (!css_enabled(test_device_sid)) {
>> +		report(0, "enabling subchannel %08x", test_device_sid);
>> +		return success;
>>   	}
>>   
>> -	ret = register_io_int_func(css_irq_io);
>> -	if (ret) {
>> -		report(0, "Could not register IRQ handler");
>> -		return;
>> -	}
>> -
>> -	lowcore_ptr->io_int_param = 0;
>> -
>>   	senseid = alloc_io_mem(sizeof(*senseid), 0);
>>   	if (!senseid) {
>>   		report(0, "Allocation of senseid");
>> -		goto error_senseid;
>> +		return success;
>>   	}
>>   
>>   	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
>> @@ -129,21 +120,34 @@ static void test_sense(void)
>>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>>   		    senseid->dev_type, senseid->dev_model);
>> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
>> +		    senseid->cu_type);
>>   
>> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
>> -	       (uint16_t)cu_type, senseid->cu_type);
>> +	success = senseid->cu_type == cu_type;
>>   
>>   error:
>>   	free_io_mem(ccw, sizeof(*ccw));
>>   error_ccw:
>>   	free_io_mem(senseid, sizeof(*senseid));
>> -error_senseid:
>> -	unregister_io_int_func(css_irq_io);
>> +	return success;
>> +}
>> +
>> +static void test_sense(void)
>> +{
>> +	report(do_test_sense(), "Got CU type expected");
>>   }
>>   
>>   static void css_init(void)
>>   {
>>   	report(!!get_chsc_scsc(), "Store Channel Characteristics");
>> +
>> +	if (register_io_int_func(css_irq_io)) {
>> +		report(0, "Could not register IRQ handler");
>> +		return;
> 
> assert() please

Yes.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 4/6] s390x: css: implementing Set CHannel Monitor

[Pierre Morel]

On 3/1/21 4:32 PM, Janosch Frank wrote:
> On 3/1/21 12:47 PM, Pierre Morel wrote:
>> We implement the call of the Set CHannel Monitor instruction,
>> starting the monitoring of the all Channel Sub System, and
>> initializing channel subsystem monitoring.
>>
>> Initial tests report the presence of the extended measurement block
>> feature, and verify the error reporting of the hypervisor for SCHM.
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   lib/s390x/css.h | 12 ++++++++++++
>>   s390x/css.c     | 35 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 1cb3de2..b8ac363 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -310,6 +310,7 @@ struct chsc_scsc {
>>   	uint32_t res_04[2];
>>   	struct chsc_header res;
>>   	uint32_t res_fmt;
>> +#define CSSC_EXTENDED_MEASUREMENT_BLOCK 48
>>   	uint64_t general_char[255];
>>   	uint64_t chsc_char[254];
>>   };
>> @@ -360,6 +361,17 @@ bool chsc(void *p, uint16_t code, uint16_t len);
>>   #define css_general_feature(bit) test_bit_inv(bit, chsc_scsc->general_char)
>>   #define css_chsc_feature(bit) test_bit_inv(bit, chsc_scsc->chsc_char)
> 
> css_test_general_feature(bit)
> css_test_chsc_feature(bit)
> 
> ?


Yes,

Thanks,
Pierre
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Janosch Frank]

On 3/8/21 3:13 PM, Pierre Morel wrote:
> 
> 
> On 3/1/21 4:00 PM, Janosch Frank wrote:
>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>> In order to ease the writing of tests based on:
> 
> ...snip...
> 
>>> -static void test_sense(void)
>>> +static bool do_test_sense(void)
>>>   {
>>>   	struct ccw1 *ccw;
>>> +	bool success = false;
>>
>> That is a very counter-intuitive name, something like "retval" might be
>> better.
>> You're free to use the normal int returns but unfortunately you can't
>> use the E* error constants like ENOMEM.
> 
> hum, I had retval and changed it to success on a proposition of Thomas...
> I find it more intuitive as a bool since this function succeed or fail, 
> no half way and is used for the reporting.
> 
> other opinion?

Alright, it's 2:1 for "success", so keep it if you want.

> 
> 
>>
>>>   	int ret;
>>>   	int len;
>>>   
>>>   	if (!test_device_sid) {
>>>   		report_skip("No device");
>>> -		return;
>>> +		return success;
>>>   	}
>>>   
>>> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
>>> -	if (ret) {
>>> -		report(0, "Could not enable the subchannel: %08x",
>>> -		       test_device_sid);
>>> -		return;
>>> +	if (!css_enabled(test_device_sid)) {
>>> +		report(0, "enabling subchannel %08x", test_device_sid);
>>> +		return success;
>>>   	}
>>>   
>>> -	ret = register_io_int_func(css_irq_io);
>>> -	if (ret) {
>>> -		report(0, "Could not register IRQ handler");
>>> -		return;
>>> -	}
>>> -
>>> -	lowcore_ptr->io_int_param = 0;
>>> -
>>>   	senseid = alloc_io_mem(sizeof(*senseid), 0);
>>>   	if (!senseid) {
>>>   		report(0, "Allocation of senseid");
>>> -		goto error_senseid;
>>> +		return success;
>>>   	}
>>>   
>>>   	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
>>> @@ -129,21 +120,34 @@ static void test_sense(void)
>>>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>>>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>>>   		    senseid->dev_type, senseid->dev_model);
>>> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
>>> +		    senseid->cu_type);
>>>   
>>> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
>>> -	       (uint16_t)cu_type, senseid->cu_type);
>>> +	success = senseid->cu_type == cu_type;
>>>   
>>>   error:
>>>   	free_io_mem(ccw, sizeof(*ccw));
>>>   error_ccw:
>>>   	free_io_mem(senseid, sizeof(*senseid));
>>> -error_senseid:
>>> -	unregister_io_int_func(css_irq_io);
>>> +	return success;
>>> +}
>>> +
>>> +static void test_sense(void)
>>> +{
>>> +	report(do_test_sense(), "Got CU type expected");
>>>   }
>>>   
>>>   static void css_init(void)
>>>   {
>>>   	report(!!get_chsc_scsc(), "Store Channel Characteristics");
>>> +
>>> +	if (register_io_int_func(css_irq_io)) {
>>> +		report(0, "Could not register IRQ handler");
>>> +		return;
>>
>> assert() please
> 
> Yes.
> 
> Thanks,
> Pierre
> 

Re: [kvm-unit-tests PATCH v4 1/6] s390x: css: Store CSS Characteristics

[Janosch Frank]

On 3/8/21 3:01 PM, Pierre Morel wrote:
> 
> 
> On 3/1/21 3:45 PM, Janosch Frank wrote:
>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>> CSS characteristics exposes the features of the Channel SubSystem.
>>> Let's use Store Channel Subsystem Characteristics to retrieve
>>> the features of the CSS.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>
>> Small nits below
>>
>>> ---
>>>   lib/s390x/css.h     | 67 ++++++++++++++++++++++++++++++++
>>>   lib/s390x/css_lib.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-
>>>   s390x/css.c         |  8 ++++
>>>   3 files changed, 167 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>>> index 3e57445..4210472 100644
>>> --- a/lib/s390x/css.h
>>> +++ b/lib/s390x/css.h
>>> @@ -288,4 +288,71 @@ int css_residual_count(unsigned int schid);
>>>   void enable_io_isc(uint8_t isc);
>>>   int wait_and_check_io_completion(int schid);
>>>   
>>> +/*
>>> + * CHSC definitions
>>> + */
>>> +struct chsc_header {
>>> +	uint16_t len;
>>> +	uint16_t code;
>>> +};
>>> +
>>> +/* Store Channel Subsystem Characteristics */
>>> +struct chsc_scsc {
>>> +	struct chsc_header req;
>>> +	uint16_t req_fmt;
>>> +	uint8_t cssid;
>>> +	uint8_t res_03;
>>> +	uint32_t res_04[2];
>>
>> I find the naming a bit weird and it could be one uint8_t field.
> 
> OK
> 
>>
>>> +	struct chsc_header res;
>>> +	uint32_t res_fmt;
>>> +	uint64_t general_char[255];
>>> +	uint64_t chsc_char[254];
>>> +};
> 
> ... snip
> 
>>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>>> index 3c24480..f46e871 100644
>>> --- a/lib/s390x/css_lib.c
>>> +++ b/lib/s390x/css_lib.c
>>> @@ -15,11 +15,102 @@
>>>   #include <asm/arch_def.h>
>>>   #include <asm/time.h>
>>>   #include <asm/arch_def.h>
>>> -
>>> +#include <alloc_page.h>
>>
>> Did you intend to remove the newline here?
> 
> Yes I do not see why we should have a new line here.

Some people like to separate asm includes from other includes.

> But I can keep it if you want.

I just wanted to know if you removed it by mistake, if you want it that
way it's ok for me.

> 
>>
> ... snip...
> 
>>>   #include <asm/arch_def.h>
>>> +#include <alloc_page.h>
>>>   
>>>   #include <malloc_io.h>
>>>   #include <css.h>
>>> @@ -140,10 +141,17 @@ error_senseid:
>>>   	unregister_io_int_func(css_irq_io);
>>>   }
>>>   
>>> +static void css_init(void)
>>> +{
>>> +	report(!!get_chsc_scsc(), "Store Channel Characteristics");
>>
>> get_chsc_scsc() returns a bool, so you shouldn't need the !! here, no?
> 
> Yes, forgotten when changed get_chsc_scsc(), remove the !!
> 
> 
> Thanks,
> Pierre
> 

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Pierre Morel]

On 3/8/21 3:36 PM, Janosch Frank wrote:
> On 3/8/21 3:13 PM, Pierre Morel wrote:
>>
>>
>> On 3/1/21 4:00 PM, Janosch Frank wrote:
>>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>>> In order to ease the writing of tests based on:
>>
>> ...snip...
>>
>>>> -static void test_sense(void)
>>>> +static bool do_test_sense(void)
>>>>    {
>>>>    	struct ccw1 *ccw;
>>>> +	bool success = false;
>>>
>>> That is a very counter-intuitive name, something like "retval" might be
>>> better.
>>> You're free to use the normal int returns but unfortunately you can't
>>> use the E* error constants like ENOMEM.
>>
>> hum, I had retval and changed it to success on a proposition of Thomas...
>> I find it more intuitive as a bool since this function succeed or fail,
>> no half way and is used for the reporting.
>>
>> other opinion?
> 
> Alright, it's 2:1 for "success", so keep it if you want.

:) OK thanks

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Thomas Huth]

On 08/03/2021 15.13, Pierre Morel wrote:
> 
> 
> On 3/1/21 4:00 PM, Janosch Frank wrote:
>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>> In order to ease the writing of tests based on:
> 
> ...snip...
> 
>>> -static void test_sense(void)
>>> +static bool do_test_sense(void)
>>>   {
>>>       struct ccw1 *ccw;
>>> +    bool success = false;
>>
>> That is a very counter-intuitive name, something like "retval" might be
>> better.
>> You're free to use the normal int returns but unfortunately you can't
>> use the E* error constants like ENOMEM.
> 
> hum, I had retval and changed it to success on a proposition of Thomas...
> I find it more intuitive as a bool since this function succeed or fail, no 
> half way and is used for the reporting.
> 
> other opinion?

I'd say either "static int ..." + retval (with 0 for success), or "static 
bool ..." and "success" (with true for success) ... but "bool" + "retval" 
sounds confusing to me.

  Thomas

Re: [kvm-unit-tests PATCH v4 5/6] s390x: css: testing measurement block format 0

[Pierre Morel]

On 3/4/21 6:05 PM, Cornelia Huck wrote:
> On Mon, 1 Mar 2021 16:54:57 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>> We test the update of the measurement block format 0, the
>>> measurement block origin is calculated from the mbo argument
>>> used by the SCHM instruction and the offset calculated using
>>> the measurement block index of the SCHIB.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   lib/s390x/css.h | 12 +++++++++
>>>   s390x/css.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 78 insertions(+)
>>>
> 
> (...)
> 
>>> diff --git a/s390x/css.c b/s390x/css.c
>>> index e8f96f3..3915ed3 100644
>>> --- a/s390x/css.c
>>> +++ b/s390x/css.c
>>> @@ -184,6 +184,71 @@ static void test_schm(void)
>>>   	report_prefix_pop();
>>>   }
>>>   
>>> +#define SCHM_UPDATE_CNT 10
>>> +static bool start_measuring(uint64_t mbo, uint16_t mbi, bool fmt1)
>>> +{
>>> +	int i;
>>> +
>>> +	if (!css_enable_mb(test_device_sid, mbo, mbi, PMCW_MBUE, fmt1)) {
>>> +		report(0, "Enabling measurement_block_format");
>>> +		return false;
>>> +	}
>>> +
>>> +	for (i = 0; i < SCHM_UPDATE_CNT; i++) {
>>> +		if (!do_test_sense()) {
>>> +			report(0, "Error during sense");
>>> +			return false;
>> Are these hard fails, i.e. would it make sense to stop testing if this
>> or the css_enable_mb() above fails?
> 
> I think so; if we can't even enable the mb or send a sense, there's
> something really broken.
> 
> (...)
> 
> Otherwise, this looks good to me (same for the next patch.)
> 

Yes, I will change these for report_abort().

Thanks both.
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Pierre Morel]

On 3/8/21 3:41 PM, Thomas Huth wrote:
> On 08/03/2021 15.13, Pierre Morel wrote:
>>
>>
>> On 3/1/21 4:00 PM, Janosch Frank wrote:
>>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>>> In order to ease the writing of tests based on:
>>
>> ...snip...
>>
>>>> -static void test_sense(void)
>>>> +static bool do_test_sense(void)
>>>>   {
>>>>       struct ccw1 *ccw;
>>>> +    bool success = false;
>>>
>>> That is a very counter-intuitive name, something like "retval" might be
>>> better.
>>> You're free to use the normal int returns but unfortunately you can't
>>> use the E* error constants like ENOMEM.
>>
>> hum, I had retval and changed it to success on a proposition of Thomas...
>> I find it more intuitive as a bool since this function succeed or 
>> fail, no half way and is used for the reporting.
>>
>> other opinion?
> 
> I'd say either "static int ..." + retval (with 0 for success), or 
> "static bool ..." and "success" (with true for success) ... but "bool" + 
> "retval" sounds confusing to me.
> 
>   Thomas
> 

OK, thanks, then I keep bool success :)

Thanks
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Pierre Morel]

On 3/8/21 3:41 PM, Thomas Huth wrote:
> On 08/03/2021 15.13, Pierre Morel wrote:
>>
>>
>> On 3/1/21 4:00 PM, Janosch Frank wrote:
>>> On 3/1/21 12:47 PM, Pierre Morel wrote:
>>>> In order to ease the writing of tests based on:
>>
>> ...snip...
>>
>>>> -static void test_sense(void)
>>>> +static bool do_test_sense(void)
>>>>   {
>>>>       struct ccw1 *ccw;
>>>> +    bool success = false;
>>>
>>> That is a very counter-intuitive name, something like "retval" might be
>>> better.
>>> You're free to use the normal int returns but unfortunately you can't
>>> use the E* error constants like ENOMEM.
>>
>> hum, I had retval and changed it to success on a proposition of Thomas...
>> I find it more intuitive as a bool since this function succeed or 
>> fail, no half way and is used for the reporting.
>>
>> other opinion?
> 
> I'd say either "static int ..." + retval (with 0 for success), or 
> "static bool ..." and "success" (with true for success) ... but "bool" + 
> "retval" sounds confusing to me.
> 
>   Thomas
> 


Hum, OK, I think I see were the unsatisfation about this function comes 
from. (I do not like it either)
Slowly understanding the benefit of assert() and report_abort() in the 
tests cases I will rework this part and do not change the test_senseid() 
test.

I will introduce a sense_id() function when needing to do I/O in the 
fmt0 test, asserting in this function that all parts already checked in 
the preceding tests are functional.

This makes all much shorter and cleaner.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen