On Sat, Sep 02, 2023 at 08:59:24PM +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 | 9 +- .../selftests/kvm/aarch64/arch_timer.c | 288 +----------------- tools/testing/selftests/kvm/arch_timer.c | 252 +++++++++++++++ .../selftests/kvm/include/timer_test.h | 52 ++++ 4 files changed, 317 insertions(+), 284 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 0b9c42fbce8c..fb8904e2c06a 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 += set_memory_region_test TEST_GEN_PROGS_riscv += kvm_binary_stats_test SPLIT_TESTS += get-reg-list +SPLIT_TESTS += arch_timer TEST_PROGS += $(TEST_PROGS_$(ARCH_DIR)) TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH_DIR)) @@ -248,13 +249,10 @@ TEST_DEP_FILES += $(patsubst %.o, %.d, $(SPLIT_TESTS_OBJS)) -include $(TEST_DEP_FILES) $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): %: %.o

• $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $< $(LIBKVM_OBJS) $(LDLIBS) -o $@

• $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $^ $(LDLIBS) -o $@

$(TEST_GEN_OBJ): $(OUTPUT)/%.o: %.c $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@ -$(SPLIT_TESTS_TARGETS): %: %.o $(SPLIT_TESTS_OBJS)

• $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $^ $(LDLIBS) -o $@

EXTRA_CLEAN += $(LIBKVM_OBJS) $(TEST_DEP_FILES) $(TEST_GEN_OBJ) $(SPLIT_TESTS_OBJS) cscope.* x := $(shell mkdir -p $(sort $(dir $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)))) @@ -273,6 +271,7 @@ $(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS)))) $(TEST_GEN_PROGS): $(LIBKVM_OBJS) $(TEST_GEN_PROGS_EXTENDED): $(LIBKVM_OBJS) +$(SPLIT_TESTS_TARGETS): $(OUTPUT)/%: $(ARCH_DIR)/%.o

The improvements to the Makefile to avoid SPLIT_TESTS_TARGETS needing its own $(CC) line should preferably be done in a separate, preliminary patch.

cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib .. cscope: diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c index b63859829a96..ceb649548751 100644 --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c @@ -1,91 +1,25 @@ // 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

-enum guest_stage {

• GUEST_STAGE_VTIMER_CVAL = 1,
• GUEST_STAGE_VTIMER_TVAL,
• GUEST_STAGE_PTIMER_CVAL,
• GUEST_STAGE_PTIMER_TVAL,
• 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]; +extern struct test_args test_args; +extern struct kvm_vcpu *vcpus[]; +extern struct test_vcpu_shared_data vcpu_shared_data[]; 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 +156,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 +172,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 +203,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..529024f58c98 --- /dev/null +++ b/tools/testing/selftests/kvm/arch_timer.c @@ -0,0 +1,252 @@ +// 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 for and waits

It looks like the text was edited to remove the 'stage' references, which is fine by me, but the 'for' should have also been removed.

• • 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.
• • 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,

+#ifdef __aarch64__

• .offset = { .reserved = 1 },

+#endif

Please run checkpatch, there are spaces instead of tabs in the struct.

+};

+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");
  

• }
• pr_info("PASS(vCPU-%d).\n", vcpu_idx);

This is new. I can live with it, but generally we don't want to modify functions while moving them.

• 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/timer_test.h b/tools/testing/selftests/kvm/include/timer_test.h new file mode 100644 index 000000000000..109e4d635627 --- /dev/null +++ b/tools/testing/selftests/kvm/include/timer_test.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • tools/testing/selftests/kvm/include/timer_test.h
• • 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

+#define msecs_to_usecs(msec) ((msec) * 1000LL)

I'd move the above to include/test_util.h

+#define GICD_BASE_GPA 0x8000000ULL +#define GICR_BASE_GPA 0x80A0000ULL

These defines belong in aarch64/arch_timer.c

+enum guest_stage {

• GUEST_STAGE_VTIMER_CVAL=1,
• GUEST_STAGE_VTIMER_TVAL,
• GUEST_STAGE_PTIMER_CVAL,
• GUEST_STAGE_PTIMER_TVAL,
• GUEST_STAGE_MAX,

+};

This enum also belongs in aarch64/arch_timer.c

+/* 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;

+};

+/* Shared variables between host and guest */ +struct test_vcpu_shared_data {

• int nr_iter;
• enum guest_stage guest_stage;
• uint64_t xcnt;

+};

+struct kvm_vm* test_vm_create(void);

Move * next to function name.

+void test_vm_cleanup(struct kvm_vm *vm);

+#endif /* SELFTEST_KVM_TIMER_TEST_H */

2.34.1

Thanks, drew

On Wed, Sep 6, 2023 at 10:14 AM Haibo Xu xiaobo55x@gmail.com wrote:

On Mon, Sep 4, 2023 at 9:24 PM Andrew Jones ajones@ventanamicro.com wrote:

...

On Sat, Sep 02, 2023 at 08:59:24PM +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 | 9 +- .../selftests/kvm/aarch64/arch_timer.c | 288 +----------------- tools/testing/selftests/kvm/arch_timer.c | 252 +++++++++++++++ .../selftests/kvm/include/timer_test.h | 52 ++++ 4 files changed, 317 insertions(+), 284 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 0b9c42fbce8c..fb8904e2c06a 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 += set_memory_region_test TEST_GEN_PROGS_riscv += kvm_binary_stats_test

SPLIT_TESTS += get-reg-list +SPLIT_TESTS += arch_timer

TEST_PROGS += $(TEST_PROGS_$(ARCH_DIR)) TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH_DIR)) @@ -248,13 +249,10 @@ TEST_DEP_FILES += $(patsubst %.o, %.d, $(SPLIT_TESTS_OBJS)) -include $(TEST_DEP_FILES)

$(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): %: %.o

• $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $< $(LIBKVM_OBJS) $(LDLIBS) -o $@
  

• $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $^ $(LDLIBS) -o $@
  

$(TEST_GEN_OBJ): $(OUTPUT)/%.o: %.c $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@

-$(SPLIT_TESTS_TARGETS): %: %.o $(SPLIT_TESTS_OBJS)

• $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $^ $(LDLIBS) -o $@
  

EXTRA_CLEAN += $(LIBKVM_OBJS) $(TEST_DEP_FILES) $(TEST_GEN_OBJ) $(SPLIT_TESTS_OBJS) cscope.*

x := $(shell mkdir -p $(sort $(dir $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)))) @@ -273,6 +271,7 @@ $(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS)))) $(TEST_GEN_PROGS): $(LIBKVM_OBJS) $(TEST_GEN_PROGS_EXTENDED): $(LIBKVM_OBJS) +$(SPLIT_TESTS_TARGETS): $(OUTPUT)/%: $(ARCH_DIR)/%.o

The improvements to the Makefile to avoid SPLIT_TESTS_TARGETS needing its own $(CC) line should preferably be done in a separate, preliminary patch.

Yes, Will move the change to a separate patch.

...

...

cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib .. cscope: diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c index b63859829a96..ceb649548751 100644 --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c @@ -1,91 +1,25 @@ // 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

-enum guest_stage {

• GUEST_STAGE_VTIMER_CVAL = 1,
  

• GUEST_STAGE_VTIMER_TVAL,
  

• GUEST_STAGE_PTIMER_CVAL,
  

• GUEST_STAGE_PTIMER_TVAL,
  

• 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]; +extern struct test_args test_args; +extern struct kvm_vcpu *vcpus[]; +extern struct test_vcpu_shared_data vcpu_shared_data[];

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 +156,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 +172,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 +203,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..529024f58c98 --- /dev/null +++ b/tools/testing/selftests/kvm/arch_timer.c @@ -0,0 +1,252 @@ +// 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 for and waits

It looks like the text was edited to remove the 'stage' references, which is fine by me, but the 'for' should have also been removed.

Sure!

...

...

• • 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.
• • 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,

+#ifdef __aarch64__

• .offset = { .reserved = 1 },

+#endif

Please run checkpatch, there are spaces instead of tabs in the struct.

Yes, the tabs were changed to spaces while copying. Will change it and run checkpatch to catch this kind of error in next version.

...

...

+};

+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");
  

• }
  

• pr_info("PASS(vCPU-%d).\n", vcpu_idx);
  

This is new. I can live with it, but generally we don't want to modify functions while moving them.

Yes, this change was supposed to go with patch 8/8.

...

...

• 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/timer_test.h b/tools/testing/selftests/kvm/include/timer_test.h new file mode 100644 index 000000000000..109e4d635627 --- /dev/null +++ b/tools/testing/selftests/kvm/include/timer_test.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • tools/testing/selftests/kvm/include/timer_test.h
• • 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

+#define msecs_to_usecs(msec) ((msec) * 1000LL)

I'd move the above to include/test_util.h

Yes, msecs_to_usecs() macro should be a common API for all the tests.

...

...

+#define GICD_BASE_GPA 0x8000000ULL +#define GICR_BASE_GPA 0x80A0000ULL

These defines belong in aarch64/arch_timer.c

These 2 defines were also defined in other test cases, shall we move them to an aarch64 specific header file? Maybe tools/testing/selftests/kvm/include/aarch64/gic.h?

...

...

+enum guest_stage {

• GUEST_STAGE_VTIMER_CVAL=1,
  

• GUEST_STAGE_VTIMER_TVAL,
  

• GUEST_STAGE_PTIMER_CVAL,
  

• GUEST_STAGE_PTIMER_TVAL,
  

• GUEST_STAGE_MAX,
  

+};

This enum also belongs in aarch64/arch_timer.c

Yes, it should be in aarch64/arch_timer.c

After moving the above enum definition to aarch64/arch_timer.c, the below errors was reported while compiling kvm/arch_timer.o

include/timer_test.h:37:26: error: field ‘guest_stage’ has incomplete type 37 | enum guest_stage guest_stage; | ^~~~~~~~~~~

Since kvm/arch_timer.c was independent of kvm/aarch64/arch_timer.c during OBJ compiling, I think it may be not possible to move the enum definition to aarch64/arch_timer.c

If we keep the definition in this header file, we can enclose it with #ifdef __aarch64__ for aarch64 only.

...

...

+/* 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;
  

+};

+/* Shared variables between host and guest */ +struct test_vcpu_shared_data {

• int nr_iter;
  

• enum guest_stage guest_stage;
  

• uint64_t xcnt;
  

+};

+struct kvm_vm* test_vm_create(void);

Move * next to function name.

Sure, thanks!

...

...

+void test_vm_cleanup(struct kvm_vm *vm);

+#endif /* SELFTEST_KVM_TIMER_TEST_H */

2.34.1

Thanks, drew

On Mon, Sep 4, 2023 at 9:33 PM Andrew Jones ajones@ventanamicro.com wrote:

On Sat, Sep 02, 2023 at 08:59:25PM +0800, Haibo Xu wrote:

...

Borrow the csr definitions and operations from kernel's arch/riscv/include/asm/csr.h to tools/ for riscv.

Signed-off-by: Haibo Xu haibo1.xu@intel.com

tools/arch/riscv/include/asm/csr.h | 521 +++++++++++++++++++++++++++++ 1 file changed, 521 insertions(+) create mode 100644 tools/arch/riscv/include/asm/csr.h

diff --git a/tools/arch/riscv/include/asm/csr.h b/tools/arch/riscv/include/asm/csr.h new file mode 100644 index 000000000000..4e86c82aacbd --- /dev/null +++ b/tools/arch/riscv/include/asm/csr.h @@ -0,0 +1,521 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Copyright (C) 2015 Regents of the University of California
• */

+#ifndef _ASM_RISCV_CSR_H +#define _ASM_RISCV_CSR_H

+#include <linux/bits.h>

+/* Status register flags */ +#define SR_SIE _AC(0x00000002, UL) /* Supervisor Interrupt Enable */ +#define SR_MIE _AC(0x00000008, UL) /* Machine Interrupt Enable */ +#define SR_SPIE _AC(0x00000020, UL) /* Previous Supervisor IE */ +#define SR_MPIE _AC(0x00000080, UL) /* Previous Machine IE */ +#define SR_SPP _AC(0x00000100, UL) /* Previously Supervisor */ +#define SR_MPP _AC(0x00001800, UL) /* Previously Machine */ +#define SR_SUM _AC(0x00040000, UL) /* Supervisor User Memory Access */

+#define SR_FS _AC(0x00006000, UL) /* Floating-point Status */ +#define SR_FS_OFF _AC(0x00000000, UL) +#define SR_FS_INITIAL _AC(0x00002000, UL) +#define SR_FS_CLEAN _AC(0x00004000, UL) +#define SR_FS_DIRTY _AC(0x00006000, UL)

+#define SR_VS _AC(0x00000600, UL) /* Vector Status */ +#define SR_VS_OFF _AC(0x00000000, UL) +#define SR_VS_INITIAL _AC(0x00000200, UL) +#define SR_VS_CLEAN _AC(0x00000400, UL) +#define SR_VS_DIRTY _AC(0x00000600, UL)

+#define SR_XS _AC(0x00018000, UL) /* Extension Status */ +#define SR_XS_OFF _AC(0x00000000, UL) +#define SR_XS_INITIAL _AC(0x00008000, UL) +#define SR_XS_CLEAN _AC(0x00010000, UL) +#define SR_XS_DIRTY _AC(0x00018000, UL)

+#define SR_FS_VS (SR_FS | SR_VS) /* Vector and Floating-Point Unit */

+#ifndef CONFIG_64BIT

How do we ensure CONFIG_64BIT is set?

Currently, no explicit checking for this. Shall we add a gatekeeper in this file to ensure it is set?

#ifndef CONFIG_64BIT #error "CONFIG_64BIT was not set" #endif

Thanks, drew

...

+#define SR_SD _AC(0x80000000, UL) /* FS/VS/XS dirty */ +#else +#define SR_SD _AC(0x8000000000000000, UL) /* FS/VS/XS dirty */ +#endif

+#ifdef CONFIG_64BIT +#define SR_UXL _AC(0x300000000, UL) /* XLEN mask for U-mode */ +#define SR_UXL_32 _AC(0x100000000, UL) /* XLEN = 32 for U-mode */ +#define SR_UXL_64 _AC(0x200000000, UL) /* XLEN = 64 for U-mode */ +#endif

+/* SATP flags */ +#ifndef CONFIG_64BIT +#define SATP_PPN _AC(0x003FFFFF, UL) +#define SATP_MODE_32 _AC(0x80000000, UL) +#define SATP_ASID_BITS 9 +#define SATP_ASID_SHIFT 22 +#define SATP_ASID_MASK _AC(0x1FF, UL) +#else +#define SATP_PPN _AC(0x00000FFFFFFFFFFF, UL) +#define SATP_MODE_39 _AC(0x8000000000000000, UL) +#define SATP_MODE_48 _AC(0x9000000000000000, UL) +#define SATP_MODE_57 _AC(0xa000000000000000, UL) +#define SATP_ASID_BITS 16 +#define SATP_ASID_SHIFT 44 +#define SATP_ASID_MASK _AC(0xFFFF, UL) +#endif

+/* Exception cause high bit - is an interrupt if set */ +#define CAUSE_IRQ_FLAG (_AC(1, UL) << (__riscv_xlen - 1))

+/* Interrupt causes (minus the high bit) */ +#define IRQ_S_SOFT 1 +#define IRQ_VS_SOFT 2 +#define IRQ_M_SOFT 3 +#define IRQ_S_TIMER 5 +#define IRQ_VS_TIMER 6 +#define IRQ_M_TIMER 7 +#define IRQ_S_EXT 9 +#define IRQ_VS_EXT 10 +#define IRQ_M_EXT 11 +#define IRQ_S_GEXT 12 +#define IRQ_PMU_OVF 13 +#define IRQ_LOCAL_MAX (IRQ_PMU_OVF + 1) +#define IRQ_LOCAL_MASK GENMASK((IRQ_LOCAL_MAX - 1), 0)

+/* Exception causes */ +#define EXC_INST_MISALIGNED 0 +#define EXC_INST_ACCESS 1 +#define EXC_INST_ILLEGAL 2 +#define EXC_BREAKPOINT 3 +#define EXC_LOAD_MISALIGNED 4 +#define EXC_LOAD_ACCESS 5 +#define EXC_STORE_MISALIGNED 6 +#define EXC_STORE_ACCESS 7 +#define EXC_SYSCALL 8 +#define EXC_HYPERVISOR_SYSCALL 9 +#define EXC_SUPERVISOR_SYSCALL 10 +#define EXC_INST_PAGE_FAULT 12 +#define EXC_LOAD_PAGE_FAULT 13 +#define EXC_STORE_PAGE_FAULT 15 +#define EXC_INST_GUEST_PAGE_FAULT 20 +#define EXC_LOAD_GUEST_PAGE_FAULT 21 +#define EXC_VIRTUAL_INST_FAULT 22 +#define EXC_STORE_GUEST_PAGE_FAULT 23

+/* PMP configuration */ +#define PMP_R 0x01 +#define PMP_W 0x02 +#define PMP_X 0x04 +#define PMP_A 0x18 +#define PMP_A_TOR 0x08 +#define PMP_A_NA4 0x10 +#define PMP_A_NAPOT 0x18 +#define PMP_L 0x80

+/* HSTATUS flags */ +#ifdef CONFIG_64BIT +#define HSTATUS_VSXL _AC(0x300000000, UL) +#define HSTATUS_VSXL_SHIFT 32 +#endif +#define HSTATUS_VTSR _AC(0x00400000, UL) +#define HSTATUS_VTW _AC(0x00200000, UL) +#define HSTATUS_VTVM _AC(0x00100000, UL) +#define HSTATUS_VGEIN _AC(0x0003f000, UL) +#define HSTATUS_VGEIN_SHIFT 12 +#define HSTATUS_HU _AC(0x00000200, UL) +#define HSTATUS_SPVP _AC(0x00000100, UL) +#define HSTATUS_SPV _AC(0x00000080, UL) +#define HSTATUS_GVA _AC(0x00000040, UL) +#define HSTATUS_VSBE _AC(0x00000020, UL)

+/* HGATP flags */ +#define HGATP_MODE_OFF _AC(0, UL) +#define HGATP_MODE_SV32X4 _AC(1, UL) +#define HGATP_MODE_SV39X4 _AC(8, UL) +#define HGATP_MODE_SV48X4 _AC(9, UL) +#define HGATP_MODE_SV57X4 _AC(10, UL)

+#define HGATP32_MODE_SHIFT 31 +#define HGATP32_VMID_SHIFT 22 +#define HGATP32_VMID GENMASK(28, 22) +#define HGATP32_PPN GENMASK(21, 0)

+#define HGATP64_MODE_SHIFT 60 +#define HGATP64_VMID_SHIFT 44 +#define HGATP64_VMID GENMASK(57, 44) +#define HGATP64_PPN GENMASK(43, 0)

+#define HGATP_PAGE_SHIFT 12

+#ifdef CONFIG_64BIT +#define HGATP_PPN HGATP64_PPN +#define HGATP_VMID_SHIFT HGATP64_VMID_SHIFT +#define HGATP_VMID HGATP64_VMID +#define HGATP_MODE_SHIFT HGATP64_MODE_SHIFT +#else +#define HGATP_PPN HGATP32_PPN +#define HGATP_VMID_SHIFT HGATP32_VMID_SHIFT +#define HGATP_VMID HGATP32_VMID +#define HGATP_MODE_SHIFT HGATP32_MODE_SHIFT +#endif

+/* VSIP & HVIP relation */ +#define VSIP_TO_HVIP_SHIFT (IRQ_VS_SOFT - IRQ_S_SOFT) +#define VSIP_VALID_MASK ((_AC(1, UL) << IRQ_S_SOFT) | \

•                          (_AC(1, UL) << IRQ_S_TIMER) | \
  

•                          (_AC(1, UL) << IRQ_S_EXT))
  

+/* AIA CSR bits */ +#define TOPI_IID_SHIFT 16 +#define TOPI_IID_MASK GENMASK(11, 0) +#define TOPI_IPRIO_MASK GENMASK(7, 0) +#define TOPI_IPRIO_BITS 8

+#define TOPEI_ID_SHIFT 16 +#define TOPEI_ID_MASK GENMASK(10, 0) +#define TOPEI_PRIO_MASK GENMASK(10, 0)

+#define ISELECT_IPRIO0 0x30 +#define ISELECT_IPRIO15 0x3f +#define ISELECT_MASK GENMASK(8, 0)

+#define HVICTL_VTI BIT(30) +#define HVICTL_IID GENMASK(27, 16) +#define HVICTL_IID_SHIFT 16 +#define HVICTL_DPR BIT(9) +#define HVICTL_IPRIOM BIT(8) +#define HVICTL_IPRIO GENMASK(7, 0)

+/* xENVCFG flags */ +#define ENVCFG_STCE (_AC(1, ULL) << 63) +#define ENVCFG_PBMTE (_AC(1, ULL) << 62) +#define ENVCFG_CBZE (_AC(1, UL) << 7) +#define ENVCFG_CBCFE (_AC(1, UL) << 6) +#define ENVCFG_CBIE_SHIFT 4 +#define ENVCFG_CBIE (_AC(0x3, UL) << ENVCFG_CBIE_SHIFT) +#define ENVCFG_CBIE_ILL _AC(0x0, UL) +#define ENVCFG_CBIE_FLUSH _AC(0x1, UL) +#define ENVCFG_CBIE_INV _AC(0x3, UL) +#define ENVCFG_FIOM _AC(0x1, UL)

+/* symbolic CSR names: */ +#define CSR_CYCLE 0xc00 +#define CSR_TIME 0xc01 +#define CSR_INSTRET 0xc02 +#define CSR_HPMCOUNTER3 0xc03 +#define CSR_HPMCOUNTER4 0xc04 +#define CSR_HPMCOUNTER5 0xc05 +#define CSR_HPMCOUNTER6 0xc06 +#define CSR_HPMCOUNTER7 0xc07 +#define CSR_HPMCOUNTER8 0xc08 +#define CSR_HPMCOUNTER9 0xc09 +#define CSR_HPMCOUNTER10 0xc0a +#define CSR_HPMCOUNTER11 0xc0b +#define CSR_HPMCOUNTER12 0xc0c +#define CSR_HPMCOUNTER13 0xc0d +#define CSR_HPMCOUNTER14 0xc0e +#define CSR_HPMCOUNTER15 0xc0f +#define CSR_HPMCOUNTER16 0xc10 +#define CSR_HPMCOUNTER17 0xc11 +#define CSR_HPMCOUNTER18 0xc12 +#define CSR_HPMCOUNTER19 0xc13 +#define CSR_HPMCOUNTER20 0xc14 +#define CSR_HPMCOUNTER21 0xc15 +#define CSR_HPMCOUNTER22 0xc16 +#define CSR_HPMCOUNTER23 0xc17 +#define CSR_HPMCOUNTER24 0xc18 +#define CSR_HPMCOUNTER25 0xc19 +#define CSR_HPMCOUNTER26 0xc1a +#define CSR_HPMCOUNTER27 0xc1b +#define CSR_HPMCOUNTER28 0xc1c +#define CSR_HPMCOUNTER29 0xc1d +#define CSR_HPMCOUNTER30 0xc1e +#define CSR_HPMCOUNTER31 0xc1f +#define CSR_CYCLEH 0xc80 +#define CSR_TIMEH 0xc81 +#define CSR_INSTRETH 0xc82 +#define CSR_HPMCOUNTER3H 0xc83 +#define CSR_HPMCOUNTER4H 0xc84 +#define CSR_HPMCOUNTER5H 0xc85 +#define CSR_HPMCOUNTER6H 0xc86 +#define CSR_HPMCOUNTER7H 0xc87 +#define CSR_HPMCOUNTER8H 0xc88 +#define CSR_HPMCOUNTER9H 0xc89 +#define CSR_HPMCOUNTER10H 0xc8a +#define CSR_HPMCOUNTER11H 0xc8b +#define CSR_HPMCOUNTER12H 0xc8c +#define CSR_HPMCOUNTER13H 0xc8d +#define CSR_HPMCOUNTER14H 0xc8e +#define CSR_HPMCOUNTER15H 0xc8f +#define CSR_HPMCOUNTER16H 0xc90 +#define CSR_HPMCOUNTER17H 0xc91 +#define CSR_HPMCOUNTER18H 0xc92 +#define CSR_HPMCOUNTER19H 0xc93 +#define CSR_HPMCOUNTER20H 0xc94 +#define CSR_HPMCOUNTER21H 0xc95 +#define CSR_HPMCOUNTER22H 0xc96 +#define CSR_HPMCOUNTER23H 0xc97 +#define CSR_HPMCOUNTER24H 0xc98 +#define CSR_HPMCOUNTER25H 0xc99 +#define CSR_HPMCOUNTER26H 0xc9a +#define CSR_HPMCOUNTER27H 0xc9b +#define CSR_HPMCOUNTER28H 0xc9c +#define CSR_HPMCOUNTER29H 0xc9d +#define CSR_HPMCOUNTER30H 0xc9e +#define CSR_HPMCOUNTER31H 0xc9f

+#define CSR_SSCOUNTOVF 0xda0

+#define CSR_SSTATUS 0x100 +#define CSR_SIE 0x104 +#define CSR_STVEC 0x105 +#define CSR_SCOUNTEREN 0x106 +#define CSR_SSCRATCH 0x140 +#define CSR_SEPC 0x141 +#define CSR_SCAUSE 0x142 +#define CSR_STVAL 0x143 +#define CSR_SIP 0x144 +#define CSR_SATP 0x180

+#define CSR_STIMECMP 0x14D +#define CSR_STIMECMPH 0x15D

+/* Supervisor-Level Window to Indirectly Accessed Registers (AIA) */ +#define CSR_SISELECT 0x150 +#define CSR_SIREG 0x151

+/* Supervisor-Level Interrupts (AIA) */ +#define CSR_STOPEI 0x15c +#define CSR_STOPI 0xdb0

+/* Supervisor-Level High-Half CSRs (AIA) */ +#define CSR_SIEH 0x114 +#define CSR_SIPH 0x154

+#define CSR_VSSTATUS 0x200 +#define CSR_VSIE 0x204 +#define CSR_VSTVEC 0x205 +#define CSR_VSSCRATCH 0x240 +#define CSR_VSEPC 0x241 +#define CSR_VSCAUSE 0x242 +#define CSR_VSTVAL 0x243 +#define CSR_VSIP 0x244 +#define CSR_VSATP 0x280 +#define CSR_VSTIMECMP 0x24D +#define CSR_VSTIMECMPH 0x25D

+#define CSR_HSTATUS 0x600 +#define CSR_HEDELEG 0x602 +#define CSR_HIDELEG 0x603 +#define CSR_HIE 0x604 +#define CSR_HTIMEDELTA 0x605 +#define CSR_HCOUNTEREN 0x606 +#define CSR_HGEIE 0x607 +#define CSR_HENVCFG 0x60a +#define CSR_HTIMEDELTAH 0x615 +#define CSR_HENVCFGH 0x61a +#define CSR_HTVAL 0x643 +#define CSR_HIP 0x644 +#define CSR_HVIP 0x645 +#define CSR_HTINST 0x64a +#define CSR_HGATP 0x680 +#define CSR_HGEIP 0xe12

+/* Virtual Interrupts and Interrupt Priorities (H-extension with AIA) */ +#define CSR_HVIEN 0x608 +#define CSR_HVICTL 0x609 +#define CSR_HVIPRIO1 0x646 +#define CSR_HVIPRIO2 0x647

+/* VS-Level Window to Indirectly Accessed Registers (H-extension with AIA) */ +#define CSR_VSISELECT 0x250 +#define CSR_VSIREG 0x251

+/* VS-Level Interrupts (H-extension with AIA) */ +#define CSR_VSTOPEI 0x25c +#define CSR_VSTOPI 0xeb0

+/* Hypervisor and VS-Level High-Half CSRs (H-extension with AIA) */ +#define CSR_HIDELEGH 0x613 +#define CSR_HVIENH 0x618 +#define CSR_HVIPH 0x655 +#define CSR_HVIPRIO1H 0x656 +#define CSR_HVIPRIO2H 0x657 +#define CSR_VSIEH 0x214 +#define CSR_VSIPH 0x254

+#define CSR_MSTATUS 0x300 +#define CSR_MISA 0x301 +#define CSR_MIDELEG 0x303 +#define CSR_MIE 0x304 +#define CSR_MTVEC 0x305 +#define CSR_MENVCFG 0x30a +#define CSR_MENVCFGH 0x31a +#define CSR_MSCRATCH 0x340 +#define CSR_MEPC 0x341 +#define CSR_MCAUSE 0x342 +#define CSR_MTVAL 0x343 +#define CSR_MIP 0x344 +#define CSR_PMPCFG0 0x3a0 +#define CSR_PMPADDR0 0x3b0 +#define CSR_MVENDORID 0xf11 +#define CSR_MARCHID 0xf12 +#define CSR_MIMPID 0xf13 +#define CSR_MHARTID 0xf14

+/* Machine-Level Window to Indirectly Accessed Registers (AIA) */ +#define CSR_MISELECT 0x350 +#define CSR_MIREG 0x351

+/* Machine-Level Interrupts (AIA) */ +#define CSR_MTOPEI 0x35c +#define CSR_MTOPI 0xfb0

+/* Virtual Interrupts for Supervisor Level (AIA) */ +#define CSR_MVIEN 0x308 +#define CSR_MVIP 0x309

+/* Machine-Level High-Half CSRs (AIA) */ +#define CSR_MIDELEGH 0x313 +#define CSR_MIEH 0x314 +#define CSR_MVIENH 0x318 +#define CSR_MVIPH 0x319 +#define CSR_MIPH 0x354

+#define CSR_VSTART 0x8 +#define CSR_VCSR 0xf +#define CSR_VL 0xc20 +#define CSR_VTYPE 0xc21 +#define CSR_VLENB 0xc22

+#ifdef CONFIG_RISCV_M_MODE +# define CSR_STATUS CSR_MSTATUS +# define CSR_IE CSR_MIE +# define CSR_TVEC CSR_MTVEC +# define CSR_SCRATCH CSR_MSCRATCH +# define CSR_EPC CSR_MEPC +# define CSR_CAUSE CSR_MCAUSE +# define CSR_TVAL CSR_MTVAL +# define CSR_IP CSR_MIP

+# define CSR_IEH CSR_MIEH +# define CSR_ISELECT CSR_MISELECT +# define CSR_IREG CSR_MIREG +# define CSR_IPH CSR_MIPH +# define CSR_TOPEI CSR_MTOPEI +# define CSR_TOPI CSR_MTOPI

+# define SR_IE SR_MIE +# define SR_PIE SR_MPIE +# define SR_PP SR_MPP

+# define RV_IRQ_SOFT IRQ_M_SOFT +# define RV_IRQ_TIMER IRQ_M_TIMER +# define RV_IRQ_EXT IRQ_M_EXT +#else /* CONFIG_RISCV_M_MODE */ +# define CSR_STATUS CSR_SSTATUS +# define CSR_IE CSR_SIE +# define CSR_TVEC CSR_STVEC +# define CSR_SCRATCH CSR_SSCRATCH +# define CSR_EPC CSR_SEPC +# define CSR_CAUSE CSR_SCAUSE +# define CSR_TVAL CSR_STVAL +# define CSR_IP CSR_SIP

+# define CSR_IEH CSR_SIEH +# define CSR_ISELECT CSR_SISELECT +# define CSR_IREG CSR_SIREG +# define CSR_IPH CSR_SIPH +# define CSR_TOPEI CSR_STOPEI +# define CSR_TOPI CSR_STOPI

+# define SR_IE SR_SIE +# define SR_PIE SR_SPIE +# define SR_PP SR_SPP

+# define RV_IRQ_SOFT IRQ_S_SOFT +# define RV_IRQ_TIMER IRQ_S_TIMER +# define RV_IRQ_EXT IRQ_S_EXT +# define RV_IRQ_PMU IRQ_PMU_OVF +# define SIP_LCOFIP (_AC(0x1, UL) << IRQ_PMU_OVF)

+#endif /* !CONFIG_RISCV_M_MODE */

+/* IE/IP (Supervisor/Machine Interrupt Enable/Pending) flags */ +#define IE_SIE (_AC(0x1, UL) << RV_IRQ_SOFT) +#define IE_TIE (_AC(0x1, UL) << RV_IRQ_TIMER) +#define IE_EIE (_AC(0x1, UL) << RV_IRQ_EXT)

+#ifdef __ASSEMBLY__ +#define __ASM_STR(x) x +#else +#define __ASM_STR(x) #x +#endif

+#ifndef __ASSEMBLY__

+#define csr_swap(csr, val) \ +({ \

• unsigned long __v = (unsigned long)(val);               \
  

• __asm__ __volatile__ ("csrrw %0, " __ASM_STR(csr) ", %1"\
  

•                       : "=r" (__v) : "rK" (__v)         \
  

•                       : "memory");                      \
  

• __v;                                                    \
  

+})

+#define csr_read(csr) \ +({ \

• register unsigned long __v;                             \
  

• __asm__ __volatile__ ("csrr %0, " __ASM_STR(csr)        \
  

•                       : "=r" (__v) :                    \
  

•                       : "memory");                      \
  

• __v;                                                    \
  

+})

+#define csr_write(csr, val) \ +({ \

• unsigned long __v = (unsigned long)(val);               \
  

• __asm__ __volatile__ ("csrw " __ASM_STR(csr) ", %0"     \
  

•                       : : "rK" (__v)                    \
  

•                       : "memory");                      \
  

+})

+#define csr_read_set(csr, val) \ +({ \

• unsigned long __v = (unsigned long)(val);               \
  

• __asm__ __volatile__ ("csrrs %0, " __ASM_STR(csr) ", %1"\
  

•                       : "=r" (__v) : "rK" (__v)         \
  

•                       : "memory");                      \
  

• __v;                                                    \
  

+})

+#define csr_set(csr, val) \ +({ \

• unsigned long __v = (unsigned long)(val);               \
  

• __asm__ __volatile__ ("csrs " __ASM_STR(csr) ", %0"     \
  

•                       : : "rK" (__v)                    \
  

•                       : "memory");                      \
  

+})

+#define csr_read_clear(csr, val) \ +({ \

• unsigned long __v = (unsigned long)(val);               \
  

• __asm__ __volatile__ ("csrrc %0, " __ASM_STR(csr) ", %1"\
  

•                       : "=r" (__v) : "rK" (__v)         \
  

•                       : "memory");                      \
  

• __v;                                                    \
  

+})

+#define csr_clear(csr, val) \ +({ \

• unsigned long __v = (unsigned long)(val);               \
  

• __asm__ __volatile__ ("csrc " __ASM_STR(csr) ", %0"     \
  

•                       : : "rK" (__v)                    \
  

•                       : "memory");                      \
  

+})

+#endif /* __ASSEMBLY__ */

+#endif /* _ASM_RISCV_CSR_H */

2.34.1

On Wed, Sep 6, 2023 at 3:13 PM Andrew Jones ajones@ventanamicro.com wrote:

On Wed, Sep 06, 2023 at 02:35:42PM +0800, Haibo Xu wrote:

...

On Mon, Sep 4, 2023 at 9:33 PM Andrew Jones ajones@ventanamicro.com wrote:

...

On Sat, Sep 02, 2023 at 08:59:25PM +0800, Haibo Xu wrote:

...

Borrow the csr definitions and operations from kernel's arch/riscv/include/asm/csr.h to tools/ for riscv.

Signed-off-by: Haibo Xu haibo1.xu@intel.com

tools/arch/riscv/include/asm/csr.h | 521 +++++++++++++++++++++++++++++ 1 file changed, 521 insertions(+) create mode 100644 tools/arch/riscv/include/asm/csr.h

diff --git a/tools/arch/riscv/include/asm/csr.h b/tools/arch/riscv/include/asm/csr.h new file mode 100644 index 000000000000..4e86c82aacbd --- /dev/null +++ b/tools/arch/riscv/include/asm/csr.h @@ -0,0 +1,521 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Copyright (C) 2015 Regents of the University of California
• */

+#ifndef _ASM_RISCV_CSR_H +#define _ASM_RISCV_CSR_H

+#include <linux/bits.h>

+/* Status register flags */ +#define SR_SIE _AC(0x00000002, UL) /* Supervisor Interrupt Enable */ +#define SR_MIE _AC(0x00000008, UL) /* Machine Interrupt Enable */ +#define SR_SPIE _AC(0x00000020, UL) /* Previous Supervisor IE */ +#define SR_MPIE _AC(0x00000080, UL) /* Previous Machine IE */ +#define SR_SPP _AC(0x00000100, UL) /* Previously Supervisor */ +#define SR_MPP _AC(0x00001800, UL) /* Previously Machine */ +#define SR_SUM _AC(0x00040000, UL) /* Supervisor User Memory Access */

+#define SR_FS _AC(0x00006000, UL) /* Floating-point Status */ +#define SR_FS_OFF _AC(0x00000000, UL) +#define SR_FS_INITIAL _AC(0x00002000, UL) +#define SR_FS_CLEAN _AC(0x00004000, UL) +#define SR_FS_DIRTY _AC(0x00006000, UL)

+#define SR_VS _AC(0x00000600, UL) /* Vector Status */ +#define SR_VS_OFF _AC(0x00000000, UL) +#define SR_VS_INITIAL _AC(0x00000200, UL) +#define SR_VS_CLEAN _AC(0x00000400, UL) +#define SR_VS_DIRTY _AC(0x00000600, UL)

+#define SR_XS _AC(0x00018000, UL) /* Extension Status */ +#define SR_XS_OFF _AC(0x00000000, UL) +#define SR_XS_INITIAL _AC(0x00008000, UL) +#define SR_XS_CLEAN _AC(0x00010000, UL) +#define SR_XS_DIRTY _AC(0x00018000, UL)

+#define SR_FS_VS (SR_FS | SR_VS) /* Vector and Floating-Point Unit */

+#ifndef CONFIG_64BIT

How do we ensure CONFIG_64BIT is set?

Currently, no explicit checking for this. Shall we add a gatekeeper in this file to ensure it is set?

Not in this file, since this file is shared by all the tools and...

...

#ifndef CONFIG_64BIT #error "CONFIG_64BIT was not set" #endif

...we'll surely hit this error right now since nothing is setting CONFIG_64BIT when compiling KVM selftests.

We need to define CONFIG_64BIT in the build somewhere prior to any headers which depend on it being included. Maybe we can simply add -DCONFIG_64BIT to CFLAGS, since all KVM selftests supported architectures are 64-bit.

Make sense! Another option can be just add "#define CONFIG_64BIT" at the begin of csr.h

(Please trim emails, as I've been doing, when discussing specific parts.)

Sure, thanks for the suggestions!

Thanks, drew

On Mon, Sep 4, 2023 at 9:31 PM Andrew Jones ajones@ventanamicro.com wrote:

On Sat, Sep 02, 2023 at 08:59:26PM +0800, Haibo Xu wrote:

...

Signed-off-by: Haibo Xu haibo1.xu@intel.com

tools/testing/selftests/kvm/include/riscv/processor.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h index 5b62a3d2aa9b..6810c887fadc 100644 --- a/tools/testing/selftests/kvm/include/riscv/processor.h +++ b/tools/testing/selftests/kvm/include/riscv/processor.h @@ -8,6 +8,7 @@ #define SELFTEST_KVM_PROCESSOR_H

#include "kvm_util.h" +#include <asm/csr.h> #include <linux/stringify.h>

static inline uint64_t __kvm_reg_id(uint64_t type, uint64_t idx, @@ -95,12 +96,8 @@ static inline uint64_t __kvm_reg_id(uint64_t type, uint64_t idx, #define PGTBL_PAGE_SIZE PGTBL_L0_BLOCK_SIZE #define PGTBL_PAGE_SIZE_SHIFT PGTBL_L0_BLOCK_SHIFT

-#define SATP_PPN _AC(0x00000FFFFFFFFFFF, UL) #define SATP_MODE_39 _AC(0x8000000000000000, UL) #define SATP_MODE_48 _AC(0x9000000000000000, UL)

SATP_MODE_39/48 are also in csr.h

Good catch! These 2 should also be removed. Will fix it in the next version.

...

-#define SATP_ASID_BITS 16 -#define SATP_ASID_SHIFT 44 -#define SATP_ASID_MASK _AC(0xFFFF, UL)

#define SBI_EXT_EXPERIMENTAL_START 0x08000000

#define SBI_EXT_EXPERIMENTAL_END 0x08FFFFFF

2.34.1

Thanks, drew

On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote:

Move vcpu_has_ext to the processor.c 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 ++ tools/testing/selftests/kvm/lib/riscv/processor.c | 9 +++++++++ tools/testing/selftests/kvm/riscv/get-reg-list.c | 14 -------------- 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h index d1e5d9f7ad45..6087c8fc263a 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..5ececa566f24 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;

+}

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..c4028bf32e3f 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;

get-reg-list will now assert on get-reg when an extension isn't present, rather than failing the __TEST_REQUIRE(), which would do a skip instead. We need both the return false version and the assert version.

-}

void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c) { struct vcpu_reg_sublist *s; -- 2.34.1

Thanks, drew

On Wed, Sep 6, 2023 at 6:10 PM Haibo Xu xiaobo55x@gmail.com wrote:

On Mon, Sep 4, 2023 at 10:04 PM Andrew Jones ajones@ventanamicro.com wrote:

...

On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote:

...

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c index d8ecacd03ecf..c4028bf32e3f 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;
  

get-reg-list will now assert on get-reg when an extension isn't present, rather than failing the __TEST_REQUIRE(), which would do a skip instead. We need both the return false version and the assert version.

Ok, Will keep this one for get-reg-list and add another one for arch-timer specific usage.

Just thought about it again, maybe we only need the "return false" version for both get-reg-list and arch-timer tests since if an extension was not available, the test can be skipped with a message.

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; }

...

...

-}

void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c) { struct vcpu_reg_sublist *s; -- 2.34.1

Thanks, drew

On Thu, Sep 7, 2023 at 5:01 PM Andrew Jones ajones@ventanamicro.com wrote:

On Thu, Sep 07, 2023 at 11:57:00AM +0800, Haibo Xu wrote:

...

On Wed, Sep 6, 2023 at 6:10 PM Haibo Xu xiaobo55x@gmail.com wrote:

...

On Mon, Sep 4, 2023 at 10:04 PM Andrew Jones ajones@ventanamicro.com wrote:

...

On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote:

...

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c index d8ecacd03ecf..c4028bf32e3f 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;
  

get-reg-list will now assert on get-reg when an extension isn't present, rather than failing the __TEST_REQUIRE(), which would do a skip instead. We need both the return false version and the assert version.

Ok, Will keep this one for get-reg-list and add another one for arch-timer specific usage.

Just thought about it again, maybe we only need the "return false" version for both get-reg-list and arch-timer tests since if an extension was not available, the test can be skipped with a message.

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;

}

Yup, I had actually seen that when reviewing a later patch in this series, but I wasn't concerned if we added the assert type anyway, since we frequently end up with the two function types for KVM queries. If we don't have a need for an assert type yet, then we don't need to introduce it. However, we should introduce the non-assert type as __vcpu_has_ext(), reserving the vcpu_has_ext() name for the assert type, per the kvm selftests naming convention.

Sure, thanks!

Thanks, drew

On Sat, Sep 02, 2023 at 08:59:30PM +0800, Haibo Xu wrote:

Add a KVM selftest to validate the Sstc timer functionality. The test was ported from arm64 arch timer test.

Signed-off-by: Haibo Xu haibo1.xu@intel.com

tools/testing/selftests/kvm/Makefile | 1 + tools/testing/selftests/kvm/arch_timer.c | 9 ++ .../selftests/kvm/include/riscv/arch_timer.h | 80 +++++++++++ .../selftests/kvm/include/riscv/processor.h | 10 ++ .../selftests/kvm/include/timer_test.h | 6 + .../testing/selftests/kvm/riscv/arch_timer.c | 130 ++++++++++++++++++ 6 files changed, 236 insertions(+) create mode 100644 tools/testing/selftests/kvm/include/riscv/arch_timer.h create mode 100644 tools/testing/selftests/kvm/riscv/arch_timer.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 01638027d059..7897d8051d8c 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -179,6 +179,7 @@ TEST_GEN_PROGS_s390x += rseq_test TEST_GEN_PROGS_s390x += set_memory_region_test TEST_GEN_PROGS_s390x += kvm_binary_stats_test +TEST_GEN_PROGS_riscv += arch_timer TEST_GEN_PROGS_riscv += demand_paging_test TEST_GEN_PROGS_riscv += dirty_log_test TEST_GEN_PROGS_riscv += get-reg-list diff --git a/tools/testing/selftests/kvm/arch_timer.c b/tools/testing/selftests/kvm/arch_timer.c index 529024f58c98..691bd454e362 100644 --- a/tools/testing/selftests/kvm/arch_timer.c +++ b/tools/testing/selftests/kvm/arch_timer.c @@ -66,8 +66,13 @@ static void *test_vcpu_run(void *arg) break; case UCALL_ABORT: sync_global_from_guest(vm, *shared_data); +#ifdef __aarch64__ fprintf(stderr, "Guest assert failed, vcpu %u; stage; %u; iter: %u\n", vcpu_idx, shared_data->guest_stage, shared_data->nr_iter); +#else

• fprintf(stderr, "Guest assert failed,  vcpu %u; iter: %u\n",
  

•         vcpu_idx, shared_data->nr_iter);
  

+#endif

We can avoid this #ifdef by just letting guest_stage output as zero for riscv.

REPORT_GUEST_ASSERT(uc);
break;

default: @@ -190,7 +195,9 @@ static void test_print_help(char *name) 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); +#ifdef __aarch64__ pr_info("\t-o: Counter offset (in counter cycles, default: 0)\n"); +#endif

We can avoid this and the next #ifdef by changing this pr_info to

"\t-o: Counter offset (in counter cycles, default: 0) [aarch64-only]\n"

pr_info("\t-h: print this help screen\n"); } @@ -217,10 +224,12 @@ static bool parse_args(int argc, char *argv[]) case 'm': test_args.migration_freq_ms = atoi_non_negative("Frequency", optarg); break; +#ifdef __aarch64__ case 'o': test_args.offset.counter_offset = strtol(optarg, NULL, 0); test_args.offset.reserved = 0; break; +#endif case 'h': default: goto err; diff --git a/tools/testing/selftests/kvm/include/riscv/arch_timer.h b/tools/testing/selftests/kvm/include/riscv/arch_timer.h new file mode 100644 index 000000000000..897edcef8fc2 --- /dev/null +++ b/tools/testing/selftests/kvm/include/riscv/arch_timer.h @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: GPL-2.0 */ +/*

• • RISC-V Arch Timer(sstc) specific interface
• • Copyright (c) 2023 Intel Corporation
• */

+#ifndef SELFTEST_KVM_ARCH_TIMER_H +#define SELFTEST_KVM_ARCH_TIMER_H

+#include <asm/csr.h>

+static unsigned long timer_freq;

+#define msec_to_cycles(msec) \

• ((timer_freq) * (uint64_t)(msec) / 1000)

+#define usec_to_cycles(usec) \

• ((timer_freq) * (uint64_t)(usec) / 1000000)

+#define cycles_to_usec(cycles) \

• ((uint64_t)(cycles) * 1000000 / (timer_freq))

+static inline uint64_t timer_get_cntct(void) +{

• return csr_read(CSR_TIME);

+}

+static inline void timer_set_cval(uint64_t cval) +{

• csr_write(CSR_STIMECMP, cval);

+}

+static inline uint64_t timer_get_cval(void) +{

• return csr_read(CSR_STIMECMP);

+}

The names in the above functions with cntct/cval are aarch64-isms. I'd name the functions more explicitly: timer_get_cycles(), timer_set/get_cmp()

+static inline void timer_irq_enable(void) +{

• csr_set(CSR_SIE, IE_TIE);

+}

+static inline void timer_irq_disable(void) +{

• csr_clear(CSR_SIE, IE_TIE);

+}

+static inline void timer_set_next_cval_ms(uint32_t msec)

timer_set_next_cmp_ms()

+{

• uint64_t now_ct = timer_get_cntct();
• uint64_t next_ct = now_ct + msec_to_cycles(msec);
• timer_set_cval(next_ct);

+}

+static inline void cpu_relax(void) +{ +#ifdef __riscv_zihintpause

• asm volatile("pause" ::: "memory");

+#else

• /* Encoding of the pause instruction */
• asm volatile(".4byte 0x100000F" ::: "memory");

+#endif +}

+static inline void __delay(uint64_t cycles) +{

• uint64_t start = timer_get_cntct();
• while ((timer_get_cntct() - start) < cycles)

• cpu_relax();
  

+}

+static inline void udelay(unsigned long usec) +{

• __delay(usec_to_cycles(usec));

+}

+#endif /* SELFTEST_KVM_ARCH_TIMER_H */ diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h index 6087c8fc263a..c69f36302d41 100644 --- a/tools/testing/selftests/kvm/include/riscv/processor.h +++ b/tools/testing/selftests/kvm/include/riscv/processor.h @@ -161,4 +161,14 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0, unsigned long arg3, unsigned long arg4, unsigned long arg5); +static inline void local_irq_enable(void) +{

• csr_set(CSR_SSTATUS, SR_SIE);

+}

+static inline void local_irq_disable(void) +{

• csr_clear(CSR_SSTATUS, SR_SIE);

+}

#endif /* SELFTEST_KVM_PROCESSOR_H */ diff --git a/tools/testing/selftests/kvm/include/timer_test.h b/tools/testing/selftests/kvm/include/timer_test.h index 109e4d635627..091c05a14c93 100644 --- a/tools/testing/selftests/kvm/include/timer_test.h +++ b/tools/testing/selftests/kvm/include/timer_test.h @@ -18,6 +18,7 @@ #define msecs_to_usecs(msec) ((msec) * 1000LL) +#ifdef __aarch64__ #define GICD_BASE_GPA 0x8000000ULL #define GICR_BASE_GPA 0x80A0000ULL @@ -28,6 +29,7 @@ enum guest_stage { GUEST_STAGE_PTIMER_TVAL, GUEST_STAGE_MAX, }; +#endif

No need for this #ifdef, just move these aarch64 things to aarch64/arch_timer.c

/* Timer test cmdline parameters */ struct test_args @@ -36,13 +38,17 @@ struct test_args int nr_iter; int timer_period_ms; int migration_freq_ms; +#ifdef __aarch64__ struct kvm_arm_counter_offset offset; +#endif

test_args shouldn't have aarch64 stuff in it. We can change it to uint64_t offset, which riscv won't use, and aarch64 will pack it into a struct kvm_arm_counter_offset when it needs to use it.

}; /* Shared variables between host and guest */ struct test_vcpu_shared_data { int nr_iter; +#ifdef __aarch64__ enum guest_stage guest_stage; +#endif

No need for #ifdef. Each arch can define it's own guest_stage enum in its own test file. riscv won't have any stages defined.

uint64_t xcnt; }; diff --git a/tools/testing/selftests/kvm/riscv/arch_timer.c b/tools/testing/selftests/kvm/riscv/arch_timer.c new file mode 100644 index 000000000000..c50a33c1e4f9 --- /dev/null +++ b/tools/testing/selftests/kvm/riscv/arch_timer.c @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • arch_timer.c - Tests the riscv64 sstc timer IRQ functionality
• • The test validates the sstc timer IRQs using vstimecmp registers.
• • It's ported from the aarch64 arch_timer test.
• • Copyright (c) 2023, Intel Corporation.
• */

+#define _GNU_SOURCE

+#include "arch_timer.h" +#include "kvm_util.h" +#include "processor.h" +#include "timer_test.h"

+extern struct test_args test_args; +extern struct kvm_vcpu *vcpus[]; +extern struct test_vcpu_shared_data vcpu_shared_data[];

+static int timer_irq = IRQ_S_TIMER;

+static void +guest_configure_timer_action(struct test_vcpu_shared_data *shared_data) +{

• timer_set_next_cval_ms(test_args.timer_period_ms);
• shared_data->xcnt = timer_get_cntct();
• timer_irq_enable();

+}

+static void guest_validate_irq(unsigned int intid,

• 		struct test_vcpu_shared_data *shared_data)
  

+{

• uint64_t xcnt = 0, xcnt_diff_us, cval = 0;

Don't need to initialize xcnt and cval, they get overwritten right away. (cval should be renamed to cmp)

• timer_irq_disable();
• xcnt = timer_get_cntct();
• cval = timer_get_cval();
• xcnt_diff_us = cycles_to_usec(xcnt - shared_data->xcnt);
• /* Make sure we are dealing with the correct timer IRQ */
• GUEST_ASSERT_EQ(intid, timer_irq);
• __GUEST_ASSERT(xcnt >= cval,

•               "xcnt = 0x%llx, cval = 0x%llx, xcnt_diff_us = 0x%llx",
  

•               xcnt, cval, xcnt_diff_us);
  

• WRITE_ONCE(shared_data->nr_iter, shared_data->nr_iter + 1);

+}

+static void guest_irq_handler(struct ex_regs *regs) +{

• unsigned int intid = regs->cause & ~CAUSE_IRQ_FLAG;
• uint32_t cpu = guest_get_vcpuid();
• struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
• guest_validate_irq(intid, shared_data);

+}

+static void guest_run(struct test_vcpu_shared_data *shared_data) +{

• uint32_t irq_iter, config_iter;
• shared_data->nr_iter = 0;
• for (config_iter = 0; config_iter < test_args.nr_iter; config_iter++) {

• /* Setup the next interrupt */
  

• guest_configure_timer_action(shared_data);
  

• /* Setup a timeout for the interrupt to arrive */
  

• udelay(msecs_to_usecs(test_args.timer_period_ms) +
  

• 	TIMER_TEST_ERR_MARGIN_US);
  

• irq_iter = READ_ONCE(shared_data->nr_iter);
  

• GUEST_ASSERT_EQ(config_iter + 1, irq_iter);
  

• }

+}

guest_run[_stage]() can be shared with aarch64, we just have a single stage=0 for riscv.

+static void guest_code(void) +{

• uint32_t cpu = guest_get_vcpuid();
• struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
• local_irq_disable();
• timer_irq_disable();
• local_irq_enable();

I don't think we need to disable all interrupts when disabling the timer interrupt.

• guest_run(shared_data);
• GUEST_DONE();

+}

+static void test_init_timer_freq(struct kvm_vm *vm) +{

• /* Timer frequency should be same for all the vCPUs, so query only vCPU-0 */
• vcpu_get_reg(vcpus[0], RISCV_TIMER_REG(frequency), &timer_freq);
• sync_global_to_guest(vm, timer_freq);
• pr_debug("timer_freq: %lu\n", timer_freq);

+}

+struct kvm_vm *test_vm_create(void) +{

• struct kvm_vm *vm;
• int nr_vcpus = test_args.nr_vcpus;
• vm = vm_create_with_vcpus(nr_vcpus, guest_code, vcpus);
• __TEST_REQUIRE(vcpu_has_ext(vcpus[0], KVM_RISCV_ISA_EXT_SSTC),

• 		   "SSTC not available, skipping test\n");
  

This will assert on get-one-reg rather than skip the test.

• vm_init_vector_tables(vm);
• vm_install_interrupt_handler(vm, guest_irq_handler);
• for (int i = 0; i < nr_vcpus; i++)

• vcpu_init_vector_tables(vcpus[i]);
  

• test_init_timer_freq(vm);
• /* Make all the test's cmdline args visible to the guest */
• sync_global_to_guest(vm, test_args);
• return vm;

+}

+void test_vm_cleanup(struct kvm_vm *vm) +{

• kvm_vm_free(vm);

+}

2.34.1

Thanks, drew

On Thu, Sep 07, 2023 at 12:20:29PM +0800, Haibo Xu wrote:

On Mon, Sep 4, 2023 at 10:58 PM Andrew Jones ajones@ventanamicro.com wrote:

...

On Sat, Sep 02, 2023 at 08:59:30PM +0800, Haibo Xu wrote:

...

Add a KVM selftest to validate the Sstc timer functionality. The test was ported from arm64 arch timer test.

Signed-off-by: Haibo Xu haibo1.xu@intel.com

...

...

diff --git a/tools/testing/selftests/kvm/riscv/arch_timer.c b/tools/testing/selftests/kvm/riscv/arch_timer.c new file mode 100644 index 000000000000..c50a33c1e4f9 --- /dev/null +++ b/tools/testing/selftests/kvm/riscv/arch_timer.c @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • arch_timer.c - Tests the riscv64 sstc timer IRQ functionality
• • The test validates the sstc timer IRQs using vstimecmp registers.
• • It's ported from the aarch64 arch_timer test.

...

guest_run[_stage]() can be shared with aarch64, we just have a single stage=0 for riscv.

Yes, we can. But if we share the guest_run[_stage]() by moving it to kvm/arch_timer.c or kvm/include/timer_test.h, we need to declare extra sub-functions somewhere in a header file(etc. guest_configure_timer_action()).

OK, whatever balances the reduction of duplicate code and avoidance of exporting helper functions. BTW, riscv may not need/want all the same helper functions as aarch64. Anyway, I guess I'll see how the next version turns out.

...

...

+static void guest_code(void) +{

• uint32_t cpu = guest_get_vcpuid();
  

• struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
  

• local_irq_disable();
  

• timer_irq_disable();
  

• local_irq_enable();
  

I don't think we need to disable all interrupts when disabling the timer interrupt.

There is no local_irq_disable() protection during the initial debug phase, but the test always fail with below error messages:

Guest assert failed, vcpu 0; stage; 0; iter: 0 ==== Test Assertion Failure ==== riscv/arch_timer.c:78: config_iter + 1 == irq_iter pid=585 tid=586 errno=4 - Interrupted system call (stack trace empty) 0x1 != 0x0 (config_iter + 1 != irq_iter)

To be frank, I am not quite sure why the local_irq_disable/enable() matters. One possible reason may be some timer irq was triggered before we set up the timecmp register.

We should ensure we know the exact, expected state of the vcpu before, during, and after the test. If a state doesn't match expectations, then the test should assert and we should go investigate the test code to see if setup/checking is correct. If it is, then we've found a bug in KVM that we need to go investigate.

For Sstc, a pending timer interrupt completely depends on stimecmp, so we need to watch that closely. Take a look at the attached simple timer test I pulled together to illustrate how stimecmp, timer interrupt enable, and all interrupt enable interact. You may want to use it to help port the arch_timer.

Thanks, drew

On Fri, Sep 8, 2023 at 9:19 AM Haibo Xu xiaobo55x@gmail.com wrote:

On Fri, Sep 8, 2023 at 3:01 AM Andrew Jones ajones@ventanamicro.com wrote:

...

On Thu, Sep 07, 2023 at 12:20:29PM +0800, Haibo Xu wrote:

...

On Mon, Sep 4, 2023 at 10:58 PM Andrew Jones ajones@ventanamicro.com wrote:

...

On Sat, Sep 02, 2023 at 08:59:30PM +0800, Haibo Xu wrote:

...

Add a KVM selftest to validate the Sstc timer functionality. The test was ported from arm64 arch timer test.

Signed-off-by: Haibo Xu haibo1.xu@intel.com

...

...

diff --git a/tools/testing/selftests/kvm/riscv/arch_timer.c b/tools/testing/selftests/kvm/riscv/arch_timer.c new file mode 100644 index 000000000000..c50a33c1e4f9 --- /dev/null +++ b/tools/testing/selftests/kvm/riscv/arch_timer.c @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • arch_timer.c - Tests the riscv64 sstc timer IRQ functionality
• • The test validates the sstc timer IRQs using vstimecmp registers.
• • It's ported from the aarch64 arch_timer test.

...

...

...

...

+static void guest_code(void) +{

• uint32_t cpu = guest_get_vcpuid();
  

• struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
  

• local_irq_disable();
  

• timer_irq_disable();
  

• local_irq_enable();
  

I don't think we need to disable all interrupts when disabling the timer interrupt.

There is no local_irq_disable() protection during the initial debug phase, but the test always fail with below error messages:

Guest assert failed, vcpu 0; stage; 0; iter: 0 ==== Test Assertion Failure ==== riscv/arch_timer.c:78: config_iter + 1 == irq_iter pid=585 tid=586 errno=4 - Interrupted system call (stack trace empty) 0x1 != 0x0 (config_iter + 1 != irq_iter)

To be frank, I am not quite sure why the local_irq_disable/enable() matters. One possible reason may be some timer irq was triggered before we set up the timecmp register.

We should ensure we know the exact, expected state of the vcpu before, during, and after the test. If a state doesn't match expectations, then the test should assert and we should go investigate the test code to see if setup/checking is correct. If it is, then we've found a bug in KVM that we need to go investigate.

For Sstc, a pending timer interrupt completely depends on stimecmp, so we need to watch that closely. Take a look at the attached simple timer test I pulled together to illustrate how stimecmp, timer interrupt enable, and all interrupt enable interact. You may want to use it to help port the arch_timer.

Thanks for sharing the test codes. Will have an investigation on it.

Hi Andrew,

You are right. The local_irq_disable() was not needed here. It's my fault that I also removed the local_irq_enable() when trying your suggestion.

Thanks for your review and help on this patch set!

Regards, Haibo

...

Thanks, drew