Re: [kvm-unit-tests PATCH 2/5] s390x: Add guest 2 AP test

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 30 Mar 2023 11:42:41 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Add a test that checks the exceptions for the PQAP, NQAP and DQAP
> adjunct processor (AP) crypto instructions.
> 
> Since triggering the exceptions doesn't require actual AP hardware,
> this test can run without complicated setup.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---

[...]

> +
> +static void test_pgms_pqap(void)
> +{
> +	unsigned long grs[3] = {};
> +	struct pqap_r0 *r0 = (struct pqap_r0 *)grs;
> +	uint8_t *data = alloc_page();
> +	uint16_t pgm;
> +	int fails = 0;
> +	int i;
> +
> +	report_prefix_push("pqap");
> +
> +	/* Wrong FC code */
> +	report_prefix_push("invalid fc");
> +	r0->fc = 42;

maybe make a macro out of it, both to avoid magic numbers and to change
it easily if code 42 will ever become defined in the future.

> +	expect_pgm_int();
> +	pqap(grs);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	memset(grs, 0, sizeof(grs));
> +	report_prefix_pop();
> +
> +	report_prefix_push("invalid gr0 bits");
> +	for (i = 42; i < 6; i++) {

42 is not < 6, this whole thing will be skipped?

> +		expect_pgm_int();
> +		grs[0] = BIT(63 - i);
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", i);
> +			fails++;
> +		}
> +	}
> +	report(!fails, "All bits tested");
> +	memset(grs, 0, sizeof(grs));
> +	fails = 0;
> +	report_prefix_pop();
> +
> +	report_prefix_push("alignment");
> +	report_prefix_push("fc=4");
> +	r0->fc = PQAP_QUERY_AP_CONF_INFO;
> +	grs[2] = (unsigned long)data;
> +	for (i = 1; i < 8; i++) {
> +		expect_pgm_int();
> +		grs[2]++;
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", i);
> +			fails++;
> +		}
> +	}
> +	report(!fails, "All bits tested");

you mean "All alignments tested" ?

> +	report_prefix_pop();
> +	report_prefix_push("fc=6");
> +	r0->fc = PQAP_BEST_AP;
> +	grs[2] = (unsigned long)data;
> +	for (i = 1; i < 8; i++) {
> +		expect_pgm_int();
> +		grs[2]++;
> +		pqap(grs);
> +		pgm = clear_pgm_int();
> +		if (pgm != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("fail on bit %d", i);
> +			fails++;
> +		}
> +	}
> +	report(!fails, "All bits tested");

same here?

> +	report_prefix_pop();
> +	report_prefix_pop();
> +
> +	free_page(data);
> +	report_prefix_pop();
> +}
> +
> +static void test_pgms_nqap(void)
> +{
> +	uint8_t gr0_zeroes_bits[] = {
> +		32, 34, 35, 40
> +	};
> +	uint64_t gr0;
> +	bool fail;
> +	int i;
> +
> +	report_prefix_push("nqap");
> +
> +	/* Registers 0 and 1 are always used, the others are
> even/odd pairs */
> +	report_prefix_push("spec");
> +	report_prefix_push("r1");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,3,6\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");

I would say 
"0", "1", "2", "3", "4", "6", "7"

since there are two ways of doing it wrong when it comes to even-odd
register pairs (r and r+1, r&~1 and r&~1+1)

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("r2");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,2,7\n"
> +		: : : "cc", "memory", "0", "1", "3", "4");

same here (with the right numbers of course)

and I would even add a test with both odd registers

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len==0");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	0,0\n"
> +		"xgr	5,5\n"
> +		".insn	rre,0xb2ad0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len>12288");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	5,5\n"
> +		"lghi	5, 12289\n"

I would also check setting all the bits above bit 13 (i.e. if an
implementation wrongly checks only the lower 16 or 32 bits of the value

> +		".insn	rre,0xb2ad0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("gr0_zero_bits");
> +	fail = false;
> +	for (i = 0; i < 4; i++) {

i < ARRAY_SIZE(gr0_zeroes_bits) might be more robust and future-proof

> +		expect_pgm_int();
> +		gr0 = BIT_ULL(63 - gr0_zeroes_bits[i]);
> +		asm volatile (
> +			"xgr	5,5\n"
> +			"lghi	5, 128\n"
> +			"lg	0, 0(%[val])\n"
> +			".insn	rre,0xb2ad0000,2,4\n"
> +			: : [val] "a" (&gr0)
> +			: "cc", "memory", "0", "1", "2", "3", "4", "5");
> +		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
> +			report_fail("setting gr0 bit %d did not result in a spec exception",
> +				    gr0_zeroes_bits[i]);
> +			fail = true;
> +		}
> +	}
> +	report(!fail, "set bit specification pgms");
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
> +static void test_pgms_dqap(void)
> +{
> +	uint8_t gr0_zeroes_bits[] = {
> +		33, 34, 35, 40, 41
> +	};
> +	uint64_t gr0;
> +	bool fail;
> +	int i;
> +
> +	report_prefix_push("dqap");
> +
> +	/* Registers 0 and 1 are always used, the others are even/odd pairs */
> +	report_prefix_push("spec");
> +	report_prefix_push("r1");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,3,6\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");

same concern here with the registers like in the previous test

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("r2");
> +	expect_pgm_int();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,2,7\n"
> +		: : : "cc", "memory", "0", "1", "3", "4");

as above, plus add a test for both odd

> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len==0");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	0,0\n"
> +		"xgr	5,5\n"
> +		".insn	rre,0xb2ae0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("len>12288");
> +	expect_pgm_int();
> +	asm volatile (
> +		"xgr	5,5\n"
> +		"lghi	5, 12289\n"

like the previous test, also test the high bits

> +		".insn	rre,0xb2ae0000,2,4\n"
> +		: : : "cc", "memory", "0", "1", "2", "3", "4", "5");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("gr0_zero_bits");
> +	fail = false;
> +	for (i = 0; i < 5; i++) {

same concern here with ARRAY_SIZE

> +		expect_pgm_int();
> +		gr0 = BIT_ULL(63 - gr0_zeroes_bits[i]);
> +		asm volatile (
> +			"xgr	5,5\n"
> +			"lghi	5, 128\n"
> +			"lg	0, 0(%[val])\n"
> +			".insn	rre,0xb2ae0000,2,4\n"
> +			: : [val] "a" (&gr0)
> +			: "cc", "memory", "0", "1", "2", "3", "4", "5");
> +		if (clear_pgm_int() != PGM_INT_CODE_SPECIFICATION) {
> +			report_info("setting gr0 bit %d did not result in a spec exception",
> +				    gr0_zeroes_bits[i]);
> +			fail = true;
> +		}
> +	}
> +	report(!fail, "set bit specification pgms");
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
> +static void test_priv(void)
> +{
> +	struct ap_config_info info = {};
> +
> +	report_prefix_push("privileged");
> +
> +	report_prefix_push("pqap");
> +	expect_pgm_int();
> +	enter_pstate();
> +	ap_pqap_qci(&info);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	/*
> +	 * Enqueue and dequeue take too many registers so a simple
> +	 * inline assembly makes more sense than using the library
> +	 * functions.
> +	 */
> +	report_prefix_push("nqap");
> +	expect_pgm_int();
> +	enter_pstate();
> +	asm volatile (
> +		".insn	rre,0xb2ad0000,0,2\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("dqap");
> +	expect_pgm_int();
> +	enter_pstate();
> +	asm volatile (
> +		".insn	rre,0xb2ae0000,0,2\n"
> +		: : : "cc", "memory", "0", "1", "2", "3");
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("ap");
> +	if (!ap_check()) {
> +		report_skip("AP instructions not available");
> +		goto done;
> +	}
> +
> +	test_priv();
> +	test_pgms_pqap();
> +	test_pgms_nqap();
> +	test_pgms_dqap();
> +
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index d97eb5e9..9b7c65c8 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -215,3 +215,7 @@ file = migration-skey.elf
>  smp = 2
>  groups = migration
>  extra_params = -append '--parallel'
> +
> +[ap]
> +file = ap.elf
> +