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

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

On Fri, 20 Dec 2024 16:22:31 +0100
"Nico Boehr" <nrb@linux.ibm.com> wrote:

> 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.

it's a leftover, will remove

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

will do

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

I'm quite sure it does __not__ do what I want it to :D
I'll fix it

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

hmmm ok I'll do that

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

not sure what's the easiest way to share with the host, but I can just
copy the defines