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