On Fri, 20 Dec 2024 16:22:31 +0100
"Nico Boehr" <nrb@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx/
> 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
