Re: [kvm-unit-tests PATCH v2 1/1] s390x: pv: Add test for large host pages backing

["Christoph Schlameuss" <schlameuss@linux.ibm.com>]

Sorry for not seeing this on the first review, but there still is something.

On Mon Nov 11, 2024 at 1:15 PM CET, Claudio Imbrenda wrote:
> Add a new test to check that the host can use 1M large pages to back
> protected guests when the corresponding feature is present.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  s390x/Makefile               |   2 +
>  lib/s390x/asm/arch_def.h     |   1 +
>  lib/s390x/asm/uv.h           |  18 ++
>  s390x/pv-edat1.c             | 463 +++++++++++++++++++++++++++++++++++
>  s390x/snippets/c/pv-memhog.c |  59 +++++
>  5 files changed, 543 insertions(+)
>  create mode 100644 s390x/pv-edat1.c
>  create mode 100644 s390x/snippets/c/pv-memhog.c

[...]

> +static void timings(void)
> +{
> +	const uint64_t seqs[] = { 0, 1, 3, 7, 17, 27, 63, 127};

Missing space in the end

> +	unsigned int purge, map1m, seq;
> +
> +	report_prefix_push("timings");
> +
> +	report_info("Averages over %u iterations, in us (best/average/worst)", N_ITER);
> +	for (purge = 0; purge < 2; purge++) {
> +		report_prefix_pushf("ptlb %s", purge ? "always" : "  once");
> +		for (seq = 0; seq < ARRAY_SIZE(seqs); seq++) {
> +			if (seqs[seq])
> +				report_prefix_pushf("seq step %3lu", seqs[seq]);
> +			else
> +				report_prefix_push("pseudorandom");
> +			for (map1m = 0; map1m < 2; map1m++) {
> +				report_prefix_pushf("%s", STR(map1m));
> +				run_iterations(&vm, seqs[seq], purge, map1m);
> +				report_prefix_pop();
> +			}
> +			report_prefix_pop();
> +		}
> +		report_prefix_pop();
> +	}
> +
> +	report_pass("Timing tests successful");
> +	report_prefix_pop();
> +}

[...]

> +static void test_one_export(struct vm *vm, int pre1m, bool exportfirst, int page, bool post1m)
> +{
> +	uint64_t addr = SZ_1M + page * PAGE_SIZE;
> +	int expected = FIRST;
> +
> +	report_prefix_pushf("page %d", page);
> +
> +	map_identity_all(vm, pre1m == 1);
> +	init_snippet(vm);
> +	import_all(vm);
> +	merge_all(vm);
> +
> +	if (pre1m != -1) {
> +		sie(vm);
> +		assert_diag500_val(vm, expected);
> +		vm->save_area.guest.grs[2] = PARAM(4, 256 + 42);
> +		expected = SECOND;
> +	}
> +
> +	if (exportfirst) {
> +		export_range(vm, addr, addr + PAGE_SIZE);
> +		if (pre1m != 1)
> +			map_identity_all(vm, true);
> +	} else {
> +		if (pre1m != 1)
> +			map_identity_all(vm, true);
> +		export_range(vm, addr, addr + PAGE_SIZE);
> +	}

The tree checks here "pre1m == 1" and "pre1m != 1" do not quite add up. pre1m
is only ever called with pre1m = -1 and pre1m = 0.
With this the first of the three map_identity_all calls here will always map
normal pages and the next two calls will never happen.

> +	expect_pgm_int();
> +	sie(vm);
> +	assert(pgm_3c_addr_is(vm, MAGIC_ADDRESS));
> +
> +	import_range(vm, addr, addr + PAGE_SIZE);
> +	if (post1m) {
> +		merge_range(vm, SZ_1M, 2 * SZ_1M);
> +	} else {
> +		map_identity_all(vm, false);
> +	}
> +	sie(vm);
> +	assert_diag500_val(vm, expected);
> +	report_pass("Successful");
> +
> +	uv_destroy_guest(vm);
> +	report_prefix_pop();
> +}
> +
> +static void test_export(void)
> +{
> +	int pre1m, post1m, exportfirst;
> +
> +	report_prefix_push("export");
> +
> +	for (pre1m = -1; pre1m < 1; pre1m++) {
> +		for (post1m = 0; post1m < 2; post1m++) {
> +			for (exportfirst = 0; exportfirst < 2; exportfirst++) {
> +				report_prefix_pushf("%s pre-run, %s post-run, export %s remap",
> +						    STR3(pre1m), STR(post1m), exportfirst ? "before" : "after");
> +
> +				test_one_export(&vm, pre1m, exportfirst, 0, post1m);
> +				test_one_export(&vm, pre1m, exportfirst, 1, post1m);
> +				test_one_export(&vm, pre1m, exportfirst, 42, post1m);
> +				test_one_export(&vm, pre1m, exportfirst, 128, post1m);
> +				test_one_export(&vm, pre1m, exportfirst, 254, post1m);
> +				test_one_export(&vm, pre1m, exportfirst, 255, post1m);
> +
> +				report_prefix_pop();
> +			}
> +		}
> +	}
> +
> +	report_prefix_pop();
> +}
> +
> +static bool check_facilities(void)
> +{
> +	if (!uv_host_requirement_checks())
> +		return false;
> +	if (!test_facility(8) || !test_facility(78)) {
> +		report_skip("EDAT1 and EDAT2 not available in the host.");

s/and/or/

> +		return false;
> +	}
> +	if (!uv_query_test_call(BIT_UVC_CMD_VERIFY_LARGE_FRAME)) {
> +		report_skip("Verify Large Frame UVC not supported.");
> +		return false;
> +	}
> +	if (!uv_query_test_feature(BIT_UV_1M_BACKING)) {
> +		report_skip("Large frames not supported for Secure Execution.");
> +		return false;
> +	}
> +	return true;
> +}

[...]