Message-ID: <20240821174434.2317a378@p-imbrenda.boeblingen.de.ibm.com>
Date: Wed, 21 Aug 2024 17:44:34 +0200
From: Claudio Imbrenda <imbrenda@...ux.ibm.com>
To: Hariharan Mari <hari55@...ux.ibm.com>
Cc: linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, shuah@...nel.org, frankja@...ux.ibm.com,
        borntraeger@...ux.ibm.com, david@...hat.com, pbonzini@...hat.com,
        schlameuss@...ux.ibm.com
Subject: Re: [PATCH v2 1/5] KVM: s390: selftests: Add regression tests for
 SORTL and DFLTCC CPU subfunctions

On Tue, 20 Aug 2024 08:48:33 +0200
Hariharan Mari <hari55@...ux.ibm.com> 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@...ux.ibm.com>
> Signed-off-by: Hariharan Mari <hari55@...ux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@...ux.ibm.com>
> ---
>  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@...ux.ibm.com>
> + *
> + *  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@...ux.ibm.com>
> + *
> + * 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();
> +}

Powered by blists - more mailing lists