On Tue, 20 Aug 2024 08:48:33 +0200
Hariharan Mari <hari55@xxxxxxxxxxxxx> wrote:
> Introduce new regression tests to verify the ASM inline block in the SORTL
> and DFLTCC CPU subfunctions for the s390x architecture. These tests ensure
> that future changes to the ASM code are properly validated.
>
> The test procedure:
>
> 1. Create a VM and request the KVM_S390_VM_CPU_MACHINE_SUBFUNC attribute
> from the KVM_S390_VM_CPU_MODEL group for this VM. This SUBFUNC attribute
> contains the results of all CPU subfunction instructions.
> 2. For each tested subfunction (SORTL and DFLTCC), execute the
> corresponding ASM instruction and capture the result array.
> 3. Perform a memory comparison between the results stored in the SUBFUNC
> attribute (obtained in step 1) and the ASM instruction results (obtained
> in step 2) for each tested subfunction.
>
> This process ensures that the KVM implementation accurately reflects the
> behavior of the actual CPU instructions for the tested subfunctions.
>
> Suggested-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
> Signed-off-by: Hariharan Mari <hari55@xxxxxxxxxxxxx>
> Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
> ---
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/include/s390x/facility.h | 50 ++++++++
> .../kvm/s390x/cpumodel_subfuncs_test.c | 115 ++++++++++++++++++
> 3 files changed, 166 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/include/s390x/facility.h
> create mode 100644 tools/testing/selftests/kvm/s390x/cpumodel_subfuncs_test.c
>
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index ac280dcba996..9f418c594b55 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -183,6 +183,7 @@ TEST_GEN_PROGS_s390x += s390x/sync_regs_test
> TEST_GEN_PROGS_s390x += s390x/tprot
> TEST_GEN_PROGS_s390x += s390x/cmma_test
> TEST_GEN_PROGS_s390x += s390x/debug_test
> +TEST_GEN_PROGS_s390x += s390x/cpumodel_subfuncs_test
> TEST_GEN_PROGS_s390x += s390x/shared_zeropage_test
> TEST_GEN_PROGS_s390x += demand_paging_test
> TEST_GEN_PROGS_s390x += dirty_log_test
> diff --git a/tools/testing/selftests/kvm/include/s390x/facility.h b/tools/testing/selftests/kvm/include/s390x/facility.h
> new file mode 100644
> index 000000000000..65eef9a722ba
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/s390x/facility.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright IBM Corp. 2024
> + *
> + * Authors:
> + * Hariharan Mari <hari55@xxxxxxxxxxxxx>
> + *
> + * Get the facility bits with the STFLE instruction
> + */
> +
> +#ifndef SELFTEST_KVM_FACILITY_H
> +#define SELFTEST_KVM_FACILITY_H
> +
> +#include <linux/bitops.h>
> +
> +#define NB_STFL_DOUBLEWORDS 32 /* alt_stfle_fac_list[16] + stfle_fac_list[16] */
in general it would be better if the comment is before the variable or
define
> +
> +uint64_t stfl_doublewords[NB_STFL_DOUBLEWORDS];
> +bool stfle_flag;
I'm not very happy of global variables defined in headers; as we
discussed offline, there are several possible solutions:
* just make them static
* put them in a separate .c in lib/s390x/
* do not use them at all, and simply call stfle every time
and probably there are also more solutions I did not think of
> +
> +static inline bool test_bit_inv(unsigned long nr,
> + const unsigned long *ptr)
if I'm not mistaken, the column limit is 100 also for the selftests;
please fix that throughout the series, it will be more readable
> +{
> + return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}
> +
> +static inline void stfle(uint64_t *fac, unsigned int nb_doublewords)
> +{
> + register unsigned long r0 asm("0") = nb_doublewords - 1;
> +
> + asm volatile(" .insn s,0xb2b00000,0(%1)\n"
> + : "+d" (r0)
> + : "a" (fac)
> + : "memory", "cc");
> +}
> +
> +static inline void setup_facilities(void)
> +{
> + stfle(stfl_doublewords, NB_STFL_DOUBLEWORDS);
> + stfle_flag = true;
> +}
> +
> +static inline bool test_facility(int nr)
> +{
> + if (!stfle_flag)
> + setup_facilities();
> + return test_bit_inv(nr, stfl_doublewords);
> +}
> +
> +#endif
> diff --git a/tools/testing/selftests/kvm/s390x/cpumodel_subfuncs_test.c b/tools/testing/selftests/kvm/s390x/cpumodel_subfuncs_test.c
> new file mode 100644
> index 000000000000..ea03ce2010bb
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/s390x/cpumodel_subfuncs_test.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright IBM Corp. 2024
> + *
> + * Authors:
> + * Hariharan Mari <hari55@xxxxxxxxxxxxx>
> + *
> + * The tests compare the result of the KVM ioctl for obtaining CPU subfunction
> + * data with those from an ASM block performing the same CPU subfunction.
> + * Currently KVM doesn't mask instruction query data reported via the CPU Model,
> + * allowing us to directly compare it with the data acquired through executing
> + * the queries in the test.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include "facility.h"
> +
> +#include "kvm_util.h"
> +
> +/**
> + * Query available CPU subfunctions
> + */
> +struct kvm_s390_vm_cpu_subfunc cpu_subfunc;
> +
> +static void get_cpu_machine_subfuntions(struct kvm_vm *vm,
> + struct kvm_s390_vm_cpu_subfunc
> + *cpu_subfunc)
here in particular you can see how ugly it looks with 80 columns :)
> +{
> + int r;
> +
> + r = __kvm_device_attr_get(vm->fd, KVM_S390_VM_CPU_MODEL,
> + KVM_S390_VM_CPU_MACHINE_SUBFUNC, cpu_subfunc);
> +
> + TEST_ASSERT(!r, "Get cpu subfunctions failed r=%d errno=%d", r, errno);
> +}
> +
> +/*
> + * Testing Sort Lists (SORTL) CPU subfunction's ASM block
> + */
> +static void test_sortl_asm_block(u8 (*query)[32])
> +{
> + asm volatile(" lghi 0,0\n"
> + " la 1,%[query]\n"
> + " .insn rre,0xb9380000,2,4\n"
> + : [query] "=R" (*query)
> + :
> + : "cc", "0", "1");
> +}
> +
> +/*
> + * Testing Deflate Conversion Call (DFLTCC) CPU subfunction's ASM block
> + */
there is no need to use multiline comments for a single line, unless
you want to use proper kdoc tags (which is definitely overkill for a
selftest)
> +static void test_dfltcc_asm_block(u8 (*query)[32])
> +{
> + asm volatile(" lghi 0,0\n"
> + " la 1,%[query]\n"
> + " .insn rrf,0xb9390000,2,4,6,0\n"
> + : [query] "=R" (*query)
> + :
> + : "cc", "0", "1");
> +}
> +
> +typedef void (*testfunc_t)(u8 (*array)[]);
> +
> +struct testdef {
> + const char *subfunc_name;
> + u8 *subfunc_array;
> + size_t array_size;
> + testfunc_t test;
> + int facility_bit;
> +} testlist[] = {
> + /* SORTL - Facility bit 150 */
> + { "SORTL", cpu_subfunc.sortl, sizeof(cpu_subfunc.sortl),
> + test_sortl_asm_block, 150 },
here as well, I think it fits in one line
> + /* DFLTCC - Facility bit 151 */
> + { "DFLTCC", cpu_subfunc.dfltcc, sizeof(cpu_subfunc.dfltcc),
> + test_dfltcc_asm_block, 151 },
> +};
> +
> +int main(int argc, char *argv[])
> +{
> + struct kvm_vm *vm;
> + int idx;
> +
> + ksft_print_header();
> +
> + vm = vm_create(1);
> +
> + memset(&cpu_subfunc, 0, sizeof(cpu_subfunc));
> + get_cpu_machine_subfuntions(vm, &cpu_subfunc);
> +
> + ksft_set_plan(ARRAY_SIZE(testlist));
> + for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
> + if (test_facility(testlist[idx].facility_bit)) {
> + u8 *array = malloc(testlist[idx].array_size);
> +
> + testlist[idx].test((u8 (*)[testlist[idx].array_size])array);
> +
> + TEST_ASSERT_EQ(memcmp(testlist[idx].subfunc_array,
> + array, testlist[idx].array_size), 0);
> +
> + ksft_test_result_pass("%s\n", testlist[idx].subfunc_name);
> + free(array);
> + } else {
> + ksft_test_result_skip("%s feature is not avaialable\n",
> + testlist[idx].subfunc_name);
> + }
> + }
> +
> + kvm_vm_free(vm);
> + ksft_finished();
> +}
