| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJve8ok-zZAG7T3t5XD-CtJn47tTE867Xaaw_YHYy1XmUL41rw@mail.gmail.com>
Date: Wed, 6 Sep 2023 11:44:26 +0800
From: Haibo Xu <xiaobo55x@...il.com>
To: Andrew Jones <ajones@...tanamicro.com>
Cc: Haibo Xu <haibo1.xu@...el.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Paolo Bonzini <pbonzini@...hat.com>,
Shuah Khan <shuah@...nel.org>, Marc Zyngier <maz@...nel.org>,
Oliver Upton <oliver.upton@...ux.dev>,
James Morse <james.morse@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Zenghui Yu <yuzenghui@...wei.com>,
Anup Patel <anup@...infault.org>,
Atish Patra <atishp@...shpatra.org>,
Guo Ren <guoren@...nel.org>,
Conor Dooley <conor.dooley@...rochip.com>,
Daniel Henrique Barboza <dbarboza@...tanamicro.com>,
Greentime Hu <greentime.hu@...ive.com>,
Sean Christopherson <seanjc@...gle.com>,
Ricardo Koller <ricarkol@...gle.com>,
Vishal Annapurve <vannapurve@...gle.com>,
Aaron Lewis <aaronlewis@...gle.com>,
David Matlack <dmatlack@...gle.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Ackerley Tng <ackerleytng@...gle.com>,
Mingwei Zhang <mizhang@...gle.com>,
Vipin Sharma <vipinsh@...gle.com>,
Lei Wang <lei4.wang@...el.com>, Like Xu <likexu@...cent.com>,
Peter Gonda <pgonda@...gle.com>,
Maxim Levitsky <mlevitsk@...hat.com>,
Thomas Huth <thuth@...hat.com>,
Philippe Mathieu-Daudé <philmd@...aro.org>,
David Woodhouse <dwmw@...zon.co.uk>,
Michal Luczaj <mhal@...x.co>, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
kvm-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 2/8] KVM: arm64: selftest: Split arch_timer test code
On Wed, Sep 6, 2023 at 10:14 AM Haibo Xu <xiaobo55x@...il.com> wrote:
>
> On Mon, Sep 4, 2023 at 9:24 PM Andrew Jones <ajones@...tanamicro.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@...tanamicro.com>
> > > Signed-off-by: Haibo Xu <haibo1.xu@...el.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
Powered by blists - more mailing lists