[kvm-unit-tests PATCH v2 0/5] s390x: SCLP Unit test

[kvm-unit-tests PATCH v2 0/5] s390x: SCLP Unit test

[Claudio Imbrenda]

This patchset contains some minor cleanup, some preparatory work and
then the SCLP unit test itself.

The unit test checks the following:
    
    * Correctly ignoring instruction bits that should be ignored
    * Privileged instruction check
    * Check for addressing exceptions
    * Specification exceptions:
      - SCCB size less than 8
      - SCCB unaligned
      - SCCB overlaps prefix or lowcore
      - SCCB address higher than 2GB
    * Return codes for
      - Invalid command
      - SCCB too short (but at least 8)
      - SCCB page boundary violation

v1 -> v2
* fix many small issues that came up during the first round of reviews
* add comments to each function
* use a static buffer for the SCCP template when used

Claudio Imbrenda (5):
  s390x: remove redundant defines
  s390x: improve error reporting for interrupts
  s390x: sclp: expose ram_size and max_ram_size
  s390x: sclp: add service call instruction wrapper
  s390x: SCLP unit test

 s390x/Makefile           |   1 +
 lib/s390x/asm/arch_def.h |  13 ++
 lib/s390x/sclp.h         |   4 +-
 lib/s390x/interrupt.c    |   4 +-
 lib/s390x/sclp.c         |  17 +-
 s390x/sclp.c             | 413 +++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 7 files changed, 445 insertions(+), 10 deletions(-)
 create mode 100644 s390x/sclp.c

-- 
2.7.4

[kvm-unit-tests PATCH v2 1/5] s390x: remove redundant defines

[Claudio Imbrenda]

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/sclp.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 4e69845..f00c3df 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -27,8 +27,6 @@
 #define SCLP_ASSIGN_STORAGE                     0x000D0001
 #define SCLP_CMD_READ_EVENT_DATA                0x00770005
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
-#define SCLP_CMD_READ_EVENT_DATA                0x00770005
-#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
 
 /* SCLP Memory hotplug codes */
-- 
2.7.4

[kvm-unit-tests PATCH v2 2/5] s390x: improve error reporting for interrupts

[Claudio Imbrenda]

Improve error reporting for unexpected external interrupts to also
print the received external interrupt code.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 lib/s390x/interrupt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 5cade23..1636207 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -118,8 +118,8 @@ void handle_ext_int(void)
 {
 	if (!ext_int_expected &&
 	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
-		report_abort("Unexpected external call interrupt: at %#lx",
-			     lc->ext_old_psw.addr);
+		report_abort("Unexpected external call interrupt (code %#x): at %#lx",
+			     lc->ext_int_code, lc->ext_old_psw.addr);
 		return;
 	}
 
-- 
2.7.4

[kvm-unit-tests PATCH v2 3/5] s390x: sclp: expose ram_size and max_ram_size

[Claudio Imbrenda]

Expose ram_size and max_ram_size through accessor functions.

We only use get_ram_size in an upcoming patch, but having an accessor
for the other one does not hurt.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 lib/s390x/sclp.h |  2 ++
 lib/s390x/sclp.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index f00c3df..6d40fb7 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -272,5 +272,7 @@ void sclp_console_setup(void);
 void sclp_print(const char *str);
 int sclp_service_call(unsigned int command, void *sccb);
 void sclp_memory_setup(void);
+uint64_t get_ram_size(void);
+uint64_t get_max_ram_size(void);
 
 #endif /* SCLP_H */
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 56fca0c..7798f04 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -167,3 +167,13 @@ void sclp_memory_setup(void)
 
 	mem_init(ram_size);
 }
+
+uint64_t get_ram_size(void)
+{
+	return ram_size;
+}
+
+uint64_t get_max_ram_size(void)
+{
+	return max_ram_size;
+}
-- 
2.7.4

[kvm-unit-tests PATCH v2 4/5] s390x: sclp: add service call instruction wrapper

[Claudio Imbrenda]

Add a wrapper for the service call instruction, and use it for SCLP
interactions instead of using inline assembly everywhere.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/asm/arch_def.h | 13 +++++++++++++
 lib/s390x/sclp.c         |  7 +------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 96cca2e..b3caff6 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -269,4 +269,17 @@ static inline int stsi(void *addr, int fc, int sel1, int sel2)
 	return cc;
 }
 
+static inline int servc(uint32_t command, unsigned long sccb)
+{
+	int cc;
+
+	asm volatile(
+		"       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
+		"       ipm     %0\n"
+		"       srl     %0,28"
+		: "=&d" (cc) : "d" (command), "a" (sccb)
+		: "cc", "memory");
+	return cc;
+}
+
 #endif
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 7798f04..e35c282 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -116,12 +116,7 @@ int sclp_service_call(unsigned int command, void *sccb)
 	int cc;
 
 	sclp_setup_int();
-	asm volatile(
-		"       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
-		"       ipm     %0\n"
-		"       srl     %0,28"
-		: "=&d" (cc) : "d" (command), "a" (__pa(sccb))
-		: "cc", "memory");
+	cc = servc(command, __pa(sccb));
 	sclp_wait_busy();
 	if (cc == 3)
 		return -1;
-- 
2.7.4

[kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

SCLP unit test. Testing the following:

* Correctly ignoring instruction bits that should be ignored
* Privileged instruction check
* Check for addressing exceptions
* Specification exceptions:
  - SCCB size less than 8
  - SCCB unaligned
  - SCCB overlaps prefix or lowcore
  - SCCB address higher than 2GB
* Return codes for
  - Invalid command
  - SCCB too short (but at least 8)
  - SCCB page boundary violation

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/sclp.c        | 413 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 417 insertions(+)
 create mode 100644 s390x/sclp.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 3744372..ddb4b48 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
 tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
+tests += $(TEST_DIR)/sclp.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
diff --git a/s390x/sclp.c b/s390x/sclp.c
new file mode 100644
index 0000000..29ac265
--- /dev/null
+++ b/s390x/sclp.c
@@ -0,0 +1,413 @@
+/*
+ * Service Call tests
+ *
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Claudio Imbrenda <imbrenda@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <sclp.h>
+
+#define PGM_BIT_SPEC	(1ULL << PGM_INT_CODE_SPECIFICATION)
+#define PGM_BIT_ADDR	(1ULL << PGM_INT_CODE_ADDRESSING)
+#define PGM_BIT_PRIV	(1ULL << PGM_INT_CODE_PRIVILEGED_OPERATION)
+#define MKPTR(x) ((void *)(uint64_t)(x))
+
+static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+static uint8_t prefix_buf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+static uint8_t sccb_template[PAGE_SIZE];
+static uint32_t valid_code;
+static struct lowcore *lc;
+
+/**
+ * Enable SCLP interrupt.
+ */
+static void sclp_setup_int_test(void)
+{
+	uint64_t mask;
+
+	ctl_set_bit(0, 9);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+}
+
+/**
+ * Perform one service call, handling exceptions and interrupts.
+ */
+static int sclp_service_call_test(unsigned int command, void *sccb)
+{
+	int cc;
+
+	sclp_mark_busy();
+	sclp_setup_int_test();
+	lc->pgm_int_code = 0;
+	cc = servc(command, __pa(sccb));
+	if (lc->pgm_int_code) {
+		sclp_handle_ext();
+		return 0;
+	}
+	if (!cc)
+		sclp_wait_busy();
+	return cc;
+}
+
+/**
+ * Perform one test at the given address, optionally using the SCCB template,
+ * checking for the expected program interrupts and return codes.
+ */
+static int test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t len,
+			uint64_t exp_pgm, uint16_t exp_rc)
+{
+	SCCBHeader *h = (SCCBHeader *)addr;
+	int res, pgm;
+
+	if (len)
+		memcpy(addr, sccb_template, len);
+	if (!exp_pgm)
+		exp_pgm = 1;
+	expect_pgm_int();
+	res = sclp_service_call_test(cmd, h);
+	if (res) {
+		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
+		return 0;
+	}
+	pgm = lc->pgm_int_code;
+	if (!((1ULL << pgm) & exp_pgm)) {
+		report_info("First failure at addr %p, size %d, cmd %#x, pgm code %d", addr, len, cmd, pgm);
+		return 0;
+	}
+	if (exp_rc && (exp_rc != h->response_code)) {
+		report_info("First failure at addr %p, size %d, cmd %#x, resp code %#x",
+				addr, len, cmd, h->response_code);
+		return 0;
+	}
+	return 1;
+}
+
+/**
+ * Wrapper for test_one_sccb to set up an SCCB template
+ */
+static int test_one_run(uint32_t cmd, uint8_t *addr, uint16_t sccb_len,
+			uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc)
+{
+	if (buf_len)
+		memset(sccb_template, 0, sizeof(sccb_template));
+	((SCCBHeader *)sccb_template)->length = sccb_len;
+	if (buf_len && buf_len < 8)
+		buf_len = 8;
+	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
+}
+
+/**
+ * Test SCCB lengths < 8
+ */
+static void test_sccb_too_short(void)
+{
+	int cx;
+
+	for (cx = 0; cx < 8; cx++)
+		if (!test_one_run(valid_code, pagebuf, cx, 8, PGM_BIT_SPEC, 0))
+			break;
+
+	report("SCCB too short", cx == 8);
+}
+
+/**
+ * Test SCCBs that are not 64bits aligned
+ */
+static void test_sccb_unaligned(void)
+{
+	int cx;
+
+	for (cx = 1; cx < 8; cx++)
+		if (!test_one_run(valid_code, cx + pagebuf, 8, 8, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB unaligned", cx == 8);
+}
+
+/**
+ * Test SCCBs whose address is in the lowcore or prefix area
+ */
+static void test_sccb_prefix(void)
+{
+	uint32_t prefix, new_prefix;
+	int cx;
+
+	// can't actually trash the lowcore, unsurprisingly things break if we do
+	for (cx = 0; cx < 8192; cx += 8)
+		if (!test_one_sccb(valid_code, MKPTR(cx), 0, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB low pages", cx == 8192);
+
+	memcpy(prefix_buf, 0, 8192);
+	new_prefix = (uint32_t)(intptr_t)prefix_buf;
+	asm volatile("stpx %0" : "=Q" (prefix));
+	asm volatile("spx %0" : : "Q" (new_prefix) : "memory");
+
+	for (cx = 0; cx < 8192; cx += 8)
+		if (!test_one_run(valid_code, MKPTR(new_prefix + cx), 8, 8, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB prefix pages", cx == 8192);
+
+	memcpy(prefix_buf, 0, 8192);
+	asm volatile("spx %0" : : "Q" (prefix) : "memory");
+}
+
+/**
+ * Test SCCBs that are above 2GB. If outside of memory, an addressing
+ * exception is also allowed.
+ */
+static void test_sccb_high(void)
+{
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	uintptr_t a[33 * 4 * 2 + 2];
+	uint64_t maxram;
+	int cx, i, pgm;
+
+	h->length = 8;
+
+	i = 0;
+	for (cx = 0; cx < 33; cx++)
+		a[i++] = 1UL << (cx + 31);
+	for (cx = 0; cx < 33; cx++)
+		a[i++] = 3UL << (cx + 31);
+	for (cx = 0; cx < 33; cx++)
+		a[i++] = 0xffffffff80000000UL << cx;
+	a[i++] = 0x80000000;
+	for (cx = 1; cx < 33; cx++, i++)
+		a[i] = a[i - 1] | (1UL << (cx + 31));
+	for (cx = 0; cx < i; cx++)
+		a[i + cx] = a[cx] + (intptr_t)pagebuf;
+	i += cx;
+	a[i++] = 0xdeadbeef00000000;
+	a[i++] = 0xdeaddeadbeef0000;
+
+	maxram = get_ram_size();
+	for (cx = 0; cx < i; cx++) {
+		pgm = PGM_BIT_SPEC | (a[cx] >= maxram ? PGM_BIT_ADDR : 0);
+		if (!test_one_sccb(valid_code, (void *)a[cx], 0, pgm, 0))
+			break;
+	}
+	report("SCCB high addresses", cx == i);
+}
+
+/**
+ * Test invalid commands, both invalid command detail codes and valid
+ * ones with invalid command class code.
+ */
+static void test_inval(void)
+{
+	uint32_t cmd;
+	int cx;
+
+	report_prefix_push("Invalid command");
+	for (cx = 0; cx < 65536; cx++) {
+		cmd = (0xdead0000) | cx;
+		if (!test_one_run(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, 0, SCLP_RC_INVALID_SCLP_COMMAND))
+			break;
+	}
+	report("Command detail code", cx == 65536);
+
+	for (cx = 0; cx < 256; cx++) {
+		cmd = (valid_code & ~0xff) | cx;
+		if (cmd == valid_code)
+			continue;
+		if (!test_one_run(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, 0, SCLP_RC_INVALID_SCLP_COMMAND))
+			break;
+	}
+	report("Command class code", cx == 256);
+	report_prefix_pop();
+}
+
+
+/**
+ * Test short SCCBs (but larger than 8).
+ */
+static void test_short(void)
+{
+	uint16_t res = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
+	int cx;
+
+	for (cx = 8; cx < 144; cx++)
+		if (!test_one_run(valid_code, pagebuf, cx, cx, 0, res))
+			break;
+	report("Insufficient SCCB length (Read SCP info)", cx == 144);
+
+	for (cx = 8; cx < 40; cx++)
+		if (!test_one_run(SCLP_READ_CPU_INFO, pagebuf, cx, cx, 0, res))
+			break;
+	report("Insufficient SCCB length (Read CPU info)", cx == 40);
+}
+
+/**
+ * Test SCCB page boundary violations.
+ */
+static void test_boundary(void)
+{
+	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
+	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
+	WriteEventData *sccb = (WriteEventData *)sccb_template;
+	int len, cx;
+
+	memset(sccb_template, 0, sizeof(sccb_template));
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (len = 32; len <= 4096; len++) {
+		cx = len & 7 ? len & ~7 : len - 8;
+		for (cx = 4096 - cx; cx < 4096; cx += 8) {
+			sccb->h.length = len;
+			if (!test_one_sccb(cmd, cx + pagebuf, len, 0, res))
+				goto out;
+		}
+	}
+out:
+	report("SCCB page boundary violation", len > 4096 && cx == 4096);
+}
+
+static void test_toolong(void)
+{
+	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
+	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
+	WriteEventData *sccb = (WriteEventData *)sccb_template;
+	int cx;
+
+	memset(sccb_template, 0, sizeof(sccb_template));
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (cx = 4097; cx < 8192; cx++) {
+		sccb->h.length = cx;
+		if (!test_one_sccb(cmd, pagebuf, PAGE_SIZE, 0, res))
+			break;
+	}
+	report("SCCB bigger than 4k", cx == 8192);
+}
+
+/**
+ * Test privileged operation.
+ */
+static void test_priv(void)
+{
+	report_prefix_push("Privileged operation");
+	pagebuf[0] = 0;
+	pagebuf[1] = 8;
+	expect_pgm_int();
+	enter_pstate();
+	servc(valid_code, __pa(pagebuf));
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+}
+
+/**
+ * Test addressing exceptions. We need to test SCCB addresses between the
+ * end of available memory and 2GB, because after 2GB a specification
+ * exception is also allowed.
+ * Only applicable if the VM has less than 2GB of memory
+ */
+static void test_addressing(void)
+{
+	unsigned long cx, maxram = get_ram_size();
+
+	if (maxram >= 0x80000000) {
+		report_skip("Invalid SCCB address");
+		return;
+	}
+	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx += 8)
+		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); cx += 4096)
+		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; cx < 0x80000000; cx += 1048576)
+		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
+			goto out;
+out:
+	report("Invalid SCCB address", cx == 0x80000000);
+}
+
+/**
+ * Test some bits in the instruction format that are specified to be ignored.
+ */
+static void test_instbits(void)
+{
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	unsigned long mask;
+	int cc;
+
+	sclp_mark_busy();
+	h->length = 8;
+
+	ctl_set_bit(0, 9);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+
+	asm volatile(
+		"       .insn   rre,0xb2204200,%1,%2\n"  /* servc %1,%2 */
+		"       ipm     %0\n"
+		"       srl     %0,28"
+		: "=&d" (cc) : "d" (valid_code), "a" (__pa(pagebuf))
+		: "cc", "memory");
+	sclp_wait_busy();
+	report("Instruction format ignored bits", cc == 0);
+}
+
+/**
+ * Find a valid SCLP command code; not all codes are always allowed, and
+ * probing should be performed in the right order.
+ */
+static void find_valid_sclp_code(void)
+{
+	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
+				    SCLP_CMDW_READ_SCP_INFO };
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	int i, cc;
+
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		sclp_mark_busy();
+		memset(h, 0, sizeof(pagebuf));
+		h->length = 4096;
+
+		valid_code = commands[i];
+		cc = sclp_service_call(commands[i], h);
+		if (cc)
+			break;
+		if (h->response_code == SCLP_RC_NORMAL_READ_COMPLETION)
+			return;
+		if (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND)
+			break;
+	}
+	valid_code = 0;
+	report_abort("READ_SCP_INFO failed");
+}
+
+int main(void)
+{
+	report_prefix_push("sclp");
+	find_valid_sclp_code();
+
+	/* Test some basic things */
+	test_instbits();
+	test_priv();
+	test_addressing();
+
+	/* Test the specification exceptions */
+	test_sccb_too_short();
+	test_sccb_unaligned();
+	test_sccb_prefix();
+	test_sccb_high();
+
+	/* Test the expected response codes */
+	test_inval();
+	test_short();
+	test_boundary();
+	test_toolong();
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f1b07cd..75e3d37 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -75,3 +75,6 @@ file = stsi.elf
 [smp]
 file = smp.elf
 extra_params =-smp 2
+
+[sclp]
+file = sclp.elf
-- 
2.7.4

Re: [kvm-unit-tests PATCH v2 3/5] s390x: sclp: expose ram_size and max_ram_size

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1465 bytes --]

On 10/25/19 7:06 PM, Claudio Imbrenda wrote:
> Expose ram_size and max_ram_size through accessor functions.
> 
> We only use get_ram_size in an upcoming patch, but having an accessor
> for the other one does not hurt.

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

I already use it for WIP patches :-)

> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  lib/s390x/sclp.h |  2 ++
>  lib/s390x/sclp.c | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index f00c3df..6d40fb7 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -272,5 +272,7 @@ void sclp_console_setup(void);
>  void sclp_print(const char *str);
>  int sclp_service_call(unsigned int command, void *sccb);
>  void sclp_memory_setup(void);
> +uint64_t get_ram_size(void);
> +uint64_t get_max_ram_size(void);
>  
>  #endif /* SCLP_H */
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 56fca0c..7798f04 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -167,3 +167,13 @@ void sclp_memory_setup(void)
>  
>  	mem_init(ram_size);
>  }
> +
> +uint64_t get_ram_size(void)
> +{
> +	return ram_size;
> +}
> +
> +uint64_t get_max_ram_size(void)
> +{
> +	return max_ram_size;
> +}
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v2 4/5] s390x: sclp: add service call instruction wrapper

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1758 bytes --]

On 10/25/19 7:06 PM, Claudio Imbrenda wrote:
> Add a wrapper for the service call instruction, and use it for SCLP
> interactions instead of using inline assembly everywhere.

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

> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/asm/arch_def.h | 13 +++++++++++++
>  lib/s390x/sclp.c         |  7 +------
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 96cca2e..b3caff6 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -269,4 +269,17 @@ static inline int stsi(void *addr, int fc, int sel1, int sel2)
>  	return cc;
>  }
>  
> +static inline int servc(uint32_t command, unsigned long sccb)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
> +		"       ipm     %0\n"
> +		"       srl     %0,28"
> +		: "=&d" (cc) : "d" (command), "a" (sccb)
> +		: "cc", "memory");
> +	return cc;
> +}
> +
>  #endif
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 7798f04..e35c282 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -116,12 +116,7 @@ int sclp_service_call(unsigned int command, void *sccb)
>  	int cc;
>  
>  	sclp_setup_int();
> -	asm volatile(
> -		"       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
> -		"       ipm     %0\n"
> -		"       srl     %0,28"
> -		: "=&d" (cc) : "d" (command), "a" (__pa(sccb))
> -		: "cc", "memory");
> +	cc = servc(command, __pa(sccb));
>  	sclp_wait_busy();
>  	if (cc == 3)
>  		return -1;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 13987 bytes --]

On 10/25/19 7:06 PM, Claudio Imbrenda wrote:
> SCLP unit test. Testing the following:
> 
> * Correctly ignoring instruction bits that should be ignored
> * Privileged instruction check
> * Check for addressing exceptions
> * Specification exceptions:
>   - SCCB size less than 8
>   - SCCB unaligned
>   - SCCB overlaps prefix or lowcore
>   - SCCB address higher than 2GB
> * Return codes for
>   - Invalid command
>   - SCCB too short (but at least 8)
>   - SCCB page boundary violation
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/sclp.c        | 413 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 +
>  3 files changed, 417 insertions(+)
>  create mode 100644 s390x/sclp.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 3744372..ddb4b48 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
>  tests += $(TEST_DIR)/stsi.elf
>  tests += $(TEST_DIR)/skrf.elf
>  tests += $(TEST_DIR)/smp.elf
> +tests += $(TEST_DIR)/sclp.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> diff --git a/s390x/sclp.c b/s390x/sclp.c
> new file mode 100644
> index 0000000..29ac265
> --- /dev/null
> +++ b/s390x/sclp.c
> @@ -0,0 +1,413 @@
> +/*
> + * Service Call tests
> + *
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Claudio Imbrenda <imbrenda@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <sclp.h>
> +
> +#define PGM_BIT_SPEC	(1ULL << PGM_INT_CODE_SPECIFICATION)
> +#define PGM_BIT_ADDR	(1ULL << PGM_INT_CODE_ADDRESSING)
> +#define PGM_BIT_PRIV	(1ULL << PGM_INT_CODE_PRIVILEGED_OPERATION)
> +#define MKPTR(x) ((void *)(uint64_t)(x))
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +static uint8_t prefix_buf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +static uint8_t sccb_template[PAGE_SIZE];
> +static uint32_t valid_code;
> +static struct lowcore *lc;
> +
> +/**
> + * Enable SCLP interrupt.
> + */
> +static void sclp_setup_int_test(void)
> +{
> +	uint64_t mask;
> +
> +	ctl_set_bit(0, 9);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +}

Or you could just export the definition in sclp.c...

> +
> +/**
> + * Perform one service call, handling exceptions and interrupts.
> + */
> +static int sclp_service_call_test(unsigned int command, void *sccb)
> +{
> +	int cc;
> +
> +	sclp_mark_busy();
> +	sclp_setup_int_test();
> +	lc->pgm_int_code = 0;
> +	cc = servc(command, __pa(sccb));
> +	if (lc->pgm_int_code) {
> +		sclp_handle_ext();
> +		return 0;
> +	}
> +	if (!cc)
> +		sclp_wait_busy();
> +	return cc;
> +}
> +
> +/**
> + * Perform one test at the given address, optionally using the SCCB template,
> + * checking for the expected program interrupts and return codes.
> + */
> +static int test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t len,
> +			uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	SCCBHeader *h = (SCCBHeader *)addr;
> +	int res, pgm;
> +
> +	if (len)
> +		memcpy(addr, sccb_template, len);
> +	if (!exp_pgm)
> +		exp_pgm = 1;
> +	expect_pgm_int();
> +	res = sclp_service_call_test(cmd, h);
> +	if (res) {
> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
> +		return 0;
> +	}
> +	pgm = lc->pgm_int_code;
> +	if (!((1ULL << pgm) & exp_pgm)) {
> +		report_info("First failure at addr %p, size %d, cmd %#x, pgm code %d", addr, len, cmd, pgm);
> +		return 0;
> +	}
> +	if (exp_rc && (exp_rc != h->response_code)) {
> +		report_info("First failure at addr %p, size %d, cmd %#x, resp code %#x",
> +				addr, len, cmd, h->response_code);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/**
> + * Wrapper for test_one_sccb to set up an SCCB template
> + */
> +static int test_one_run(uint32_t cmd, uint8_t *addr, uint16_t sccb_len,
> +			uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	if (buf_len)
> +		memset(sccb_template, 0, sizeof(sccb_template));
> +	((SCCBHeader *)sccb_template)->length = sccb_len;
> +	if (buf_len && buf_len < 8)
> +		buf_len = 8;
> +	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
> +}
> +
> +/**
> + * Test SCCB lengths < 8
> + */
> +static void test_sccb_too_short(void)
> +{
> +	int cx;
> +
> +	for (cx = 0; cx < 8; cx++)
> +		if (!test_one_run(valid_code, pagebuf, cx, 8, PGM_BIT_SPEC, 0))
> +			break;
> +
> +	report("SCCB too short", cx == 8);
> +}
> +
> +/**
> + * Test SCCBs that are not 64bits aligned
> + */
> +static void test_sccb_unaligned(void)
> +{
> +	int cx;
> +
> +	for (cx = 1; cx < 8; cx++)
> +		if (!test_one_run(valid_code, cx + pagebuf, 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB unaligned", cx == 8);
> +}
> +
> +/**
> + * Test SCCBs whose address is in the lowcore or prefix area
> + */
> +static void test_sccb_prefix(void)
> +{
> +	uint32_t prefix, new_prefix;
> +	int cx;
> +
> +	// can't actually trash the lowcore, unsurprisingly things break if we do
> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_sccb(valid_code, MKPTR(cx), 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB low pages", cx == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);
> +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> +	asm volatile("stpx %0" : "=Q" (prefix));
> +	asm volatile("spx %0" : : "Q" (new_prefix) : "memory");
> +
> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_run(valid_code, MKPTR(new_prefix + cx), 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB prefix pages", cx == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);
> +	asm volatile("spx %0" : : "Q" (prefix) : "memory");
> +}
> +
> +/**
> + * Test SCCBs that are above 2GB. If outside of memory, an addressing
> + * exception is also allowed.
> + */
> +static void test_sccb_high(void)
> +{
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	uintptr_t a[33 * 4 * 2 + 2];
> +	uint64_t maxram;
> +	int cx, i, pgm;
> +
> +	h->length = 8;
> +
> +	i = 0;
> +	for (cx = 0; cx < 33; cx++)
> +		a[i++] = 1UL << (cx + 31);
> +	for (cx = 0; cx < 33; cx++)
> +		a[i++] = 3UL << (cx + 31);
> +	for (cx = 0; cx < 33; cx++)
> +		a[i++] = 0xffffffff80000000UL << cx;
> +	a[i++] = 0x80000000;
> +	for (cx = 1; cx < 33; cx++, i++)
> +		a[i] = a[i - 1] | (1UL << (cx + 31));
> +	for (cx = 0; cx < i; cx++)
> +		a[i + cx] = a[cx] + (intptr_t)pagebuf;
> +	i += cx;
> +	a[i++] = 0xdeadbeef00000000;
> +	a[i++] = 0xdeaddeadbeef0000;
> +
> +	maxram = get_ram_size();
> +	for (cx = 0; cx < i; cx++) {
> +		pgm = PGM_BIT_SPEC | (a[cx] >= maxram ? PGM_BIT_ADDR : 0);
> +		if (!test_one_sccb(valid_code, (void *)a[cx], 0, pgm, 0))
> +			break;
> +	}
> +	report("SCCB high addresses", cx == i);
> +}
> +
> +/**
> + * Test invalid commands, both invalid command detail codes and valid
> + * ones with invalid command class code.
> + */
> +static void test_inval(void)
> +{
> +	uint32_t cmd;
> +	int cx;
> +
> +	report_prefix_push("Invalid command");
> +	for (cx = 0; cx < 65536; cx++) {
> +		cmd = (0xdead0000) | cx;
> +		if (!test_one_run(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, 0, SCLP_RC_INVALID_SCLP_COMMAND))
> +			break;
> +	}
> +	report("Command detail code", cx == 65536);
> +
> +	for (cx = 0; cx < 256; cx++) {
> +		cmd = (valid_code & ~0xff) | cx;
> +		if (cmd == valid_code)
> +			continue;
> +		if (!test_one_run(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, 0, SCLP_RC_INVALID_SCLP_COMMAND))
> +			break;
> +	}
> +	report("Command class code", cx == 256);
> +	report_prefix_pop();
> +}
> +
> +
> +/**
> + * Test short SCCBs (but larger than 8).
> + */
> +static void test_short(void)
> +{
> +	uint16_t res = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
> +	int cx;
> +
> +	for (cx = 8; cx < 144; cx++)
> +		if (!test_one_run(valid_code, pagebuf, cx, cx, 0, res))
> +			break;
> +	report("Insufficient SCCB length (Read SCP info)", cx == 144);
> +
> +	for (cx = 8; cx < 40; cx++)
> +		if (!test_one_run(SCLP_READ_CPU_INFO, pagebuf, cx, cx, 0, res))
> +			break;
> +	report("Insufficient SCCB length (Read CPU info)", cx == 40);
> +}
> +
> +/**
> + * Test SCCB page boundary violations.
> + */
> +static void test_boundary(void)
> +{
> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
> +	WriteEventData *sccb = (WriteEventData *)sccb_template;
> +	int len, cx;
> +
> +	memset(sccb_template, 0, sizeof(sccb_template));
> +	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> +	for (len = 32; len <= 4096; len++) {
> +		cx = len & 7 ? len & ~7 : len - 8;
> +		for (cx = 4096 - cx; cx < 4096; cx += 8) {
> +			sccb->h.length = len;
> +			if (!test_one_sccb(cmd, cx + pagebuf, len, 0, res))
> +				goto out;
> +		}
> +	}
> +out:
> +	report("SCCB page boundary violation", len > 4096 && cx == 4096);
> +}
> +
> +static void test_toolong(void)
> +{
> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;

Why use variables for constants that are never touched?

> +	WriteEventData *sccb = (WriteEventData *)sccb_template;
> +	int cx;
> +
> +	memset(sccb_template, 0, sizeof(sccb_template));
> +	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> +	for (cx = 4097; cx < 8192; cx++) {
> +		sccb->h.length = cx;
> +		if (!test_one_sccb(cmd, pagebuf, PAGE_SIZE, 0, res))
> +			break;
> +	}
> +	report("SCCB bigger than 4k", cx == 8192);
> +}
> +
> +/**
> + * Test privileged operation.
> + */
> +static void test_priv(void)
> +{
> +	report_prefix_push("Privileged operation");
> +	pagebuf[0] = 0;
> +	pagebuf[1] = 8;

Id much rather have a proper cast using the header struct.

> +	expect_pgm_int();
> +	enter_pstate();
> +	servc(valid_code, __pa(pagebuf));
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +/**
> + * Test addressing exceptions. We need to test SCCB addresses between the
> + * end of available memory and 2GB, because after 2GB a specification
> + * exception is also allowed.
> + * Only applicable if the VM has less than 2GB of memory
> + */
> +static void test_addressing(void)
> +{
> +	unsigned long cx, maxram = get_ram_size();
> +
> +	if (maxram >= 0x80000000) {
> +		report_skip("Invalid SCCB address");
> +		return;
> +	}
> +	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx += 8)
> +		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); cx += 4096)
> +		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +	for (; cx < 0x80000000; cx += 1048576)
> +		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +out:
> +	report("Invalid SCCB address", cx == 0x80000000);
> +}
> +
> +/**
> + * Test some bits in the instruction format that are specified to be ignored.
> + */
> +static void test_instbits(void)
> +{
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	unsigned long mask;
> +	int cc;
> +
> +	sclp_mark_busy();
> +	h->length = 8;
> +
> +	ctl_set_bit(0, 9);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);

Huh, you already got a function for that at the top.

> +
> +	asm volatile(
> +		"       .insn   rre,0xb2204200,%1,%2\n"  /* servc %1,%2 */
> +		"       ipm     %0\n"
> +		"       srl     %0,28"
> +		: "=&d" (cc) : "d" (valid_code), "a" (__pa(pagebuf))
> +		: "cc", "memory");
> +	sclp_wait_busy();
> +	report("Instruction format ignored bits", cc == 0);
> +}
> +
> +/**
> + * Find a valid SCLP command code; not all codes are always allowed, and
> + * probing should be performed in the right order.
> + */
> +static void find_valid_sclp_code(void)
> +{
> +	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
> +				    SCLP_CMDW_READ_SCP_INFO };
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	int i, cc;
> +
> +	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> +		sclp_mark_busy();
> +		memset(h, 0, sizeof(pagebuf));

pagebuf is 8k, but you can only use 4k in sclp.
We don't need to clear 2 pages.

> +		h->length = 4096;
> +
> +		valid_code = commands[i];
> +		cc = sclp_service_call(commands[i], h);
> +		if (cc)
> +			break;
> +		if (h->response_code == SCLP_RC_NORMAL_READ_COMPLETION)
> +			return;
> +		if (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND)
> +			break;

Depending on line length you could add that to the cc check.
Maybe you could also group the error conditions before the success
conditions or the other way around.

> +	}
> +	valid_code = 0;
> +	report_abort("READ_SCP_INFO failed");
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("sclp");
> +	find_valid_sclp_code();
> +
> +	/* Test some basic things */
> +	test_instbits();
> +	test_priv();
> +	test_addressing();
> +
> +	/* Test the specification exceptions */
> +	test_sccb_too_short();
> +	test_sccb_unaligned();
> +	test_sccb_prefix();
> +	test_sccb_high();
> +
> +	/* Test the expected response codes */
> +	test_inval();
> +	test_short();
> +	test_boundary();
> +	test_toolong();
> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index f1b07cd..75e3d37 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -75,3 +75,6 @@ file = stsi.elf
>  [smp]
>  file = smp.elf
>  extra_params =-smp 2
> +
> +[sclp]
> +file = sclp.elf

Don't we need a newline here?

> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH v2 4/5] s390x: sclp: add service call instruction wrapper

[David Hildenbrand]

On 25.10.19 19:06, Claudio Imbrenda wrote:
> Add a wrapper for the service call instruction, and use it for SCLP
> interactions instead of using inline assembly everywhere.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   lib/s390x/asm/arch_def.h | 13 +++++++++++++
>   lib/s390x/sclp.c         |  7 +------
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 96cca2e..b3caff6 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -269,4 +269,17 @@ static inline int stsi(void *addr, int fc, int sel1, int sel2)
>   	return cc;
>   }
>   
> +static inline int servc(uint32_t command, unsigned long sccb)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
> +		"       ipm     %0\n"
> +		"       srl     %0,28"
> +		: "=&d" (cc) : "d" (command), "a" (sccb)
> +		: "cc", "memory");
> +	return cc;
> +}
> +
>   #endif
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 7798f04..e35c282 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -116,12 +116,7 @@ int sclp_service_call(unsigned int command, void *sccb)
>   	int cc;
>   
>   	sclp_setup_int();
> -	asm volatile(
> -		"       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
> -		"       ipm     %0\n"
> -		"       srl     %0,28"
> -		: "=&d" (cc) : "d" (command), "a" (__pa(sccb))
> -		: "cc", "memory");
> +	cc = servc(command, __pa(sccb));
>   	sclp_wait_busy();
>   	if (cc == 3)
>   		return -1;
> 

I do wonder if we should really do that. Shouldn't we always set/wait if 
busy (especially, if testing for an error condition that won't trigger)? 
IOW, shouldn't we have a modified sclp_service_call() that returns the 
CC (and also calls sclp_setup_int()/sclp_wait_busy())?

We could also simply make sclp_service_call() return the cc and handle 
the cc in the existing callers.

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 0/5] s390x: SCLP Unit test

[David Hildenbrand]

On 25.10.19 19:06, Claudio Imbrenda wrote:
> This patchset contains some minor cleanup, some preparatory work and
> then the SCLP unit test itself.
> 
> The unit test checks the following:
>      
>      * Correctly ignoring instruction bits that should be ignored
>      * Privileged instruction check
>      * Check for addressing exceptions
>      * Specification exceptions:
>        - SCCB size less than 8
>        - SCCB unaligned
>        - SCCB overlaps prefix or lowcore
>        - SCCB address higher than 2GB
>      * Return codes for
>        - Invalid command
>        - SCCB too short (but at least 8)
>        - SCCB page boundary violation
> 
> v1 -> v2
> * fix many small issues that came up during the first round of reviews
> * add comments to each function
> * use a static buffer for the SCCP template when used
> 
> Claudio Imbrenda (5):
>    s390x: remove redundant defines
>    s390x: improve error reporting for interrupts
>    s390x: sclp: expose ram_size and max_ram_size
>    s390x: sclp: add service call instruction wrapper
>    s390x: SCLP unit test

Queued patch 1-3 to

https://github.com/davidhildenbrand/kvm-unit-tests.git s390x-next

Thanks!

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[David Hildenbrand]

On 25.10.19 19:06, Claudio Imbrenda wrote:
> SCLP unit test. Testing the following:
> 
> * Correctly ignoring instruction bits that should be ignored
> * Privileged instruction check
> * Check for addressing exceptions
> * Specification exceptions:
>    - SCCB size less than 8
>    - SCCB unaligned
>    - SCCB overlaps prefix or lowcore
>    - SCCB address higher than 2GB
> * Return codes for
>    - Invalid command
>    - SCCB too short (but at least 8)
>    - SCCB page boundary violation
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   s390x/Makefile      |   1 +
>   s390x/sclp.c        | 413 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg |   3 +
>   3 files changed, 417 insertions(+)
>   create mode 100644 s390x/sclp.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 3744372..ddb4b48 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
>   tests += $(TEST_DIR)/stsi.elf
>   tests += $(TEST_DIR)/skrf.elf
>   tests += $(TEST_DIR)/smp.elf
> +tests += $(TEST_DIR)/sclp.elf
>   tests_binary = $(patsubst %.elf,%.bin,$(tests))
>   
>   all: directories test_cases test_cases_binary
> diff --git a/s390x/sclp.c b/s390x/sclp.c
> new file mode 100644
> index 0000000..29ac265
> --- /dev/null
> +++ b/s390x/sclp.c
> @@ -0,0 +1,413 @@
> +/*
> + * Service Call tests
> + *
> + * Copyright (c) 2019 IBM Corp
> + *
> + * Authors:
> + *  Claudio Imbrenda <imbrenda@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <sclp.h>
> +
> +#define PGM_BIT_SPEC	(1ULL << PGM_INT_CODE_SPECIFICATION)
> +#define PGM_BIT_ADDR	(1ULL << PGM_INT_CODE_ADDRESSING)
> +#define PGM_BIT_PRIV	(1ULL << PGM_INT_CODE_PRIVILEGED_OPERATION)
> +#define MKPTR(x) ((void *)(uint64_t)(x))
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +static uint8_t prefix_buf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +static uint8_t sccb_template[PAGE_SIZE];
> +static uint32_t valid_code;
> +static struct lowcore *lc;
> +
> +/**
> + * Enable SCLP interrupt.
> + */
> +static void sclp_setup_int_test(void)
> +{
> +	uint64_t mask;
> +
> +	ctl_set_bit(0, 9);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +}
> +
> +/**
> + * Perform one service call, handling exceptions and interrupts.
> + */
> +static int sclp_service_call_test(unsigned int command, void *sccb)
> +{
> +	int cc;
> +
> +	sclp_mark_busy();
> +	sclp_setup_int_test();
> +	lc->pgm_int_code = 0;
> +	cc = servc(command, __pa(sccb));
> +	if (lc->pgm_int_code) {
> +		sclp_handle_ext();
> +		return 0;
> +	}
> +	if (!cc)
> +		sclp_wait_busy();
> +	return cc;
> +}
> +
> +/**
> + * Perform one test at the given address, optionally using the SCCB template,
> + * checking for the expected program interrupts and return codes.
> + */
> +static int test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t len,
> +			uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	SCCBHeader *h = (SCCBHeader *)addr;
> +	int res, pgm;
> +
> +	if (len)
> +		memcpy(addr, sccb_template, len);
> +	if (!exp_pgm)
> +		exp_pgm = 1;
> +	expect_pgm_int();
> +	res = sclp_service_call_test(cmd, h);
> +	if (res) {
> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
> +		return 0;
> +	}
> +	pgm = lc->pgm_int_code;
> +	if (!((1ULL << pgm) & exp_pgm)) {
> +		report_info("First failure at addr %p, size %d, cmd %#x, pgm code %d", addr, len, cmd, pgm);
> +		return 0;
> +	}
> +	if (exp_rc && (exp_rc != h->response_code)) {
> +		report_info("First failure at addr %p, size %d, cmd %#x, resp code %#x",
> +				addr, len, cmd, h->response_code);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/**
> + * Wrapper for test_one_sccb to set up an SCCB template
> + */
> +static int test_one_run(uint32_t cmd, uint8_t *addr, uint16_t sccb_len,
> +			uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	if (buf_len)
> +		memset(sccb_template, 0, sizeof(sccb_template));
> +	((SCCBHeader *)sccb_template)->length = sccb_len;
> +	if (buf_len && buf_len < 8)
> +		buf_len = 8;
> +	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
> +}
> +
> +/**
> + * Test SCCB lengths < 8
> + */
> +static void test_sccb_too_short(void)
> +{
> +	int cx;
> +
> +	for (cx = 0; cx < 8; cx++)
> +		if (!test_one_run(valid_code, pagebuf, cx, 8, PGM_BIT_SPEC, 0))
> +			break;
> +
> +	report("SCCB too short", cx == 8);
> +}
> +
> +/**
> + * Test SCCBs that are not 64bits aligned
> + */
> +static void test_sccb_unaligned(void)
> +{
> +	int cx;
> +
> +	for (cx = 1; cx < 8; cx++)
> +		if (!test_one_run(valid_code, cx + pagebuf, 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB unaligned", cx == 8);
> +}
> +
> +/**
> + * Test SCCBs whose address is in the lowcore or prefix area
> + */
> +static void test_sccb_prefix(void)
> +{
> +	uint32_t prefix, new_prefix;
> +	int cx;
> +
> +	// can't actually trash the lowcore, unsurprisingly things break if we do
> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_sccb(valid_code, MKPTR(cx), 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB low pages", cx == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);
> +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> +	asm volatile("stpx %0" : "=Q" (prefix));
> +	asm volatile("spx %0" : : "Q" (new_prefix) : "memory");
> +
> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_run(valid_code, MKPTR(new_prefix + cx), 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB prefix pages", cx == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);
> +	asm volatile("spx %0" : : "Q" (prefix) : "memory");
> +}
> +
> +/**
> + * Test SCCBs that are above 2GB. If outside of memory, an addressing
> + * exception is also allowed.
> + */
> +static void test_sccb_high(void)
> +{
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	uintptr_t a[33 * 4 * 2 + 2];
> +	uint64_t maxram;
> +	int cx, i, pgm;
> +
> +	h->length = 8;
> +
> +	i = 0;
> +	for (cx = 0; cx < 33; cx++)
> +		a[i++] = 1UL << (cx + 31);
> +	for (cx = 0; cx < 33; cx++)
> +		a[i++] = 3UL << (cx + 31);
> +	for (cx = 0; cx < 33; cx++)
> +		a[i++] = 0xffffffff80000000UL << cx;
> +	a[i++] = 0x80000000;
> +	for (cx = 1; cx < 33; cx++, i++)
> +		a[i] = a[i - 1] | (1UL << (cx + 31));
> +	for (cx = 0; cx < i; cx++)
> +		a[i + cx] = a[cx] + (intptr_t)pagebuf;
> +	i += cx;
> +	a[i++] = 0xdeadbeef00000000;
> +	a[i++] = 0xdeaddeadbeef0000;
> +
> +	maxram = get_ram_size();
> +	for (cx = 0; cx < i; cx++) {
> +		pgm = PGM_BIT_SPEC | (a[cx] >= maxram ? PGM_BIT_ADDR : 0);
> +		if (!test_one_sccb(valid_code, (void *)a[cx], 0, pgm, 0))
> +			break;
> +	}
> +	report("SCCB high addresses", cx == i);
> +}
> +
> +/**
> + * Test invalid commands, both invalid command detail codes and valid
> + * ones with invalid command class code.
> + */
> +static void test_inval(void)
> +{
> +	uint32_t cmd;
> +	int cx;
> +
> +	report_prefix_push("Invalid command");
> +	for (cx = 0; cx < 65536; cx++) {
> +		cmd = (0xdead0000) | cx;
> +		if (!test_one_run(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, 0, SCLP_RC_INVALID_SCLP_COMMAND))
> +			break;
> +	}
> +	report("Command detail code", cx == 65536);
> +
> +	for (cx = 0; cx < 256; cx++) {
> +		cmd = (valid_code & ~0xff) | cx;
> +		if (cmd == valid_code)
> +			continue;
> +		if (!test_one_run(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, 0, SCLP_RC_INVALID_SCLP_COMMAND))
> +			break;
> +	}
> +	report("Command class code", cx == 256);
> +	report_prefix_pop();
> +}
> +
> +
> +/**
> + * Test short SCCBs (but larger than 8).
> + */
> +static void test_short(void)
> +{
> +	uint16_t res = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
> +	int cx;
> +
> +	for (cx = 8; cx < 144; cx++)
> +		if (!test_one_run(valid_code, pagebuf, cx, cx, 0, res))
> +			break;
> +	report("Insufficient SCCB length (Read SCP info)", cx == 144);
> +
> +	for (cx = 8; cx < 40; cx++)
> +		if (!test_one_run(SCLP_READ_CPU_INFO, pagebuf, cx, cx, 0, res))
> +			break;
> +	report("Insufficient SCCB length (Read CPU info)", cx == 40);
> +}
> +
> +/**
> + * Test SCCB page boundary violations.
> + */
> +static void test_boundary(void)
> +{
> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
> +	WriteEventData *sccb = (WriteEventData *)sccb_template;
> +	int len, cx;
> +
> +	memset(sccb_template, 0, sizeof(sccb_template));
> +	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> +	for (len = 32; len <= 4096; len++) {
> +		cx = len & 7 ? len & ~7 : len - 8;
> +		for (cx = 4096 - cx; cx < 4096; cx += 8) {
> +			sccb->h.length = len;
> +			if (!test_one_sccb(cmd, cx + pagebuf, len, 0, res))
> +				goto out;
> +		}
> +	}
> +out:
> +	report("SCCB page boundary violation", len > 4096 && cx == 4096);
> +}
> +
> +static void test_toolong(void)
> +{
> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
> +	WriteEventData *sccb = (WriteEventData *)sccb_template;
> +	int cx;
> +
> +	memset(sccb_template, 0, sizeof(sccb_template));
> +	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> +	for (cx = 4097; cx < 8192; cx++) {
> +		sccb->h.length = cx;
> +		if (!test_one_sccb(cmd, pagebuf, PAGE_SIZE, 0, res))
> +			break;
> +	}
> +	report("SCCB bigger than 4k", cx == 8192);
> +}
> +
> +/**
> + * Test privileged operation.
> + */
> +static void test_priv(void)
> +{
> +	report_prefix_push("Privileged operation");
> +	pagebuf[0] = 0;
> +	pagebuf[1] = 8;
> +	expect_pgm_int();
> +	enter_pstate();
> +	servc(valid_code, __pa(pagebuf));
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +/**
> + * Test addressing exceptions. We need to test SCCB addresses between the
> + * end of available memory and 2GB, because after 2GB a specification
> + * exception is also allowed.
> + * Only applicable if the VM has less than 2GB of memory
> + */
> +static void test_addressing(void)
> +{
> +	unsigned long cx, maxram = get_ram_size();
> +
> +	if (maxram >= 0x80000000) {
> +		report_skip("Invalid SCCB address");
> +		return;
> +	}
> +	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx += 8)
> +		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); cx += 4096)
> +		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +	for (; cx < 0x80000000; cx += 1048576)
> +		if (!test_one_sccb(valid_code, (void *)cx, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +out:
> +	report("Invalid SCCB address", cx == 0x80000000);
> +}
> +
> +/**
> + * Test some bits in the instruction format that are specified to be ignored.
> + */
> +static void test_instbits(void)
> +{
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	unsigned long mask;
> +	int cc;
> +
> +	sclp_mark_busy();
> +	h->length = 8;
> +
> +	ctl_set_bit(0, 9);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +
> +	asm volatile(
> +		"       .insn   rre,0xb2204200,%1,%2\n"  /* servc %1,%2 */
> +		"       ipm     %0\n"
> +		"       srl     %0,28"
> +		: "=&d" (cc) : "d" (valid_code), "a" (__pa(pagebuf))
> +		: "cc", "memory");
> +	sclp_wait_busy();
> +	report("Instruction format ignored bits", cc == 0);
> +}
> +
> +/**
> + * Find a valid SCLP command code; not all codes are always allowed, and
> + * probing should be performed in the right order.
> + */
> +static void find_valid_sclp_code(void)
> +{
> +	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
> +				    SCLP_CMDW_READ_SCP_INFO };
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	int i, cc;
> +
> +	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> +		sclp_mark_busy();
> +		memset(h, 0, sizeof(pagebuf));
> +		h->length = 4096;
> +
> +		valid_code = commands[i];
> +		cc = sclp_service_call(commands[i], h);
> +		if (cc)
> +			break;
> +		if (h->response_code == SCLP_RC_NORMAL_READ_COMPLETION)
> +			return;
> +		if (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND)
> +			break;
> +	}
> +	valid_code = 0;
> +	report_abort("READ_SCP_INFO failed");
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("sclp");
> +	find_valid_sclp_code();
> +
> +	/* Test some basic things */
> +	test_instbits();
> +	test_priv();
> +	test_addressing();
> +
> +	/* Test the specification exceptions */
> +	test_sccb_too_short();
> +	test_sccb_unaligned();
> +	test_sccb_prefix();
> +	test_sccb_high();
> +
> +	/* Test the expected response codes */
> +	test_inval();
> +	test_short();
> +	test_boundary();
> +	test_toolong();
> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index f1b07cd..75e3d37 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -75,3 +75,6 @@ file = stsi.elf
>   [smp]
>   file = smp.elf
>   extra_params =-smp 2
> +
> +[sclp]
> +file = sclp.elf
> 

Can we just please rename all "cx" into something like "len"? Or is 
there a real need to have "cx" in there?

Also, I still dislike "test_one_sccb". Can't we just just do something like

expect_pgm_int();
rc = test_one_sccb(...)
report("whatever pgm", rc == WHATEVER);
report("whatever rc", lc->pgm_int_code == WHATEVER);

In the callers to make these tests readable and cleanup test_one_sccb(). 
I don't care if that produces more LOC as long as I can actually read 
and understand the test cases.

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

On Mon, 4 Nov 2019 10:45:07 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

[...]

> > +/**
> > + * Enable SCLP interrupt.
> > + */
> > +static void sclp_setup_int_test(void)
> > +{
> > +	uint64_t mask;
> > +
> > +	ctl_set_bit(0, 9);
> > +	mask = extract_psw_mask();
> > +	mask |= PSW_MASK_EXT;
> > +	load_psw_mask(mask);
> > +}  
> 
> Or you could just export the definition in sclp.c...

I could, but is it worth it to export the definition just for this
one use?


[...]

> > +static void test_toolong(void)
> > +{
> > +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> > +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;  
> 
> Why use variables for constants that are never touched?

readability mostly. the names of the constants are rather long.
the compiler will notice it and do the Right Thing™

> > +	WriteEventData *sccb = (WriteEventData *)sccb_template;
> > +	int cx;
> > +
> > +	memset(sccb_template, 0, sizeof(sccb_template));
> > +	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> > +	for (cx = 4097; cx < 8192; cx++) {
> > +		sccb->h.length = cx;
> > +		if (!test_one_sccb(cmd, pagebuf, PAGE_SIZE, 0,
> > res))
> > +			break;
> > +	}
> > +	report("SCCB bigger than 4k", cx == 8192);
> > +}
> > +
> > +/**
> > + * Test privileged operation.
> > + */
> > +static void test_priv(void)
> > +{
> > +	report_prefix_push("Privileged operation");
> > +	pagebuf[0] = 0;
> > +	pagebuf[1] = 8;  
> 
> Id much rather have a proper cast using the header struct.

ok, will fix

> > +	expect_pgm_int();
> > +	enter_pstate();
> > +	servc(valid_code, __pa(pagebuf));
> > +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> > +	report_prefix_pop();
> > +}
> > +
> > +/**
> > + * Test addressing exceptions. We need to test SCCB addresses
> > between the
> > + * end of available memory and 2GB, because after 2GB a
> > specification
> > + * exception is also allowed.
> > + * Only applicable if the VM has less than 2GB of memory
> > + */
> > +static void test_addressing(void)
> > +{
> > +	unsigned long cx, maxram = get_ram_size();
> > +
> > +	if (maxram >= 0x80000000) {
> > +		report_skip("Invalid SCCB address");
> > +		return;
> > +	}
> > +	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx
> > += 8)
> > +		if (!test_one_sccb(valid_code, (void *)cx, 0,
> > PGM_BIT_ADDR, 0))
> > +			goto out;
> > +	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff,
> > 0x80000000); cx += 4096)
> > +		if (!test_one_sccb(valid_code, (void *)cx, 0,
> > PGM_BIT_ADDR, 0))
> > +			goto out;
> > +	for (; cx < 0x80000000; cx += 1048576)
> > +		if (!test_one_sccb(valid_code, (void *)cx, 0,
> > PGM_BIT_ADDR, 0))
> > +			goto out;
> > +out:
> > +	report("Invalid SCCB address", cx == 0x80000000);
> > +}
> > +
> > +/**
> > + * Test some bits in the instruction format that are specified to
> > be ignored.
> > + */
> > +static void test_instbits(void)
> > +{
> > +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> > +	unsigned long mask;
> > +	int cc;
> > +
> > +	sclp_mark_busy();
> > +	h->length = 8;
> > +
> > +	ctl_set_bit(0, 9);
> > +	mask = extract_psw_mask();
> > +	mask |= PSW_MASK_EXT;
> > +	load_psw_mask(mask);  
> 
> Huh, you already got a function for that at the top.

oops. will fix
 
> > +
> > +	asm volatile(
> > +		"       .insn   rre,0xb2204200,%1,%2\n"  /* servc
> > %1,%2 */
> > +		"       ipm     %0\n"
> > +		"       srl     %0,28"
> > +		: "=&d" (cc) : "d" (valid_code),
> > "a" (__pa(pagebuf))
> > +		: "cc", "memory");
> > +	sclp_wait_busy();
> > +	report("Instruction format ignored bits", cc == 0);
> > +}
> > +
> > +/**
> > + * Find a valid SCLP command code; not all codes are always
> > allowed, and
> > + * probing should be performed in the right order.
> > + */
> > +static void find_valid_sclp_code(void)
> > +{
> > +	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
> > +				    SCLP_CMDW_READ_SCP_INFO };
> > +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> > +	int i, cc;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> > +		sclp_mark_busy();
> > +		memset(h, 0, sizeof(pagebuf));  
> 
> pagebuf is 8k, but you can only use 4k in sclp.
> We don't need to clear 2 pages.

well, technically I don't even need to clear the whole buffer at all.
I should probably simply clear just the header.

> > +		h->length = 4096;
> > +
> > +		valid_code = commands[i];
> > +		cc = sclp_service_call(commands[i], h);
> > +		if (cc)
> > +			break;
> > +		if (h->response_code ==
> > SCLP_RC_NORMAL_READ_COMPLETION)
> > +			return;
> > +		if (h->response_code !=
> > SCLP_RC_INVALID_SCLP_COMMAND)
> > +			break;  
> 
> Depending on line length you could add that to the cc check.
> Maybe you could also group the error conditions before the success
> conditions or the other way around.

yeah it woud fit, but I'm not sure it would be more readable:

if (cc || (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND))
                        break;

I think readability is more important that saving lines of source code,
especially when the compiler will be smart enough to do the Right Thing™

also, that is copy-pasted directly from lib/s390x/sclp.c

> > +	}
> > +	valid_code = 0;
> > +	report_abort("READ_SCP_INFO failed");
> > +}
> > +
> > +int main(void)
> > +{
> > +	report_prefix_push("sclp");
> > +	find_valid_sclp_code();
> > +
> > +	/* Test some basic things */
> > +	test_instbits();
> > +	test_priv();
> > +	test_addressing();
> > +
> > +	/* Test the specification exceptions */
> > +	test_sccb_too_short();
> > +	test_sccb_unaligned();
> > +	test_sccb_prefix();
> > +	test_sccb_high();
> > +
> > +	/* Test the expected response codes */
> > +	test_inval();
> > +	test_short();
> > +	test_boundary();
> > +	test_toolong();
> > +
> > +	return report_summary();
> > +}
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index f1b07cd..75e3d37 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -75,3 +75,6 @@ file = stsi.elf
> >  [smp]
> >  file = smp.elf
> >  extra_params =-smp 2
> > +
> > +[sclp]
> > +file = sclp.elf  
> 
> Don't we need a newline here?

no, the file ended already with a newline, the three lines are added
above the final newline, so there is always a newline at the end of the
file.

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

On Mon, 4 Nov 2019 11:58:20 +0100
David Hildenbrand <david@redhat.com> wrote:

[...]

> Can we just please rename all "cx" into something like "len"? Or is 
> there a real need to have "cx" in there?

if cx is such a nuisance to you, sure, I can rename it to i

> Also, I still dislike "test_one_sccb". Can't we just just do
> something like
> 
> expect_pgm_int();
> rc = test_one_sccb(...)
> report("whatever pgm", rc == WHATEVER);
> report("whatever rc", lc->pgm_int_code == WHATEVER);
> 
> In the callers to make these tests readable and cleanup
> test_one_sccb(). I don't care if that produces more LOC as long as I
> can actually read and understand the test cases.

if you think that makes it more readable, ok I guess...

consider that the output will be unreadable, though

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[David Hildenbrand]

On 04.11.19 12:29, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 11:58:20 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
> [...]
> 
>> Can we just please rename all "cx" into something like "len"? Or is
>> there a real need to have "cx" in there?
> 
> if cx is such a nuisance to you, sure, I can rename it to i

better than random characters :)

> 
>> Also, I still dislike "test_one_sccb". Can't we just just do
>> something like
>>
>> expect_pgm_int();
>> rc = test_one_sccb(...)
>> report("whatever pgm", rc == WHATEVER);
>> report("whatever rc", lc->pgm_int_code == WHATEVER);
>>
>> In the callers to make these tests readable and cleanup
>> test_one_sccb(). I don't care if that produces more LOC as long as I
>> can actually read and understand the test cases.
> 
> if you think that makes it more readable, ok I guess...
> 
> consider that the output will be unreadable, though
> 

I think his will turn out more readable.

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

On Mon, 4 Nov 2019 12:31:32 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.11.19 12:29, Claudio Imbrenda wrote:
> > On Mon, 4 Nov 2019 11:58:20 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> > 
> > [...]
> >   
> >> Can we just please rename all "cx" into something like "len"? Or is
> >> there a real need to have "cx" in there?  
> > 
> > if cx is such a nuisance to you, sure, I can rename it to i  
> 
> better than random characters :)

will be in v3

> >   
> >> Also, I still dislike "test_one_sccb". Can't we just just do
> >> something like
> >>
> >> expect_pgm_int();
> >> rc = test_one_sccb(...)
> >> report("whatever pgm", rc == WHATEVER);
> >> report("whatever rc", lc->pgm_int_code == WHATEVER);
> >>
> >> In the callers to make these tests readable and cleanup
> >> test_one_sccb(). I don't care if that produces more LOC as long as
> >> I can actually read and understand the test cases.  
> > 
> > if you think that makes it more readable, ok I guess...
> > 
> > consider that the output will be unreadable, though
> >   
> 
> I think his will turn out more readable.

two output lines per SCLP call? I  don't think so

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[David Hildenbrand]

On 04.11.19 12:49, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 12:31:32 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.11.19 12:29, Claudio Imbrenda wrote:
>>> On Mon, 4 Nov 2019 11:58:20 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>> [...]
>>>    
>>>> Can we just please rename all "cx" into something like "len"? Or is
>>>> there a real need to have "cx" in there?
>>>
>>> if cx is such a nuisance to you, sure, I can rename it to i
>>
>> better than random characters :)
> 
> will be in v3
> 
>>>    
>>>> Also, I still dislike "test_one_sccb". Can't we just just do
>>>> something like
>>>>
>>>> expect_pgm_int();
>>>> rc = test_one_sccb(...)
>>>> report("whatever pgm", rc == WHATEVER);
>>>> report("whatever rc", lc->pgm_int_code == WHATEVER);
>>>>
>>>> In the callers to make these tests readable and cleanup
>>>> test_one_sccb(). I don't care if that produces more LOC as long as
>>>> I can actually read and understand the test cases.
>>>
>>> if you think that makes it more readable, ok I guess...
>>>
>>> consider that the output will be unreadable, though
>>>    
>>
>> I think his will turn out more readable.
> 
> two output lines per SCLP call? I  don't think so

To clarify, we don't always need two checks. E.g., I would like to see 
instead of

+static void test_sccb_too_short(void)
+{
+	int cx;
+
+	for (cx = 0; cx < 8; cx++)
+		if (!test_one_run(valid_code, pagebuf, cx, 8, PGM_BIT_SPEC, 0))
+			break;
+
+	report("SCCB too short", cx == 8);
+}

Something like

static void test_sccb_too_short(void)
{
	int i;

	for (i = 0; i < 8; i++) {
		expect_pgm_int();
		test_one_sccb(...); // or however that will be called
		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
	}
}

If possible.

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

On Mon, 4 Nov 2019 12:55:48 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.11.19 12:49, Claudio Imbrenda wrote:
> > On Mon, 4 Nov 2019 12:31:32 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 04.11.19 12:29, Claudio Imbrenda wrote:  
> >>> On Mon, 4 Nov 2019 11:58:20 +0100
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> [...]
> >>>      
> >>>> Can we just please rename all "cx" into something like "len"? Or
> >>>> is there a real need to have "cx" in there?  
> >>>
> >>> if cx is such a nuisance to you, sure, I can rename it to i  
> >>
> >> better than random characters :)  
> > 
> > will be in v3
> >   
> >>>      
> >>>> Also, I still dislike "test_one_sccb". Can't we just just do
> >>>> something like
> >>>>
> >>>> expect_pgm_int();
> >>>> rc = test_one_sccb(...)
> >>>> report("whatever pgm", rc == WHATEVER);
> >>>> report("whatever rc", lc->pgm_int_code == WHATEVER);
> >>>>
> >>>> In the callers to make these tests readable and cleanup
> >>>> test_one_sccb(). I don't care if that produces more LOC as long
> >>>> as I can actually read and understand the test cases.  
> >>>
> >>> if you think that makes it more readable, ok I guess...
> >>>
> >>> consider that the output will be unreadable, though
> >>>      
> >>
> >> I think his will turn out more readable.  
> > 
> > two output lines per SCLP call? I  don't think so  
> 
> To clarify, we don't always need two checks. E.g., I would like to
> see instead of
> 
> +static void test_sccb_too_short(void)
> +{
> +	int cx;
> +
> +	for (cx = 0; cx < 8; cx++)
> +		if (!test_one_run(valid_code, pagebuf, cx, 8,
> PGM_BIT_SPEC, 0))
> +			break;
> +
> +	report("SCCB too short", cx == 8);
> +}
> 
> Something like
> 
> static void test_sccb_too_short(void)
> {
> 	int i;
> 
> 	for (i = 0; i < 8; i++) {
> 		expect_pgm_int();
> 		test_one_sccb(...); // or however that will be called
> 		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> 	}
> }
> 
> If possible.
> 

so, thousands of output lines for the whole test, ok

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[David Hildenbrand]

On 04.11.19 13:06, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 12:55:48 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.11.19 12:49, Claudio Imbrenda wrote:
>>> On Mon, 4 Nov 2019 12:31:32 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>    
>>>> On 04.11.19 12:29, Claudio Imbrenda wrote:
>>>>> On Mon, 4 Nov 2019 11:58:20 +0100
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> [...]
>>>>>       
>>>>>> Can we just please rename all "cx" into something like "len"? Or
>>>>>> is there a real need to have "cx" in there?
>>>>>
>>>>> if cx is such a nuisance to you, sure, I can rename it to i
>>>>
>>>> better than random characters :)
>>>
>>> will be in v3
>>>    
>>>>>       
>>>>>> Also, I still dislike "test_one_sccb". Can't we just just do
>>>>>> something like
>>>>>>
>>>>>> expect_pgm_int();
>>>>>> rc = test_one_sccb(...)
>>>>>> report("whatever pgm", rc == WHATEVER);
>>>>>> report("whatever rc", lc->pgm_int_code == WHATEVER);
>>>>>>
>>>>>> In the callers to make these tests readable and cleanup
>>>>>> test_one_sccb(). I don't care if that produces more LOC as long
>>>>>> as I can actually read and understand the test cases.
>>>>>
>>>>> if you think that makes it more readable, ok I guess...
>>>>>
>>>>> consider that the output will be unreadable, though
>>>>>       
>>>>
>>>> I think his will turn out more readable.
>>>
>>> two output lines per SCLP call? I  don't think so
>>
>> To clarify, we don't always need two checks. E.g., I would like to
>> see instead of
>>
>> +static void test_sccb_too_short(void)
>> +{
>> +	int cx;
>> +
>> +	for (cx = 0; cx < 8; cx++)
>> +		if (!test_one_run(valid_code, pagebuf, cx, 8,
>> PGM_BIT_SPEC, 0))
>> +			break;
>> +
>> +	report("SCCB too short", cx == 8);
>> +}
>>
>> Something like
>>
>> static void test_sccb_too_short(void)
>> {
>> 	int i;
>>
>> 	for (i = 0; i < 8; i++) {
>> 		expect_pgm_int();
>> 		test_one_sccb(...); // or however that will be called
>> 		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> 	}
>> }
>>
>> If possible.
>>
> 
> so, thousands of output lines for the whole test, ok
> 

A couple of things to note

a) You perform 8 checks, so report the result of 8 checks
b) We really don't care about the number of lines in a log file as long 
as we can roughly identify what went wrong (e.g., push/pop a prefix here)
c) We really *don't* need full coverage here. The same applies to other 
tests. Simply testing against the boundary conditions is good enough.


expect_pgm_int();
test_one_sccb(..., 0, ...);
check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);

expect_pgm_int();
test_one_sccb(..., 7, ...);
check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);

Just as we handle it in other tests.

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

On Mon, 4 Nov 2019 14:47:54 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.11.19 13:06, Claudio Imbrenda wrote:
> > On Mon, 4 Nov 2019 12:55:48 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 04.11.19 12:49, Claudio Imbrenda wrote:  
> >>> On Mon, 4 Nov 2019 12:31:32 +0100
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>      
> >>>> On 04.11.19 12:29, Claudio Imbrenda wrote:  
> >>>>> On Mon, 4 Nov 2019 11:58:20 +0100
> >>>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>>
> >>>>> [...]
> >>>>>         
> >>>>>> Can we just please rename all "cx" into something like "len"?
> >>>>>> Or is there a real need to have "cx" in there?  
> >>>>>
> >>>>> if cx is such a nuisance to you, sure, I can rename it to i  
> >>>>
> >>>> better than random characters :)  
> >>>
> >>> will be in v3
> >>>      
> >>>>>         
> >>>>>> Also, I still dislike "test_one_sccb". Can't we just just do
> >>>>>> something like
> >>>>>>
> >>>>>> expect_pgm_int();
> >>>>>> rc = test_one_sccb(...)
> >>>>>> report("whatever pgm", rc == WHATEVER);
> >>>>>> report("whatever rc", lc->pgm_int_code == WHATEVER);
> >>>>>>
> >>>>>> In the callers to make these tests readable and cleanup
> >>>>>> test_one_sccb(). I don't care if that produces more LOC as long
> >>>>>> as I can actually read and understand the test cases.  
> >>>>>
> >>>>> if you think that makes it more readable, ok I guess...
> >>>>>
> >>>>> consider that the output will be unreadable, though
> >>>>>         
> >>>>
> >>>> I think his will turn out more readable.  
> >>>
> >>> two output lines per SCLP call? I  don't think so  
> >>
> >> To clarify, we don't always need two checks. E.g., I would like to
> >> see instead of
> >>
> >> +static void test_sccb_too_short(void)
> >> +{
> >> +	int cx;
> >> +
> >> +	for (cx = 0; cx < 8; cx++)
> >> +		if (!test_one_run(valid_code, pagebuf, cx, 8,
> >> PGM_BIT_SPEC, 0))
> >> +			break;
> >> +
> >> +	report("SCCB too short", cx == 8);
> >> +}
> >>
> >> Something like
> >>
> >> static void test_sccb_too_short(void)
> >> {
> >> 	int i;
> >>
> >> 	for (i = 0; i < 8; i++) {
> >> 		expect_pgm_int();
> >> 		test_one_sccb(...); // or however that will be
> >> called check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> >> 	}
> >> }
> >>
> >> If possible.
> >>  
> > 
> > so, thousands of output lines for the whole test, ok
> >   
> 
> A couple of things to note
> 
> a) You perform 8 checks, so report the result of 8 checks
> b) We really don't care about the number of lines in a log file as
> long as we can roughly identify what went wrong (e.g., push/pop a
> prefix here) c) We really *don't* need full coverage here. The same
> applies to other tests. Simply testing against the boundary
> conditions is good enough.
> 
> 
> expect_pgm_int();
> test_one_sccb(..., 0, ...);
> check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> 
> expect_pgm_int();
> test_one_sccb(..., 7, ...);
> check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> 
> Just as we handle it in other tests.

the fact that the other test are not as extensive as they should be
doesn't mean this test should cover less.

In fact, I have found bugs in some implementations of SCLP exactly
because I did not test only the boundary conditions. 

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[David Hildenbrand]

On 04.11.19 15:24, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 14:47:54 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.11.19 13:06, Claudio Imbrenda wrote:
>>> On Mon, 4 Nov 2019 12:55:48 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>    
>>>> On 04.11.19 12:49, Claudio Imbrenda wrote:
>>>>> On Mon, 4 Nov 2019 12:31:32 +0100
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>       
>>>>>> On 04.11.19 12:29, Claudio Imbrenda wrote:
>>>>>>> On Mon, 4 Nov 2019 11:58:20 +0100
>>>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>          
>>>>>>>> Can we just please rename all "cx" into something like "len"?
>>>>>>>> Or is there a real need to have "cx" in there?
>>>>>>>
>>>>>>> if cx is such a nuisance to you, sure, I can rename it to i
>>>>>>
>>>>>> better than random characters :)
>>>>>
>>>>> will be in v3
>>>>>       
>>>>>>>          
>>>>>>>> Also, I still dislike "test_one_sccb". Can't we just just do
>>>>>>>> something like
>>>>>>>>
>>>>>>>> expect_pgm_int();
>>>>>>>> rc = test_one_sccb(...)
>>>>>>>> report("whatever pgm", rc == WHATEVER);
>>>>>>>> report("whatever rc", lc->pgm_int_code == WHATEVER);
>>>>>>>>
>>>>>>>> In the callers to make these tests readable and cleanup
>>>>>>>> test_one_sccb(). I don't care if that produces more LOC as long
>>>>>>>> as I can actually read and understand the test cases.
>>>>>>>
>>>>>>> if you think that makes it more readable, ok I guess...
>>>>>>>
>>>>>>> consider that the output will be unreadable, though
>>>>>>>          
>>>>>>
>>>>>> I think his will turn out more readable.
>>>>>
>>>>> two output lines per SCLP call? I  don't think so
>>>>
>>>> To clarify, we don't always need two checks. E.g., I would like to
>>>> see instead of
>>>>
>>>> +static void test_sccb_too_short(void)
>>>> +{
>>>> +	int cx;
>>>> +
>>>> +	for (cx = 0; cx < 8; cx++)
>>>> +		if (!test_one_run(valid_code, pagebuf, cx, 8,
>>>> PGM_BIT_SPEC, 0))
>>>> +			break;
>>>> +
>>>> +	report("SCCB too short", cx == 8);
>>>> +}
>>>>
>>>> Something like
>>>>
>>>> static void test_sccb_too_short(void)
>>>> {
>>>> 	int i;
>>>>
>>>> 	for (i = 0; i < 8; i++) {
>>>> 		expect_pgm_int();
>>>> 		test_one_sccb(...); // or however that will be
>>>> called check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>>> 	}
>>>> }
>>>>
>>>> If possible.
>>>>   
>>>
>>> so, thousands of output lines for the whole test, ok
>>>    
>>
>> A couple of things to note
>>
>> a) You perform 8 checks, so report the result of 8 checks
>> b) We really don't care about the number of lines in a log file as
>> long as we can roughly identify what went wrong (e.g., push/pop a
>> prefix here) c) We really *don't* need full coverage here. The same
>> applies to other tests. Simply testing against the boundary
>> conditions is good enough.
>>
>>
>> expect_pgm_int();
>> test_one_sccb(..., 0, ...);
>> check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>
>> expect_pgm_int();
>> test_one_sccb(..., 7, ...);
>> check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>
>> Just as we handle it in other tests.
> 
> the fact that the other test are not as extensive as they should be
> doesn't mean this test should cover less.

All I'm saying is that you might test too much and I am not sure if that 
is really needed everywhere in this patch. But I'll leave that up to you.

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Thomas Huth]

On 04/11/2019 12.19, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 10:45:07 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
[...]
>>> +static void test_toolong(void)
>>> +{
>>> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
>>> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
>>
>> Why use variables for constants that are never touched?
> 
> readability mostly. the names of the constants are rather long.
> the compiler will notice it and do the Right Thing™

I'd like to suggest to add the "const" keyword to both variables in that 
case, then it's clear that they are not used to be modified.

>>> +		h->length = 4096;
>>> +
>>> +		valid_code = commands[i];
>>> +		cc = sclp_service_call(commands[i], h);
>>> +		if (cc)
>>> +			break;
>>> +		if (h->response_code ==
>>> SCLP_RC_NORMAL_READ_COMPLETION)
>>> +			return;
>>> +		if (h->response_code !=
>>> SCLP_RC_INVALID_SCLP_COMMAND)
>>> +			break;
>>
>> Depending on line length you could add that to the cc check.
>> Maybe you could also group the error conditions before the success
>> conditions or the other way around.
> 
> yeah it woud fit, but I'm not sure it would be more readable:
> 
> if (cc || (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND))
>                          break;

In case you go with that solution, please drop the innermost parentheses.

  Thomas

Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Claudio Imbrenda]

On Fri, 8 Nov 2019 10:35:32 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 04/11/2019 12.19, Claudio Imbrenda wrote:
> > On Mon, 4 Nov 2019 10:45:07 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:  
> [...]
> >>> +static void test_toolong(void)
> >>> +{
> >>> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> >>> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;  
> >>
> >> Why use variables for constants that are never touched?  
> > 
> > readability mostly. the names of the constants are rather long.
> > the compiler will notice it and do the Right Thing™  
> 
> I'd like to suggest to add the "const" keyword to both variables in
> that case, then it's clear that they are not used to be modified.

good point

> >>> +		h->length = 4096;
> >>> +
> >>> +		valid_code = commands[i];
> >>> +		cc = sclp_service_call(commands[i], h);
> >>> +		if (cc)
> >>> +			break;
> >>> +		if (h->response_code ==
> >>> SCLP_RC_NORMAL_READ_COMPLETION)
> >>> +			return;
> >>> +		if (h->response_code !=
> >>> SCLP_RC_INVALID_SCLP_COMMAND)
> >>> +			break;  
> >>
> >> Depending on line length you could add that to the cc check.
> >> Maybe you could also group the error conditions before the success
> >> conditions or the other way around.  
> > 
> > yeah it woud fit, but I'm not sure it would be more readable:
> > 
> > if (cc || (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND))
> >                          break;  
> 
> In case you go with that solution, please drop the innermost
> parentheses.

why so much hatred for parentheses? :D

but no, I'm not going to do it, it's not just less readable, it's
actually wrong!

SCLP_RC_NORMAL_READ_COMPLETION != SCLP_RC_INVALID_SCLP_COMMAND

the correct version would be:

if (cc ||
	h->response_code != SCLP_RC_INVALID_SCLP_COMMAND &&
	h->response_code != SCLP_RC_NORMAL_READ_COMPLETION)

which is more lines, and significantly less readable.