[kvm-unit-tests PATCH v2 0/5] CSS Mesurement Block

[kvm-unit-tests PATCH v2 0/5] 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.

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.

Regards,
Pierre

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

 lib/s390x/css.h     | 117 ++++++++++++++++++++++-
 lib/s390x/css_lib.c | 223 +++++++++++++++++++++++++++++++++++++++++---
 s390x/css.c         | 186 ++++++++++++++++++++++++++++++++----
 3 files changed, 493 insertions(+), 33 deletions(-)

-- 
2.17.1

changelog:

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 v2 1/5] 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>
---
 lib/s390x/css.h     |  69 +++++++++++++++++++++++++++
 lib/s390x/css_lib.c | 112 +++++++++++++++++++++++++++++++++++++++++++-
 s390x/css.c         |  12 +++++
 3 files changed, 192 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 3e57445..1e317d1 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -288,4 +288,73 @@ 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 {
+	u16 len;
+	u16 code;
+};
+
+/* Store Channel Subsystem Characteristics */
+struct chsc_scsc {
+	struct chsc_header req;
+	u16 req_fmt;
+	u8 cssid;
+	u8 res_03;
+	u32 res_04[2];
+	struct chsc_header res;
+	u32 res_fmt;
+	u64 general_char[255];
+	u64 chsc_char[254];
+};
+extern struct chsc_scsc *chsc_scsc;
+#define CHSC_SCSC	0x0010
+#define CHSC_SCSC_LEN	0x0010
+
+int 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;
+}
+
+int 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..88aa7a1 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -15,11 +15,121 @@
 #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 *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 2 - 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 int check_response(void *p)
+{
+	struct chsc_header *h = p;
+
+	if (h->code == CHSC_RSP_OK) {
+		report(1, "CHSC command completed.");
+		return 0;
+	}
+	if (h->code > CHSC_RSP_MAX)
+		h->code = 0;
+	report(0, "Response code %04x: %s", h->code, chsc_rsp_description[h->code]);
+	return -1;
+}
+
+int chsc(void *p, uint16_t code, uint16_t len)
+{
+	struct chsc_header *h = p;
+	int cc;
+
+	report_prefix_push("Channel Subsystem Call");
+	h->code = code;
+	h->len = len;
+	cc = _chsc(p);
+	switch (cc) {
+	case 3:
+		report(0, "Subchannel invalid or not enabled.");
+		break;
+	case 2:
+		report(0, "CHSC subchannel busy.");
+		break;
+	case 1:
+		report(0, "Subchannel invalid or not enabled.");
+		break;
+	case 0:
+		cc = check_response(p + len);
+		break;
+	}
+
+	report_prefix_pop();
+	return cc;
+}
+
+int get_chsc_scsc(void)
+{
+	int i, n;
+	int ret = 0;
+	char buffer[510];
+	char *p;
+
+	report_prefix_push("Channel Subsystem Call");
+
+	if (chsc_scsc) {
+		report_info("chsc_scsc already initialized");
+		goto end;
+	}
+
+	chsc_scsc = alloc_page();
+	report_info("scsc_scsc at: %016lx", (u64)chsc_scsc);
+	if (!chsc_scsc) {
+		ret = -1;
+		report(0, "could not allocate chsc_scsc page!");
+		goto end;
+	}
+
+	report_info("scsc format %x\n", chsc_scsc->req_fmt);
+	ret = chsc(chsc_scsc, CHSC_SCSC, CHSC_SCSC_LEN);
+	if (ret) {
+		report(0, "chsc: CC %d", ret);
+		goto end;
+	}
+
+	for (i = 0, p = buffer; i < CSS_GENERAL_FEAT_BITLEN; i++) {
+		if (css_general_feature(i)) {
+			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
+			p += n;
+		}
+	}
+	report_info("General features: %s", buffer);
+
+	for (i = 0, p = buffer, ret = 0; i < CSS_CHSC_FEAT_BITLEN; i++) {
+		if (css_chsc_feature(i)) {
+			n = snprintf(p, sizeof(buffer) - ret, "%d,", i);
+			p += n;
+		}
+	}
+	report_info("CHSC features: %s", buffer);
+
+end:
+	report_prefix_pop();
+	return ret;
+}
 
 /*
  * css_enumerate:
diff --git a/s390x/css.c b/s390x/css.c
index 1a61a5c..18dbf01 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,21 @@ error_senseid:
 	unregister_io_int_func(css_irq_io);
 }
 
+static void css_init(void)
+{
+	int ret;
+
+	ret = get_chsc_scsc();
+	if (!ret)
+		report(1, " ");
+}
+
 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 v2 2/5] 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>
---
 lib/s390x/css.h     |  1 +
 lib/s390x/css_lib.c | 37 ++++++++++++++++++++++----------
 s390x/css.c         | 51 ++++++++++++++++++++++++++-------------------
 3 files changed, 56 insertions(+), 33 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 1e317d1..fa8775f 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 88aa7a1..5426a6b 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -180,6 +180,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
@@ -229,18 +254,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 18dbf01..d4b3cc8 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;
-	}
-
-	ret = register_io_int_func(css_irq_io);
-	if (ret) {
-		report(0, "Could not register IRQ handler");
-		return;
+	if (!css_enabled(test_device_sid)) {
+		report(0, "enabling subchannel %08x", test_device_sid);
+		return success;
 	}
 
-	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,16 +120,21 @@ 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)
@@ -146,8 +142,19 @@ static void css_init(void)
 	int ret;
 
 	ret = get_chsc_scsc();
-	if (!ret)
-		report(1, " ");
+	if (ret) {
+		report(0, "Could not get CHSC Characteristics");
+		return;
+	}
+
+	ret = register_io_int_func(css_irq_io);
+	if (ret) {
+		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 v2 3/5] 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.

An initial test reports the presence of the extended measurement
block feature.

Several tests on SCHM verify the error reporting of the hypervisor.

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

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index fa8775f..0e3254a 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -82,6 +82,7 @@ struct pmcw {
 	uint32_t intparm;
 #define PMCW_DNV	0x0001
 #define PMCW_ENABLE	0x0080
+#define PMCW_MBUE	0x0010
 #define PMCW_ISC_MASK	0x3800
 #define PMCW_ISC_SHIFT	11
 	uint16_t flags;
@@ -94,6 +95,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 +103,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 {
@@ -306,6 +309,7 @@ struct chsc_scsc {
 	u32 res_04[2];
 	struct chsc_header res;
 	u32 res_fmt;
+#define CSSC_EXTENDED_MEASUREMENT_BLOCK 48
 	u64 general_char[255];
 	u64 chsc_char[254];
 };
@@ -358,4 +362,17 @@ int 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);
+
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 5426a6b..355881d 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -267,6 +267,80 @@ retry:
 	return -1;
 }
 
+static bool schib_update(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 */
+	pmcw->flags |= flags;
+
+	if (format1)
+		pmcw->flags2 |= PMCW_MBF1;
+	else
+		pmcw->flags2 &= ~PMCW_MBF1;
+
+	pmcw->mbi = mbi;
+	schib.mbo = mb;
+
+	/* 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 to verify the measurement block origin
+	 */
+	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 Mesurement 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
+ * Return value:
+ *   On success: 0
+ *   On error the CC of the faulty instruction
+ *      or -1 if the retry count is exceeded.
+ */
+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(schid, mb, mbi, flags, format1))
+		mdelay(10); /* the hardware was not ready, give it some time */
+
+	return schib.mbo == mb && pmcw->mbi == mbi;
+}
+
 static struct irb irb;
 
 void css_irq_io(void)
diff --git a/s390x/css.c b/s390x/css.c
index d4b3cc8..a382235 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -157,6 +157,41 @@ 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 flags must be 0 */
+	report_prefix_push("Bad flags");
+	expect_pgm_int();
+	schm(NULL, 0x04);
+	check_pgm_int_code(PGM_INT_CODE_OPERAND);
+	report_prefix_pop();
+
+	/* bits 36-61 flags must be 0 */
+	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);
@@ -166,6 +201,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 v2 4/5] s390x: css: testing measurement block format 0

[Pierre Morel]

We tests the update of the mesurement block format 0, the
mesurement 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 | 14 +++++++++++++
 s390x/css.c     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 0e3254a..5478f45 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -374,5 +374,19 @@ 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);
+#define SCHM_DCTM	1 /* activate Device Connection TiMe */
+#define SCHM_MBU	2 /* activate Measurement Block Update */
+
+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 a382235..f3fdc0c 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -189,7 +189,61 @@ static void test_schm(void)
 	schm(NULL, SCHM_MBU);
 	report(1,"SCHM call without address");
 	report_prefix_pop();
+}
+
+#define SCHM_UPDATE_CNT 10
+static bool start_measure(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;
+}
+
+static void test_schm_fmt0(void)
+{
+	struct measurement_block_format0 *mb0;
+
+	report_prefix_push("Format 0");
+
+	mb0 = alloc_io_mem(sizeof(struct measurement_block_format0), 0);
+	if (!mb0) {
+		report_abort("measurement_block_format0 allocation failed");
+		goto end;
+	}
+
+	schm(NULL, 0); /* Clear previous MB address */
+	schm(mb0, SCHM_MBU);
 
+	/* Expect error for non aligned MB */
+	report_prefix_push("Unaligned MB index");
+	report_xfail(start_measure(0, 0x01, false), mb0->ssch_rsch_count != 0,
+		     "SSCH measured %d", mb0->ssch_rsch_count);
+	report_prefix_pop();
+
+	memset(mb0, 0, sizeof(*mb0));
+
+	/* Expect success */
+	report_prefix_push("Valid MB address and index");
+	report(start_measure(0, 0, false) &&
+	       mb0->ssch_rsch_count == SCHM_UPDATE_CNT,
+	       "SSCH measured %d", mb0->ssch_rsch_count);
+	report_prefix_pop();
+
+	free_io_mem(mb0, sizeof(struct measurement_block_format0));
+end:
+	report_prefix_pop();
 }
 
 static struct {
@@ -202,6 +256,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 v2 5/5] s390x: css: testing measurement block format 1

[Pierre Morel]

Measurement block format 1 is made available by the extended
mesurement 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 | 14 ++++++++++++++
 s390x/css.c     | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 5478f45..ee525f1 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -389,4 +389,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/s390x/css.c b/s390x/css.c
index f3fdc0c..ec5e365 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -246,6 +246,41 @@ end:
 	report_prefix_pop();
 }
 
+static void test_schm_fmt1(void)
+{
+	struct measurement_block_format1 *mb1;
+
+	report_prefix_push("Format 1");
+
+	mb1 = alloc_io_mem(sizeof(struct measurement_block_format1), 0);
+	if (!mb1) {
+		report_abort("measurement_block_format1 allocation failed");
+		goto end;
+	}
+
+	schm(NULL, 0); /* Clear previous MB address */
+	schm(0, SCHM_MBU);
+
+	/* Expect error for non aligned MB */
+	report_prefix_push("Unaligned MB origin");
+	report_xfail(start_measure((u64)mb1 + 1, 0, true), mb1->ssch_rsch_count != 0,
+		     "SSCH measured %d", mb1->ssch_rsch_count);
+	report_prefix_pop();
+
+	memset(mb1, 0, sizeof(*mb1));
+
+	/* Expect success */
+	report_prefix_push("Valid MB address and index");
+	report(start_measure((u64)mb1, 0, true) &&
+	       mb1->ssch_rsch_count == SCHM_UPDATE_CNT,
+	       "SSCH measured %d", mb1->ssch_rsch_count);
+	report_prefix_pop();
+
+	free_io_mem(mb1, sizeof(struct measurement_block_format1));
+end:
+	report_prefix_pop();
+}
+
 static struct {
 	const char *name;
 	void (*func)(void);
@@ -257,6 +292,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 v2 1/5] s390x: css: Store CSS Characteristics

[Cornelia Huck]

On Wed, 10 Feb 2021 14:20:10 +0100
Pierre Morel <pmorel@linux.ibm.com> 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>
> ---
>  lib/s390x/css.h     |  69 +++++++++++++++++++++++++++
>  lib/s390x/css_lib.c | 112 +++++++++++++++++++++++++++++++++++++++++++-
>  s390x/css.c         |  12 +++++
>  3 files changed, 192 insertions(+), 1 deletion(-)
> 

(...)

> +static const char *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 2 - 1.",

"2G - 1", I think?

> +	"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.",
> +};

(...)

This matches both the Linux implementation and my memories, so

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

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

[Cornelia Huck]

On Wed, 10 Feb 2021 14:20:11 +0100
Pierre Morel <pmorel@linux.ibm.com> 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>
> ---
>  lib/s390x/css.h     |  1 +
>  lib/s390x/css_lib.c | 37 ++++++++++++++++++++++----------
>  s390x/css.c         | 51 ++++++++++++++++++++++++++-------------------
>  3 files changed, 56 insertions(+), 33 deletions(-)

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

Re: [kvm-unit-tests PATCH v2 3/5] s390x: css: implementing Set CHannel Monitor

[Cornelia Huck]

On Wed, 10 Feb 2021 14:20:12 +0100
Pierre Morel <pmorel@linux.ibm.com> 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.
> 
> An initial test reports the presence of the extended measurement
> block feature.
> 
> Several tests on SCHM verify the error reporting of the hypervisor.

Combine these two into one sentence?

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

Also, you add the infrastructure for enabling measurements at the
subchannel -- either mention this in the patch description or move it
to a separate patch or the first user?

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

(...)

> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 5426a6b..355881d 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -267,6 +267,80 @@ retry:
>  	return -1;
>  }
>  
> +static bool schib_update(int schid, uint64_t mb, uint16_t mbi, uint16_t flags,
> +		  bool format1)

Maybe schib_update_mb()?

> +{
> +	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 */
> +	pmcw->flags |= flags;

Do we also want to be able to disable it again?

> +
> +	if (format1)
> +		pmcw->flags2 |= PMCW_MBF1;
> +	else
> +		pmcw->flags2 &= ~PMCW_MBF1;
> +
> +	pmcw->mbi = mbi;
> +	schib.mbo = mb;
> +
> +	/* 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 to verify the measurement block origin
> +	 */
> +	cc = stsch(schid, &schib);
> +	if (cc) {
> +		report_info("stsch: updating sch %08x failed with cc=%d",
> +			    schid, cc);
> +		return false;
> +	}

Hm, you only do the stsch, but do not check the result (that is done by
the caller) -- remove the misleading comment or replace it with "Read
the SCHIB again"?

> +
> +	return true;
> +}
> +

(...)

Otherwise, LGTM.

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

[Cornelia Huck]

On Wed, 10 Feb 2021 14:20:13 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We tests the update of the mesurement block format 0, the

s/tests/test/
s/mesurement/measurement/

> mesurement 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 | 14 +++++++++++++
>  s390x/css.c     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 

(...)

> +static void test_schm_fmt0(void)
> +{
> +	struct measurement_block_format0 *mb0;
> +
> +	report_prefix_push("Format 0");
> +
> +	mb0 = alloc_io_mem(sizeof(struct measurement_block_format0), 0);
> +	if (!mb0) {
> +		report_abort("measurement_block_format0 allocation failed");
> +		goto end;
> +	}
> +
> +	schm(NULL, 0); /* Clear previous MB address */

I think it would be better to clean out the mb after a particular test
has run, so that the following tests can start with a clean slate.

> +	schm(mb0, SCHM_MBU);
>  
> +	/* Expect error for non aligned MB */
> +	report_prefix_push("Unaligned MB index");
> +	report_xfail(start_measure(0, 0x01, false), mb0->ssch_rsch_count != 0,
> +		     "SSCH measured %d", mb0->ssch_rsch_count);
> +	report_prefix_pop();
> +
> +	memset(mb0, 0, sizeof(*mb0));
> +
> +	/* Expect success */
> +	report_prefix_push("Valid MB address and index");
> +	report(start_measure(0, 0, false) &&
> +	       mb0->ssch_rsch_count == SCHM_UPDATE_CNT,
> +	       "SSCH measured %d", mb0->ssch_rsch_count);
> +	report_prefix_pop();
> +
> +	free_io_mem(mb0, sizeof(struct measurement_block_format0));

Before you free the memory, you really need to stop measurements
again... even though nothing happens right now, because you're not doing
I/O after this point.

> +end:
> +	report_prefix_pop();
>  }
>  
>  static struct {
> @@ -202,6 +256,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 v2 5/5] s390x: css: testing measurement block format 1

[Cornelia Huck]

On Wed, 10 Feb 2021 14:20:14 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Measurement block format 1 is made available by the extended
> mesurement block facility and is indicated in the SCHIB by

s/mesurement/measurement/

> 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 | 14 ++++++++++++++
>  s390x/css.c     | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)

(...)

> +static void test_schm_fmt1(void)
> +{
> +	struct measurement_block_format1 *mb1;
> +
> +	report_prefix_push("Format 1");
> +
> +	mb1 = alloc_io_mem(sizeof(struct measurement_block_format1), 0);
> +	if (!mb1) {
> +		report_abort("measurement_block_format1 allocation failed");
> +		goto end;
> +	}
> +
> +	schm(NULL, 0); /* Clear previous MB address */

Same comment as for the last patch.

> +	schm(0, SCHM_MBU);
> +
> +	/* Expect error for non aligned MB */
> +	report_prefix_push("Unaligned MB origin");
> +	report_xfail(start_measure((u64)mb1 + 1, 0, true), mb1->ssch_rsch_count != 0,
> +		     "SSCH measured %d", mb1->ssch_rsch_count);
> +	report_prefix_pop();
> +
> +	memset(mb1, 0, sizeof(*mb1));
> +
> +	/* Expect success */
> +	report_prefix_push("Valid MB address and index");
> +	report(start_measure((u64)mb1, 0, true) &&
> +	       mb1->ssch_rsch_count == SCHM_UPDATE_CNT,
> +	       "SSCH measured %d", mb1->ssch_rsch_count);
> +	report_prefix_pop();
> +
> +	free_io_mem(mb1, sizeof(struct measurement_block_format1));

Also here, you need to stop the measurements before freeing the block.

> +end:
> +	report_prefix_pop();
> +}
> +
>  static struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -257,6 +292,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 }
>  };
>  

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

[Pierre Morel]

On 2/12/21 11:32 AM, Cornelia Huck wrote:
> On Wed, 10 Feb 2021 14:20:10 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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>
>> ---
>>   lib/s390x/css.h     |  69 +++++++++++++++++++++++++++
>>   lib/s390x/css_lib.c | 112 +++++++++++++++++++++++++++++++++++++++++++-
>>   s390x/css.c         |  12 +++++
>>   3 files changed, 192 insertions(+), 1 deletion(-)
>>
> 
> (...)
> 
>> +static const char *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 2 - 1.",
> 
> "2G - 1", I think?

:) yes

> 
>> +	"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.",
>> +};
> 
> (...)
> 
> This matches both the Linux implementation and my memories, so
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2/12/21 11:36 AM, Cornelia Huck wrote:
> On Wed, 10 Feb 2021 14:20:11 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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>
>> ---
>>   lib/s390x/css.h     |  1 +
>>   lib/s390x/css_lib.c | 37 ++++++++++++++++++++++----------
>>   s390x/css.c         | 51 ++++++++++++++++++++++++++-------------------
>>   3 files changed, 56 insertions(+), 33 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 3/5] s390x: css: implementing Set CHannel Monitor

[Pierre Morel]

On 2/12/21 11:53 AM, Cornelia Huck wrote:
> On Wed, 10 Feb 2021 14:20:12 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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.
>>
>> An initial test reports the presence of the extended measurement
>> block feature.
>>
>> Several tests on SCHM verify the error reporting of the hypervisor.
> 
> Combine these two into one sentence?
> 
> "Initial tests report the presence of the extended measurement block
> feature, and verify the error reporting of the hypervisor for SCHM."
> 
> Also, you add the infrastructure for enabling measurements at the
> subchannel -- either mention this in the patch description or move it
> to a separate patch or the first user?

yes, I change for one of these solutions, thanks.

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 19 +++++++++++-
>>   lib/s390x/css_lib.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/css.c         | 36 ++++++++++++++++++++++
>>   3 files changed, 128 insertions(+), 1 deletion(-)
>>
> 
> (...)
> 
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 5426a6b..355881d 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -267,6 +267,80 @@ retry:
>>   	return -1;
>>   }
>>   
>> +static bool schib_update(int schid, uint64_t mb, uint16_t mbi, uint16_t flags,
>> +		  bool format1)
> 
> Maybe schib_update_mb()?

yes, it is dedicated.

> 
>> +{
>> +	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 */
>> +	pmcw->flags |= flags;
> 
> Do we also want to be able to disable it again?

Yes, I can add the disabling of the measurement on a channel.
In the test I disable it for the system but we may need this later.


> 
>> +
>> +	if (format1)
>> +		pmcw->flags2 |= PMCW_MBF1;
>> +	else
>> +		pmcw->flags2 &= ~PMCW_MBF1;
>> +
>> +	pmcw->mbi = mbi;
>> +	schib.mbo = mb;
>> +
>> +	/* 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 to verify the measurement block origin
>> +	 */
>> +	cc = stsch(schid, &schib);
>> +	if (cc) {
>> +		report_info("stsch: updating sch %08x failed with cc=%d",
>> +			    schid, cc);
>> +		return false;
>> +	}
> 
> Hm, you only do the stsch, but do not check the result (that is done by
> the caller) -- remove the misleading comment or replace it with "Read
> the SCHIB again"?

right, "Read the SCHIB again"

> 
>> +
>> +	return true;
>> +}
>> +
> 
> (...)
> 
> Otherwise, LGTM.
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Pierre Morel]

On 2/12/21 12:12 PM, Cornelia Huck wrote:
> On Wed, 10 Feb 2021 14:20:13 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We tests the update of the mesurement block format 0, the
> 
> s/tests/test/
> s/mesurement/measurement/

hum, yes :(

> 
>> mesurement 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 | 14 +++++++++++++
>>   s390x/css.c     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+)
>>
> 
> (...)
> 
>> +static void test_schm_fmt0(void)
>> +{
>> +	struct measurement_block_format0 *mb0;
>> +
>> +	report_prefix_push("Format 0");
>> +
>> +	mb0 = alloc_io_mem(sizeof(struct measurement_block_format0), 0);
>> +	if (!mb0) {
>> +		report_abort("measurement_block_format0 allocation failed");
>> +		goto end;
>> +	}
>> +
>> +	schm(NULL, 0); /* Clear previous MB address */
> 
> I think it would be better to clean out the mb after a particular test
> has run, so that the following tests can start with a clean slate.

The allocation includes zeroing the memory.
and I do a memset(mb, 0...) after the failed test.

Is there something else to clear?

> 
>> +	schm(mb0, SCHM_MBU);
>>   
>> +	/* Expect error for non aligned MB */
>> +	report_prefix_push("Unaligned MB index");
>> +	report_xfail(start_measure(0, 0x01, false), mb0->ssch_rsch_count != 0,
>> +		     "SSCH measured %d", mb0->ssch_rsch_count);
>> +	report_prefix_pop();
>> +
>> +	memset(mb0, 0, sizeof(*mb0));
>> +
>> +	/* Expect success */
>> +	report_prefix_push("Valid MB address and index");
>> +	report(start_measure(0, 0, false) &&
>> +	       mb0->ssch_rsch_count == SCHM_UPDATE_CNT,
>> +	       "SSCH measured %d", mb0->ssch_rsch_count);
>> +	report_prefix_pop();
>> +
>> +	free_io_mem(mb0, sizeof(struct measurement_block_format0));
> 
> Before you free the memory, you really need to stop measurements
> again... even though nothing happens right now, because you're not doing
> I/O after this point.

Yes, it is cleaner.

Thanks, pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH v2 5/5] s390x: css: testing measurement block format 1

[Pierre Morel]

On 2/12/21 12:15 PM, Cornelia Huck wrote:
> On Wed, 10 Feb 2021 14:20:14 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Measurement block format 1 is made available by the extended
>> mesurement block facility and is indicated in the SCHIB by
> 
> s/mesurement/measurement/

50% good and yes, 50% bad, I change it thanks.

> 
>> 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 | 14 ++++++++++++++
>>   s390x/css.c     | 36 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
> 
> (...)
> 
>> +static void test_schm_fmt1(void)
>> +{
>> +	struct measurement_block_format1 *mb1;
>> +
>> +	report_prefix_push("Format 1");
>> +
>> +	mb1 = alloc_io_mem(sizeof(struct measurement_block_format1), 0);
>> +	if (!mb1) {
>> +		report_abort("measurement_block_format1 allocation failed");
>> +		goto end;
>> +	}
>> +
>> +	schm(NULL, 0); /* Clear previous MB address */
> 
> Same comment as for the last patch.

Yes,

> 
>> +	schm(0, SCHM_MBU);
>> +
>> +	/* Expect error for non aligned MB */
>> +	report_prefix_push("Unaligned MB origin");
>> +	report_xfail(start_measure((u64)mb1 + 1, 0, true), mb1->ssch_rsch_count != 0,
>> +		     "SSCH measured %d", mb1->ssch_rsch_count);
>> +	report_prefix_pop();
>> +
>> +	memset(mb1, 0, sizeof(*mb1));
>> +
>> +	/* Expect success */
>> +	report_prefix_push("Valid MB address and index");
>> +	report(start_measure((u64)mb1, 0, true) &&
>> +	       mb1->ssch_rsch_count == SCHM_UPDATE_CNT,
>> +	       "SSCH measured %d", mb1->ssch_rsch_count);
>> +	report_prefix_pop();
>> +
>> +	free_io_mem(mb1, sizeof(struct measurement_block_format1));
> 
> Also here, you need to stop the measurements before freeing the block.

yes, I will.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen