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

[kvm-unit-tests PATCH v1 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

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         |  16 +-
 s390x/sclp.c             | 373 +++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 7 files changed, 404 insertions(+), 10 deletions(-)
 create mode 100644 s390x/sclp.c

-- 
2.7.4

[kvm-unit-tests PATCH v1 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>
---
 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 v1 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>
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 v1 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>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 lib/s390x/sclp.h | 2 ++
 lib/s390x/sclp.c | 9 +++++++++
 2 files changed, 11 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..a57096c 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -167,3 +167,12 @@ 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 v1 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>
---
 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 a57096c..376040e 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 v1 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        | 373 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 377 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..d7a9212
--- /dev/null
+++ b/s390x/sclp.c
@@ -0,0 +1,373 @@
+/*
+ * Store System Information 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>
+
+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 uint32_t valid_sclp_code;
+static struct lowcore *lc;
+
+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);
+}
+
+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;
+}
+
+static int test_one_sccb(uint32_t cmd, uint64_t addr, uint16_t len,
+			void *data, uint64_t exp_pgm, uint16_t exp_rc)
+{
+	SCCBHeader *h = (SCCBHeader *)addr;
+	int res, pgm;
+
+	if (data && len)
+		memcpy((void *)addr, data, 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 %#lx, cc %d)",
+			cmd, addr, res);
+		return 0;
+	}
+	pgm = lc->pgm_int_code;
+	if (!((1ULL << pgm) & exp_pgm)) {
+		report_info("First failure at addr %#lx, 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 %#lx, size %d, cmd %#x, resp code %#x",
+				addr, len, cmd, h->response_code);
+		return 0;
+	}
+	return 1;
+}
+
+static int test_one_run(uint32_t cmd, uint64_t addr, uint16_t len,
+			uint16_t clear, uint64_t exp_pgm, uint16_t exp_rc)
+{
+	char sccb[4096];
+	void *p = sccb;
+
+	if (!len && !clear)
+		p = NULL;
+	else
+		memset(sccb, 0, sizeof(sccb));
+	((SCCBHeader *)sccb)->length = len;
+	if (clear && (clear < 8))
+		clear = 8;
+	return test_one_sccb(cmd, addr, clear, p, exp_pgm, exp_rc);
+}
+
+#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 PGBUF	((uintptr_t)pagebuf)
+#define VALID	(valid_sclp_code)
+
+static void test_sccb_too_short(void)
+{
+	int cx;
+
+	for (cx = 0; cx < 8; cx++)
+		if (!test_one_run(VALID, PGBUF, cx, 8, PGM_BIT_SPEC, 0))
+			break;
+
+	report("SCCB too short", cx == 8);
+}
+
+static void test_sccb_unaligned(void)
+{
+	int cx;
+
+	for (cx = 1; cx < 8; cx++)
+		if (!test_one_run(VALID, cx + PGBUF, 8, 8, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB unaligned", cx == 8);
+}
+
+static void test_sccb_prefix(void)
+{
+	uint32_t prefix, new_prefix;
+	int cx;
+
+	for (cx = 0; cx < 8192; cx += 8)
+		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB low pages", cx == 8192);
+
+	new_prefix = (uint32_t)(intptr_t)prefix_buf;
+	memcpy(prefix_buf, 0, 8192);
+	asm volatile("stpx %0": "+Q"(prefix));
+	asm volatile("spx %0": "+Q"(new_prefix));
+
+	for (cx = 0; cx < 8192; cx += 8)
+		if (!test_one_run(VALID, 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));
+}
+
+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_run(VALID, a[cx], 0, 0, pgm, 0))
+			break;
+	}
+	report("SCCB high addresses", cx == i);
+}
+
+static void test_spec(void)
+{
+	test_sccb_too_short();
+	test_sccb_unaligned();
+	test_sccb_prefix();
+	test_sccb_high();
+}
+
+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 (cmd == VALID)
+			continue;
+		if (!test_one_run(cmd, PGBUF, 4096, 0, 0, SCLP_RC_INVALID_SCLP_COMMAND))
+			break;
+	}
+	report("Command detail code", cx == 65536);
+
+	for (cx = 0; cx < 256; cx++) {
+		cmd = (VALID & ~0xff) | cx;
+		if (cmd == VALID)
+			continue;
+		if (!test_one_run(cmd, PGBUF, 4096, 4096, 0, SCLP_RC_INVALID_SCLP_COMMAND))
+			break;
+	}
+	report("Command class code", cx == 256);
+	report_prefix_pop();
+}
+
+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, PGBUF, 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, PGBUF, cx, cx, 0, res))
+			break;
+	report("Insufficient SCCB length (Read CPU info)", cx == 40);
+}
+
+static void test_boundary(void)
+{
+	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
+	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
+	char sccb_static[4096] = {0};
+	WriteEventData *sccb = (WriteEventData *)sccb_static;
+	int len, cx;
+
+	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 + PGBUF, len, sccb, 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;
+	char sccb_static[8192] = {0};
+	WriteEventData *sccb = (WriteEventData *)sccb_static;
+	int cx;
+
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (cx = 4097; cx < 8192; cx++) {
+		sccb->h.length = cx;
+		if (!test_one_sccb(cmd, PGBUF, cx, sccb, 0, res))
+			break;
+	}
+	report("SCCB bigger than 4k", cx == 8192);
+}
+
+static void test_resp(void)
+{
+	test_inval();
+	test_short();
+	test_boundary();
+	test_toolong();
+}
+
+static void test_priv(void)
+{
+	pagebuf[0] = 0;
+	pagebuf[1] = 8;
+	expect_pgm_int();
+	enter_pstate();
+	servc(valid_sclp_code, __pa(pagebuf));
+	report_prefix_push("Priv check");
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+}
+
+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_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); cx += 4096)
+		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; cx < 0x80000000; cx += 1048576)
+		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
+			goto out;
+out:
+	report("Invalid SCCB address", cx == 0x80000000);
+}
+
+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_sclp_code), "a" (__pa(pagebuf))
+		: "cc", "memory");
+	sclp_wait_busy();
+	report("Instruction format ignored bits", cc == 0);
+}
+
+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_sclp_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_sclp_code = 0;
+	report_abort("READ_SCP_INFO failed");
+}
+
+int main(void)
+{
+	report_prefix_push("sclp");
+	find_valid_sclp_code();
+	test_instbits();
+	test_priv();
+	test_addressing();
+	test_spec();
+	test_resp();
+	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 v1 1/5] s390x: remove redundant defines

[Thomas Huth]

On 22/10/2019 12.53, Claudio Imbrenda wrote:
> 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>
> ---
>  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 */
> 

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

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

[Thomas Huth]

On 22/10/2019 12.53, Claudio Imbrenda wrote:
> 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>
> 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;
>  	}
>  
> 

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

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

[Thomas Huth]

On 22/10/2019 12.53, 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>
> ---
>  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 a57096c..376040e 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;
> 

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

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

[Thomas Huth]

On 22/10/2019 12.53, 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.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  lib/s390x/sclp.h | 2 ++
>  lib/s390x/sclp.c | 9 +++++++++
>  2 files changed, 11 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..a57096c 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -167,3 +167,12 @@ 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;
> +}

In case you respin, please add an empty line between the two functions.

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

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

[David Hildenbrand]

On 22.10.19 12:53, Claudio Imbrenda wrote:
> 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>
> ---
>   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 */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 22.10.19 12:53, Claudio Imbrenda wrote:
> 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>
> 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;
>   	}
>   
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 22.10.19 14:16, Thomas Huth wrote:
> On 22/10/2019 12.53, 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.
>>
>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   lib/s390x/sclp.h | 2 ++
>>   lib/s390x/sclp.c | 9 +++++++++
>>   2 files changed, 11 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..a57096c 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -167,3 +167,12 @@ 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;
>> +}
> 
> In case you respin, please add an empty line between the two functions.
>

Indeed

Reviewed-by: David Hildenbrand <david@redhat.com>

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

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 22.10.19 12:53, Claudio Imbrenda wrote:
> Add a wrapper for the service call instruction, and use it for SCLP
> interactions instead of using inline assembly everywhere.

The description is weird.

"Let's factor out the assembly for the service call instruction, we want 
to reuse that for actual SCLP service call tests soon."

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.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 a57096c..376040e 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;
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 22.10.19 12:53, 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        | 373 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg |   3 +
>   3 files changed, 377 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..d7a9212
> --- /dev/null
> +++ b/s390x/sclp.c
> @@ -0,0 +1,373 @@
> +/*
> + * Store System Information 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>
> +
> +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 uint32_t valid_sclp_code;
> +static struct lowcore *lc;
> +
> +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);
> +}
> +
> +static int sclp_service_call_test(unsigned int command, void *sccb)

Wouldn't it be easier to pass an uint8_t*, so you can simply forward 
pagebuf through all functions?

> +{
> +	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();

You receive a PGM interrupt and handle an external interrupt? That looks 
strange. Please elaborate what's going on here.

> +		return 0;
> +	}
> +	if (!cc)
> +		sclp_wait_busy();
> +	return cc;
> +}
> +
> +static int test_one_sccb(uint32_t cmd, uint64_t addr, uint16_t len,
> +			void *data, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	SCCBHeader *h = (SCCBHeader *)addr;
> +	int res, pgm;
> +
> +	if (data && len)
> +		memcpy((void *)addr, data, 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 %#lx, cc %d)",
> +			cmd, addr, res);
> +		return 0;
> +	}
> +	pgm = lc->pgm_int_code;
> +	if (!((1ULL << pgm) & exp_pgm)) {
> +		report_info("First failure at addr %#lx, 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 %#lx, size %d, cmd %#x, resp code %#x",
> +				addr, len, cmd, h->response_code);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +static int test_one_run(uint32_t cmd, uint64_t addr, uint16_t len,
> +			uint16_t clear, uint64_t exp_pgm, uint16_t exp_rc)

I somewhat dislike passing in "exp_pgm" and "exp_rc". Why can't you 
handle both things in the caller where the tests actually become readable?

> +{
> +	char sccb[4096];

I prefer uint8_t sccb[PAGE_SIZE]

> +	void *p = sccb;
> +
> +	if (!len && !clear)
> +		p = NULL;
> +	else
> +		memset(sccb, 0, sizeof(sccb));
> +	((SCCBHeader *)sccb)->length = len;
> +	if (clear && (clear < 8))
> +		clear = 8;

Can you elaborate what "clear" means. It is passed as "length".
/me confused

> +	return test_one_sccb(cmd, addr, clear, p, exp_pgm, exp_rc);
> +}
> +
> +#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 PGBUF	((uintptr_t)pagebuf)
> +#define VALID	(valid_sclp_code)

I dislike both defines, can you get rid of these?

> +
> +static void test_sccb_too_short(void)
> +{
> +	int cx;

cx is passed as "len". What does cx stand for? Can we give this a better 
name?

[...] (not reviewing the other stuff in detail because I am still confused)

> +static void test_resp(void)
> +{
> +	test_inval();
> +	test_short();
> +	test_boundary();
> +	test_toolong();
> +}

Can you just get rid of this function and call all tests from main?
(just separate them in logical blocks eventually with comments)

> +
> +static void test_priv(void)
> +{
> +	pagebuf[0] = 0;
> +	pagebuf[1] = 8;
> +	expect_pgm_int();
> +	enter_pstate();
> +	servc(valid_sclp_code, __pa(pagebuf));
> +	report_prefix_push("Priv check");
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();

Can we push at the beginning of the function and pop at the end?

report_prefix_push("Privileged Operation");

expect_pgm_int();
enter_pstate();
servc(valid_sclp_code, __pa(pagebuf));
check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);

report_prefix_pop();

Also shouldn't you better mark sclp busy and wait for interrupts to make
sore you handle it correctly in case the check *fails* and servc 
completes (cc==0)?

> +}
> +
> +static void test_addressing(void)
> +{
> +	unsigned long cx, maxram = get_ram_size();
> +
> +	if (maxram >= 0x80000000) {
> +		report_skip("Invalid SCCB address");
> +		return;
> +	}

Do we really need maxram here, can't we simply use very high addresses 
like in all other tests?

E.g. just user address "-PAGE_SIZE"

> +	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx += 8)
> +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); cx += 4096)
> +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +	for (; cx < 0x80000000; cx += 1048576)
> +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
> +			goto out;
> +out:
> +	report("Invalid SCCB address", cx == 0x80000000);
> +}
> +
> +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_sclp_code), "a" (__pa(pagebuf))
> +		: "cc", "memory");
> +	sclp_wait_busy();
> +	report("Instruction format ignored bits", cc == 0);
> +}
> +
> +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_sclp_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_sclp_code = 0;
> +	report_abort("READ_SCP_INFO failed");
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("sclp");
> +	find_valid_sclp_code();
> +	test_instbits();
> +	test_priv();
> +	test_addressing();
> +	test_spec();
> +	test_resp();
> +	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
> 


-- 

Thanks,

David / dhildenb

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

[Thomas Huth]

----- Original Message -----
> From: "Claudio Imbrenda" <imbrenda@linux.ibm.com>
> Sent: Tuesday, October 22, 2019 12:53:04 PM
> 
> 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        | 373
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 +
>  3 files changed, 377 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..d7a9212
> --- /dev/null
> +++ b/s390x/sclp.c
> @@ -0,0 +1,373 @@
> +/*
> + * Store System Information tests

Copy-n-paste from stsi.c ?

> + * 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>
[...]
> +static int test_one_run(uint32_t cmd, uint64_t addr, uint16_t len,
> +			uint16_t clear, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	char sccb[4096];
> +	void *p = sccb;
> +
> +	if (!len && !clear)
> +		p = NULL;
> +	else
> +		memset(sccb, 0, sizeof(sccb));
> +	((SCCBHeader *)sccb)->length = len;
> +	if (clear && (clear < 8))

Please remove the parentheses around "clear < 8".

> +		clear = 8;
> +	return test_one_sccb(cmd, addr, clear, p, exp_pgm, exp_rc);
> +}
> +
> +#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 PGBUF	((uintptr_t)pagebuf)
> +#define VALID	(valid_sclp_code)
> +
> +static void test_sccb_too_short(void)
> +{
> +	int cx;
> +
> +	for (cx = 0; cx < 8; cx++)
> +		if (!test_one_run(VALID, PGBUF, cx, 8, PGM_BIT_SPEC, 0))
> +			break;
> +
> +	report("SCCB too short", cx == 8);
> +}
> +
> +static void test_sccb_unaligned(void)
> +{
> +	int cx;
> +
> +	for (cx = 1; cx < 8; cx++)
> +		if (!test_one_run(VALID, cx + PGBUF, 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB unaligned", cx == 8);
> +}
> +
> +static void test_sccb_prefix(void)
> +{
> +	uint32_t prefix, new_prefix;
> +	int cx;
> +
> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB low pages", cx == 8192);
> +
> +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> +	memcpy(prefix_buf, 0, 8192);
> +	asm volatile("stpx %0": "+Q"(prefix));

Isn't "=Q" sufficient enough here?

> +	asm volatile("spx %0": "+Q"(new_prefix));

Shouldn't that be just an input parameter instead? ... and maybe also better add "memory" to the clobber list, since the memory layout has changed.

> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_run(VALID, 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));

dito?

> +}

 Thomas

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

[Claudio Imbrenda]

On Wed, 23 Oct 2019 08:48:33 -0400 (EDT)
Thomas Huth <thuth@redhat.com> wrote:

> ----- Original Message -----
> > From: "Claudio Imbrenda" <imbrenda@linux.ibm.com>
> > Sent: Tuesday, October 22, 2019 12:53:04 PM
> > 
> > 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        | 373
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  s390x/unittests.cfg |   3 +
> >  3 files changed, 377 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..d7a9212
> > --- /dev/null
> > +++ b/s390x/sclp.c
> > @@ -0,0 +1,373 @@
> > +/*
> > + * Store System Information tests  
> 
> Copy-n-paste from stsi.c ?

Oops

> 
> > + * 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>  
> [...]
> > +static int test_one_run(uint32_t cmd, uint64_t addr, uint16_t len,
> > +			uint16_t clear, uint64_t exp_pgm, uint16_t
> > exp_rc) +{
> > +	char sccb[4096];
> > +	void *p = sccb;
> > +
> > +	if (!len && !clear)
> > +		p = NULL;
> > +	else
> > +		memset(sccb, 0, sizeof(sccb));
> > +	((SCCBHeader *)sccb)->length = len;
> > +	if (clear && (clear < 8))  
> 
> Please remove the parentheses around "clear < 8".

I usually prefer to have more parentheses than necessary
than fewer, but I'll fix it

> 
> > +		clear = 8;
> > +	return test_one_sccb(cmd, addr, clear, p, exp_pgm, exp_rc);
> > +}
> > +
> > +#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 PGBUF	((uintptr_t)pagebuf)
> > +#define VALID	(valid_sclp_code)
> > +
> > +static void test_sccb_too_short(void)
> > +{
> > +	int cx;
> > +
> > +	for (cx = 0; cx < 8; cx++)
> > +		if (!test_one_run(VALID, PGBUF, cx, 8,
> > PGM_BIT_SPEC, 0))
> > +			break;
> > +
> > +	report("SCCB too short", cx == 8);
> > +}
> > +
> > +static void test_sccb_unaligned(void)
> > +{
> > +	int cx;
> > +
> > +	for (cx = 1; cx < 8; cx++)
> > +		if (!test_one_run(VALID, cx + PGBUF, 8, 8,
> > PGM_BIT_SPEC, 0))
> > +			break;
> > +	report("SCCB unaligned", cx == 8);
> > +}
> > +
> > +static void test_sccb_prefix(void)
> > +{
> > +	uint32_t prefix, new_prefix;
> > +	int cx;
> > +
> > +	for (cx = 0; cx < 8192; cx += 8)
> > +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_SPEC,
> > 0))
> > +			break;
> > +	report("SCCB low pages", cx == 8192);
> > +
> > +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> > +	memcpy(prefix_buf, 0, 8192);
> > +	asm volatile("stpx %0": "+Q"(prefix));  
> 
> Isn't "=Q" sufficient enough here?

Ooops. think I copypasted this from somewhere else. Will fix.

> 
> > +	asm volatile("spx %0": "+Q"(new_prefix));  
> 
> Shouldn't that be just an input parameter instead? ... and maybe also
> better add "memory" to the clobber list, since the memory layout has
> changed.

same

> 
> > +	for (cx = 0; cx < 8192; cx += 8)
> > +		if (!test_one_run(VALID, 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));  
> 
> dito?

same

> 
> > +}  
> 
>  Thomas
> 

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

[Claudio Imbrenda]

On Wed, 23 Oct 2019 14:14:56 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 22.10.19 12:53, 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        | 373
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > s390x/unittests.cfg |   3 + 3 files changed, 377 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..d7a9212
> > --- /dev/null
> > +++ b/s390x/sclp.c
> > @@ -0,0 +1,373 @@
> > +/*
> > + * Store System Information 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>
> > +
> > +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 uint32_t valid_sclp_code; +static struct lowcore *lc;
> > +
> > +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);
> > +}
> > +
> > +static int sclp_service_call_test(unsigned int command, void
> > *sccb)  
> 
> Wouldn't it be easier to pass an uint8_t*, so you can simply forward 
> pagebuf through all functions?

I'm not sure I understand what you mean here. Sometimes I'm passing
random numbers for the addresses. so either I have to cast addresses to
integers, or integers to addresses.

I'll change it to accept pointers, we can see if it looks cleaner

> 
> > +{
> > +	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();  
> 
> You receive a PGM interrupt and handle an external interrupt? That
> looks strange. Please elaborate what's going on here.

sclp_handle_ext clears the external interrupt bit, which we have set
earlier, and then sets busy=false. we need to do both things, even
though we have not received an external interrupt. I simply did not
want to reinvent the wheel

> 
> > +		return 0;
> > +	}
> > +	if (!cc)
> > +		sclp_wait_busy();
> > +	return cc;
> > +}
> > +
> > +static int test_one_sccb(uint32_t cmd, uint64_t addr, uint16_t len,
> > +			void *data, uint64_t exp_pgm, uint16_t
> > exp_rc) +{
> > +	SCCBHeader *h = (SCCBHeader *)addr;
> > +	int res, pgm;
> > +
> > +	if (data && len)
> > +		memcpy((void *)addr, data, 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
> > %#lx, cc %d)",
> > +			cmd, addr, res);
> > +		return 0;
> > +	}
> > +	pgm = lc->pgm_int_code;
> > +	if (!((1ULL << pgm) & exp_pgm)) {
> > +		report_info("First failure at addr %#lx, 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 %#lx, size %d,
> > cmd %#x, resp code %#x",
> > +				addr, len, cmd, h->response_code);
> > +		return 0;
> > +	}
> > +	return 1;
> > +}
> > +
> > +static int test_one_run(uint32_t cmd, uint64_t addr, uint16_t len,
> > +			uint16_t clear, uint64_t exp_pgm, uint16_t
> > exp_rc)  
> 
> I somewhat dislike passing in "exp_pgm" and "exp_rc". Why can't you 
> handle both things in the caller where the tests actually become
> readable?

how would it be more readable? this means that after each call I should
have a lot of checks for all the possible errors, and all those checks
would be all the same. Lots of possibilities for copy-paste errors, and
it would inflate the code a lot.

> 
> > +{
> > +	char sccb[4096];  
> 
> I prefer uint8_t sccb[PAGE_SIZE]

will fix

> 
> > +	void *p = sccb;
> > +
> > +	if (!len && !clear)
> > +		p = NULL;
> > +	else
> > +		memset(sccb, 0, sizeof(sccb));
> > +	((SCCBHeader *)sccb)->length = len;
> > +	if (clear && (clear < 8))
> > +		clear = 8;  
> 
> Can you elaborate what "clear" means. It is passed as "length".
> /me confused

clear indicates how much of the actual memory will be overwritten (or
cleared). if you have an 8-byte SCCB and 4096 for clear, the SCCB will
have length 8, but all 4096 bytes will be written to memory (the
remaining 4088 will be 0, hence "clear")

I will rename len to sccb_len and clear to buf_len, maybe it helps 

> 
> > +	return test_one_sccb(cmd, addr, clear, p, exp_pgm, exp_rc);
> > +}
> > +
> > +#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 PGBUF	((uintptr_t)pagebuf)
> > +#define VALID	(valid_sclp_code)  
> 
> I dislike both defines, can you get rid of these?

I was almost expecting this. the code will get more verbose, but ok

> 
> > +
> > +static void test_sccb_too_short(void)
> > +{
> > +	int cx;  
> 
> cx is passed as "len". What does cx stand for? Can we give this a
> better name?

it's just a counter, like "i" or so.

> 
> [...] (not reviewing the other stuff in detail because I am still
> confused)
> 
> > +static void test_resp(void)
> > +{
> > +	test_inval();
> > +	test_short();
> > +	test_boundary();
> > +	test_toolong();
> > +}  
> 
> Can you just get rid of this function and call all tests from main?
> (just separate them in logical blocks eventually with comments)

ok

> 
> > +
> > +static void test_priv(void)
> > +{
> > +	pagebuf[0] = 0;
> > +	pagebuf[1] = 8;
> > +	expect_pgm_int();
> > +	enter_pstate();
> > +	servc(valid_sclp_code, __pa(pagebuf));
> > +	report_prefix_push("Priv check");
> > +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> > +	report_prefix_pop();  
> 
> Can we push at the beginning of the function and pop at the end?
> 
> report_prefix_push("Privileged Operation");
> 
> expect_pgm_int();
> enter_pstate();
> servc(valid_sclp_code, __pa(pagebuf));
> check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> 
> report_prefix_pop();

ok

> 
> Also shouldn't you better mark sclp busy and wait for interrupts to
> make sore you handle it correctly in case the check *fails* and servc 
> completes (cc==0)?

In case we actually get the privileged operation exception, we will not
clear the sclp_busy flag. and check_pgm_int_code will try to write to
the console, waiting for sclp_busy to become 0, which will not happen.

The solution is to copy-paste most of sclp_service_call_test. This will
also bring no benefits, since once we have failed such a basic test, we
don't really have any guarantees that the SCLP implementation will be
sound. We simply report an error and move on.


> 
> > +}
> > +
> > +static void test_addressing(void)
> > +{
> > +	unsigned long cx, maxram = get_ram_size();
> > +
> > +	if (maxram >= 0x80000000) {
> > +		report_skip("Invalid SCCB address");
> > +		return;
> > +	}  
> 
> Do we really need maxram here, can't we simply use very high
> addresses like in all other tests?

no. the address must be both above available memory and below 2GB.
I need to know where the memory ends, so I can try to test addresses
right after the end of memory.

If the VM is started with more than 2GB of RAM, this test doesn't
make any sense, and is therefore skipped.

> 
> E.g. just user address "-PAGE_SIZE"

this might result in a specification exception, instead of an
addressing exception, since the address is above 2GB. For invalid
addresses over 2GB both are acceptable. Here I want to test explicitly
the addressing exceptions.

> 
> > +	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx
> > += 8)
> > +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR,
> > 0))
> > +			goto out;
> > +	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff,
> > 0x80000000); cx += 4096)
> > +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR,
> > 0))
> > +			goto out;
> > +	for (; cx < 0x80000000; cx += 1048576)
> > +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR,
> > 0))
> > +			goto out;
> > +out:
> > +	report("Invalid SCCB address", cx == 0x80000000);
> > +}
> > +
> > +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_sclp_code),
> > "a" (__pa(pagebuf))
> > +		: "cc", "memory");
> > +	sclp_wait_busy();
> > +	report("Instruction format ignored bits", cc == 0);
> > +}
> > +
> > +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_sclp_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_sclp_code = 0;
> > +	report_abort("READ_SCP_INFO failed");
> > +}
> > +
> > +int main(void)
> > +{
> > +	report_prefix_push("sclp");
> > +	find_valid_sclp_code();
> > +	test_instbits();
> > +	test_priv();
> > +	test_addressing();
> > +	test_spec();
> > +	test_resp();
> > +	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
> >   
> 
>