Re: [kvm-unit-tests PATCH v2 4/4] s390x: Add storage key removal facility

[Thomas Huth <thuth@redhat.com>]

On 28/08/2019 13.36, Janosch Frank wrote:
> The storage key removal facility (stfle bit 169) makes all key related
> instructions result in a special operation exception if they handle a
> key.
> 
> Let's make sure that the skey and pfmf tests only run non key code
> (pfmf) or not at all (skey).
> 
> Also let's test this new facility. As lots of instructions are
> affected by this, only some of them are tested for now.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile |   1 +
>  s390x/pfmf.c   |  10 ++++
>  s390x/skey.c   |   5 ++
>  s390x/skrf.c   | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 144 insertions(+)
>  create mode 100644 s390x/skrf.c
[...]
> +static void test_mvcos(void)
> +{
> +	uint64_t r3 = 64;
> +	uint8_t *src = pagebuf;
> +	uint8_t *dst = pagebuf + PAGE_SIZE;
> +	/* K bit set, as well as keys */
> +	register unsigned long oac asm("0") = 0xf002f002;
> +
> +	report_prefix_push("mvcos");
> +	expect_pgm_int();
> +	asm volatile("mvcos	%[dst],%[src],%[len]"
> +		     : [dst] "+Q" (*(dst))
> +		     : [src] "Q" (*(src)), [len] "d" (r3), "d" (oac)

Just a nit: I think you could write "*dst" instead of "*(dst)" and
"*src" instead of "*(src)".

> +		     : "cc", "memory");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +static void test_spka(void)
> +{
> +	report_prefix_push("spka");
> +	expect_pgm_int();
> +	asm volatile("spka	0xf0(0)\n");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +static void test_tprot(void)
> +{
> +	report_prefix_push("tprot");
> +	expect_pgm_int();
> +	asm volatile("tprot	%[addr],0xf0(0)\n"
> +		     : : [addr] "a" (pagebuf) : );
> +	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> +	report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("skrf");
> +	if (!test_facility(169)) {
> +		report_skip("storage key removal facility not available\n");
> +		goto done;
> +	}
> +
> +	test_facilities();
> +	test_skey();
> +	test_pfmf();
> +	test_psw_key();
> +	test_mvcos();
> +	test_spka();
> +	test_tprot();
> +
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}

I can't say much about the technical details here (since I don't have
the doc for that "removal facility"), but apart from that, the patch
looks fine to me now.

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

(and I'll wait one or two more days for additional reviews before
queuing the patches)