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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22.10.19 12:53, Claudio Imbrenda wrote:
SCLP unit test. Testing the following:

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

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

diff --git a/s390x/Makefile b/s390x/Makefile
index 3744372..ddb4b48 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -16,6 +16,7 @@ tests += $(TEST_DIR)/diag288.elf
  tests += $(TEST_DIR)/stsi.elf
  tests += $(TEST_DIR)/skrf.elf
  tests += $(TEST_DIR)/smp.elf
+tests += $(TEST_DIR)/sclp.elf
  tests_binary = $(patsubst %.elf,%.bin,$(tests))
all: directories test_cases test_cases_binary
diff --git a/s390x/sclp.c b/s390x/sclp.c
new file mode 100644
index 0000000..d7a9212
--- /dev/null
+++ b/s390x/sclp.c
@@ -0,0 +1,373 @@
+/*
+ * Store System Information tests
+ *
+ * Copyright (c) 2019 IBM Corp
+ *
+ * Authors:
+ *  Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <sclp.h>
+
+static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+static uint8_t prefix_buf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+static uint32_t valid_sclp_code;
+static struct lowcore *lc;
+
+static void sclp_setup_int_test(void)
+{
+	uint64_t mask;
+
+	ctl_set_bit(0, 9);
+
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+}
+
+static int sclp_service_call_test(unsigned int command, void *sccb)

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

+{
+	int cc;
+
+	sclp_mark_busy();
+	sclp_setup_int_test();
+	lc->pgm_int_code = 0;
+	cc = servc(command, __pa(sccb));
+	if (lc->pgm_int_code) {
+		sclp_handle_ext();

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

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

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

+{
+	char sccb[4096];

I prefer uint8_t sccb[PAGE_SIZE]

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

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

+	return test_one_sccb(cmd, addr, clear, p, exp_pgm, exp_rc);
+}
+
+#define PGM_BIT_SPEC	(1ULL << PGM_INT_CODE_SPECIFICATION)
+#define PGM_BIT_ADDR	(1ULL << PGM_INT_CODE_ADDRESSING)
+#define PGM_BIT_PRIV	(1ULL << PGM_INT_CODE_PRIVILEGED_OPERATION)
+
+#define PGBUF	((uintptr_t)pagebuf)
+#define VALID	(valid_sclp_code)

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

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

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

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

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

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

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

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

report_prefix_push("Privileged Operation");

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

report_prefix_pop();

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

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

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

E.g. just user address "-PAGE_SIZE"

+	for (cx = maxram; cx < MIN(maxram + 65536, 0x80000000); cx += 8)
+		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; cx < MIN((maxram + 0x7fffff) & ~0xfffff, 0x80000000); cx += 4096)
+		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
+			goto out;
+	for (; cx < 0x80000000; cx += 1048576)
+		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_ADDR, 0))
+			goto out;
+out:
+	report("Invalid SCCB address", cx == 0x80000000);
+}
+
+static void test_instbits(void)
+{
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	unsigned long mask;
+	int cc;
+
+	sclp_mark_busy();
+	h->length = 8;
+
+	ctl_set_bit(0, 9);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+
+	asm volatile(
+		"       .insn   rre,0xb2204200,%1,%2\n"  /* servc %1,%2 */
+		"       ipm     %0\n"
+		"       srl     %0,28"
+		: "=&d" (cc) : "d" (valid_sclp_code), "a" (__pa(pagebuf))
+		: "cc", "memory");
+	sclp_wait_busy();
+	report("Instruction format ignored bits", cc == 0);
+}
+
+static void find_valid_sclp_code(void)
+{
+	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
+				    SCLP_CMDW_READ_SCP_INFO };
+	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	int i, cc;
+
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		sclp_mark_busy();
+		memset(h, 0, sizeof(pagebuf));
+		h->length = 4096;
+
+		valid_sclp_code = commands[i];
+		cc = sclp_service_call(commands[i], h);
+		if (cc)
+			break;
+		if (h->response_code == SCLP_RC_NORMAL_READ_COMPLETION)
+			return;
+		if (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND)
+			break;
+	}
+	valid_sclp_code = 0;
+	report_abort("READ_SCP_INFO failed");
+}
+
+int main(void)
+{
+	report_prefix_push("sclp");
+	find_valid_sclp_code();
+	test_instbits();
+	test_priv();
+	test_addressing();
+	test_spec();
+	test_resp();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f1b07cd..75e3d37 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -75,3 +75,6 @@ file = stsi.elf
  [smp]
  file = smp.elf
  extra_params =-smp 2
+
+[sclp]
+file = sclp.elf



--

Thanks,

David / dhildenb





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux