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

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

On Tue, 19 Nov 2024 16:48:37 +0100
"Christoph Schlameuss" <schlameuss@linux.ibm.com> wrote:

> 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

will fix

> [...]
> 
> > +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.

yeah, because the loop used to go to 1, and I don't remember why I
changed it... maybe it should actually go all the way to 1 again...

> 
> > +	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/

will fix differently

> 
> > +		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;
> > +}  
> 
> [...]
>