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

[Thomas Huth <thuth@redhat.com>]

On 11/11/2019 18.20, Claudio Imbrenda wrote:
> SCLP unit test. Testing the following:
> 
> * Correctly ignoring instruction bits that should be ignored
> * Privileged instruction check
> * Check for addressing exceptions
> * Specification exceptions:
>   - SCCB size less than 8
>   - SCCB unaligned
>   - SCCB overlaps prefix or lowcore
>   - SCCB address higher than 2GB
> * Return codes for
>   - Invalid command
>   - SCCB too short (but at least 8)
>   - SCCB page boundary violation
[...]
> +
> +#define PGM_NONE	1
> +#define PGM_BIT_SPEC	(1ULL << PGM_INT_CODE_SPECIFICATION)
> +#define PGM_BIT_ADDR	(1ULL << PGM_INT_CODE_ADDRESSING)
> +#define PGM_BIT_PRIV	(1ULL << PGM_INT_CODE_PRIVILEGED_OPERATION)
> +#define MKPTR(x) ((void *)(uint64_t)(x))
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +static uint8_t prefix_buf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +static uint8_t sccb_template[PAGE_SIZE];
> +static uint32_t valid_code;
> +static struct lowcore *lc;
> +
> +/**
> + * Enable SCLP interrupt.
> + */
> +static void sclp_setup_int_test(void)
> +{
> +	uint64_t mask;
> +
> +	ctl_set_bit(0, 9);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +}

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

> +/**
> + * Perform one service call, handling exceptions and interrupts.
> + */
> +static int sclp_service_call_test(unsigned int command, void *sccb)
> +{
> +	int cc;
> +
> +	sclp_mark_busy();
> +	sclp_setup_int_test();
> +	cc = servc(command, __pa(sccb));
> +	if (lc->pgm_int_code) {
> +		sclp_handle_ext();
> +		return 0;
> +	}
> +	if (!cc)
> +		sclp_wait_busy();
> +	return cc;
> +}
> +
> +/**
> + * Perform one test at the given address, optionally using the SCCB template,

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

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

Could use bool with true + false instead.

> + */
> +static int test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t len, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	SCCBHeader *h = (SCCBHeader *)addr;
> +	int res, pgm;
> +
> +	/* Copy the template to the test address if needed */
> +	if (len)
> +		memcpy(addr, sccb_template, len);

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

> +	expect_pgm_int();
> +	res = sclp_service_call_test(cmd, h);
> +	if (res) {
> +		report_info("SCLP not ready (command %#x, address %p, cc %d)", cmd, addr, res);
> +		return 0;
> +	}
> +	pgm = clear_pgm_int();
> +	/* Check if the program exception was one of the expected ones */
> +	if (!((1ULL << pgm) & exp_pgm)) {
> +		report_info("First failure at addr %p, size %d, cmd %#x, pgm code %d", addr, len, cmd, pgm);
> +		return 0;
> +	}
> +	/* Check if the response code is the one expected */
> +	if (exp_rc && (exp_rc != h->response_code)) {

You can drop the parentheses around "exp_rc != h->response_code".

> +		report_info("First failure at addr %p, size %d, cmd %#x, resp code %#x",
> +				addr, len, cmd, h->response_code);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/**
> + * Wrapper for test_one_sccb to set up a simple SCCB template.
> + * Returns 1 in case of success or 0 in case of failure
> + */
> +static int test_one_simple(uint32_t cmd, uint8_t *addr, uint16_t sccb_len,
> +			uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc)
> +{
> +	if (buf_len)
> +		memset(sccb_template, 0, sizeof(sccb_template));
> +	((SCCBHeader *)sccb_template)->length = sccb_len;
> +	return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc);
> +}
[...]
> +/**
> + * Test SCCBs whose address is in the lowcore or prefix area.
> + */
> +static void test_sccb_prefix(void)
> +{
> +	uint32_t prefix, new_prefix;
> +	int offset;
> +
> +	/* can't actually trash the lowcore, unsurprisingly things break if we do */
> +	for (offset = 0; offset < 8192; offset += 8)
> +		if (!test_one_sccb(valid_code, MKPTR(offset), 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB low pages", offset == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);
> +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> +	asm volatile("stpx %0" : "=Q" (prefix));
> +	asm volatile("spx %0" : : "Q" (new_prefix) : "memory");
> +
> +	for (offset = 0; offset < 8192; offset += 8)
> +		if (!test_one_simple(valid_code, MKPTR(new_prefix + offset), 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB prefix pages", offset == 8192);
> +
> +	memcpy(prefix_buf, 0, 8192);

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

> +	asm volatile("spx %0" : : "Q" (prefix) : "memory");
> +}
> +
> +/**
> + * Test SCCBs that are above 2GB. If outside of memory, an addressing
> + * exception is also allowed.
> + */
> +static void test_sccb_high(void)
> +{
> +	SCCBHeader *h = (SCCBHeader *)pagebuf;
> +	uintptr_t a[33 * 4 * 2 + 2];
> +	uint64_t maxram;
> +	int i, pgm, len = 0;
> +
> +	h->length = 8;
> +
> +	for (i = 0; i < 33; i++)
> +		a[len++] = 1UL << (i + 31);
> +	for (i = 0; i < 33; i++)
> +		a[len++] = 3UL << (i + 31);
> +	for (i = 0; i < 33; i++)
> +		a[len++] = 0xffffffff80000000UL << i;
> +	a[len++] = 0x80000000;
> +	for (i = 1; i < 33; i++, len++)
> +		a[len] = a[len - 1] | (1UL << (i + 31));
> +	for (i = 0; i < len; i++)
> +		a[len + i] = a[i] + (intptr_t)h;
> +	len += i;
> +	a[len++] = 0xdeadbeef00000000;
> +	a[len++] = 0xdeaddeadbeef0000;

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

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

Please remove the parentheses around 0xdead0000

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

 Thomas