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

David Hildenbrand
2019-10-23T12:15:05Z

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@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
> + *
> + * 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 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