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

[Thomas Huth <thuth@redhat.com>]

----- Original Message -----
> From: "Claudio Imbrenda" <imbrenda@linux.ibm.com>
> Sent: Tuesday, October 22, 2019 12:53:04 PM
> 
> SCLP unit test. Testing the following:
> 
> * Correctly ignoring instruction bits that should be ignored
> * Privileged instruction check
> * Check for addressing exceptions
> * Specification exceptions:
>   - SCCB size less than 8
>   - SCCB unaligned
>   - SCCB overlaps prefix or lowcore
>   - SCCB address higher than 2GB
> * Return codes for
>   - Invalid command
>   - SCCB too short (but at least 8)
>   - SCCB page boundary violation
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/sclp.c        | 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

Copy-n-paste from stsi.c ?

> + * 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>
[...]
> +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)
> +{
> +	char sccb[4096];
> +	void *p = sccb;
> +
> +	if (!len && !clear)
> +		p = NULL;
> +	else
> +		memset(sccb, 0, sizeof(sccb));
> +	((SCCBHeader *)sccb)->length = len;
> +	if (clear && (clear < 8))

Please remove the parentheses around "clear < 8".

> +		clear = 8;
> +	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)
> +
> +static void test_sccb_too_short(void)
> +{
> +	int cx;
> +
> +	for (cx = 0; cx < 8; cx++)
> +		if (!test_one_run(VALID, PGBUF, cx, 8, PGM_BIT_SPEC, 0))
> +			break;
> +
> +	report("SCCB too short", cx == 8);
> +}
> +
> +static void test_sccb_unaligned(void)
> +{
> +	int cx;
> +
> +	for (cx = 1; cx < 8; cx++)
> +		if (!test_one_run(VALID, cx + PGBUF, 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB unaligned", cx == 8);
> +}
> +
> +static void test_sccb_prefix(void)
> +{
> +	uint32_t prefix, new_prefix;
> +	int cx;
> +
> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_run(VALID, cx, 0, 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report("SCCB low pages", cx == 8192);
> +
> +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> +	memcpy(prefix_buf, 0, 8192);
> +	asm volatile("stpx %0": "+Q"(prefix));

Isn't "=Q" sufficient enough here?

> +	asm volatile("spx %0": "+Q"(new_prefix));

Shouldn't that be just an input parameter instead? ... and maybe also better add "memory" to the clobber list, since the memory layout has changed.

> +	for (cx = 0; cx < 8192; cx += 8)
> +		if (!test_one_run(VALID, 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));

dito?

> +}

 Thomas