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

["Nico Boehr" <nrb@linux.ibm.com>]

On Wed Dec 18, 2024 at 2:51 PM CET, Claudio Imbrenda wrote:
[...]
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 611dcd3f..7527be48 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
[...]
> +static inline int uv_merge(uint64_t handle, unsigned long gaddr)
> +{
> +	struct uv_cb_cts uvcb = {
> +		.header.cmd = UVC_CMD_VERIFY_LARGE_FRAME,
> +		.header.len = sizeof(uvcb),
> +		.guest_handle = handle,
> +		.gaddr = gaddr,
> +	};
> +
> +	return uv_call(0, (uint64_t)&uvcb);
> +}

This function seems unused and uvc_merge() below looks very similar.

[...]

> diff --git a/s390x/pv-edat1.c b/s390x/pv-edat1.c
> new file mode 100644
> index 00000000..3f96c716
> --- /dev/null
> +++ b/s390x/pv-edat1.c
[...]
> +#define FIRST		42
> +#define SECOND		23

It was not obvious to me what these mean. It would be easier for me to
understand if they had some name like GUEST_READ_DONE_GET_PARAM and
SHOULD_EXIT_LOOP (or so) and share the define with the guest or at least have
defines with the same name in the guest (see also below).

[...]
> +static inline void assert_diag500_val(struct vm *vm, uint64_t val)
> +{
> +	assert(pv_icptdata_check_diag(vm, 0x500));
> +	assert(vm->save_area.guest.grs[2] == val);
> +}

I would appreciate it if you could base on Ninas STFLE series and use
snippet_check_force_exit_value() here. See
https://lore.kernel.org/kvm/20240620141700.4124157-6-nsg@linux.ibm.com/
See also below.

[...]
> +static void test_run(void)
> +{
> +	int init1m, import1m, merge, run1m;
> +
> +	report_prefix_push("test run");
> +
> +	for (init1m = 0; init1m < 1; init1m++) {

Are you sure this does what you want it to do?

[...]
> +static void test_merge(void)
> +{
> +	uint64_t tmp, mem;
> +	int cc;
> +
> +	report_prefix_push("merge");
> +	init_snippet(&vm);
> +
> +	mem = guest_start(&vm);
> +
> +	map_identity_all(&vm, false);
> +	install_page(root, mem + 0x101000, (void *)(mem + 0x102000));
> +	install_page(root, mem + 0x102000, (void *)(mem + 0x101000));
> +	install_page(root, mem + 0x205000, (void *)(mem + 0x305000));

(see below)

[...]
> +	/* Not all pages are aligned correctly */
> +	report(uvc_merge(&vm, mem + 0x100000) == 0x104, "Pages not consecutive");
> +	report(uvc_merge(&vm, mem + 0x200000) == 0x104, "Pages not in the same 1M frame");

It would be easier for me to understand if the regions were named, e.g. with a
variable for each region, for example:

uint64_t non_consecutive = mem + 0x100000

and then above

install_page(root, mem + 0x101000, (void *)(non_consecutive + 0x2000));
install_page(root, mem + 0x102000, (void *)(non_consecutive + 0x1000));

[...]
> diff --git a/s390x/snippets/c/pv-memhog.c b/s390x/snippets/c/pv-memhog.c
> new file mode 100644
> index 00000000..43f0c2b1
> --- /dev/null
> +++ b/s390x/snippets/c/pv-memhog.c
> @@ -0,0 +1,59 @@
[...]
> +int main(void)
> +{
> +	uint64_t param, addr, i, n;
> +
> +	READ_ONCE(*MIDPAGE_PTR(SZ_1M + 42 * PAGE_SIZE));
> +	param = get_value(42);

(see below)

> +	n = (param >> 32) & 0x1fffffff;
> +	n = n ? n : N_PAGES;
> +	param &= 0x7fffffff;
> +
> +	while (true) {
> +		for (i = 0; i < n; i++) {
> +			addr = ((param ? i * param : i * i * i) * PAGE_SIZE) & MASK_2G;
> +			WRITE_ONCE(*MIDPAGE_PTR(addr), addr);
> +		}
> +
> +		i = get_value(23);
> +		if (i != 42)

I would like some defines for 23 and 42 and possibly share them with the host.