[v3,0/9] RISCV: Add kvm Sstc timer selftests

  The RISC-V arch_timer selftests is used to validate Sstc timer
functionality in a guest, which sets up periodic timer interrupts
and check the basic interrupt status upon its receipt.

This KVM selftests was ported from aarch64 arch_timer and tested
with Linux v6.6-rc1 on a Qemu riscv64 virt machine.

---
Changed since v2:
  * Rebase to Linux 6.6-rc1
  * Add separate patch for kvm/Makefile improvement
  * Move aarch64 specific macros to aarch64/arch_timer.c
  * Add -DCONFIG_64BIT to kvm/Makefile CFLAGS to ensure
    only 64bit registers were available in csr.h
  * Avoid some #ifdef in kvm/arch_timer.c by setting some
    aarch64 specific variable to 0 on risc-v

Haibo Xu (9):
  KVM: selftests: Unify the codes for guest exception handling
  KVM: selftests: Unify the makefile rule for split targets
  KVM: arm64: selftests: Split arch_timer test code
  tools: riscv: Add header file csr.h
  KVM: riscv: selftests: Switch to use macro from csr.h
  KVM: riscv: selftests: Add exception handling support
  KVM: riscv: selftests: Add guest helper to get vcpu id
  KVM: riscv: selftests: Change vcpu_has_ext to a common function
  KVM: riscv: selftests: Add sstc timer test

 tools/arch/riscv/include/asm/csr.h            | 521 ++++++++++++++++++
 tools/testing/selftests/kvm/Makefile          |  14 +-
 .../selftests/kvm/aarch64/arch_timer.c        | 291 +---------
 .../selftests/kvm/aarch64/debug-exceptions.c  |   4 +-
 .../selftests/kvm/aarch64/page_fault_test.c   |   4 +-
 .../testing/selftests/kvm/aarch64/vgic_irq.c  |   4 +-
 tools/testing/selftests/kvm/arch_timer.c      | 250 +++++++++
 .../selftests/kvm/include/aarch64/processor.h |  12 +-
 .../selftests/kvm/include/kvm_util_base.h     |   9 +
 .../selftests/kvm/include/riscv/arch_timer.h  |  80 +++
 .../selftests/kvm/include/riscv/processor.h   |  63 ++-
 .../testing/selftests/kvm/include/test_util.h |   2 +
 .../selftests/kvm/include/timer_test.h        |  43 ++
 .../selftests/kvm/include/x86_64/processor.h  |   5 -
 .../selftests/kvm/lib/aarch64/processor.c     |   6 +-
 .../selftests/kvm/lib/riscv/handlers.S        | 101 ++++
 .../selftests/kvm/lib/riscv/processor.c       |  86 +++
 .../selftests/kvm/lib/x86_64/processor.c      |   4 +-
 .../testing/selftests/kvm/riscv/arch_timer.c  | 107 ++++
 .../selftests/kvm/riscv/get-reg-list.c        |  16 +-
 tools/testing/selftests/kvm/x86_64/amx_test.c |   4 +-
 .../selftests/kvm/x86_64/fix_hypercall_test.c |   4 +-
 .../selftests/kvm/x86_64/hyperv_evmcs.c       |   4 +-
 .../selftests/kvm/x86_64/hyperv_features.c    |   8 +-
 .../testing/selftests/kvm/x86_64/hyperv_ipi.c |   6 +-
 .../selftests/kvm/x86_64/kvm_pv_test.c        |   4 +-
 .../selftests/kvm/x86_64/monitor_mwait_test.c |   4 +-
 .../kvm/x86_64/pmu_event_filter_test.c        |   8 +-
 .../smaller_maxphyaddr_emulation_test.c       |   4 +-
 .../selftests/kvm/x86_64/svm_int_ctl_test.c   |   4 +-
 .../kvm/x86_64/svm_nested_shutdown_test.c     |   4 +-
 .../kvm/x86_64/svm_nested_soft_inject_test.c  |   4 +-
 .../kvm/x86_64/ucna_injection_test.c          |   8 +-
 .../kvm/x86_64/userspace_msr_exit_test.c      |   4 +-
 .../vmx_exception_with_invalid_guest_state.c  |   4 +-
 .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  |   4 +-
 .../selftests/kvm/x86_64/xapic_ipi_test.c     |   4 +-
 .../selftests/kvm/x86_64/xcr0_cpuid_test.c    |   4 +-
 .../selftests/kvm/x86_64/xen_shinfo_test.c    |   4 +-
 39 files changed, 1338 insertions(+), 374 deletions(-)
 create mode 100644 tools/arch/riscv/include/asm/csr.h
 create mode 100644 tools/testing/selftests/kvm/arch_timer.c
 create mode 100644 tools/testing/selftests/kvm/include/riscv/arch_timer.h
 create mode 100644 tools/testing/selftests/kvm/include/timer_test.h
 create mode 100644 tools/testing/selftests/kvm/lib/riscv/handlers.S
 create mode 100644 tools/testing/selftests/kvm/riscv/arch_timer.c
  

On Thu, Sep 14, 2023 at 09:36:57AM +0800, Haibo Xu wrote:
> Split the arch-neutral test code out of aarch64/arch_timer.c
> and put them into a common arch_timer.c. This is a preparation
> to share timer test codes in riscv.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |   3 +-
>  .../selftests/kvm/aarch64/arch_timer.c        | 275 +-----------------
>  tools/testing/selftests/kvm/arch_timer.c      | 248 ++++++++++++++++
>  .../testing/selftests/kvm/include/test_util.h |   2 +
>  .../selftests/kvm/include/timer_test.h        |  42 +++
>  5 files changed, 300 insertions(+), 270 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/arch_timer.c
>  create mode 100644 tools/testing/selftests/kvm/include/timer_test.h
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 7972269e8c5f..0102a0297b84 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -140,7 +140,6 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
>  TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
>  
>  TEST_GEN_PROGS_aarch64 += aarch64/aarch32_id_regs
> -TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>  TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>  TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
>  TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
> @@ -150,6 +149,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config
>  TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
>  TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
>  TEST_GEN_PROGS_aarch64 += access_tracking_perf_test
> +TEST_GEN_PROGS_aarch64 += arch_timer
>  TEST_GEN_PROGS_aarch64 += demand_paging_test
>  TEST_GEN_PROGS_aarch64 += dirty_log_test
>  TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
> @@ -188,6 +188,7 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test
>  TEST_GEN_PROGS_riscv += set_memory_region_test
>  TEST_GEN_PROGS_riscv += kvm_binary_stats_test
>  
> +SPLIT_TESTS += arch_timer
>  SPLIT_TESTS += get-reg-list
>  
>  TEST_PROGS += $(TEST_PROGS_$(ARCH_DIR))
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> index b63859829a96..4688b258247c 100644
> --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -1,64 +1,19 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * arch_timer.c - Tests the aarch64 timer IRQ functionality
> - *
>   * The test validates both the virtual and physical timer IRQs using
> - * CVAL and TVAL registers. This consitutes the four stages in the test.
> - * The guest's main thread configures the timer interrupt for a stage
> - * and waits for it to fire, with a timeout equal to the timer period.
> - * It asserts that the timeout doesn't exceed the timer period.
> - *
> - * On the other hand, upon receipt of an interrupt, the guest's interrupt
> - * handler validates the interrupt by checking if the architectural state
> - * is in compliance with the specifications.
> - *
> - * The test provides command-line options to configure the timer's
> - * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> - * To stress-test the timer stack even more, an option to migrate the
> - * vCPUs across pCPUs (-m), at a particular rate, is also provided.
> + * CVAL and TVAL registers.
>   *
>   * Copyright (c) 2021, Google LLC.
>   */
>  #define _GNU_SOURCE
>  
> -#include <stdlib.h>
> -#include <pthread.h>
> -#include <linux/kvm.h>
> -#include <linux/sizes.h>
> -#include <linux/bitmap.h>
> -#include <sys/sysinfo.h>
> -
> -#include "kvm_util.h"
> -#include "processor.h"
> -#include "delay.h"
>  #include "arch_timer.h"
> +#include "delay.h"
>  #include "gic.h"
> +#include "processor.h"
> +#include "timer_test.h"
>  #include "vgic.h"
>  
> -#define NR_VCPUS_DEF			4
> -#define NR_TEST_ITERS_DEF		5
> -#define TIMER_TEST_PERIOD_MS_DEF	10
> -#define TIMER_TEST_ERR_MARGIN_US	100
> -#define TIMER_TEST_MIGRATION_FREQ_MS	2
> -
> -struct test_args {
> -	int nr_vcpus;
> -	int nr_iter;
> -	int timer_period_ms;
> -	int migration_freq_ms;
> -	struct kvm_arm_counter_offset offset;
> -};
> -
> -static struct test_args test_args = {
> -	.nr_vcpus = NR_VCPUS_DEF,
> -	.nr_iter = NR_TEST_ITERS_DEF,
> -	.timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> -	.migration_freq_ms = TIMER_TEST_MIGRATION_FREQ_MS,
> -	.offset = { .reserved = 1 },
> -};
> -
> -#define msecs_to_usecs(msec)		((msec) * 1000LL)
> -
>  #define GICD_BASE_GPA			0x8000000ULL
>  #define GICR_BASE_GPA			0x80A0000ULL
>  
> @@ -70,22 +25,8 @@ enum guest_stage {
>  	GUEST_STAGE_MAX,
>  };
>  
> -/* Shared variables between host and guest */
> -struct test_vcpu_shared_data {
> -	int nr_iter;
> -	enum guest_stage guest_stage;
> -	uint64_t xcnt;
> -};
> -
> -static struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> -static pthread_t pt_vcpu_run[KVM_MAX_VCPUS];
> -static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
> -
>  static int vtimer_irq, ptimer_irq;
>  
> -static unsigned long *vcpu_done_map;
> -static pthread_mutex_t vcpu_done_map_lock;
> -
>  static void
>  guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
>  {
> @@ -222,137 +163,6 @@ static void guest_code(void)
>  	GUEST_DONE();
>  }
>  
> -static void *test_vcpu_run(void *arg)
> -{
> -	unsigned int vcpu_idx = (unsigned long)arg;
> -	struct ucall uc;
> -	struct kvm_vcpu *vcpu = vcpus[vcpu_idx];
> -	struct kvm_vm *vm = vcpu->vm;
> -	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[vcpu_idx];
> -
> -	vcpu_run(vcpu);
> -
> -	/* Currently, any exit from guest is an indication of completion */
> -	pthread_mutex_lock(&vcpu_done_map_lock);
> -	__set_bit(vcpu_idx, vcpu_done_map);
> -	pthread_mutex_unlock(&vcpu_done_map_lock);
> -
> -	switch (get_ucall(vcpu, &uc)) {
> -	case UCALL_SYNC:
> -	case UCALL_DONE:
> -		break;
> -	case UCALL_ABORT:
> -		sync_global_from_guest(vm, *shared_data);
> -		fprintf(stderr, "Guest assert failed,  vcpu %u; stage; %u; iter: %u\n",
> -			vcpu_idx, shared_data->guest_stage, shared_data->nr_iter);
> -		REPORT_GUEST_ASSERT(uc);
> -		break;
> -	default:
> -		TEST_FAIL("Unexpected guest exit\n");
> -	}
> -
> -	return NULL;
> -}
> -
> -static uint32_t test_get_pcpu(void)
> -{
> -	uint32_t pcpu;
> -	unsigned int nproc_conf;
> -	cpu_set_t online_cpuset;
> -
> -	nproc_conf = get_nprocs_conf();
> -	sched_getaffinity(0, sizeof(cpu_set_t), &online_cpuset);
> -
> -	/* Randomly find an available pCPU to place a vCPU on */
> -	do {
> -		pcpu = rand() % nproc_conf;
> -	} while (!CPU_ISSET(pcpu, &online_cpuset));
> -
> -	return pcpu;
> -}
> -
> -static int test_migrate_vcpu(unsigned int vcpu_idx)
> -{
> -	int ret;
> -	cpu_set_t cpuset;
> -	uint32_t new_pcpu = test_get_pcpu();
> -
> -	CPU_ZERO(&cpuset);
> -	CPU_SET(new_pcpu, &cpuset);
> -
> -	pr_debug("Migrating vCPU: %u to pCPU: %u\n", vcpu_idx, new_pcpu);
> -
> -	ret = pthread_setaffinity_np(pt_vcpu_run[vcpu_idx],
> -				     sizeof(cpuset), &cpuset);
> -
> -	/* Allow the error where the vCPU thread is already finished */
> -	TEST_ASSERT(ret == 0 || ret == ESRCH,
> -		    "Failed to migrate the vCPU:%u to pCPU: %u; ret: %d\n",
> -		    vcpu_idx, new_pcpu, ret);
> -
> -	return ret;
> -}
> -
> -static void *test_vcpu_migration(void *arg)
> -{
> -	unsigned int i, n_done;
> -	bool vcpu_done;
> -
> -	do {
> -		usleep(msecs_to_usecs(test_args.migration_freq_ms));
> -
> -		for (n_done = 0, i = 0; i < test_args.nr_vcpus; i++) {
> -			pthread_mutex_lock(&vcpu_done_map_lock);
> -			vcpu_done = test_bit(i, vcpu_done_map);
> -			pthread_mutex_unlock(&vcpu_done_map_lock);
> -
> -			if (vcpu_done) {
> -				n_done++;
> -				continue;
> -			}
> -
> -			test_migrate_vcpu(i);
> -		}
> -	} while (test_args.nr_vcpus != n_done);
> -
> -	return NULL;
> -}
> -
> -static void test_run(struct kvm_vm *vm)
> -{
> -	pthread_t pt_vcpu_migration;
> -	unsigned int i;
> -	int ret;
> -
> -	pthread_mutex_init(&vcpu_done_map_lock, NULL);
> -	vcpu_done_map = bitmap_zalloc(test_args.nr_vcpus);
> -	TEST_ASSERT(vcpu_done_map, "Failed to allocate vcpu done bitmap\n");
> -
> -	for (i = 0; i < (unsigned long)test_args.nr_vcpus; i++) {
> -		ret = pthread_create(&pt_vcpu_run[i], NULL, test_vcpu_run,
> -				     (void *)(unsigned long)i);
> -		TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
> -	}
> -
> -	/* Spawn a thread to control the vCPU migrations */
> -	if (test_args.migration_freq_ms) {
> -		srand(time(NULL));
> -
> -		ret = pthread_create(&pt_vcpu_migration, NULL,
> -					test_vcpu_migration, NULL);
> -		TEST_ASSERT(!ret, "Failed to create the migration pthread\n");
> -	}
> -
> -
> -	for (i = 0; i < test_args.nr_vcpus; i++)
> -		pthread_join(pt_vcpu_run[i], NULL);
> -
> -	if (test_args.migration_freq_ms)
> -		pthread_join(pt_vcpu_migration, NULL);
> -
> -	bitmap_free(vcpu_done_map);
> -}
> -
>  static void test_init_timer_irq(struct kvm_vm *vm)
>  {
>  	/* Timer initid should be same for all the vCPUs, so query only vCPU-0 */
> @@ -369,7 +179,7 @@ static void test_init_timer_irq(struct kvm_vm *vm)
>  
>  static int gic_fd;
>  
> -static struct kvm_vm *test_vm_create(void)
> +struct kvm_vm *test_vm_create(void)
>  {
>  	struct kvm_vm *vm;
>  	unsigned int i;
> @@ -400,81 +210,8 @@ static struct kvm_vm *test_vm_create(void)
>  	return vm;
>  }
>  
> -static void test_vm_cleanup(struct kvm_vm *vm)
> +void test_vm_cleanup(struct kvm_vm *vm)
>  {
>  	close(gic_fd);
>  	kvm_vm_free(vm);
>  }
> -
> -static void test_print_help(char *name)
> -{
> -	pr_info("Usage: %s [-h] [-n nr_vcpus] [-i iterations] [-p timer_period_ms]\n",
> -		name);
> -	pr_info("\t-n: Number of vCPUs to configure (default: %u; max: %u)\n",
> -		NR_VCPUS_DEF, KVM_MAX_VCPUS);
> -	pr_info("\t-i: Number of iterations per stage (default: %u)\n",
> -		NR_TEST_ITERS_DEF);
> -	pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
> -		TIMER_TEST_PERIOD_MS_DEF);
> -	pr_info("\t-m: Frequency (in ms) of vCPUs to migrate to different pCPU. 0 to turn off (default: %u)\n",
> -		TIMER_TEST_MIGRATION_FREQ_MS);
> -	pr_info("\t-o: Counter offset (in counter cycles, default: 0)\n");
> -	pr_info("\t-h: print this help screen\n");
> -}
> -
> -static bool parse_args(int argc, char *argv[])
> -{
> -	int opt;
> -
> -	while ((opt = getopt(argc, argv, "hn:i:p:m:o:")) != -1) {
> -		switch (opt) {
> -		case 'n':
> -			test_args.nr_vcpus = atoi_positive("Number of vCPUs", optarg);
> -			if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
> -				pr_info("Max allowed vCPUs: %u\n",
> -					KVM_MAX_VCPUS);
> -				goto err;
> -			}
> -			break;
> -		case 'i':
> -			test_args.nr_iter = atoi_positive("Number of iterations", optarg);
> -			break;
> -		case 'p':
> -			test_args.timer_period_ms = atoi_positive("Periodicity", optarg);
> -			break;
> -		case 'm':
> -			test_args.migration_freq_ms = atoi_non_negative("Frequency", optarg);
> -			break;
> -		case 'o':
> -			test_args.offset.counter_offset = strtol(optarg, NULL, 0);
> -			test_args.offset.reserved = 0;
> -			break;
> -		case 'h':
> -		default:
> -			goto err;
> -		}
> -	}
> -
> -	return true;
> -
> -err:
> -	test_print_help(argv[0]);
> -	return false;
> -}
> -
> -int main(int argc, char *argv[])
> -{
> -	struct kvm_vm *vm;
> -
> -	if (!parse_args(argc, argv))
> -		exit(KSFT_SKIP);
> -
> -	__TEST_REQUIRE(!test_args.migration_freq_ms || get_nprocs() >= 2,
> -		       "At least two physical CPUs needed for vCPU migration");
> -
> -	vm = test_vm_create();
> -	test_run(vm);
> -	test_vm_cleanup(vm);
> -
> -	return 0;
> -}
> diff --git a/tools/testing/selftests/kvm/arch_timer.c b/tools/testing/selftests/kvm/arch_timer.c
> new file mode 100644
> index 000000000000..ea3dd1a772b0
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arch_timer.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * arch_timer.c - Tests the arch timer IRQ functionality
> + *
> + * The guest's main thread configures the timer interrupt and waits
> + * for it to fire, with a timeout equal to the timer period.
> + * It asserts that the timeout doesn't exceed the timer period.

While this text is a faithful port of the original, I think we should
change it to "...exceed the timer period plus an error margin of 100 us.

(And maybe we should allow that error margin to be configured. But
making it configurable should be done in separate patches either before
or after the splitting of the test. I'm OK with leaving it
non-configurable for now. We can change it later if we want to.)

> + *
> + * On the other hand, upon receipt of an interrupt, the guest's interrupt
> + * handler validates the interrupt by checking if the architectural state
> + * is in compliance with the specifications.
> + *
> + * The test provides command-line options to configure the timer's
> + * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> + * To stress-test the timer stack even more, an option to migrate the
> + * vCPUs across pCPUs (-m), at a particular rate, is also provided.
> + *
> + * Copyright (c) 2021, Google LLC.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <stdlib.h>
> +#include <pthread.h>
> +#include <linux/sizes.h>
> +#include <linux/bitmap.h>
> +#include <sys/sysinfo.h>
> +
> +#include "timer_test.h"
> +
> +struct test_args test_args = {
> +	.nr_vcpus = NR_VCPUS_DEF,
> +	.nr_iter = NR_TEST_ITERS_DEF,
> +	.timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> +	.migration_freq_ms = TIMER_TEST_MIGRATION_FREQ_MS,
> +	.offset = { .reserved = 1 },
> +};
> +
> +struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> +struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
> +
> +static pthread_t pt_vcpu_run[KVM_MAX_VCPUS];
> +static unsigned long *vcpu_done_map;
> +static pthread_mutex_t vcpu_done_map_lock;
> +
> +static void *test_vcpu_run(void *arg)
> +{
> +	unsigned int vcpu_idx = (unsigned long)arg;
> +	struct ucall uc;
> +	struct kvm_vcpu *vcpu = vcpus[vcpu_idx];
> +	struct kvm_vm *vm = vcpu->vm;
> +	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[vcpu_idx];
> +
> +	vcpu_run(vcpu);
> +
> +	/* Currently, any exit from guest is an indication of completion */
> +	pthread_mutex_lock(&vcpu_done_map_lock);
> +	__set_bit(vcpu_idx, vcpu_done_map);
> +	pthread_mutex_unlock(&vcpu_done_map_lock);
> +
> +	switch (get_ucall(vcpu, &uc)) {
> +	case UCALL_SYNC:
> +	case UCALL_DONE:
> +		break;
> +	case UCALL_ABORT:
> +		sync_global_from_guest(vm, *shared_data);
> +		fprintf(stderr, "Guest assert failed,  vcpu %u; stage; %u; iter: %u\n",
> +			vcpu_idx, shared_data->guest_stage, shared_data->nr_iter);
> +		REPORT_GUEST_ASSERT(uc);
> +		break;
> +	default:
> +		TEST_FAIL("Unexpected guest exit\n");
> +	}
> +
> +	return NULL;
> +}
> +
> +static uint32_t test_get_pcpu(void)
> +{
> +	uint32_t pcpu;
> +	unsigned int nproc_conf;
> +	cpu_set_t online_cpuset;
> +
> +	nproc_conf = get_nprocs_conf();
> +	sched_getaffinity(0, sizeof(cpu_set_t), &online_cpuset);
> +
> +	/* Randomly find an available pCPU to place a vCPU on */
> +	do {
> +		pcpu = rand() % nproc_conf;
> +	} while (!CPU_ISSET(pcpu, &online_cpuset));
> +
> +	return pcpu;
> +}
> +
> +static int test_migrate_vcpu(unsigned int vcpu_idx)
> +{
> +	int ret;
> +	cpu_set_t cpuset;
> +	uint32_t new_pcpu = test_get_pcpu();
> +
> +	CPU_ZERO(&cpuset);
> +	CPU_SET(new_pcpu, &cpuset);
> +
> +	pr_debug("Migrating vCPU: %u to pCPU: %u\n", vcpu_idx, new_pcpu);
> +
> +	ret = pthread_setaffinity_np(pt_vcpu_run[vcpu_idx],
> +				     sizeof(cpuset), &cpuset);
> +
> +	/* Allow the error where the vCPU thread is already finished */
> +	TEST_ASSERT(ret == 0 || ret == ESRCH,
> +		    "Failed to migrate the vCPU:%u to pCPU: %u; ret: %d\n",
> +		    vcpu_idx, new_pcpu, ret);
> +
> +	return ret;
> +}
> +
> +static void *test_vcpu_migration(void *arg)
> +{
> +	unsigned int i, n_done;
> +	bool vcpu_done;
> +
> +	do {
> +		usleep(msecs_to_usecs(test_args.migration_freq_ms));
> +
> +		for (n_done = 0, i = 0; i < test_args.nr_vcpus; i++) {
> +			pthread_mutex_lock(&vcpu_done_map_lock);
> +			vcpu_done = test_bit(i, vcpu_done_map);
> +			pthread_mutex_unlock(&vcpu_done_map_lock);
> +
> +			if (vcpu_done) {
> +				n_done++;
> +				continue;
> +			}
> +
> +			test_migrate_vcpu(i);
> +		}
> +	} while (test_args.nr_vcpus != n_done);
> +
> +	return NULL;
> +}
> +
> +static void test_run(struct kvm_vm *vm)
> +{
> +	pthread_t pt_vcpu_migration;
> +	unsigned int i;
> +	int ret;
> +
> +	pthread_mutex_init(&vcpu_done_map_lock, NULL);
> +	vcpu_done_map = bitmap_zalloc(test_args.nr_vcpus);
> +	TEST_ASSERT(vcpu_done_map, "Failed to allocate vcpu done bitmap\n");
> +
> +	for (i = 0; i < (unsigned long)test_args.nr_vcpus; i++) {
> +		ret = pthread_create(&pt_vcpu_run[i], NULL, test_vcpu_run,
> +				     (void *)(unsigned long)i);
> +		TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
> +	}
> +
> +	/* Spawn a thread to control the vCPU migrations */
> +	if (test_args.migration_freq_ms) {
> +		srand(time(NULL));
> +
> +		ret = pthread_create(&pt_vcpu_migration, NULL,
> +					test_vcpu_migration, NULL);
> +		TEST_ASSERT(!ret, "Failed to create the migration pthread\n");
> +	}
> +
> +
> +	for (i = 0; i < test_args.nr_vcpus; i++)
> +		pthread_join(pt_vcpu_run[i], NULL);
> +
> +	if (test_args.migration_freq_ms)
> +		pthread_join(pt_vcpu_migration, NULL);
> +
> +	bitmap_free(vcpu_done_map);
> +}
> +
> +static void test_print_help(char *name)
> +{
> +	pr_info("Usage: %s [-h] [-n nr_vcpus] [-i iterations] [-p timer_period_ms]\n",
> +		name);
> +	pr_info("\t-n: Number of vCPUs to configure (default: %u; max: %u)\n",
> +		NR_VCPUS_DEF, KVM_MAX_VCPUS);
> +	pr_info("\t-i: Number of iterations per stage (default: %u)\n",
> +		NR_TEST_ITERS_DEF);
> +	pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
> +		TIMER_TEST_PERIOD_MS_DEF);
> +	pr_info("\t-m: Frequency (in ms) of vCPUs to migrate to different pCPU. 0 to turn off (default: %u)\n",
> +		TIMER_TEST_MIGRATION_FREQ_MS);
> +	pr_info("\t-o: Counter offset (in counter cycles, default: 0)\n");
> +	pr_info("\t-h: print this help screen\n");
> +}
> +
> +static bool parse_args(int argc, char *argv[])
> +{
> +	int opt;
> +
> +	while ((opt = getopt(argc, argv, "hn:i:p:m:o:")) != -1) {
> +		switch (opt) {
> +		case 'n':
> +			test_args.nr_vcpus = atoi_positive("Number of vCPUs", optarg);
> +			if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
> +				pr_info("Max allowed vCPUs: %u\n",
> +					KVM_MAX_VCPUS);
> +				goto err;
> +			}
> +			break;
> +		case 'i':
> +			test_args.nr_iter = atoi_positive("Number of iterations", optarg);
> +			break;
> +		case 'p':
> +			test_args.timer_period_ms = atoi_positive("Periodicity", optarg);
> +			break;
> +		case 'm':
> +			test_args.migration_freq_ms = atoi_non_negative("Frequency", optarg);
> +			break;
> +		case 'o':
> +			test_args.offset.counter_offset = strtol(optarg, NULL, 0);
> +			test_args.offset.reserved = 0;
> +			break;
> +		case 'h':
> +		default:
> +			goto err;
> +		}
> +	}
> +
> +	return true;
> +
> +err:
> +	test_print_help(argv[0]);
> +	return false;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vm *vm;
> +
> +	if (!parse_args(argc, argv))
> +		exit(KSFT_SKIP);
> +
> +	__TEST_REQUIRE(!test_args.migration_freq_ms || get_nprocs() >= 2,
> +		       "At least two physical CPUs needed for vCPU migration");
> +
> +	vm = test_vm_create();
> +	test_run(vm);
> +	test_vm_cleanup(vm);
> +
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index 7e614adc6cf4..c019675f6259 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -20,6 +20,8 @@
>  #include <sys/mman.h>
>  #include "kselftest.h"
>  
> +#define msecs_to_usecs(msec)    ((msec) * 1000LL)
> +
>  static inline int _no_printf(const char *format, ...) { return 0; }
>  
>  #ifdef DEBUG
> diff --git a/tools/testing/selftests/kvm/include/timer_test.h b/tools/testing/selftests/kvm/include/timer_test.h
> new file mode 100644
> index 000000000000..04e8aff2dc22
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/timer_test.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * timer test specific header
> + *
> + * Copyright (C) 2018, Google LLC
> + */
> +
> +#ifndef SELFTEST_KVM_TIMER_TEST_H
> +#define SELFTEST_KVM_TIMER_TEST_H
> +
> +#include "kvm_util.h"
> +
> +#define NR_VCPUS_DEF            4
> +#define NR_TEST_ITERS_DEF       5
> +#define TIMER_TEST_PERIOD_MS_DEF    10
> +#define TIMER_TEST_ERR_MARGIN_US    100
> +#define TIMER_TEST_MIGRATION_FREQ_MS    2
> +
> +/* Timer test cmdline parameters */
> +struct test_args {
> +	int nr_vcpus;
> +	int nr_iter;
> +	int timer_period_ms;
> +	int migration_freq_ms;
> +	struct kvm_arm_counter_offset offset;

nit:
I see a later patch changes this arm-specific part. This patch could add a
TODO comment here, which would get removed with the later patch.

> +};
> +
> +/* Shared variables between host and guest */
> +struct test_vcpu_shared_data {
> +	int nr_iter;
> +	int guest_stage;
> +	uint64_t xcnt;
> +};
> +
> +extern struct test_args test_args;
> +extern struct kvm_vcpu *vcpus[];
> +extern struct test_vcpu_shared_data vcpu_shared_data[];
> +
> +struct kvm_vm *test_vm_create(void);
> +void test_vm_cleanup(struct kvm_vm *vm);
> +
> +#endif /* SELFTEST_KVM_TIMER_TEST_H */
> -- 
> 2.34.1
>

Besides my comment about the test description including the error margin,
which could probably be done as a separate patch anyway since we should
modify as little as possible when moving code, then this looks good to me

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew
  

On Thu, Sep 14, 2023 at 09:37:02AM +0800, Haibo Xu wrote:
> Move vcpu_has_ext to the processor.c and rename it to __vcpu_has_ext
> so that other test cases can use it for vCPU extension check.
> 
> Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> ---
>  .../selftests/kvm/include/riscv/processor.h      |  2 ++
>  .../testing/selftests/kvm/lib/riscv/processor.c  |  9 +++++++++
>  tools/testing/selftests/kvm/riscv/get-reg-list.c | 16 +---------------
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h
> index 2c975d9cead2..7d5517648ea7 100644
> --- a/tools/testing/selftests/kvm/include/riscv/processor.h
> +++ b/tools/testing/selftests/kvm/include/riscv/processor.h
> @@ -42,6 +42,8 @@ static inline uint64_t __kvm_reg_id(uint64_t type, uint64_t idx,
>  #define RISCV_ISA_EXT_REG(idx)	__kvm_reg_id(KVM_REG_RISCV_ISA_EXT, \
>  					     idx, KVM_REG_SIZE_ULONG)
>  
> +bool __vcpu_has_ext(struct kvm_vcpu *vcpu, int ext);
> +
>  struct ex_regs {
>  	unsigned long ra;
>  	unsigned long sp;
> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> index 39a1e9902dec..e527ad0abc30 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> @@ -15,6 +15,15 @@
>  
>  static vm_vaddr_t exception_handlers;
>  
> +bool __vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> +{
> +	unsigned long value = 0;
> +
> +	__vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> +
> +	return !!value;

I'd rather not assume that value will remain zero across a system call
which fails. Let's write this as

     unsigned long value = 0;
     int ret;

     ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);

     return !ret && !!value;

> +}
> +
>  static uint64_t page_align(struct kvm_vm *vm, uint64_t v)
>  {
>  	return (v + vm->page_size) & ~(vm->page_size - 1);
> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index d8ecacd03ecf..0dcff823f287 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> @@ -44,20 +44,6 @@ bool check_reject_set(int err)
>  	return err == EINVAL;
>  }
>  
> -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> -{
> -	int ret;
> -	unsigned long value;
> -
> -	ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> -	if (ret) {
> -		printf("Failed to get ext %d", ext);
> -		return false;
> -	}
> -
> -	return !!value;
> -}
> -
>  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>  {
>  	struct vcpu_reg_sublist *s;
> @@ -77,7 +63,7 @@ void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>  		__vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(s->feature), 1);
>  
>  		/* Double check whether the desired extension was enabled */
> -		__TEST_REQUIRE(vcpu_has_ext(vcpu, s->feature),
> +		__TEST_REQUIRE(__vcpu_has_ext(vcpu, s->feature),
>  			       "%s not available, skipping tests\n", s->name);
>  	}
>  }
> -- 
> 2.34.1
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew
  

On Thu, Sep 14, 2023 at 5:05 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Sep 14, 2023 at 09:37:02AM +0800, Haibo Xu wrote:
> > Move vcpu_has_ext to the processor.c and rename it to __vcpu_has_ext
> > so that other test cases can use it for vCPU extension check.
> >
> > Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> > ---
> >  .../selftests/kvm/include/riscv/processor.h      |  2 ++
> >  .../testing/selftests/kvm/lib/riscv/processor.c  |  9 +++++++++
> >  tools/testing/selftests/kvm/riscv/get-reg-list.c | 16 +---------------
> >  3 files changed, 12 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h
> > index 2c975d9cead2..7d5517648ea7 100644
> > --- a/tools/testing/selftests/kvm/include/riscv/processor.h
> > +++ b/tools/testing/selftests/kvm/include/riscv/processor.h
> > @@ -42,6 +42,8 @@ static inline uint64_t __kvm_reg_id(uint64_t type, uint64_t idx,
> >  #define RISCV_ISA_EXT_REG(idx)       __kvm_reg_id(KVM_REG_RISCV_ISA_EXT, \
> >                                            idx, KVM_REG_SIZE_ULONG)
> >
> > +bool __vcpu_has_ext(struct kvm_vcpu *vcpu, int ext);
> > +
> >  struct ex_regs {
> >       unsigned long ra;
> >       unsigned long sp;
> > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> > index 39a1e9902dec..e527ad0abc30 100644
> > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> > @@ -15,6 +15,15 @@
> >
> >  static vm_vaddr_t exception_handlers;
> >
> > +bool __vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> > +{
> > +     unsigned long value = 0;
> > +
> > +     __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> > +
> > +     return !!value;
>
> I'd rather not assume that value will remain zero across a system call
> which fails. Let's write this as
>
>      unsigned long value = 0;
>      int ret;
>
>      ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
>
>      return !ret && !!value;
>

Sure. Thanks for the suggestion!

> > +}
> > +
> >  static uint64_t page_align(struct kvm_vm *vm, uint64_t v)
> >  {
> >       return (v + vm->page_size) & ~(vm->page_size - 1);
> > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > index d8ecacd03ecf..0dcff823f287 100644
> > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > @@ -44,20 +44,6 @@ bool check_reject_set(int err)
> >       return err == EINVAL;
> >  }
> >
> > -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> > -{
> > -     int ret;
> > -     unsigned long value;
> > -
> > -     ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> > -     if (ret) {
> > -             printf("Failed to get ext %d", ext);
> > -             return false;
> > -     }
> > -
> > -     return !!value;
> > -}
> > -
> >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> >  {
> >       struct vcpu_reg_sublist *s;
> > @@ -77,7 +63,7 @@ void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> >               __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(s->feature), 1);
> >
> >               /* Double check whether the desired extension was enabled */
> > -             __TEST_REQUIRE(vcpu_has_ext(vcpu, s->feature),
> > +             __TEST_REQUIRE(__vcpu_has_ext(vcpu, s->feature),
> >                              "%s not available, skipping tests\n", s->name);
> >       }
> >  }
> > --
> > 2.34.1
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> Thanks,
> drew