Re: [PATCH v2] KVM: arm64: selftests: Add arch_timer_edge_cases selftest

[Marc Zyngier <maz@kernel.org>]

On Thu, 28 Sep 2023 22:02:01 +0100,
Colton Lewis <coltonlewis@google.com> 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@google.com>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
> 
> 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@google.com/
> 
>  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.