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

Commit Message

Claudio Imbrenda Oct. 25, 2019, 5:06 p.m. UTC

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 <[e-mail]>
---
 s390x/Makefile      |   1 +
 s390x/sclp.c        | 413 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 417 insertions(+)
 create mode 100644 s390x/sclp.c

Comments

Janosch Frank Nov. 4, 2019, 9:45 a.m. UTC | #1

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

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

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

Why use variables for constants that are never touched?

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

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

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

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

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

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

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

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

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

Don't we need a newline here?

>

David Hildenbrand Nov. 4, 2019, 10:58 a.m. UTC | #2

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

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

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

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

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

Claudio Imbrenda Nov. 4, 2019, 11:19 a.m. UTC | #3

On Mon, 4 Nov 2019 10:45:07 +0100
Janosch Frank <[e-mail]> wrote:

[...]

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

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


[...]

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

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

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

ok, will fix

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

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

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

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

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

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

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

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

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

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

Claudio Imbrenda Nov. 4, 2019, 11:29 a.m. UTC | #4

On Mon, 4 Nov 2019 11:58:20 +0100
David Hildenbrand <[e-mail]> wrote:

[...]

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

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

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

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

consider that the output will be unreadable, though

David Hildenbrand Nov. 4, 2019, 11:31 a.m. UTC | #5

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

better than random characters :)

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

I think his will turn out more readable.

Claudio Imbrenda Nov. 4, 2019, 11:49 a.m. UTC | #6

On Mon, 4 Nov 2019 12:31:32 +0100
David Hildenbrand <[e-mail]> wrote:

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

will be in v3

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

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

David Hildenbrand Nov. 4, 2019, 11:55 a.m. UTC | #7

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

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

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

Something like

static void test_sccb_too_short(void)
{
	int i;

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

If possible.

Claudio Imbrenda Nov. 4, 2019, 12:06 p.m. UTC | #8

On Mon, 4 Nov 2019 12:55:48 +0100
David Hildenbrand <[e-mail]> wrote:

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

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

David Hildenbrand Nov. 4, 2019, 1:47 p.m. UTC | #9

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

A couple of things to note

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


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

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

Just as we handle it in other tests.

Claudio Imbrenda Nov. 4, 2019, 2:24 p.m. UTC | #10

On Mon, 4 Nov 2019 14:47:54 +0100
David Hildenbrand <[e-mail]> wrote:

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

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

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

David Hildenbrand Nov. 4, 2019, 2:29 p.m. UTC | #11

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

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

Thomas Huth Nov. 8, 2019, 9:35 a.m. UTC | #12

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

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

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

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

  Thomas

Claudio Imbrenda Nov. 8, 2019, 9:46 a.m. UTC | #13

On Fri, 8 Nov 2019 10:35:32 +0100
Thomas Huth <[e-mail]> wrote:

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

good point

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

why so much hatred for parentheses? :D

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

SCLP_RC_NORMAL_READ_COMPLETION != SCLP_RC_INVALID_SCLP_COMMAND

the correct version would be:

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

which is more lines, and significantly less readable.

Patch

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