[kvm-unit-tests PATCH v8 0/6] s390x: SCLP Unit test

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

v7 -> v8
* fixed existing stfl asm wrapper
* now using stfl asm wrapper in intercept.c
* patched the program interrupt handler to clear the sclp_busy bit
* removed now unnecessary expect_pgm_int from the unit test
v6 -> v7
* renamed spx() and stpx() wrappers to set_prefix and get_prefix
* set_prefix now takes a value and get_prefix now returns a value
* put back some inline assembly for spx and stpx as a consequence
* used LC_SIZE instead of 2 * PAGE_SIZE everywhere in the unit test
v5 -> v6
* fixed a bug in test_addressing
* improved comments in test_sccb_prefix
* replaced all inline assembly usages of spx and stpx with the wrappers
* added one more wrapper for test_one_sccb for read-only tests
v4 -> v5
* updated usage of report()
* added SPX and STPX wrappers to the library
* improved readability
* addressed some more comments
v3 -> v4
* export sclp_setup_int instead of copying it
* add more comments
* rename some more variables to improve readability
* improve the prefix test
* improved the invalid address test
* addressed further comments received during review
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 (6):
  s390x: export sclp_setup_int
  s390x: sclp: add service call instruction wrapper
  s390x: lib: fix stfl wrapper asm
  s390x: lib: add SPX and STPX instruction wrapper
  s390x: lib: fix program interrupt handler if sclp_busy was set
  s390x: SCLP unit test

 s390x/Makefile           |   1 +
 lib/s390x/asm/arch_def.h |  26 +++
 lib/s390x/asm/facility.h |   2 +-
 lib/s390x/sclp.h         |   1 +
 lib/s390x/interrupt.c    |   5 +-
 lib/s390x/sclp.c         |   9 +-
 s390x/intercept.c        |  24 +-
 s390x/sclp.c             | 474 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   8 +
 9 files changed, 526 insertions(+), 24 deletions(-)
 create mode 100644 s390x/sclp.c

-- 
2.24.1

[kvm-unit-tests PATCH v8 1/6] s390x: export sclp_setup_int

[Claudio Imbrenda]

Export sclp_setup_int() so that it can be used in tests.

Needed for an upcoming unit test.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sclp.h | 1 +
 lib/s390x/sclp.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 6d40fb7..675f07e 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -265,6 +265,7 @@ typedef struct ReadEventData {
 } __attribute__((packed)) ReadEventData;
 
 extern char _sccb[];
+void sclp_setup_int(void);
 void sclp_handle_ext(void);
 void sclp_wait_busy(void);
 void sclp_mark_busy(void);
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 7798f04..123b639 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -45,7 +45,7 @@ static void mem_init(phys_addr_t mem_end)
 	page_alloc_ops_enable();
 }
 
-static void sclp_setup_int(void)
+void sclp_setup_int(void)
 {
 	uint64_t mask;
 
-- 
2.24.1

[kvm-unit-tests PATCH v8 2/6] 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 cf6e1ca..1a5e3c6 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -271,4 +271,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 123b639..4054d0e 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.24.1

[kvm-unit-tests PATCH v8 3/6] s390x: lib: fix stfl wrapper asm

[Claudio Imbrenda]

the stfl wrapper in lib/s390x/asm/facility.h was lacking the "memory"
clobber in the inline asm.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/asm/facility.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
index 5103dd4..e34dc2c 100644
--- a/lib/s390x/asm/facility.h
+++ b/lib/s390x/asm/facility.h
@@ -24,7 +24,7 @@ static inline bool test_facility(int nr)
 
 static inline void stfl(void)
 {
-	asm volatile("	stfl	0(0)\n");
+	asm volatile("	stfl	0(0)\n" : : : "memory");
 }
 
 static inline void stfle(uint8_t *fac, unsigned int len)
-- 
2.24.1

[kvm-unit-tests PATCH v8 4/6] s390x: lib: add SPX and STPX instruction wrapper

[Claudio Imbrenda]

Add a wrapper for the SET PREFIX and STORE PREFIX instructions, and
use it instead of using inline assembly.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 13 +++++++++++++
 s390x/intercept.c        | 24 +++++++++---------------
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 1a5e3c6..15a4d49 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -284,4 +284,17 @@ static inline int servc(uint32_t command, unsigned long sccb)
 	return cc;
 }
 
+static inline void set_prefix(uint32_t new_prefix)
+{
+	asm volatile("	spx %0" : : "Q" (new_prefix) : "memory");
+}
+
+static inline uint32_t get_prefix(void)
+{
+	uint32_t current_prefix;
+
+	asm volatile("	stpx %0" : "=Q" (current_prefix));
+	return current_prefix;
+}
+
 #endif
diff --git a/s390x/intercept.c b/s390x/intercept.c
index 3696e33..5f46b82 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -13,6 +13,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
+#include <asm/facility.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
@@ -26,13 +27,10 @@ static void test_stpx(void)
 	uint32_t new_prefix = (uint32_t)(intptr_t)pagebuf;
 
 	/* Can we successfully change the prefix? */
-	asm volatile (
-		" stpx	%0\n"
-		" spx	%2\n"
-		" stpx	%1\n"
-		" spx	%0\n"
-		: "+Q"(old_prefix), "+Q"(tst_prefix)
-		: "Q"(new_prefix));
+	old_prefix = get_prefix();
+	set_prefix(new_prefix);
+	tst_prefix = get_prefix();
+	set_prefix(old_prefix);
 	report(old_prefix == 0 && tst_prefix == new_prefix, "store prefix");
 
 	expect_pgm_int();
@@ -63,14 +61,10 @@ static void test_spx(void)
 	 * some facility bits there ... at least some of them should be
 	 * set in our buffer afterwards.
 	 */
-	asm volatile (
-		" stpx	%0\n"
-		" spx	%1\n"
-		" stfl	0\n"
-		" spx	%0\n"
-		: "+Q"(old_prefix)
-		: "Q"(new_prefix)
-		: "memory");
+	old_prefix = get_prefix();
+	set_prefix(new_prefix);
+	stfl();
+	set_prefix(old_prefix);
 	report(pagebuf[GEN_LC_STFL] != 0, "stfl to new prefix");
 
 	expect_pgm_int();
-- 
2.24.1

[kvm-unit-tests PATCH v8 5/6] s390x: lib: fix program interrupt handler if sclp_busy was set

[Claudio Imbrenda]

Fix the program interrupt handler for the case where sclp_busy is set.

The interrupt handler will attempt to write an error message on the
console using the SCLP, and will wait for sclp_busy to become false
before doing so. If an exception happenes between setting the flag and
the SCLP call, or if the call itself raises an exception, we need to
clear the flag so we can successfully print the error message.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/interrupt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 05f30be..ccb376a 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -106,10 +106,13 @@ static void fixup_pgm_int(void)
 
 void handle_pgm_int(void)
 {
-	if (!pgm_int_expected)
+	if (!pgm_int_expected) {
+		/* Force sclp_busy to false, otherwise we will loop forever */
+		sclp_handle_ext();
 		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
 			     lc->pgm_int_code, lc->pgm_old_psw.addr,
 			     lc->pgm_int_id);
+	}
 
 	pgm_int_expected = false;
 	fixup_pgm_int();
-- 
2.24.1

[kvm-unit-tests PATCH v8 6/6] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   8 +
 3 files changed, 483 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..215347e
--- /dev/null
+++ b/s390x/sclp.c
@@ -0,0 +1,474 @@
+/*
+ * 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))
+
+#define LC_SIZE (2 * PAGE_SIZE)
+
+static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
+static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
+static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
+static uint32_t valid_code;						/* valid command code for READ SCP INFO */
+static struct lowcore *lc;
+
+/**
+ * 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();
+	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.
+ *
+ * The parameter buf_len indicates the number of bytes of the template that
+ * should be copied to the test address, and should be 0 when the test
+ * address is invalid, in which case nothing is copied.
+ *
+ * The template is used to simplify tests where the same buffer content is
+ * used many times in a row, at different addresses.
+ *
+ * Returns true in case of success or false in case of failure
+ */
+static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
+		memcpy(addr, sccb_template, buf_len);
+	if (exp_pgm != PGM_NONE)
+		expect_pgm_int();
+	/* perform the actual call */
+	res = sclp_service_call_test(cmd, h);
+	if (res) {
+		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
+		return false;
+	}
+	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, buf_len %d, cmd %#x, pgm code %d",
+				addr, buf_len, cmd, pgm);
+		return false;
+	}
+	/* Check if the response code is the one expected */
+	if (exp_rc && exp_rc != h->response_code) {
+		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
+				addr, buf_len, cmd, h->response_code);
+		return false;
+	}
+	return true;
+}
+
+/**
+ * Wrapper for test_one_sccb to be used when the template should not be
+ * copied and the memory address should not be touched.
+ */
+static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
+{
+	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
+}
+
+/**
+ * Wrapper for test_one_sccb to set up a simple SCCB template.
+ *
+ * The parameter sccb_len indicates the value that will be saved in the SCCB
+ * length field of the SCCB, buf_len indicates the number of bytes of
+ * template that need to be copied to the actual test address. In many cases
+ * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
+ * itself can be larger.
+ *
+ * Returns true in case of success or false in case of failure
+ */
+static bool 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)
+{
+	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(len == 8, "SCCB too short");
+}
+
+/**
+ * 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(offset == 8, "SCCB unaligned");
+}
+
+/**
+ * Test SCCBs whose address is in the lowcore or prefix area.
+ */
+static void test_sccb_prefix(void)
+{
+	uint8_t scratch[LC_SIZE];
+	uint32_t prefix, new_prefix;
+	int offset;
+
+	/*
+	 * copy the current lowcore to the future new location, otherwise we
+	 * will not have a valid lowcore after setting the new prefix.
+	 */
+	memcpy(prefix_buf, 0, LC_SIZE);
+	/* save the current prefix (it's probably going to be 0) */
+	prefix = get_prefix();
+	/*
+	 * save the current content of absolute pages 0 and 1, so we can
+	 * restore them after we trash them later on
+	 */
+	memcpy(scratch, (void *)(intptr_t)prefix, LC_SIZE);
+	/* set the new prefix to prefix_buf */
+	new_prefix = (uint32_t)(intptr_t)prefix_buf;
+	set_prefix(new_prefix);
+
+	/*
+	 * testing with SCCB addresses in the lowcore; since we can't
+	 * actually trash the lowcore (unsurprisingly, things break if we
+	 * do), this will be a read-only test.
+	 */
+	for (offset = 0; offset < LC_SIZE; offset += 8)
+		if (!test_one_ro(valid_code, MKPTR(offset), PGM_BIT_SPEC, 0))
+			break;
+	report(offset == LC_SIZE, "SCCB low pages");
+
+	/*
+	 * the SCLP should not even touch the memory, but we will write the
+	 * SCCBs all over the two pages starting at absolute address 0, thus
+	 * trashing them; we will need to restore them later.
+	 */
+	for (offset = 0; offset < LC_SIZE; offset += 8)
+		if (!test_one_simple(valid_code, MKPTR(new_prefix + offset), 8, 8, PGM_BIT_SPEC, 0))
+			break;
+	report(offset == LC_SIZE, "SCCB prefix pages");
+
+	/* restore the previous contents of absolute pages 0 and 1 */
+	memcpy(prefix_buf, 0, LC_SIZE);
+	/* restore the prefix to the original value */
+	set_prefix(prefix);
+}
+
+/**
+ * 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];	/* for the list of addresses to test */
+
+	uint64_t maxram;
+	int i, pgm, len = 0;
+
+	h->length = 8;
+	/* addresses with 1 bit set in the first 33 bits */
+	for (i = 0; i < 33; i++)
+		a[len++] = 1UL << (i + 31);
+	/* addresses with 2 consecutive bits set in the first 33 bits */
+	for (i = 0; i < 33; i++)
+		a[len++] = 3UL << (i + 31);
+	/* addresses with all bits set in bits 0..N */
+	for (i = 0; i < 33; i++)
+		a[len++] = 0xffffffff80000000UL << i;
+	/* addresses with all bits set in bits N..33 */
+	a[len++] = 0x80000000;
+	for (i = 1; i < 33; i++, len++)
+		a[len] = a[len - 1] | (1UL << (i + 31));
+	/* all the addresses above, but adding the offset of a valid buffer */
+	for (i = 0; i < len; i++)
+		a[len + i] = a[i] + (intptr_t)h;
+	len += i;
+	/* two more hand-crafted addresses */
+	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_ro(valid_code, (void *)a[i], pgm, 0))
+			break;
+	}
+	report(i == len, "SCCB high addresses");
+}
+
+/**
+ * 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(i == 65536, "Command detail code");
+
+	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(i == 256, "Command class code");
+	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 len;
+
+	for (len = 8; len < 144; len++)
+		if (!test_one_simple(valid_code, pagebuf, len, len, PGM_NONE, res))
+			break;
+	report(len == 144, "Insufficient SCCB length (Read SCP info)");
+
+	for (len = 8; len < 40; len++)
+		if (!test_one_simple(SCLP_READ_CPU_INFO, pagebuf, len, len, PGM_NONE, res))
+			break;
+	report(len == 40, "Insufficient SCCB length (Read CPU info)");
+}
+
+/**
+ * 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, offset;
+
+	memset(sccb_template, 0, sizeof(sccb_template));
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (len = 32; len <= 4096; len++) {
+		offset = len & 7 ? len & ~7 : len - 8;
+		for (offset = 4096 - offset; offset < 4096; offset += 8) {
+			sccb->h.length = len;
+			if (!test_one_sccb(cmd, offset + pagebuf, len, PGM_NONE, res))
+				goto out;
+		}
+	}
+out:
+	report(len > 4096 && offset == 4096, "SCCB page boundary violation");
+}
+
+/**
+ * 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 len;
+
+	memset(sccb_template, 0, sizeof(sccb_template));
+	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+	for (len = 4097; len < 8192; len++) {
+		sccb->h.length = len;
+		if (!test_one_sccb(cmd, pagebuf, PAGE_SIZE, PGM_NONE, res))
+			break;
+	}
+	report(len == 8192, "SCCB bigger than 4k");
+}
+
+/**
+ * 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();
+
+	/* the VM has more than 2GB of memory */
+	if (maxram >= 0x80000000) {
+		report_skip("Invalid SCCB address");
+		return;
+	}
+	/* test all possible valid addresses immediately after the end of memory
+	 * up to 64KB after the end of memory
+	 */
+	for (i = 0; i < 0x10000 && i + maxram < 0x80000000; i += 8)
+		if (!test_one_ro(valid_code, MKPTR(i + maxram), PGM_BIT_ADDR, 0))
+			goto out;
+	/* test more addresses until we reach 1MB after end of memory;
+	 * increment by a prime number (times 8) in order to test all
+	 * possible valid offsets inside pages
+	 */
+	for (; i < 0x100000 && i + maxram < 0x80000000 ; i += 808)
+		if (!test_one_ro(valid_code, MKPTR(i + maxram), PGM_BIT_ADDR, 0))
+			goto out;
+	/* test the remaining addresses until we reach address 2GB;
+	 * increment by a prime number (times 8) in order to test all
+	 * possible valid offsets inside pages
+	 */
+	for (; i + maxram < 0x80000000; i += 800024)
+		if (!test_one_ro(valid_code, MKPTR(i + maxram), PGM_BIT_ADDR, 0))
+			goto out;
+out:
+	report(i + maxram >= 0x80000000, "Invalid SCCB address");
+}
+
+/**
+ * Test some bits in the instruction format that are specified to be ignored.
+ */
+static void test_instbits(void)
+{
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	int cc;
+
+	sclp_mark_busy();
+	h->length = 8;
+	sclp_setup_int();
+
+	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");
+	/* No exception, but also no command accepted, so no interrupt is
+	 * expected. We need to clear the flag manually otherwise we will
+	 * loop forever when we try to report failure.
+	 */
+	if (cc)
+		sclp_handle_ext();
+	else
+		sclp_wait_busy();
+	report(cc == 0, "Instruction format ignored bits");
+}
+
+/**
+ * Find a valid READ INFO 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;
+	}
+	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..07013b2 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -75,3 +75,11 @@ file = stsi.elf
 [smp]
 file = smp.elf
 extra_params =-smp 2
+
+[sclp-1g]
+file = sclp.elf
+extra_params = -m 1G
+
+[sclp-3g]
+file = sclp.elf
+extra_params = -m 3G
-- 
2.24.1

Re: [kvm-unit-tests PATCH v8 3/6] s390x: lib: fix stfl wrapper asm

[Thomas Huth]

On 20/01/2020 19.42, Claudio Imbrenda wrote:
> the stfl wrapper in lib/s390x/asm/facility.h was lacking the "memory"
> clobber in the inline asm.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/asm/facility.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
> index 5103dd4..e34dc2c 100644
> --- a/lib/s390x/asm/facility.h
> +++ b/lib/s390x/asm/facility.h
> @@ -24,7 +24,7 @@ static inline bool test_facility(int nr)
>  
>  static inline void stfl(void)
>  {
> -	asm volatile("	stfl	0(0)\n");
> +	asm volatile("	stfl	0(0)\n" : : : "memory");
>  }
>  
>  static inline void stfle(uint8_t *fac, unsigned int len)
> 

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

Re: [kvm-unit-tests PATCH v8 5/6] s390x: lib: fix program interrupt handler if sclp_busy was set

[Thomas Huth]

On 20/01/2020 19.42, Claudio Imbrenda wrote:
> Fix the program interrupt handler for the case where sclp_busy is set.
> 
> The interrupt handler will attempt to write an error message on the
> console using the SCLP, and will wait for sclp_busy to become false
> before doing so. If an exception happenes between setting the flag and
> the SCLP call, or if the call itself raises an exception, we need to
> clear the flag so we can successfully print the error message.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 05f30be..ccb376a 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -106,10 +106,13 @@ static void fixup_pgm_int(void)
>  
>  void handle_pgm_int(void)
>  {
> -	if (!pgm_int_expected)
> +	if (!pgm_int_expected) {
> +		/* Force sclp_busy to false, otherwise we will loop forever */
> +		sclp_handle_ext();
>  		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
>  			     lc->pgm_int_code, lc->pgm_old_psw.addr,
>  			     lc->pgm_int_id);
> +	}
>  
>  	pgm_int_expected = false;
>  	fixup_pgm_int();
> 

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

Re: [kvm-unit-tests PATCH v8 3/6] s390x: lib: fix stfl wrapper asm

[David Hildenbrand]

On 20.01.20 19:42, Claudio Imbrenda wrote:
> the stfl wrapper in lib/s390x/asm/facility.h was lacking the "memory"
> clobber in the inline asm.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/asm/facility.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
> index 5103dd4..e34dc2c 100644
> --- a/lib/s390x/asm/facility.h
> +++ b/lib/s390x/asm/facility.h
> @@ -24,7 +24,7 @@ static inline bool test_facility(int nr)
>  
>  static inline void stfl(void)
>  {
> -	asm volatile("	stfl	0(0)\n");
> +	asm volatile("	stfl	0(0)\n" : : : "memory");
>  }
>  
>  static inline void stfle(uint8_t *fac, unsigned int len)
> 

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

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 5/6] s390x: lib: fix program interrupt handler if sclp_busy was set

[David Hildenbrand]

On 20.01.20 19:42, Claudio Imbrenda wrote:
> Fix the program interrupt handler for the case where sclp_busy is set.
> 
> The interrupt handler will attempt to write an error message on the
> console using the SCLP, and will wait for sclp_busy to become false
> before doing so. If an exception happenes between setting the flag and
> the SCLP call, or if the call itself raises an exception, we need to
> clear the flag so we can successfully print the error message.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 05f30be..ccb376a 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -106,10 +106,13 @@ static void fixup_pgm_int(void)
>  
>  void handle_pgm_int(void)
>  {
> -	if (!pgm_int_expected)
> +	if (!pgm_int_expected) {
> +		/* Force sclp_busy to false, otherwise we will loop forever */
> +		sclp_handle_ext();
>  		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
>  			     lc->pgm_int_code, lc->pgm_old_psw.addr,
>  			     lc->pgm_int_id);
> +	}
>  
>  	pgm_int_expected = false;
>  	fixup_pgm_int();
> 

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

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 3/6] s390x: lib: fix stfl wrapper asm

[Janosch Frank]

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

On 1/20/20 7:42 PM, Claudio Imbrenda wrote:
> the stfl wrapper in lib/s390x/asm/facility.h was lacking the "memory"
> clobber in the inline asm.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/asm/facility.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
> index 5103dd4..e34dc2c 100644
> --- a/lib/s390x/asm/facility.h
> +++ b/lib/s390x/asm/facility.h
> @@ -24,7 +24,7 @@ static inline bool test_facility(int nr)
>  
>  static inline void stfl(void)
>  {
> -	asm volatile("	stfl	0(0)\n");
> +	asm volatile("	stfl	0(0)\n" : : : "memory");
>  }
>  
>  static inline void stfle(uint8_t *fac, unsigned int len)
> 

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


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

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[David Hildenbrand]

[...]

>  all: directories test_cases test_cases_binary
> diff --git a/s390x/sclp.c b/s390x/sclp.c
> new file mode 100644
> index 0000000..215347e
> --- /dev/null
> +++ b/s390x/sclp.c
> @@ -0,0 +1,474 @@
> +/*
> + * Service Call tests
> + *
> + * Copyright (c) 2019 IBM Corp

Should be 2020 now. Will fix that up.

[...]

> +/**
> + * Perform one test at the given address, optionally using the SCCB template,
> + * checking for the expected program interrupts and return codes.
> + *
> + * The parameter buf_len indicates the number of bytes of the template that
> + * should be copied to the test address, and should be 0 when the test
> + * address is invalid, in which case nothing is copied.
> + *
> + * The template is used to simplify tests where the same buffer content is
> + * used many times in a row, at different addresses.
> + *
> + * Returns true in case of success or false in case of failure
> + */
> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
> +		memcpy(addr, sccb_template, buf_len);
> +	if (exp_pgm != PGM_NONE)
> +		expect_pgm_int();

I still dislike the handling here. Any caller can simply do a

expect_pgm_int();
test_one_sccb(...)
check_pgm_int_code(PGM_);

Same thing goes for exp_rc. We really should drop exp_rc and exp_pgm
handling from the function completely and handle it in the caller. That
makes the tests actually readable.

But I feel like I am repeating myself. I'll queue this patch with the
2020 fixed up and might send a fixup myself (if I find some spare minutes).

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 0/6] s390x: SCLP Unit test

[David Hildenbrand]

On 20.01.20 19:42, Claudio Imbrenda wrote:
> This patchset contains some minor cleanup, some preparatory work and
> then the SCLP unit test itself.
> 
> The unit test checks the following:
>     
>     * Correctly ignoring instruction bits that should be ignored
>     * Privileged instruction check
>     * Check for addressing exceptions
>     * Specification exceptions:
>       - SCCB size less than 8
>       - SCCB unaligned
>       - SCCB overlaps prefix or lowcore
>       - SCCB address higher than 2GB
>     * Return codes for
>       - Invalid command
>       - SCCB too short (but at least 8)
>       - SCCB page boundary violation
> 
> v7 -> v8
> * fixed existing stfl asm wrapper
> * now using stfl asm wrapper in intercept.c
> * patched the program interrupt handler to clear the sclp_busy bit
> * removed now unnecessary expect_pgm_int from the unit test
> v6 -> v7
> * renamed spx() and stpx() wrappers to set_prefix and get_prefix
> * set_prefix now takes a value and get_prefix now returns a value
> * put back some inline assembly for spx and stpx as a consequence
> * used LC_SIZE instead of 2 * PAGE_SIZE everywhere in the unit test
> v5 -> v6
> * fixed a bug in test_addressing
> * improved comments in test_sccb_prefix
> * replaced all inline assembly usages of spx and stpx with the wrappers
> * added one more wrapper for test_one_sccb for read-only tests
> v4 -> v5
> * updated usage of report()
> * added SPX and STPX wrappers to the library
> * improved readability
> * addressed some more comments
> v3 -> v4
> * export sclp_setup_int instead of copying it
> * add more comments
> * rename some more variables to improve readability
> * improve the prefix test
> * improved the invalid address test
> * addressed further comments received during review
> 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 (6):
>   s390x: export sclp_setup_int
>   s390x: sclp: add service call instruction wrapper
>   s390x: lib: fix stfl wrapper asm
>   s390x: lib: add SPX and STPX instruction wrapper
>   s390x: lib: fix program interrupt handler if sclp_busy was set
>   s390x: SCLP unit test
> 
>  s390x/Makefile           |   1 +
>  lib/s390x/asm/arch_def.h |  26 +++
>  lib/s390x/asm/facility.h |   2 +-
>  lib/s390x/sclp.h         |   1 +
>  lib/s390x/interrupt.c    |   5 +-
>  lib/s390x/sclp.c         |   9 +-
>  s390x/intercept.c        |  24 +-
>  s390x/sclp.c             | 474 +++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg      |   8 +
>  9 files changed, 526 insertions(+), 24 deletions(-)
>  create mode 100644 s390x/sclp.c
> 

Queued to

https://github.com/davidhildenbrand/qemu.git s390-tcg-next


Thanks!
-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 0/6] s390x: SCLP Unit test

[David Hildenbrand]

On 22.01.20 10:55, David Hildenbrand wrote:
> On 20.01.20 19:42, Claudio Imbrenda wrote:
>> This patchset contains some minor cleanup, some preparatory work and
>> then the SCLP unit test itself.
>>
>> The unit test checks the following:
>>     
>>     * Correctly ignoring instruction bits that should be ignored
>>     * Privileged instruction check
>>     * Check for addressing exceptions
>>     * Specification exceptions:
>>       - SCCB size less than 8
>>       - SCCB unaligned
>>       - SCCB overlaps prefix or lowcore
>>       - SCCB address higher than 2GB
>>     * Return codes for
>>       - Invalid command
>>       - SCCB too short (but at least 8)
>>       - SCCB page boundary violation
>>
>> v7 -> v8
>> * fixed existing stfl asm wrapper
>> * now using stfl asm wrapper in intercept.c
>> * patched the program interrupt handler to clear the sclp_busy bit
>> * removed now unnecessary expect_pgm_int from the unit test
>> v6 -> v7
>> * renamed spx() and stpx() wrappers to set_prefix and get_prefix
>> * set_prefix now takes a value and get_prefix now returns a value
>> * put back some inline assembly for spx and stpx as a consequence
>> * used LC_SIZE instead of 2 * PAGE_SIZE everywhere in the unit test
>> v5 -> v6
>> * fixed a bug in test_addressing
>> * improved comments in test_sccb_prefix
>> * replaced all inline assembly usages of spx and stpx with the wrappers
>> * added one more wrapper for test_one_sccb for read-only tests
>> v4 -> v5
>> * updated usage of report()
>> * added SPX and STPX wrappers to the library
>> * improved readability
>> * addressed some more comments
>> v3 -> v4
>> * export sclp_setup_int instead of copying it
>> * add more comments
>> * rename some more variables to improve readability
>> * improve the prefix test
>> * improved the invalid address test
>> * addressed further comments received during review
>> 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 (6):
>>   s390x: export sclp_setup_int
>>   s390x: sclp: add service call instruction wrapper
>>   s390x: lib: fix stfl wrapper asm
>>   s390x: lib: add SPX and STPX instruction wrapper
>>   s390x: lib: fix program interrupt handler if sclp_busy was set
>>   s390x: SCLP unit test
>>
>>  s390x/Makefile           |   1 +
>>  lib/s390x/asm/arch_def.h |  26 +++
>>  lib/s390x/asm/facility.h |   2 +-
>>  lib/s390x/sclp.h         |   1 +
>>  lib/s390x/interrupt.c    |   5 +-
>>  lib/s390x/sclp.c         |   9 +-
>>  s390x/intercept.c        |  24 +-
>>  s390x/sclp.c             | 474 +++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg      |   8 +
>>  9 files changed, 526 insertions(+), 24 deletions(-)
>>  create mode 100644 s390x/sclp.c
>>
> 
> Queued to
> 
> https://github.com/davidhildenbrand/qemu.git s390-tcg-next

Lol, wrong shortcut (thanks Thomas :) )

Queued to

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

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[David Hildenbrand]

On 20.01.20 19:42, 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   8 +
>  3 files changed, 483 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..215347e
> --- /dev/null
> +++ b/s390x/sclp.c
> @@ -0,0 +1,474 @@
> +/*
> + * 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))
> +
> +#define LC_SIZE (2 * PAGE_SIZE)
> +
> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
> +static struct lowcore *lc;
> +
> +/**
> + * 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();
> +	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.
> + *
> + * The parameter buf_len indicates the number of bytes of the template that
> + * should be copied to the test address, and should be 0 when the test
> + * address is invalid, in which case nothing is copied.
> + *
> + * The template is used to simplify tests where the same buffer content is
> + * used many times in a row, at different addresses.
> + *
> + * Returns true in case of success or false in case of failure
> + */
> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
> +		memcpy(addr, sccb_template, buf_len);
> +	if (exp_pgm != PGM_NONE)
> +		expect_pgm_int();
> +	/* perform the actual call */
> +	res = sclp_service_call_test(cmd, h);
> +	if (res) {
> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
> +		return false;
> +	}
> +	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, buf_len %d, cmd %#x, pgm code %d",
> +				addr, buf_len, cmd, pgm);
> +		return false;
> +	}
> +	/* Check if the response code is the one expected */
> +	if (exp_rc && exp_rc != h->response_code) {
> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
> +				addr, buf_len, cmd, h->response_code);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +/**
> + * Wrapper for test_one_sccb to be used when the template should not be
> + * copied and the memory address should not be touched.
> + */
> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
> +}
> +
> +/**
> + * Wrapper for test_one_sccb to set up a simple SCCB template.
> + *
> + * The parameter sccb_len indicates the value that will be saved in the SCCB
> + * length field of the SCCB, buf_len indicates the number of bytes of
> + * template that need to be copied to the actual test address. In many cases
> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
> + * itself can be larger.
> + *
> + * Returns true in case of success or false in case of failure
> + */
> +static bool 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)
> +{
> +	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);

Doing a fresh ./configure + make on RHEL7 gives me

[linux1@rhkvm01 kvm-unit-tests]$ make
gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
s390x/sclp.c: In function 'test_one_simple':
s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
  ((SCCBHeader *)sccb_template)->length = sccb_len;
  ^
s390x/sclp.c: At top level:
cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
cc1: all warnings being treated as errors
make: *** [s390x/sclp.o] Error 1



-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[David Hildenbrand]

On 22.01.20 11:10, David Hildenbrand wrote:
> On 20.01.20 19:42, 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>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/Makefile      |   1 +
>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |   8 +
>>  3 files changed, 483 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..215347e
>> --- /dev/null
>> +++ b/s390x/sclp.c
>> @@ -0,0 +1,474 @@
>> +/*
>> + * 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))
>> +
>> +#define LC_SIZE (2 * PAGE_SIZE)
>> +
>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>> +static struct lowcore *lc;
>> +
>> +/**
>> + * 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();
>> +	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.
>> + *
>> + * The parameter buf_len indicates the number of bytes of the template that
>> + * should be copied to the test address, and should be 0 when the test
>> + * address is invalid, in which case nothing is copied.
>> + *
>> + * The template is used to simplify tests where the same buffer content is
>> + * used many times in a row, at different addresses.
>> + *
>> + * Returns true in case of success or false in case of failure
>> + */
>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>> +		memcpy(addr, sccb_template, buf_len);
>> +	if (exp_pgm != PGM_NONE)
>> +		expect_pgm_int();
>> +	/* perform the actual call */
>> +	res = sclp_service_call_test(cmd, h);
>> +	if (res) {
>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>> +		return false;
>> +	}
>> +	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, buf_len %d, cmd %#x, pgm code %d",
>> +				addr, buf_len, cmd, pgm);
>> +		return false;
>> +	}
>> +	/* Check if the response code is the one expected */
>> +	if (exp_rc && exp_rc != h->response_code) {
>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>> +				addr, buf_len, cmd, h->response_code);
>> +		return false;
>> +	}
>> +	return true;
>> +}
>> +
>> +/**
>> + * Wrapper for test_one_sccb to be used when the template should not be
>> + * copied and the memory address should not be touched.
>> + */
>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>> +{
>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>> +}
>> +
>> +/**
>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>> + *
>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>> + * length field of the SCCB, buf_len indicates the number of bytes of
>> + * template that need to be copied to the actual test address. In many cases
>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>> + * itself can be larger.
>> + *
>> + * Returns true in case of success or false in case of failure
>> + */
>> +static bool 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)
>> +{
>> +	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);
> 
> Doing a fresh ./configure + make on RHEL7 gives me
> 
> [linux1@rhkvm01 kvm-unit-tests]$ make
> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
> s390x/sclp.c: In function 'test_one_simple':
> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>   ^
> s390x/sclp.c: At top level:
> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
> cc1: all warnings being treated as errors
> make: *** [s390x/sclp.o] Error 1

The following makes it work:


diff --git a/s390x/sclp.c b/s390x/sclp.c
index c13fa60..0b8117a 100644
--- a/s390x/sclp.c
+++ b/s390x/sclp.c
@@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
 static bool 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)
 {
+       SCCBHeader *header = (void *)sccb_template;
+
        memset(sccb_template, 0, sizeof(sccb_template));
-       ((SCCBHeader *)sccb_template)->length = sccb_len;
+       header->length = sccb_len;
        return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
 }
 

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[Thomas Huth]

On 22/01/2020 11.22, David Hildenbrand wrote:
> On 22.01.20 11:10, David Hildenbrand wrote:
>> On 20.01.20 19:42, 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>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  s390x/Makefile      |   1 +
>>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>>  s390x/unittests.cfg |   8 +
>>>  3 files changed, 483 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..215347e
>>> --- /dev/null
>>> +++ b/s390x/sclp.c
>>> @@ -0,0 +1,474 @@
>>> +/*
>>> + * 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))
>>> +
>>> +#define LC_SIZE (2 * PAGE_SIZE)
>>> +
>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>>> +static struct lowcore *lc;
>>> +
>>> +/**
>>> + * 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();
>>> +	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.
>>> + *
>>> + * The parameter buf_len indicates the number of bytes of the template that
>>> + * should be copied to the test address, and should be 0 when the test
>>> + * address is invalid, in which case nothing is copied.
>>> + *
>>> + * The template is used to simplify tests where the same buffer content is
>>> + * used many times in a row, at different addresses.
>>> + *
>>> + * Returns true in case of success or false in case of failure
>>> + */
>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>>> +		memcpy(addr, sccb_template, buf_len);
>>> +	if (exp_pgm != PGM_NONE)
>>> +		expect_pgm_int();
>>> +	/* perform the actual call */
>>> +	res = sclp_service_call_test(cmd, h);
>>> +	if (res) {
>>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>>> +		return false;
>>> +	}
>>> +	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, buf_len %d, cmd %#x, pgm code %d",
>>> +				addr, buf_len, cmd, pgm);
>>> +		return false;
>>> +	}
>>> +	/* Check if the response code is the one expected */
>>> +	if (exp_rc && exp_rc != h->response_code) {
>>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>>> +				addr, buf_len, cmd, h->response_code);
>>> +		return false;
>>> +	}
>>> +	return true;
>>> +}
>>> +
>>> +/**
>>> + * Wrapper for test_one_sccb to be used when the template should not be
>>> + * copied and the memory address should not be touched.
>>> + */
>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>>> +{
>>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>>> +}
>>> +
>>> +/**
>>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>>> + *
>>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>>> + * length field of the SCCB, buf_len indicates the number of bytes of
>>> + * template that need to be copied to the actual test address. In many cases
>>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>>> + * itself can be larger.
>>> + *
>>> + * Returns true in case of success or false in case of failure
>>> + */
>>> +static bool 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)
>>> +{
>>> +	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);
>>
>> Doing a fresh ./configure + make on RHEL7 gives me
>>
>> [linux1@rhkvm01 kvm-unit-tests]$ make
>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>> s390x/sclp.c: In function 'test_one_simple':
>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>   ^
>> s390x/sclp.c: At top level:
>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>> cc1: all warnings being treated as errors
>> make: *** [s390x/sclp.o] Error 1
> 
> The following makes it work:
> 
> 
> diff --git a/s390x/sclp.c b/s390x/sclp.c
> index c13fa60..0b8117a 100644
> --- a/s390x/sclp.c
> +++ b/s390x/sclp.c
> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>  static bool 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)
>  {
> +       SCCBHeader *header = (void *)sccb_template;
> +
>         memset(sccb_template, 0, sizeof(sccb_template));
> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
> +       header->length = sccb_len;

While that might silence the compiler warning, we still might get
aliasing problems here, I think.
The right way to solve this problem is to turn sccb_template into a
union of the various structs/arrays that you want to use and then access
the fields through the union instead ("type-punning through union").

 Thomas

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[David Hildenbrand]

On 22.01.20 11:31, Thomas Huth wrote:
> On 22/01/2020 11.22, David Hildenbrand wrote:
>> On 22.01.20 11:10, David Hildenbrand wrote:
>>> On 20.01.20 19:42, 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>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  s390x/Makefile      |   1 +
>>>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  s390x/unittests.cfg |   8 +
>>>>  3 files changed, 483 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..215347e
>>>> --- /dev/null
>>>> +++ b/s390x/sclp.c
>>>> @@ -0,0 +1,474 @@
>>>> +/*
>>>> + * 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))
>>>> +
>>>> +#define LC_SIZE (2 * PAGE_SIZE)
>>>> +
>>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>>>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>>>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>>>> +static struct lowcore *lc;
>>>> +
>>>> +/**
>>>> + * 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();
>>>> +	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.
>>>> + *
>>>> + * The parameter buf_len indicates the number of bytes of the template that
>>>> + * should be copied to the test address, and should be 0 when the test
>>>> + * address is invalid, in which case nothing is copied.
>>>> + *
>>>> + * The template is used to simplify tests where the same buffer content is
>>>> + * used many times in a row, at different addresses.
>>>> + *
>>>> + * Returns true in case of success or false in case of failure
>>>> + */
>>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>>>> +		memcpy(addr, sccb_template, buf_len);
>>>> +	if (exp_pgm != PGM_NONE)
>>>> +		expect_pgm_int();
>>>> +	/* perform the actual call */
>>>> +	res = sclp_service_call_test(cmd, h);
>>>> +	if (res) {
>>>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>>>> +		return false;
>>>> +	}
>>>> +	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, buf_len %d, cmd %#x, pgm code %d",
>>>> +				addr, buf_len, cmd, pgm);
>>>> +		return false;
>>>> +	}
>>>> +	/* Check if the response code is the one expected */
>>>> +	if (exp_rc && exp_rc != h->response_code) {
>>>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>>>> +				addr, buf_len, cmd, h->response_code);
>>>> +		return false;
>>>> +	}
>>>> +	return true;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Wrapper for test_one_sccb to be used when the template should not be
>>>> + * copied and the memory address should not be touched.
>>>> + */
>>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>>>> +{
>>>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>>>> +}
>>>> +
>>>> +/**
>>>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>>>> + *
>>>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>>>> + * length field of the SCCB, buf_len indicates the number of bytes of
>>>> + * template that need to be copied to the actual test address. In many cases
>>>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>>>> + * itself can be larger.
>>>> + *
>>>> + * Returns true in case of success or false in case of failure
>>>> + */
>>>> +static bool 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)
>>>> +{
>>>> +	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);
>>>
>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>
>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>> s390x/sclp.c: In function 'test_one_simple':
>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>   ^
>>> s390x/sclp.c: At top level:
>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>> cc1: all warnings being treated as errors
>>> make: *** [s390x/sclp.o] Error 1
>>
>> The following makes it work:
>>
>>
>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>> index c13fa60..0b8117a 100644
>> --- a/s390x/sclp.c
>> +++ b/s390x/sclp.c
>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>  static bool 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)
>>  {
>> +       SCCBHeader *header = (void *)sccb_template;
>> +
>>         memset(sccb_template, 0, sizeof(sccb_template));
>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>> +       header->length = sccb_len;
> 
> While that might silence the compiler warning, we still might get
> aliasing problems here, I think.
> The right way to solve this problem is to turn sccb_template into a
> union of the various structs/arrays that you want to use and then access
> the fields through the union instead ("type-punning through union").

We do have the exact same thing in lib/s390x/sclp.c already, no?

Especially, new compilers don't seem to care?

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[Thomas Huth]

On 22/01/2020 11.32, David Hildenbrand wrote:
> On 22.01.20 11:31, Thomas Huth wrote:
>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>> On 22.01.20 11:10, David Hildenbrand wrote:
>>>> On 20.01.20 19:42, 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>
>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>  s390x/Makefile      |   1 +
>>>>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  s390x/unittests.cfg |   8 +
>>>>>  3 files changed, 483 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..215347e
>>>>> --- /dev/null
>>>>> +++ b/s390x/sclp.c
>>>>> @@ -0,0 +1,474 @@
>>>>> +/*
>>>>> + * 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))
>>>>> +
>>>>> +#define LC_SIZE (2 * PAGE_SIZE)
>>>>> +
>>>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>>>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>>>>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>>>>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>>>>> +static struct lowcore *lc;
>>>>> +
>>>>> +/**
>>>>> + * 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();
>>>>> +	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.
>>>>> + *
>>>>> + * The parameter buf_len indicates the number of bytes of the template that
>>>>> + * should be copied to the test address, and should be 0 when the test
>>>>> + * address is invalid, in which case nothing is copied.
>>>>> + *
>>>>> + * The template is used to simplify tests where the same buffer content is
>>>>> + * used many times in a row, at different addresses.
>>>>> + *
>>>>> + * Returns true in case of success or false in case of failure
>>>>> + */
>>>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>>>>> +		memcpy(addr, sccb_template, buf_len);
>>>>> +	if (exp_pgm != PGM_NONE)
>>>>> +		expect_pgm_int();
>>>>> +	/* perform the actual call */
>>>>> +	res = sclp_service_call_test(cmd, h);
>>>>> +	if (res) {
>>>>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>>>>> +		return false;
>>>>> +	}
>>>>> +	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, buf_len %d, cmd %#x, pgm code %d",
>>>>> +				addr, buf_len, cmd, pgm);
>>>>> +		return false;
>>>>> +	}
>>>>> +	/* Check if the response code is the one expected */
>>>>> +	if (exp_rc && exp_rc != h->response_code) {
>>>>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>>>>> +				addr, buf_len, cmd, h->response_code);
>>>>> +		return false;
>>>>> +	}
>>>>> +	return true;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Wrapper for test_one_sccb to be used when the template should not be
>>>>> + * copied and the memory address should not be touched.
>>>>> + */
>>>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>>>>> +{
>>>>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>>>>> + *
>>>>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>>>>> + * length field of the SCCB, buf_len indicates the number of bytes of
>>>>> + * template that need to be copied to the actual test address. In many cases
>>>>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>>>>> + * itself can be larger.
>>>>> + *
>>>>> + * Returns true in case of success or false in case of failure
>>>>> + */
>>>>> +static bool 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)
>>>>> +{
>>>>> +	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);
>>>>
>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>
>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>> s390x/sclp.c: In function 'test_one_simple':
>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>   ^
>>>> s390x/sclp.c: At top level:
>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>> cc1: all warnings being treated as errors
>>>> make: *** [s390x/sclp.o] Error 1
>>>
>>> The following makes it work:
>>>
>>>
>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>> index c13fa60..0b8117a 100644
>>> --- a/s390x/sclp.c
>>> +++ b/s390x/sclp.c
>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>  static bool 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)
>>>  {
>>> +       SCCBHeader *header = (void *)sccb_template;
>>> +
>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>> +       header->length = sccb_len;
>>
>> While that might silence the compiler warning, we still might get
>> aliasing problems here, I think.
>> The right way to solve this problem is to turn sccb_template into a
>> union of the various structs/arrays that you want to use and then access
>> the fields through the union instead ("type-punning through union").
> 
> We do have the exact same thing in lib/s390x/sclp.c already, no?

Maybe we should carefully check that code, too...

> Especially, new compilers don't seem to care?

I've seen horrible bugs due to these aliasing problems in the past -
without compiler warnings showing up! Certain versions of GCC assume
that they can re-order code with pointers that point to types of
different sizes, i.e. in the above example, I think they could assume
that they could re-order the memset() and the header->length = ... line.
I'd feel better if we play safe and use a union here.

 Thomas

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[David Hildenbrand]

On 22.01.20 11:39, Thomas Huth wrote:
> On 22/01/2020 11.32, David Hildenbrand wrote:
>> On 22.01.20 11:31, Thomas Huth wrote:
>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>> On 22.01.20 11:10, David Hildenbrand wrote:
>>>>> On 20.01.20 19:42, 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>
>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> ---
>>>>>>  s390x/Makefile      |   1 +
>>>>>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  s390x/unittests.cfg |   8 +
>>>>>>  3 files changed, 483 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..215347e
>>>>>> --- /dev/null
>>>>>> +++ b/s390x/sclp.c
>>>>>> @@ -0,0 +1,474 @@
>>>>>> +/*
>>>>>> + * 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))
>>>>>> +
>>>>>> +#define LC_SIZE (2 * PAGE_SIZE)
>>>>>> +
>>>>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>>>>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>>>>>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>>>>>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>>>>>> +static struct lowcore *lc;
>>>>>> +
>>>>>> +/**
>>>>>> + * 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();
>>>>>> +	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.
>>>>>> + *
>>>>>> + * The parameter buf_len indicates the number of bytes of the template that
>>>>>> + * should be copied to the test address, and should be 0 when the test
>>>>>> + * address is invalid, in which case nothing is copied.
>>>>>> + *
>>>>>> + * The template is used to simplify tests where the same buffer content is
>>>>>> + * used many times in a row, at different addresses.
>>>>>> + *
>>>>>> + * Returns true in case of success or false in case of failure
>>>>>> + */
>>>>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>>>>>> +		memcpy(addr, sccb_template, buf_len);
>>>>>> +	if (exp_pgm != PGM_NONE)
>>>>>> +		expect_pgm_int();
>>>>>> +	/* perform the actual call */
>>>>>> +	res = sclp_service_call_test(cmd, h);
>>>>>> +	if (res) {
>>>>>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>>>>>> +		return false;
>>>>>> +	}
>>>>>> +	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, buf_len %d, cmd %#x, pgm code %d",
>>>>>> +				addr, buf_len, cmd, pgm);
>>>>>> +		return false;
>>>>>> +	}
>>>>>> +	/* Check if the response code is the one expected */
>>>>>> +	if (exp_rc && exp_rc != h->response_code) {
>>>>>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>>>>>> +				addr, buf_len, cmd, h->response_code);
>>>>>> +		return false;
>>>>>> +	}
>>>>>> +	return true;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * Wrapper for test_one_sccb to be used when the template should not be
>>>>>> + * copied and the memory address should not be touched.
>>>>>> + */
>>>>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>>>>>> +{
>>>>>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>>>>>> + *
>>>>>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>>>>>> + * length field of the SCCB, buf_len indicates the number of bytes of
>>>>>> + * template that need to be copied to the actual test address. In many cases
>>>>>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>>>>>> + * itself can be larger.
>>>>>> + *
>>>>>> + * Returns true in case of success or false in case of failure
>>>>>> + */
>>>>>> +static bool 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)
>>>>>> +{
>>>>>> +	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);
>>>>>
>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>
>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>   ^
>>>>> s390x/sclp.c: At top level:
>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>> cc1: all warnings being treated as errors
>>>>> make: *** [s390x/sclp.o] Error 1
>>>>
>>>> The following makes it work:
>>>>
>>>>
>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>> index c13fa60..0b8117a 100644
>>>> --- a/s390x/sclp.c
>>>> +++ b/s390x/sclp.c
>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>  static bool 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)
>>>>  {
>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>> +
>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>> +       header->length = sccb_len;
>>>
>>> While that might silence the compiler warning, we still might get
>>> aliasing problems here, I think.
>>> The right way to solve this problem is to turn sccb_template into a
>>> union of the various structs/arrays that you want to use and then access
>>> the fields through the union instead ("type-punning through union").
>>
>> We do have the exact same thing in lib/s390x/sclp.c already, no?
> 
> Maybe we should carefully check that code, too...
> 
>> Especially, new compilers don't seem to care?
> 
> I've seen horrible bugs due to these aliasing problems in the past -
> without compiler warnings showing up! Certain versions of GCC assume
> that they can re-order code with pointers that point to types of
> different sizes, i.e. in the above example, I think they could assume
> that they could re-order the memset() and the header->length = ... line.
> I'd feel better if we play safe and use a union here.

Should we simply allow type-punning?

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[David Hildenbrand]

On 22.01.20 11:40, David Hildenbrand wrote:
> On 22.01.20 11:39, Thomas Huth wrote:
>> On 22/01/2020 11.32, David Hildenbrand wrote:
>>> On 22.01.20 11:31, Thomas Huth wrote:
>>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>>> On 22.01.20 11:10, David Hildenbrand wrote:
>>>>>> On 20.01.20 19:42, 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>
>>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>>> ---
>>>>>>>  s390x/Makefile      |   1 +
>>>>>>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  s390x/unittests.cfg |   8 +
>>>>>>>  3 files changed, 483 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..215347e
>>>>>>> --- /dev/null
>>>>>>> +++ b/s390x/sclp.c
>>>>>>> @@ -0,0 +1,474 @@
>>>>>>> +/*
>>>>>>> + * 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))
>>>>>>> +
>>>>>>> +#define LC_SIZE (2 * PAGE_SIZE)
>>>>>>> +
>>>>>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>>>>>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>>>>>>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>>>>>>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>>>>>>> +static struct lowcore *lc;
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * 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();
>>>>>>> +	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.
>>>>>>> + *
>>>>>>> + * The parameter buf_len indicates the number of bytes of the template that
>>>>>>> + * should be copied to the test address, and should be 0 when the test
>>>>>>> + * address is invalid, in which case nothing is copied.
>>>>>>> + *
>>>>>>> + * The template is used to simplify tests where the same buffer content is
>>>>>>> + * used many times in a row, at different addresses.
>>>>>>> + *
>>>>>>> + * Returns true in case of success or false in case of failure
>>>>>>> + */
>>>>>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>>>>>>> +		memcpy(addr, sccb_template, buf_len);
>>>>>>> +	if (exp_pgm != PGM_NONE)
>>>>>>> +		expect_pgm_int();
>>>>>>> +	/* perform the actual call */
>>>>>>> +	res = sclp_service_call_test(cmd, h);
>>>>>>> +	if (res) {
>>>>>>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>>>>>>> +		return false;
>>>>>>> +	}
>>>>>>> +	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, buf_len %d, cmd %#x, pgm code %d",
>>>>>>> +				addr, buf_len, cmd, pgm);
>>>>>>> +		return false;
>>>>>>> +	}
>>>>>>> +	/* Check if the response code is the one expected */
>>>>>>> +	if (exp_rc && exp_rc != h->response_code) {
>>>>>>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>>>>>>> +				addr, buf_len, cmd, h->response_code);
>>>>>>> +		return false;
>>>>>>> +	}
>>>>>>> +	return true;
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Wrapper for test_one_sccb to be used when the template should not be
>>>>>>> + * copied and the memory address should not be touched.
>>>>>>> + */
>>>>>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>>>>>>> +{
>>>>>>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>>>>>>> + *
>>>>>>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>>>>>>> + * length field of the SCCB, buf_len indicates the number of bytes of
>>>>>>> + * template that need to be copied to the actual test address. In many cases
>>>>>>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>>>>>>> + * itself can be larger.
>>>>>>> + *
>>>>>>> + * Returns true in case of success or false in case of failure
>>>>>>> + */
>>>>>>> +static bool 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)
>>>>>>> +{
>>>>>>> +	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);
>>>>>>
>>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>>
>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>   ^
>>>>>> s390x/sclp.c: At top level:
>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>>> cc1: all warnings being treated as errors
>>>>>> make: *** [s390x/sclp.o] Error 1
>>>>>
>>>>> The following makes it work:
>>>>>
>>>>>
>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>>> index c13fa60..0b8117a 100644
>>>>> --- a/s390x/sclp.c
>>>>> +++ b/s390x/sclp.c
>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>>  static bool 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)
>>>>>  {
>>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>>> +
>>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>> +       header->length = sccb_len;
>>>>
>>>> While that might silence the compiler warning, we still might get
>>>> aliasing problems here, I think.
>>>> The right way to solve this problem is to turn sccb_template into a
>>>> union of the various structs/arrays that you want to use and then access
>>>> the fields through the union instead ("type-punning through union").
>>>
>>> We do have the exact same thing in lib/s390x/sclp.c already, no?
>>
>> Maybe we should carefully check that code, too...
>>
>>> Especially, new compilers don't seem to care?
>>
>> I've seen horrible bugs due to these aliasing problems in the past -
>> without compiler warnings showing up! Certain versions of GCC assume
>> that they can re-order code with pointers that point to types of
>> different sizes, i.e. in the above example, I think they could assume
>> that they could re-order the memset() and the header->length = ... line.
>> I'd feel better if we play safe and use a union here.
> 
> Should we simply allow type-punning?

For now I squashed

diff --git a/s390x/sclp.c b/s390x/sclp.c
index c13fa60..7d92bf3 100644
--- a/s390x/sclp.c
+++ b/s390x/sclp.c
@@ -26,7 +26,12 @@

 static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/*
scratch pages used for some tests */
 static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
/* temporary lowcore for test_sccb_prefix */
-static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
+/* SCCB template to be used */
+static union {
+	uint8_t raw[PAGE_SIZE];
+	SCCBHeader header;
+	WriteEventData data;
+} sccb_template;
 static uint32_t valid_code;						/* valid command code for READ SCP INFO */
 static struct lowcore *lc;

@@ -69,7 +74,7 @@ static bool test_one_sccb(uint32_t cmd, uint8_t *addr,
uint16_t buf_len, uint64_

 	/* Copy the template to the test address if needed */
 	if (buf_len)
-		memcpy(addr, sccb_template, buf_len);
+		memcpy(addr, sccb_template.raw, buf_len);
 	if (exp_pgm != PGM_NONE)
 		expect_pgm_int();
 	/* perform the actual call */
@@ -117,8 +122,8 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr,
uint64_t exp_pgm, uint16_t
 static bool 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)
 {
-	memset(sccb_template, 0, sizeof(sccb_template));
-	((SCCBHeader *)sccb_template)->length = sccb_len;
+	memset(sccb_template.raw, 0, sizeof(sccb_template.raw));
+	sccb_template.header.length = sccb_len;
 	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
 }

@@ -299,10 +304,10 @@ 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;
+	WriteEventData *sccb = &sccb_template.data;
 	int len, offset;

-	memset(sccb_template, 0, sizeof(sccb_template));
+	memset(sccb_template.raw, 0, sizeof(sccb_template.raw));
 	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
 	for (len = 32; len <= 4096; len++) {
 		offset = len & 7 ? len & ~7 : len - 8;
@@ -323,10 +328,10 @@ 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;
+	WriteEventData *sccb = &sccb_template.data;
 	int len;

-	memset(sccb_template, 0, sizeof(sccb_template));
+	memset(sccb_template.raw, 0, sizeof(sccb_template.raw));
 	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
 	for (len = 4097; len < 8192; len++) {
 		sccb->h.length = len;
-- 
2.24.1




-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[Thomas Huth]

On 22/01/2020 12.20, David Hildenbrand wrote:
> On 22.01.20 11:40, David Hildenbrand wrote:
>> On 22.01.20 11:39, Thomas Huth wrote:
>>> On 22/01/2020 11.32, David Hildenbrand wrote:
>>>> On 22.01.20 11:31, Thomas Huth wrote:
>>>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>>>> On 22.01.20 11:10, David Hildenbrand wrote:
>>>>>>> On 20.01.20 19:42, 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>
>>>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>>>> ---
>>>>>>>>  s390x/Makefile      |   1 +
>>>>>>>>  s390x/sclp.c        | 474 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  s390x/unittests.cfg |   8 +
>>>>>>>>  3 files changed, 483 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..215347e
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/s390x/sclp.c
>>>>>>>> @@ -0,0 +1,474 @@
>>>>>>>> +/*
>>>>>>>> + * 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))
>>>>>>>> +
>>>>>>>> +#define LC_SIZE (2 * PAGE_SIZE)
>>>>>>>> +
>>>>>>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* scratch pages used for some tests */
>>>>>>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/* temporary lowcore for test_sccb_prefix */
>>>>>>>> +static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
>>>>>>>> +static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>>>>>>>> +static struct lowcore *lc;
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * 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();
>>>>>>>> +	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.
>>>>>>>> + *
>>>>>>>> + * The parameter buf_len indicates the number of bytes of the template that
>>>>>>>> + * should be copied to the test address, and should be 0 when the test
>>>>>>>> + * address is invalid, in which case nothing is copied.
>>>>>>>> + *
>>>>>>>> + * The template is used to simplify tests where the same buffer content is
>>>>>>>> + * used many times in a row, at different addresses.
>>>>>>>> + *
>>>>>>>> + * Returns true in case of success or false in case of failure
>>>>>>>> + */
>>>>>>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_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 (buf_len)
>>>>>>>> +		memcpy(addr, sccb_template, buf_len);
>>>>>>>> +	if (exp_pgm != PGM_NONE)
>>>>>>>> +		expect_pgm_int();
>>>>>>>> +	/* perform the actual call */
>>>>>>>> +	res = sclp_service_call_test(cmd, h);
>>>>>>>> +	if (res) {
>>>>>>>> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
>>>>>>>> +		return false;
>>>>>>>> +	}
>>>>>>>> +	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, buf_len %d, cmd %#x, pgm code %d",
>>>>>>>> +				addr, buf_len, cmd, pgm);
>>>>>>>> +		return false;
>>>>>>>> +	}
>>>>>>>> +	/* Check if the response code is the one expected */
>>>>>>>> +	if (exp_rc && exp_rc != h->response_code) {
>>>>>>>> +		report_info("First failure at addr %p, buf_len %d, cmd %#x, resp code %#x",
>>>>>>>> +				addr, buf_len, cmd, h->response_code);
>>>>>>>> +		return false;
>>>>>>>> +	}
>>>>>>>> +	return true;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * Wrapper for test_one_sccb to be used when the template should not be
>>>>>>>> + * copied and the memory address should not be touched.
>>>>>>>> + */
>>>>>>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t exp_rc)
>>>>>>>> +{
>>>>>>>> +	return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * Wrapper for test_one_sccb to set up a simple SCCB template.
>>>>>>>> + *
>>>>>>>> + * The parameter sccb_len indicates the value that will be saved in the SCCB
>>>>>>>> + * length field of the SCCB, buf_len indicates the number of bytes of
>>>>>>>> + * template that need to be copied to the actual test address. In many cases
>>>>>>>> + * it's enough to clear/copy the first 8 bytes of the buffer, while the SCCB
>>>>>>>> + * itself can be larger.
>>>>>>>> + *
>>>>>>>> + * Returns true in case of success or false in case of failure
>>>>>>>> + */
>>>>>>>> +static bool 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)
>>>>>>>> +{
>>>>>>>> +	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);
>>>>>>>
>>>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>>>
>>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>>   ^
>>>>>>> s390x/sclp.c: At top level:
>>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>>>> cc1: all warnings being treated as errors
>>>>>>> make: *** [s390x/sclp.o] Error 1
>>>>>>
>>>>>> The following makes it work:
>>>>>>
>>>>>>
>>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>>>> index c13fa60..0b8117a 100644
>>>>>> --- a/s390x/sclp.c
>>>>>> +++ b/s390x/sclp.c
>>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>>>  static bool 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)
>>>>>>  {
>>>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>>>> +
>>>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>> +       header->length = sccb_len;
>>>>>
>>>>> While that might silence the compiler warning, we still might get
>>>>> aliasing problems here, I think.
>>>>> The right way to solve this problem is to turn sccb_template into a
>>>>> union of the various structs/arrays that you want to use and then access
>>>>> the fields through the union instead ("type-punning through union").
>>>>
>>>> We do have the exact same thing in lib/s390x/sclp.c already, no?
>>>
>>> Maybe we should carefully check that code, too...
>>>
>>>> Especially, new compilers don't seem to care?
>>>
>>> I've seen horrible bugs due to these aliasing problems in the past -
>>> without compiler warnings showing up! Certain versions of GCC assume
>>> that they can re-order code with pointers that point to types of
>>> different sizes, i.e. in the above example, I think they could assume
>>> that they could re-order the memset() and the header->length = ... line.
>>> I'd feel better if we play safe and use a union here.
>>
>> Should we simply allow type-punning?
> 
> For now I squashed
> 
> diff --git a/s390x/sclp.c b/s390x/sclp.c
> index c13fa60..7d92bf3 100644
> --- a/s390x/sclp.c
> +++ b/s390x/sclp.c
> @@ -26,7 +26,12 @@
> 
>  static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE)));	/*
> scratch pages used for some tests */
>  static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
> /* temporary lowcore for test_sccb_prefix */
> -static uint8_t sccb_template[PAGE_SIZE];				/* SCCB template to be used */
> +/* SCCB template to be used */
> +static union {
> +	uint8_t raw[PAGE_SIZE];
> +	SCCBHeader header;
> +	WriteEventData data;
> +} sccb_template;
>  static uint32_t valid_code;						/* valid command code for READ SCP INFO */
>  static struct lowcore *lc;
> 
> @@ -69,7 +74,7 @@ static bool test_one_sccb(uint32_t cmd, uint8_t *addr,
> uint16_t buf_len, uint64_
> 
>  	/* Copy the template to the test address if needed */
>  	if (buf_len)
> -		memcpy(addr, sccb_template, buf_len);
> +		memcpy(addr, sccb_template.raw, buf_len);
>  	if (exp_pgm != PGM_NONE)
>  		expect_pgm_int();
>  	/* perform the actual call */
> @@ -117,8 +122,8 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr,
> uint64_t exp_pgm, uint16_t
>  static bool 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)
>  {
> -	memset(sccb_template, 0, sizeof(sccb_template));
> -	((SCCBHeader *)sccb_template)->length = sccb_len;
> +	memset(sccb_template.raw, 0, sizeof(sccb_template.raw));
> +	sccb_template.header.length = sccb_len;
>  	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
>  }
> 
> @@ -299,10 +304,10 @@ 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;
> +	WriteEventData *sccb = &sccb_template.data;
>  	int len, offset;
> 
> -	memset(sccb_template, 0, sizeof(sccb_template));
> +	memset(sccb_template.raw, 0, sizeof(sccb_template.raw));
>  	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>  	for (len = 32; len <= 4096; len++) {
>  		offset = len & 7 ? len & ~7 : len - 8;
> @@ -323,10 +328,10 @@ 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;
> +	WriteEventData *sccb = &sccb_template.data;
>  	int len;
> 
> -	memset(sccb_template, 0, sizeof(sccb_template));
> +	memset(sccb_template.raw, 0, sizeof(sccb_template.raw));
>  	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>  	for (len = 4097; len < 8192; len++) {
>  		sccb->h.length = len;

Thanks, that looks better now!

 Thomas

Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test

[Thomas Huth]

On 22/01/2020 11.40, David Hildenbrand wrote:
> On 22.01.20 11:39, Thomas Huth wrote:
>> On 22/01/2020 11.32, David Hildenbrand wrote:
>>> On 22.01.20 11:31, Thomas Huth wrote:
>>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>>> On 22.01.20 11:10, David Hildenbrand wrote:
[...]
>>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>>
>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>   ^
>>>>>> s390x/sclp.c: At top level:
>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>>> cc1: all warnings being treated as errors
>>>>>> make: *** [s390x/sclp.o] Error 1
>>>>>
>>>>> The following makes it work:
>>>>>
>>>>>
>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>>> index c13fa60..0b8117a 100644
>>>>> --- a/s390x/sclp.c
>>>>> +++ b/s390x/sclp.c
>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>>  static bool 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)
>>>>>  {
>>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>>> +
>>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>> +       header->length = sccb_len;
>>>>
>>>> While that might silence the compiler warning, we still might get
>>>> aliasing problems here, I think.
>>>> The right way to solve this problem is to turn sccb_template into a
>>>> union of the various structs/arrays that you want to use and then access
>>>> the fields through the union instead ("type-punning through union").
>>>
>>> We do have the exact same thing in lib/s390x/sclp.c already, no?
>>
>> Maybe we should carefully check that code, too...
>>
>>> Especially, new compilers don't seem to care?
>>
>> I've seen horrible bugs due to these aliasing problems in the past -
>> without compiler warnings showing up! Certain versions of GCC assume
>> that they can re-order code with pointers that point to types of
>> different sizes, i.e. in the above example, I think they could assume
>> that they could re-order the memset() and the header->length = ... line.
>> I'd feel better if we play safe and use a union here.
> 
> Should we simply allow type-punning?

Maybe yes. The kernel also compiles with "-fno-strict-aliasing", and
since kvm-unit-tests is mainly a "playground" for people who do kernel
development, too, we should maybe also compile the unit tests with
"-fno-strict-aliasing".

Paolo, Andrew, Laurent, what do you think?

 Thomas

strict aliasing in kvm-unit-tests (was: Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test)

[Thomas Huth]

On 22/01/2020 13.16, Thomas Huth wrote:
> On 22/01/2020 11.40, David Hildenbrand wrote:
>> On 22.01.20 11:39, Thomas Huth wrote:
>>> On 22/01/2020 11.32, David Hildenbrand wrote:
>>>> On 22.01.20 11:31, Thomas Huth wrote:
>>>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>>>> On 22.01.20 11:10, David Hildenbrand wrote:
> [...]
>>>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>>>
>>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>>   ^
>>>>>>> s390x/sclp.c: At top level:
>>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>>>> cc1: all warnings being treated as errors
>>>>>>> make: *** [s390x/sclp.o] Error 1
>>>>>>
>>>>>> The following makes it work:
>>>>>>
>>>>>>
>>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>>>> index c13fa60..0b8117a 100644
>>>>>> --- a/s390x/sclp.c
>>>>>> +++ b/s390x/sclp.c
>>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>>>  static bool 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)
>>>>>>  {
>>>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>>>> +
>>>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>> +       header->length = sccb_len;
>>>>>
>>>>> While that might silence the compiler warning, we still might get
>>>>> aliasing problems here, I think.
>>>>> The right way to solve this problem is to turn sccb_template into a
>>>>> union of the various structs/arrays that you want to use and then access
>>>>> the fields through the union instead ("type-punning through union").
>>>>
>>>> We do have the exact same thing in lib/s390x/sclp.c already, no?
>>>
>>> Maybe we should carefully check that code, too...
>>>
>>>> Especially, new compilers don't seem to care?
>>>
>>> I've seen horrible bugs due to these aliasing problems in the past -
>>> without compiler warnings showing up! Certain versions of GCC assume
>>> that they can re-order code with pointers that point to types of
>>> different sizes, i.e. in the above example, I think they could assume
>>> that they could re-order the memset() and the header->length = ... line.
>>> I'd feel better if we play safe and use a union here.
>>
>> Should we simply allow type-punning?
> 
> Maybe yes. The kernel also compiles with "-fno-strict-aliasing", and
> since kvm-unit-tests is mainly a "playground" for people who do kernel
> development, too, we should maybe also compile the unit tests with
> "-fno-strict-aliasing".
> 
> Paolo, Andrew, Laurent, what do you think?

By the way, when compiling the x86 code with -O2 instead of -O1, I also get:

lib/x86/intel-iommu.c: In function ‘vtd_setup_msi’:
lib/x86/intel-iommu.c:324:4: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
   *(uint64_t *)&msi_addr, *(uint32_t *)&msi_data);
    ^~~~~~~~~~~~~~~~~~~~~
lib/x86/intel-iommu.c:324:28: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
   *(uint64_t *)&msi_addr, *(uint32_t *)&msi_data);
                            ^~~~~~~~~~~~~~~~~~~~~
lib/x86/intel-iommu.c:326:29: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
  return pci_setup_msi(dev, *(uint64_t *)&msi_addr,
                             ^~~~~~~~~~~~~~~~~~~~~
lib/x86/intel-iommu.c:327:10: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
         *(uint32_t *)&msi_data);
          ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

... so maybe -fno-strict-aliasing would be a good idea for that, too?

 Thomas

Re: strict aliasing in kvm-unit-tests (was: Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test)

[Paolo Bonzini]

On 22/01/20 15:15, Thomas Huth wrote:
> On 22/01/2020 13.16, Thomas Huth wrote:
>> On 22/01/2020 11.40, David Hildenbrand wrote:
>>> On 22.01.20 11:39, Thomas Huth wrote:
>>>> On 22/01/2020 11.32, David Hildenbrand wrote:
>>>>> On 22.01.20 11:31, Thomas Huth wrote:
>>>>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>>>>> On 22.01.20 11:10, David Hildenbrand wrote:
>> [...]
>>>>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>>>>
>>>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>>>   ^
>>>>>>>> s390x/sclp.c: At top level:
>>>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>>>>> cc1: all warnings being treated as errors
>>>>>>>> make: *** [s390x/sclp.o] Error 1
>>>>>>>
>>>>>>> The following makes it work:
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>>>>> index c13fa60..0b8117a 100644
>>>>>>> --- a/s390x/sclp.c
>>>>>>> +++ b/s390x/sclp.c
>>>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>>>>  static bool 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)
>>>>>>>  {
>>>>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>>>>> +
>>>>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>> +       header->length = sccb_len;
>>>>>>
>>>>>> While that might silence the compiler warning, we still might get
>>>>>> aliasing problems here, I think.
>>>>>> The right way to solve this problem is to turn sccb_template into a
>>>>>> union of the various structs/arrays that you want to use and then access
>>>>>> the fields through the union instead ("type-punning through union").
>>>>>
>>>>> We do have the exact same thing in lib/s390x/sclp.c already, no?
>>>>
>>>> Maybe we should carefully check that code, too...
>>>>
>>>>> Especially, new compilers don't seem to care?
>>>>
>>>> I've seen horrible bugs due to these aliasing problems in the past -
>>>> without compiler warnings showing up! Certain versions of GCC assume
>>>> that they can re-order code with pointers that point to types of
>>>> different sizes, i.e. in the above example, I think they could assume
>>>> that they could re-order the memset() and the header->length = ... line.
>>>> I'd feel better if we play safe and use a union here.
>>>
>>> Should we simply allow type-punning?
>>
>> Maybe yes. The kernel also compiles with "-fno-strict-aliasing", and
>> since kvm-unit-tests is mainly a "playground" for people who do kernel
>> development, too, we should maybe also compile the unit tests with
>> "-fno-strict-aliasing".
>>
>> Paolo, Andrew, Laurent, what do you think?

I think enabling -fno-strict-aliasing is a good idea.

Paolo

Re: strict aliasing in kvm-unit-tests (was: Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test)

[Laurent Vivier]

On 22/01/2020 15:42, Paolo Bonzini wrote:
> On 22/01/20 15:15, Thomas Huth wrote:
>> On 22/01/2020 13.16, Thomas Huth wrote:
>>> On 22/01/2020 11.40, David Hildenbrand wrote:
>>>> On 22.01.20 11:39, Thomas Huth wrote:
>>>>> On 22/01/2020 11.32, David Hildenbrand wrote:
>>>>>> On 22.01.20 11:31, Thomas Huth wrote:
>>>>>>> On 22/01/2020 11.22, David Hildenbrand wrote:
>>>>>>>> On 22.01.20 11:10, David Hildenbrand wrote:
>>> [...]
>>>>>>>>> Doing a fresh ./configure + make on RHEL7 gives me
>>>>>>>>>
>>>>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make
>>>>>>>>> gcc  -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror  -fomit-frame-pointer    -Wno-frame-address   -fno-pic    -Wclobbered  -Wunused-but-set-parameter  -Wmissing-parameter-type  -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes   -c -o s390x/sclp.o s390x/sclp.c
>>>>>>>>> s390x/sclp.c: In function 'test_one_simple':
>>>>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>>>>>>>>>   ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>>>>   ^
>>>>>>>>> s390x/sclp.c: At top level:
>>>>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror]
>>>>>>>>> cc1: all warnings being treated as errors
>>>>>>>>> make: *** [s390x/sclp.o] Error 1
>>>>>>>>
>>>>>>>> The following makes it work:
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c
>>>>>>>> index c13fa60..0b8117a 100644
>>>>>>>> --- a/s390x/sclp.c
>>>>>>>> +++ b/s390x/sclp.c
>>>>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t
>>>>>>>>  static bool 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)
>>>>>>>>  {
>>>>>>>> +       SCCBHeader *header = (void *)sccb_template;
>>>>>>>> +
>>>>>>>>         memset(sccb_template, 0, sizeof(sccb_template));
>>>>>>>> -       ((SCCBHeader *)sccb_template)->length = sccb_len;
>>>>>>>> +       header->length = sccb_len;
>>>>>>>
>>>>>>> While that might silence the compiler warning, we still might get
>>>>>>> aliasing problems here, I think.
>>>>>>> The right way to solve this problem is to turn sccb_template into a
>>>>>>> union of the various structs/arrays that you want to use and then access
>>>>>>> the fields through the union instead ("type-punning through union").
>>>>>>
>>>>>> We do have the exact same thing in lib/s390x/sclp.c already, no?
>>>>>
>>>>> Maybe we should carefully check that code, too...
>>>>>
>>>>>> Especially, new compilers don't seem to care?
>>>>>
>>>>> I've seen horrible bugs due to these aliasing problems in the past -
>>>>> without compiler warnings showing up! Certain versions of GCC assume
>>>>> that they can re-order code with pointers that point to types of
>>>>> different sizes, i.e. in the above example, I think they could assume
>>>>> that they could re-order the memset() and the header->length = ... line.
>>>>> I'd feel better if we play safe and use a union here.
>>>>
>>>> Should we simply allow type-punning?
>>>
>>> Maybe yes. The kernel also compiles with "-fno-strict-aliasing", and
>>> since kvm-unit-tests is mainly a "playground" for people who do kernel
>>> development, too, we should maybe also compile the unit tests with
>>> "-fno-strict-aliasing".
>>>
>>> Paolo, Andrew, Laurent, what do you think?
> 
> I think enabling -fno-strict-aliasing is a good idea.

I agree too

Laurent