Re: [kvm-unit-tests PATCH v5 4/4] s390x: SCLP unit test

[Thomas Huth <thuth@redhat.com>]

On 08/01/2020 17.13, 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        | 462 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   8 +
>  3 files changed, 471 insertions(+)
>  create mode 100644 s390x/sclp.c
[...]
> +/**
> + * Test SCCBs whose address is in the lowcore or prefix area.
> + */
> +static void test_sccb_prefix(void)
> +{
> +	uint8_t scratch[2 * PAGE_SIZE];
> +	uint32_t prefix, new_prefix;
> +	int offset;
> +
> +	/*
> +	 * copy the current lowcore to the future new location, otherwise we
> +	 * will not have a valid lowcore after setting the new prefix.
> +	 */
> +	memcpy(prefix_buf, 0, 2 * PAGE_SIZE);
> +	/* save the current prefix (it's probably going to be 0) */
> +	prefix = stpx();
> +	/*
> +	 * save the current content of absolute pages 0 and 1, so we can
> +	 * restore them after we trash them later on
> +	 */
> +	memcpy(scratch, (void *)(intptr_t)prefix, 2 * PAGE_SIZE);
> +	/* set the new prefix to prefix_buf */
> +	new_prefix = (uint32_t)(intptr_t)prefix_buf;
> +	spx(new_prefix);
> +
> +	/*
> +	 * testing with SCCB addresses in the lowcore; since we can't
> +	 * actually trash the lowcore (unsurprisingly, things break if we
> +	 * do), this will be a read-only test.
> +	 */
> +	for (offset = 0; offset < 2 * PAGE_SIZE; offset += 8)
> +		if (!test_one_sccb(valid_code, MKPTR(offset), 0, PGM_BIT_SPEC, 0))
> +			break;
> +	report(offset == 2 * PAGE_SIZE, "SCCB low pages");
> +
> +	/*
> +	 * this will trash the contents of the two pages at absolute
> +	 * address 0; we will need to restore them later.
> +	 */

I'm still a bit confused by this comment - will SCLP really trash the
contents here, or will there be a specification exception (since
PGM_BIT_SPEC is given below)? ... maybe you could clarify the comment in
case you respin again (or it could be fixed when picking up the patch)?

> +	for (offset = 0; offset < 2 * PAGE_SIZE; offset += 8)
> +		if (!test_one_simple(valid_code, MKPTR(new_prefix + offset), 8, 8, PGM_BIT_SPEC, 0))
> +			break;
> +	report(offset == 2 * PAGE_SIZE, "SCCB prefix pages");
> +
> +	/* restore the previous contents of absolute pages 0 and 1 */
> +	memcpy(prefix_buf, 0, 2 * PAGE_SIZE);
> +	/* restore the prefix to the original value */
> +	spx(prefix);
> +}

Remaining parts look ok to me now, so with the comment clarified:

Reviewed-by: Thomas Huth <thuth@redhat.com>