[kvm-unit-tests PATCH v3 0/2] s390x: SCLP Unit test

[kvm-unit-tests PATCH v3 0/2] 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

v2 -> v3
* generally improved the naming of variables
* added and fixed comments
* renamed test_one_run to test_one_simple
* added some const where needed
* addresed many more small comments received during review
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 (2):
  s390x: sclp: add service call instruction wrapper
  s390x: SCLP unit test

 s390x/Makefile           |   1 +
 lib/s390x/asm/arch_def.h |  13 ++
 lib/s390x/sclp.c         |   7 +-
 s390x/sclp.c             | 417 +++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 5 files changed, 435 insertions(+), 6 deletions(-)
 create mode 100644 s390x/sclp.c

-- 
2.7.4

[kvm-unit-tests PATCH v3 1/2] 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>
Reviewed-by: Janosch Frank <frankja@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 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 v3 2/2] 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        | 417 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 421 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..3267a92
--- /dev/null
+++ b/s390x/sclp.c
@@ -0,0 +1,417 @@
+/*
+ * 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_NONE	1
+#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();
+	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.
+ * Returns 1 in case of success or 0 in case of failure
+ */
+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;
+
+	/* Copy the template to the test address if needed */
+	if (len)
+		memcpy(addr, sccb_template, len);
+	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 = clear_pgm_int();
+	/* Check if the program exception was one of the expected ones */
+	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;
+	}
+	/* Check if the response code is the one expected */
+	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 a simple SCCB template.
+ * Returns 1 in case of success or 0 in case of failure
+ */
+static int test_one_simple(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;
+	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
+}
+
+/**
+ * Test SCCB lengths < 8.
+ */
+static void test_sccb_too_short(void)
+{
+	int len;
+
+	for (len = 0; len < 8; len++)
+		if (!test_one_simple(valid_code, pagebuf, len, 8, PGM_BIT_SPEC, 0))
+			break;
+
+	report("SCCB too short", len == 8);
+}
+
+/**
+ * Test SCCBs that are not 64-bit aligned.
+ */
+static void test_sccb_unaligned(void)
+{
+	int offset;
+
+	for (offset = 1; offset < 8; offset++)
+		if (!test_one_simple(valid_code, offset + pagebuf, 8, 8, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB unaligned", offset == 8);
+}
+
+/**
+ * Test SCCBs whose address is in the lowcore or prefix area.
+ */
+static void test_sccb_prefix(void)
+{
+	uint32_t prefix, new_prefix;
+	int offset;
+
+	/* can't actually trash the lowcore, unsurprisingly things break if we do */
+	for (offset = 0; offset < 8192; offset += 8)
+		if (!test_one_sccb(valid_code, MKPTR(offset), 0, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB low pages", offset == 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 (offset = 0; offset < 8192; offset += 8)
+		if (!test_one_simple(valid_code, MKPTR(new_prefix + offset), 8, 8, PGM_BIT_SPEC, 0))
+			break;
+	report("SCCB prefix pages", offset == 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 i, pgm, len = 0;
+
+	h->length = 8;
+
+	for (i = 0; i < 33; i++)
+		a[len++] = 1UL << (i + 31);
+	for (i = 0; i < 33; i++)
+		a[len++] = 3UL << (i + 31);
+	for (i = 0; i < 33; i++)
+		a[len++] = 0xffffffff80000000UL << i;
+	a[len++] = 0x80000000;
+	for (i = 1; i < 33; i++, len++)
+		a[len] = a[len - 1] | (1UL << (i + 31));
+	for (i = 0; i < len; i++)
+		a[len + i] = a[i] + (intptr_t)h;
+	len += i;
+	a[len++] = 0xdeadbeef00000000;
+	a[len++] = 0xdeaddeadbeef0000;
+
+	maxram = get_ram_size();
+	for (i = 0; i < len; i++) {
+		pgm = PGM_BIT_SPEC | (a[i] >= maxram ? PGM_BIT_ADDR : 0);
+		if (!test_one_sccb(valid_code, (void *)a[i], 0, pgm, 0))
+			break;
+	}
+	report("SCCB high addresses", i == len);
+}
+
+/**
+ * Test invalid commands, both invalid command detail codes and valid
+ * ones with invalid command class code.
+ */
+static void test_inval(void)
+{
+	const uint16_t res = SCLP_RC_INVALID_SCLP_COMMAND;
+	uint32_t cmd;
+	int i;
+
+	report_prefix_push("Invalid command");
+	for (i = 0; i < 65536; i++) {
+		cmd = (0xdead0000) | i;
+		if (!test_one_simple(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, PGM_NONE, res))
+			break;
+	}
+	report("Command detail code", i == 65536);
+
+	for (i = 0; i < 256; i++) {
+		cmd = (valid_code & ~0xff) | i;
+		if (cmd == valid_code)
+			continue;
+		if (!test_one_simple(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, PGM_NONE, res))
+			break;
+	}
+	report("Command class code", i == 256);
+	report_prefix_pop();
+}
+
+
+/**
+ * Test short SCCBs (but larger than 8).
+ */
+static void test_short(void)
+{
+	const uint16_t res = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
+	int i;
+
+	for (i = 8; i < 144; i++)
+		if (!test_one_simple(valid_code, pagebuf, i, i, PGM_NONE, res))
+			break;
+	report("Insufficient SCCB length (Read SCP info)", i == 144);
+
+	for (i = 8; i < 40; i++)
+		if (!test_one_simple(SCLP_READ_CPU_INFO, pagebuf, i, i, PGM_NONE, res))
+			break;
+	report("Insufficient SCCB length (Read CPU info)", i == 40);
+}
+
+/**
+ * Test SCCB page boundary violations.
+ */
+static void test_boundary(void)
+{
+	const uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
+	const uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
+	WriteEventData *sccb = (WriteEventData *)sccb_template;
+	int len, i;
+
+	memset(sccb_template, 0, sizeof(sccb_template));
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (len = 32; len <= 4096; len++) {
+		i = len & 7 ? len & ~7 : len - 8;
+		for (i = 4096 - i; i < 4096; i += 8) {
+			sccb->h.length = len;
+			if (!test_one_sccb(cmd, i + pagebuf, len, PGM_NONE, res))
+				goto out;
+		}
+	}
+out:
+	report("SCCB page boundary violation", len > 4096 && i == 4096);
+}
+
+/**
+ * Test excessively long SCCBs.
+ */
+static void test_toolong(void)
+{
+	const uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
+	const uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
+	WriteEventData *sccb = (WriteEventData *)sccb_template;
+	int i;
+
+	memset(sccb_template, 0, sizeof(sccb_template));
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (i = 4097; i < 8192; i++) {
+		sccb->h.length = i;
+		if (!test_one_sccb(cmd, pagebuf, PAGE_SIZE, PGM_NONE, res))
+			break;
+	}
+	report("SCCB bigger than 4k", i == 8192);
+}
+
+/**
+ * Test privileged operation.
+ */
+static void test_priv(void)
+{
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+
+	report_prefix_push("Privileged operation");
+	h->length = 8;
+	expect_pgm_int();
+	enter_pstate();
+	servc(valid_code, __pa(h));
+	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 i, maxram = get_ram_size();
+
+	if (maxram >= 0x80000000) {
+		report_skip("Invalid SCCB address");
+		return;
+	}
+	for (i = maxram; i < MIN(maxram + 65536, 0x80000000); i += 8)
+		if (!test_one_sccb(valid_code, (void *)i, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; i < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); i += 4096)
+		if (!test_one_sccb(valid_code, (void *)i, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; i < 0x80000000; i += 1048576)
+		if (!test_one_sccb(valid_code, (void *)i, 0, PGM_BIT_ADDR, 0))
+			goto out;
+out:
+	report("Invalid SCCB address", i == 0x80000000);
+}
+
+/**
+ * Test some bits in the instruction format that are specified to be ignored.
+ */
+static void test_instbits(void)
+{
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	int cc;
+
+	expect_pgm_int();
+	sclp_mark_busy();
+	h->length = 8;
+	sclp_setup_int_test();
+
+	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");
+	if (lc->pgm_int_code) {
+		sclp_handle_ext();
+		cc = 1;
+	} else if (!cc)
+		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)
+{
+	const 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(*h));
+		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 v3 2/2] s390x: SCLP unit test

[Thomas Huth]

On 11/11/2019 18.20, 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
[...]
> +
> +#define PGM_NONE	1
> +#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);
> +}

I don't have a strong opinion here, but I think I'd slightly prefer to
use the function from lib/s390x/sclp.c instead, too.

> +/**
> + * 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();
> +	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,

I think you should at least mention the meaning of the "len" parameter
here, otherwise this is rather confusing (see below, my comment to
sccb_template).

> + * checking for the expected program interrupts and return codes.
> + * Returns 1 in case of success or 0 in case of failure

Could use bool with true + false instead.

> + */
> +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;
> +
> +	/* Copy the template to the test address if needed */
> +	if (len)
> +		memcpy(addr, sccb_template, len);

Honestly, that sccb_template is rather confusing. Why does the caller
has to provide both, the data in the sccb_template and the "addr"
variable for yet another buffer? Wouldn't it be simpler if the caller
simply sets up everything in a place of choice and then only passes the
"addr" to the buffer?

> +	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 = clear_pgm_int();
> +	/* Check if the program exception was one of the expected ones */
> +	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;
> +	}
> +	/* Check if the response code is the one expected */
> +	if (exp_rc && (exp_rc != h->response_code)) {

You can drop the parentheses around "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 a simple SCCB template.
> + * Returns 1 in case of success or 0 in case of failure
> + */
> +static int test_one_simple(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;
> +	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
> +}
[...]
> +/**
> + * Test SCCBs whose address is in the lowcore or prefix area.
> + */
> +static void test_sccb_prefix(void)
> +{
> +	uint32_t prefix, new_prefix;
> +	int offset;
> +
> +	/* can't actually trash the lowcore, unsurprisingly things break if we do */
> +	for (offset = 0; offset < 8192; offset += 8)
> +		if (!test_one_sccb(valid_code, MKPTR(offset), 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB low pages", offset == 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 (offset = 0; offset < 8192; offset += 8)
> +		if (!test_one_simple(valid_code, MKPTR(new_prefix + offset), 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB prefix pages", offset == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);

What's that memcpy() good for? A comment would be helpful.

> +	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 i, pgm, len = 0;
> +
> +	h->length = 8;
> +
> +	for (i = 0; i < 33; i++)
> +		a[len++] = 1UL << (i + 31);
> +	for (i = 0; i < 33; i++)
> +		a[len++] = 3UL << (i + 31);
> +	for (i = 0; i < 33; i++)
> +		a[len++] = 0xffffffff80000000UL << i;
> +	a[len++] = 0x80000000;
> +	for (i = 1; i < 33; i++, len++)
> +		a[len] = a[len - 1] | (1UL << (i + 31));
> +	for (i = 0; i < len; i++)
> +		a[len + i] = a[i] + (intptr_t)h;
> +	len += i;
> +	a[len++] = 0xdeadbeef00000000;
> +	a[len++] = 0xdeaddeadbeef0000;

IMHO a short comment in the code right in front of the above code block
would be helpful to understand what you're doing here.

> +	maxram = get_ram_size();
> +	for (i = 0; i < len; i++) {
> +		pgm = PGM_BIT_SPEC | (a[i] >= maxram ? PGM_BIT_ADDR : 0);
> +		if (!test_one_sccb(valid_code, (void *)a[i], 0, pgm, 0))
> +			break;
> +	}
> +	report("SCCB high addresses", i == len);
> +}
> +
> +/**
> + * Test invalid commands, both invalid command detail codes and valid
> + * ones with invalid command class code.
> + */
> +static void test_inval(void)
> +{
> +	const uint16_t res = SCLP_RC_INVALID_SCLP_COMMAND;
> +	uint32_t cmd;
> +	int i;
> +
> +	report_prefix_push("Invalid command");
> +	for (i = 0; i < 65536; i++) {
> +		cmd = (0xdead0000) | i;

Please remove the parentheses around 0xdead0000

> +		if (!test_one_simple(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, PGM_NONE, res))
> +			break;
> +	}
> +	report("Command detail code", i == 65536);
> +
> +	for (i = 0; i < 256; i++) {
> +		cmd = (valid_code & ~0xff) | i;
> +		if (cmd == valid_code)
> +			continue;
> +		if (!test_one_simple(cmd, pagebuf, PAGE_SIZE, PAGE_SIZE, PGM_NONE, res))
> +			break;
> +	}
> +	report("Command class code", i == 256);
> +	report_prefix_pop();
> +}

 Thomas

Re: [kvm-unit-tests PATCH v3 2/2] s390x: SCLP unit test

[Claudio Imbrenda]

On Wed, 13 Nov 2019 10:34:02 +0100
Thomas Huth <thuth@redhat.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);
> > +}  
> 
> I don't have a strong opinion here, but I think I'd slightly prefer to
> use the function from lib/s390x/sclp.c instead, too.

fine, I'll export it

> > +/**
> > + * 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();
> > +	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,  
> 
> I think you should at least mention the meaning of the "len" parameter
> here, otherwise this is rather confusing (see below, my comment to
> sccb_template).

I'll rename it and add comments

> > + * checking for the expected program interrupts and return codes.
> > + * Returns 1 in case of success or 0 in case of failure  
> 
> Could use bool with true + false instead.
> 
> > + */
> > +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;
> > +
> > +	/* Copy the template to the test address if needed */
> > +	if (len)
> > +		memcpy(addr, sccb_template, len);  
> 
> Honestly, that sccb_template is rather confusing. Why does the caller
> has to provide both, the data in the sccb_template and the "addr"
> variable for yet another buffer? Wouldn't it be simpler if the caller
> simply sets up everything in a place of choice and then only passes
> the "addr" to the buffer?

because you will test the same buffer at different addresses. this
mechanism abstracts this. instead of having to clear the buffer and set
the values for each address, you can simply set the template once and
then call the same function, changing only the target address.

also, the target address is not always a buffer, in many cases it is in
fact an invalid address, which should generate exceptions. 

> > +	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 = clear_pgm_int();
> > +	/* Check if the program exception was one of the expected
> > ones */
> > +	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;
> > +	}
> > +	/* Check if the response code is the one expected */
> > +	if (exp_rc && (exp_rc != h->response_code)) {  
> 
> You can drop the parentheses around "exp_rc != h->response_code".

fine, although I don't understand you hatred toward parentheses :)

> > +		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 a simple SCCB template.
> > + * Returns 1 in case of success or 0 in case of failure
> > + */
> > +static int test_one_simple(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;
> > +	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
> > +}  
> [...]
> > +/**
> > + * Test SCCBs whose address is in the lowcore or prefix area.
> > + */
> > +static void test_sccb_prefix(void)
> > +{
> > +	uint32_t prefix, new_prefix;
> > +	int offset;
> > +
> > +	/* can't actually trash the lowcore, unsurprisingly things
> > break if we do */
> > +	for (offset = 0; offset < 8192; offset += 8)
> > +		if (!test_one_sccb(valid_code, MKPTR(offset), 0,
> > PGM_BIT_SPEC, 0))
> > +			break;
> > +	report("SCCB low pages", offset == 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 (offset = 0; offset < 8192; offset += 8)
> > +		if (!test_one_simple(valid_code, MKPTR(new_prefix
> > + offset), 8, 8, PGM_BIT_SPEC, 0))
> > +			break;
> > +	report("SCCB prefix pages", offset == 8192);
> > +
> > +	memcpy(prefix_buf, 0, 8192);  
> 
> What's that memcpy() good for? A comment would be helpful.

we just moved the prefix to a temporary one, and thrashed the old one.
we can't simply set the old prefix and call it a day, things will break.

I added multiple comments to that function for v4 to explain what's
going on

> > +	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 i, pgm, len = 0;
> > +
> > +	h->length = 8;
> > +
> > +	for (i = 0; i < 33; i++)
> > +		a[len++] = 1UL << (i + 31);
> > +	for (i = 0; i < 33; i++)
> > +		a[len++] = 3UL << (i + 31);
> > +	for (i = 0; i < 33; i++)
> > +		a[len++] = 0xffffffff80000000UL << i;
> > +	a[len++] = 0x80000000;
> > +	for (i = 1; i < 33; i++, len++)
> > +		a[len] = a[len - 1] | (1UL << (i + 31));
> > +	for (i = 0; i < len; i++)
> > +		a[len + i] = a[i] + (intptr_t)h;
> > +	len += i;
> > +	a[len++] = 0xdeadbeef00000000;
> > +	a[len++] = 0xdeaddeadbeef0000;  
> 
> IMHO a short comment in the code right in front of the above code
> block would be helpful to understand what you're doing here.

v4 will have comments explaining each of the loops

> > +	maxram = get_ram_size();
> > +	for (i = 0; i < len; i++) {
> > +		pgm = PGM_BIT_SPEC | (a[i] >= maxram ?
> > PGM_BIT_ADDR : 0);
> > +		if (!test_one_sccb(valid_code, (void *)a[i], 0,
> > pgm, 0))
> > +			break;
> > +	}
> > +	report("SCCB high addresses", i == len);
> > +}
> > +
> > +/**
> > + * Test invalid commands, both invalid command detail codes and
> > valid
> > + * ones with invalid command class code.
> > + */
> > +static void test_inval(void)
> > +{
> > +	const uint16_t res = SCLP_RC_INVALID_SCLP_COMMAND;
> > +	uint32_t cmd;
> > +	int i;
> > +
> > +	report_prefix_push("Invalid command");
> > +	for (i = 0; i < 65536; i++) {
> > +		cmd = (0xdead0000) | i;  
> 
> Please remove the parentheses around 0xdead0000

ok, these ones were actually superfluous :)
probably a leftover from previous versions
 
> > +		if (!test_one_simple(cmd, pagebuf, PAGE_SIZE,
> > PAGE_SIZE, PGM_NONE, res))
> > +			break;
> > +	}
> > +	report("Command detail code", i == 65536);
> > +
> > +	for (i = 0; i < 256; i++) {
> > +		cmd = (valid_code & ~0xff) | i;
> > +		if (cmd == valid_code)
> > +			continue;
> > +		if (!test_one_simple(cmd, pagebuf, PAGE_SIZE,
> > PAGE_SIZE, PGM_NONE, res))
> > +			break;
> > +	}
> > +	report("Command class code", i == 256);
> > +	report_prefix_pop();
> > +}  
> 
>  Thomas
> 

Re: [kvm-unit-tests PATCH v3 2/2] s390x: SCLP unit test

[Thomas Huth]

On 13/11/2019 13.40, Claudio Imbrenda wrote:
> On Wed, 13 Nov 2019 10:34:02 +0100
> Thomas Huth <thuth@redhat.com> wrote:
[...]
>>> +/**
>>> + * Perform one test at the given address, optionally using the
>>> SCCB template,  
>>
>> I think you should at least mention the meaning of the "len" parameter
>> here, otherwise this is rather confusing (see below, my comment to
>> sccb_template).
> 
> I'll rename it and add comments
> 
>>> + * checking for the expected program interrupts and return codes.
>>> + * Returns 1 in case of success or 0 in case of failure  
>>
>> Could use bool with true + false instead.
>>
>>> + */
>>> +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;
>>> +
>>> +	/* Copy the template to the test address if needed */
>>> +	if (len)
>>> +		memcpy(addr, sccb_template, len);  
>>
>> Honestly, that sccb_template is rather confusing. Why does the caller
>> has to provide both, the data in the sccb_template and the "addr"
>> variable for yet another buffer? Wouldn't it be simpler if the caller
>> simply sets up everything in a place of choice and then only passes
>> the "addr" to the buffer?
> 
> because you will test the same buffer at different addresses. this
> mechanism abstracts this. instead of having to clear the buffer and set
> the values for each address, you can simply set the template once and
> then call the same function, changing only the target address.
> 
> also, the target address is not always a buffer, in many cases it is in
> fact an invalid address, which should generate exceptions. 

Hmm, ok, I guess some additional comments like this in the source code
would be helpful.

>>> +	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 = clear_pgm_int();
>>> +	/* Check if the program exception was one of the expected
>>> ones */
>>> +	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;
>>> +	}
>>> +	/* Check if the response code is the one expected */
>>> +	if (exp_rc && (exp_rc != h->response_code)) {  
>>
>> You can drop the parentheses around "exp_rc != h->response_code".
> 
> fine, although I don't understand you hatred toward parentheses :)

I took a LISP class at university once ... never quite recovered from
that...

No, honestly, the problem is rather that these additional parentheses
slow me down when I read the source code. If I see such if-statements,
my brain starts to think something like "There are parentheses here, so
there must be some additional non-trivial logic in this if-statement...
let's try to understand that..." and it takes a second to realize that
it's not the case and the parentheses are just superfluous.

>>> +/**
>>> + * Test SCCBs whose address is in the lowcore or prefix area.
>>> + */
>>> +static void test_sccb_prefix(void)
>>> +{
>>> +	uint32_t prefix, new_prefix;
>>> +	int offset;
>>> +
>>> +	/* can't actually trash the lowcore, unsurprisingly things
>>> break if we do */
>>> +	for (offset = 0; offset < 8192; offset += 8)
>>> +		if (!test_one_sccb(valid_code, MKPTR(offset), 0,
>>> PGM_BIT_SPEC, 0))
>>> +			break;
>>> +	report("SCCB low pages", offset == 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 (offset = 0; offset < 8192; offset += 8)
>>> +		if (!test_one_simple(valid_code, MKPTR(new_prefix
>>> + offset), 8, 8, PGM_BIT_SPEC, 0))
>>> +			break;
>>> +	report("SCCB prefix pages", offset == 8192);
>>> +
>>> +	memcpy(prefix_buf, 0, 8192);  
>>
>> What's that memcpy() good for? A comment would be helpful.
> 
> we just moved the prefix to a temporary one, and thrashed the old one.
> we can't simply set the old prefix and call it a day, things will break.

Did the test really trash the old one? ... hmm, I guess I got the code
wrong, that prefix addressing is always so confusing. Is SCLP working
with absolute or real addresses?

 Thomas

Re: [kvm-unit-tests PATCH v3 2/2] s390x: SCLP unit test

[Claudio Imbrenda]

On Wed, 13 Nov 2019 14:05:24 +0100
Thomas Huth <thuth@redhat.com> wrote:

[...]

> Hmm, ok, I guess some additional comments like this in the source code
> would be helpful.

ok, I'll add plenty

> >>> +	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 = clear_pgm_int();
> >>> +	/* Check if the program exception was one of the expected
> >>> ones */
> >>> +	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;
> >>> +	}
> >>> +	/* Check if the response code is the one expected */
> >>> +	if (exp_rc && (exp_rc != h->response_code)) {    
> >>
> >> You can drop the parentheses around "exp_rc != h->response_code".  
> > 
> > fine, although I don't understand you hatred toward parentheses :)  
> 
> I took a LISP class at university once ... never quite recovered from
> that...
> 
> No, honestly, the problem is rather that these additional parentheses
> slow me down when I read the source code. If I see such if-statements,
> my brain starts to think something like "There are parentheses here,
> so there must be some additional non-trivial logic in this
> if-statement... let's try to understand that..." and it takes a
> second to realize that it's not the case and the parentheses are just
> superfluous.

on the other hand it helps people who don't remember the C operator
precedence by heart :)

but yeah won't be in v4

> >>> +/**
> >>> + * Test SCCBs whose address is in the lowcore or prefix area.
> >>> + */
> >>> +static void test_sccb_prefix(void)
> >>> +{
> >>> +	uint32_t prefix, new_prefix;
> >>> +	int offset;
> >>> +
> >>> +	/* can't actually trash the lowcore, unsurprisingly
> >>> things break if we do */
> >>> +	for (offset = 0; offset < 8192; offset += 8)
> >>> +		if (!test_one_sccb(valid_code, MKPTR(offset), 0,
> >>> PGM_BIT_SPEC, 0))
> >>> +			break;
> >>> +	report("SCCB low pages", offset == 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 (offset = 0; offset < 8192; offset += 8)
> >>> +		if (!test_one_simple(valid_code, MKPTR(new_prefix
> >>> + offset), 8, 8, PGM_BIT_SPEC, 0))

here ^ (read below)

> >>> +			break;
> >>> +	report("SCCB prefix pages", offset == 8192);
> >>> +
> >>> +	memcpy(prefix_buf, 0, 8192);    
> >>
> >> What's that memcpy() good for? A comment would be helpful.  
> > 
> > we just moved the prefix to a temporary one, and thrashed the old
> > one. we can't simply set the old prefix and call it a day, things
> > will break.  
> 
> Did the test really trash the old one? ... hmm, I guess I got the code

yes, the default prefix is 0 and we are writing at absolute 0 (see
above where exactly). the SCLP call *should* not succeed anyway, but we
need to test it

I'm reworking this function because it possibly doesn't test all cases.
I added plenty of comments there too to explain what's going on

> wrong, that prefix addressing is always so confusing. Is SCLP working
> with absolute or real addresses?

yes.

both lowcore (real 0, absolute $PREFIX<<13) and absolute 0 (real
$PREFIX<<13, absolute 0) are forbidden for SCLP, so real or absolute
does not make a difference.