On Thu, 28 Sep 2023 22:02:01 +0100, Colton Lewis <coltonlewis@xxxxxxxxxx> wrote: > > Add a new arch_timer_edge_cases selftests that validates: > > * timers above the max TVAL value > * timers in the past > * moving counters ahead and behind pending timers > * reprograming timers > * timers fired multiple times > * masking/unmasking using the timer control mask > > These are intentionally unusual scenarios to stress compliance with > the arm architecture. > > Co-developed-by: Ricardo Koller <ricarkol@xxxxxxxxxx> > Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx> > Signed-off-by: Colton Lewis <coltonlewis@xxxxxxxxxx> > --- > > v2: > * Rebase to v6.6-rc3 > * Use new GUEST_ASSERT macros > * Remove spinlock in favor of atomic operations > > v1: https://lore.kernel.org/kvm/20230516213731.387132-1-coltonlewis@xxxxxxxxxx/ > > tools/testing/selftests/kvm/Makefile | 1 + > .../kvm/aarch64/arch_timer_edge_cases.c | 1105 +++++++++++++++++ > .../kvm/include/aarch64/arch_timer.h | 18 +- > 3 files changed, 1123 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/kvm/aarch64/arch_timer_edge_cases.c > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index a3bb36fb3cfc..e940b7c6b818 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -141,6 +141,7 @@ 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/arch_timer_edge_cases > TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions > TEST_GEN_PROGS_aarch64 += aarch64/hypercalls > TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer_edge_cases.c b/tools/testing/selftests/kvm/aarch64/arch_timer_edge_cases.c > new file mode 100644 > index 000000000000..a3761a361de9 > --- /dev/null > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer_edge_cases.c > @@ -0,0 +1,1105 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * arch_timer_edge_cases.c - Tests the aarch64 timer IRQ functionality. > + * > + * The test validates some edge cases related to the arch-timer: > + * - timers above the max TVAL value. > + * - timers in the past > + * - moving counters ahead and behind pending timers. > + * - reprograming timers. > + * - timers fired multiple times. > + * - masking/unmasking using the timer control mask. > + * > + * Copyright (c) 2021, Google LLC. > + */ > + > +#define _GNU_SOURCE > + > +#include <stdlib.h> > +#include <pthread.h> > +#include <linux/kvm.h> > +#include <linux/atomic.h> > +#include <linux/bitmap.h> > +#include <linux/sizes.h> > +#include <sched.h> > +#include <sys/sysinfo.h> > + > +#include "kvm_util.h" > +#include "processor.h" > +#include "delay.h" > +#include "arch_timer.h" > +#include "gic.h" > +#include "vgic.h" > + > +#define msecs_to_usecs(msec) ((msec) * 1000LL) > + > +#define CVAL_MAX ~0ULL > +/* tval is a signed 32-bit int. */ > +#define TVAL_MAX INT_MAX > +#define TVAL_MIN INT_MIN > + > +#define GICD_BASE_GPA 0x8000000ULL > +#define GICR_BASE_GPA 0x80A0000ULL We already have 3 tests that do their own GIC setup. Maybe it is time someone either make vgic_v3_setup() deal with fixed addresses, or move this into a helper. > + > +/* After how much time we say there is no IRQ. */ > +#define TIMEOUT_NO_IRQ_US msecs_to_usecs(50) > + > +#define TEST_MARGIN_US 1000ULL > + > +/* A nice counter value to use as the starting one for most tests. */ > +#define DEF_CNT (CVAL_MAX / 2) > + > +/* Number of runs. */ > +#define NR_TEST_ITERS_DEF 5 > + > +/* Default wait test time in ms. */ > +#define WAIT_TEST_MS 10 > + > +/* Default "long" wait test time in ms. */ > +#define LONG_WAIT_TEST_MS 100 > + > +/* Shared with IRQ handler. */ > +struct test_vcpu_shared_data { > + atomic_t handled; > + atomic_t spurious; > +} shared_data; > + > +struct test_args { > + /* Virtual or physical timer and counter tests. */ > + enum arch_timer timer; > + /* Delay used for most timer tests. */ > + uint64_t wait_ms; > + /* Delay used in the test_long_timer_delays test. */ > + uint64_t long_wait_ms; > + /* Number of iterations. */ > + int iterations; > + /* Whether to test the physical timer. */ > + bool test_physical; > + /* Whether to test the virtual timer. */ > + bool test_virtual; > +}; > + > +struct test_args test_args = { > + .wait_ms = WAIT_TEST_MS, > + .long_wait_ms = LONG_WAIT_TEST_MS, > + .iterations = NR_TEST_ITERS_DEF, > + .test_physical = true, > + .test_virtual = true, > +}; > + > +static int vtimer_irq, ptimer_irq; > + > +enum sync_cmd { > + SET_REG_KVM_REG_ARM_TIMER_CNT = 100001, > + USERSPACE_USLEEP, > + USERSPACE_SCHED_YIELD, > + USERSPACE_MIGRATE_SELF, > +}; > + > +typedef void (*sleep_method_t)(enum arch_timer timer, uint64_t usec); > + > +static void sleep_poll(enum arch_timer timer, uint64_t usec); > +static void sleep_sched_poll(enum arch_timer timer, uint64_t usec); > +static void sleep_in_userspace(enum arch_timer timer, uint64_t usec); > +static void sleep_migrate(enum arch_timer timer, uint64_t usec); > + > +sleep_method_t sleep_method[] = { > + sleep_poll, > + sleep_sched_poll, > + sleep_migrate, > + sleep_in_userspace, > +}; > + > +typedef void (*wfi_method_t)(void); > + > +static void wait_for_non_spurious_irq(void); > +static void wait_poll_for_irq(void); > +static void wait_sched_poll_for_irq(void); > +static void wait_migrate_poll_for_irq(void); > + > +wfi_method_t wfi_method[] = { > + wait_for_non_spurious_irq, > + wait_poll_for_irq, > + wait_sched_poll_for_irq, > + wait_migrate_poll_for_irq, > +}; > + > +#define for_each_wfi_method(i) \ > + for ((i) = 0; (i) < ARRAY_SIZE(wfi_method); (i)++) > + > +#define for_each_sleep_method(i) \ > + for ((i) = 0; (i) < ARRAY_SIZE(sleep_method); (i)++) > + > +enum timer_view { > + TIMER_CVAL = 1, > + TIMER_TVAL, > +}; > + > +#define ASSERT_IRQS_HANDLED(_nr, _args...) do { \ > + int _h = atomic_read(&shared_data.handled); \ > + __GUEST_ASSERT(_h == (_nr), "Handled %d IRQS but expected %d", _h, _nr, ##_args); \ > + } while (0) > + > +#define GUEST_SYNC_CLOCK(__cmd, __val) \ > + GUEST_SYNC_ARGS(__cmd, __val, 0, 0, 0) > + > +#define USERSPACE_CMD(__cmd) \ > + GUEST_SYNC_ARGS(__cmd, 0, 0, 0, 0) > + > +#define USERSPACE_SCHEDULE() \ > + USERSPACE_CMD(USERSPACE_SCHED_YIELD) > + > +#define USERSPACE_MIGRATE_VCPU() \ > + USERSPACE_CMD(USERSPACE_MIGRATE_SELF) > + > +#define SLEEP_IN_USERSPACE(__usecs) \ > + GUEST_SYNC_ARGS(USERSPACE_USLEEP, (__usecs), 0, 0, 0) > + > +#define IAR_SPURIOUS 1023 > + > +static void set_counter(enum arch_timer timer, uint64_t counter) > +{ > + GUEST_SYNC_ARGS(SET_REG_KVM_REG_ARM_TIMER_CNT, counter, timer, 0, 0); > +} > + > +static uint32_t next_pcpu(void) > +{ > + uint32_t max = get_nprocs(); > + uint32_t cur = sched_getcpu(); > + uint32_t next = cur; > + cpu_set_t cpuset; > + > + TEST_ASSERT(max > 1, "Need at least two physical cpus"); > + > + sched_getaffinity(getpid(), sizeof(cpuset), &cpuset); > + > + do { > + next = (next + 1) % CPU_SETSIZE; > + } while (!CPU_ISSET(next, &cpuset)); > + > + return next; > +} > + > +static void guest_irq_handler(struct ex_regs *regs) > +{ > + unsigned int intid = gic_get_and_ack_irq(); > + enum arch_timer timer; > + uint64_t cnt, cval; > + uint32_t ctl; > + bool timer_condition, istatus; > + > + if (intid == IAR_SPURIOUS) { > + atomic_inc(&shared_data.spurious); > + goto out; > + } > + > + if (intid == ptimer_irq) > + timer = PHYSICAL; > + else if (intid == vtimer_irq) > + timer = VIRTUAL; > + else > + goto out; > + > + ctl = timer_get_ctl(timer); > + cval = timer_get_cval(timer); > + cnt = timer_get_cntct(timer); > + timer_condition = cnt >= cval; > + istatus = (ctl & CTL_ISTATUS) && (ctl & CTL_ENABLE); > + GUEST_ASSERT_EQ(timer_condition, istatus); > + > + /* Disable and mask the timer. */ > + timer_set_ctl(timer, CTL_IMASK); What is the point of masking if the timer is disabled? > + > + atomic_inc(&shared_data.handled); You don't have any ordering between atomic operations and system register access. Could it be a problem? > + > +out: > + gic_set_eoi(intid); > +} > + > +static void set_cval_irq(enum arch_timer timer, uint64_t cval_cycles, > + uint32_t ctl) > +{ > + atomic_set(&shared_data.handled, 0); > + atomic_set(&shared_data.spurious, 0); > + timer_set_cval(timer, cval_cycles); > + timer_set_ctl(timer, ctl); Same question. > +} > + > +static void set_tval_irq(enum arch_timer timer, uint64_t tval_cycles, > + uint32_t ctl) > +{ > + atomic_set(&shared_data.handled, 0); > + atomic_set(&shared_data.spurious, 0); > + timer_set_tval(timer, tval_cycles); > + timer_set_ctl(timer, ctl); > +} > + > +static void set_xval_irq(enum arch_timer timer, uint64_t xval, uint32_t ctl, > + enum timer_view tv) > +{ > + switch (tv) { > + case TIMER_CVAL: > + set_cval_irq(timer, xval, ctl); > + break; > + case TIMER_TVAL: > + set_tval_irq(timer, xval, ctl); > + break; > + default: > + GUEST_FAIL("Could not get timer %d", timer); > + } > +} > + > +/* > + * Should be called with IRQs masked. > + * > + * Note that this can theoretically hang forever, so we rely on having > + * a timeout mechanism in the "runner", like: > + * tools/testing/selftests/kselftest/runner.sh. > + */ > +static void wait_for_non_spurious_irq(void) > +{ > + int h; > + > + for (h = atomic_read(&shared_data.handled); h == atomic_read(&shared_data.handled);) { > + asm volatile ("wfi\n" > + "msr daifclr, #2\n" > + /* handle IRQ */ > + "msr daifset, #2\n":::"memory"); There is no guarantee that a pending interrupt would fire at the point where the comment is. R_RBZYL clearly state that you need a context synchronisation event between these two instructions if you want interrupts to be handled. > + } > +} > + > +/* > + * Wait for an non-spurious IRQ by polling in the guest (userspace=0) or in > + * userspace (e.g., userspace=1 and userspace_cmd=USERSPACE_SCHED_YIELD). > + * > + * Should be called with IRQs masked. Not really needed like the wfi above, but > + * it should match the others. > + * > + * Note that this can theoretically hang forever, so we rely on having > + * a timeout mechanism in the "runner", like: > + * tools/testing/selftests/kselftest/runner.sh. > + */ > +static void poll_for_non_spurious_irq(bool userspace, enum sync_cmd userspace_cmd) > +{ > + int h; > + > + h = atomic_read(&shared_data.handled); > + > + local_irq_enable(); So half of this code is using local_irq_*(), and the rest is directly poking at DAIF. Amusingly enough, the two aren't even playing with the same set of bits. > + while (h == atomic_read(&shared_data.handled)) { > + if (userspace) > + USERSPACE_CMD(userspace_cmd); > + else > + cpu_relax(); > + } > + local_irq_disable(); > +} > + > +static void wait_poll_for_irq(void) > +{ > + poll_for_non_spurious_irq(false, -1); Am I the only one who cries when seeing this -1 cast on an unsuspected enum, together with a flag saying "hey, I'm giving you crap arguments"? What was wrong having an actual enum value for it? > +} > + > +static void wait_sched_poll_for_irq(void) > +{ > + poll_for_non_spurious_irq(true, USERSPACE_SCHED_YIELD); > +} > + > +static void wait_migrate_poll_for_irq(void) > +{ > + poll_for_non_spurious_irq(true, USERSPACE_MIGRATE_SELF); > +} > + > +/* > + * Sleep for usec microseconds by polling in the guest (userspace=0) or in > + * userspace (e.g., userspace=1 and userspace_cmd=USERSPACE_SCHEDULE). > + */ > +static void guest_poll(enum arch_timer test_timer, uint64_t usec, > + bool userspace, enum sync_cmd userspace_cmd) > +{ > + uint64_t cycles = usec_to_cycles(usec); > + /* Whichever timer we are testing with, sleep with the other. */ > + enum arch_timer sleep_timer = 1 - test_timer; > + uint64_t start = timer_get_cntct(sleep_timer); > + > + while ((timer_get_cntct(sleep_timer) - start) < cycles) { > + if (userspace) > + USERSPACE_CMD(userspace_cmd); > + else > + cpu_relax(); > + } > +} > + > +static void sleep_poll(enum arch_timer timer, uint64_t usec) > +{ > + guest_poll(timer, usec, false, -1); More of the same stuff... Frankly, I even have a hard time understanding what this code is trying to achieve, let alone the lack of correctness from an architecture perspective. M. -- Without deviation from the norm, progress is not possible.
