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

[Thomas Huth <thuth@redhat.com>]

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