Re: [kvm-unit-tests PATCH v1 2/2] s390x: add exittime tests

Thomas Huth
2022-08-30T12:52:50Z

On 30/08/2022 13.56, Nico Boehr wrote:
> Add a test to measure the execution time of several instructions. This
> can be helpful in finding performance regressions in hypervisor code.
>
> All tests are currently reported as PASS, since the baseline for their
> execution time depends on the respective environment and since needs to
> be determined on a case-by-case basis.
>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
> s390x/Makefile | 1 +
> s390x/exittime.c | 258 ++++++++++++++++++++++++++++++++++++++++++++
> s390x/unittests.cfg | 4 +
> 3 files changed, 263 insertions(+)
> create mode 100644 s390x/exittime.c
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index efd5e0c13102..5dcac244767f 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -34,6 +34,7 @@ tests += $(TEST_DIR)/migration.elf
> tests += $(TEST_DIR)/pv-attest.elf
> tests += $(TEST_DIR)/migration-cmm.elf
> tests += $(TEST_DIR)/migration-skey.elf
> +tests += $(TEST_DIR)/exittime.elf
>
> pv-tests += $(TEST_DIR)/pv-diags.elf
>
> diff --git a/s390x/exittime.c b/s390x/exittime.c
> new file mode 100644
> index 000000000000..dc8329116b77
> --- /dev/null
> +++ b/s390x/exittime.c
> @@ -0,0 +1,258 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Measure run time of various instructions. Can be used to find runtime
> + * regressions of instructions which cause exits.
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + * Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +#include <smp.h>
> +#include <sclp.h>
> +#include <asm/time.h>
> +#include <asm/sigp.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +
> +char pagebuf[PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
> +
> +static void test_sigp_sense_running(long destcpu)
> +{
> + smp_sigp(destcpu, SIGP_SENSE_RUNNING, 0, NULL);
> +}
> +
> +static void test_nop(long ignore)
> +{
> + asm volatile("nop" : : : "memory");
> +}

What's the purpose of testing "nop"? ... it does not trap to the
hypervisor... ? Is it just for reference? Then a comment might be helpful here.

> +static void test_diag9c(long destcpu)
> +{
> + asm volatile("diag %[destcpu],0,0x9c"
> + :
> + : [destcpu] "d" (destcpu)
> + :
> + );
> +}
> +
> +static long setup_get_this_cpuaddr(long ignore)
> +{
> + return stap();
> +}
> +
> +static void test_diag44(long ignore)
> +{
> + asm volatile("diag 0,0,0x44" : : : );

Drop the " : : : " please.

> +}
> +
> +static void test_stnsm(long ignore)
> +{
> + int out;
> +
> + asm volatile(
> + "stnsm %[out],0xff"
> + : [out] "=Q" (out)
> + :
> + : "memory"

I don't think you need the "memory" clobber here, do you?

> + );
> +}
> +
> +static void test_stosm(long ignore)
> +{
> + int out;
> +
> + asm volatile(
> + "stosm %[out],0"
> + : [out] "=Q" (out)
> + :
> + : "memory"

dito

> + );
> +}
> +
> +static long setup_ssm(long ignore)
> +{
> + long system_mask = 0;
> +
> + asm volatile(
> + "stosm %[system_mask],0"
> + : [system_mask] "=Q" (system_mask)
> + :
> + : "memory"
> + );
> +
> + return system_mask;
> +}
> +
> +static void test_ssm(long old_system_mask)
> +{
> + asm volatile(
> + "ssm %[old_system_mask]"
> + :
> + : [old_system_mask] "Q" (old_system_mask)
> + :
> + );
> +}
> +
> +static long setup_lctl4(long ignore)
> +{
> + long ctl4_orig = 0;
> +
> + asm volatile(
> + "stctg 4,4,%[ctl4_orig]"
> + : [ctl4_orig] "=S" (ctl4_orig)
> + :
> + : "memory"
> + );
> +
> + return ctl4_orig;
> +}
> +
> +static void test_lctl4(long ctl4_orig)
> +{
> + asm volatile(
> + "lctlg 4,4,%[ctl4_orig]"
> + :
> + : [ctl4_orig] "S" (ctl4_orig)
> + :
> + );
> +}
> +
> +static void test_stpx(long ignore)
> +{
> + unsigned int prefix;
> +
> + asm volatile(
> + "stpx %[prefix]"
> + : [prefix] "=S" (prefix)

STPX seems to have only a short displacement, so it should have "=Q" instead
of "=S" ?

> + :
> + : "memory"
> + );
> +}
> +
> +static void test_stfl(long ignore)
> +{
> + asm volatile(
> + "stfl 0" : : : "memory"
> + );
> +}
> +
> +static void test_epsw(long ignore)
> +{
> + long r1, r2;
> +
> + asm volatile(
> + "epsw %[r1], %[r2]"
> + : [r1] "=d" (r1), [r2] "=d" (r2)
> + :
> + :
> + );
> +}
> +
> +static void test_illegal(long ignore)
> +{
> + expect_pgm_int();
> + asm volatile(
> + ".word 0"
> + :
> + :
> + :
> + );
> + clear_pgm_int();
> +}
> +
> +static long setup_servc(long arg)
> +{
> + memset(pagebuf, 0, PAGE_SIZE);
> + return arg;
> +}
> +
> +static void test_servc(long ignore)
> +{
> + SCCB *sccb = (SCCB*) pagebuf;
> + sccb->h.length = 8;
> + servc(0, (unsigned long) sccb);
> +}
> +
> +static void test_stsi(long fc)
> +{
> + stsi(pagebuf, fc, 2, 2);
> +}
> +
> +struct test {
> + char name[100];

"const char *name" instead of using an array here, please. Otherwise this
wastes a lot of space in the binary.

...
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index f7b1fc3dbca1..c11d1d987c82 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -185,3 +185,7 @@ groups = migration
> [migration-skey]
> file = migration-skey.elf
> groups = migration
> +
> +[exittime]
> +file = exittime.elf
> +smp = 2

I wonder whether we should execute this test by default, since nothing can
fail here? I assume this is rather something that you want to run manually?

Thomas