Re: [kvm-unit-tests PATCH v4 3/3] s390x: SCLP unit test

Thomas Huth
2019-11-20T18:16:15Z

On 19/11/2019 10.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 | 465 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> s390x/unittests.cfg | 3 +
> 3 files changed, 469 insertions(+)
> create mode 100644 s390x/sclp.c
[...]
> +/**
> + * Test SCCB page boundary violations.
> + */
> +static void test_boundary(void)
> +{
> + const uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> + const uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
> + WriteEventData *sccb = (WriteEventData *)sccb_template;
> + int len, offset;
> +
> + memset(sccb_template, 0, sizeof(sccb_template));
> + sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> + for (len = 32; len <= 4096; len++) {
> + offset = len & 7 ? len & ~7 : len - 8;

I needed some time to understand that line. I think it would be easier
that way:

offset = (len - 1) & ~7;

?

Anyway, no need to respin just because of that line ... the rest of the
patch looks ok to me.

> 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

It's a little bit sad that some of the tests require < 2G of RAM while
other tests should (also) be done with > 2G if I understood that
correctly. So currently not all tests can be run automatically but just
starting the "run_tests.sh" script, right?

It does not have to be right now (i.e. could also be a follow-up patch
later), but what about adding two sections to the unittests.cfg file,
one with less and one with more than 2G of RAM? E.g.:

[sclp-1g]
file = sclp.elf
extra_params = -m 1G

[sclp-3g]
file = sclp.elf
extra_params = -m 3G -append "somemagicparametertoonlyrunthebigmemtest"

?

Thomas