Re: [PATCH 15/16] KVM: arm64: selftests: Augment existing timer test to handle variable offsets

From: Colton Lewis <coltonlewis@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	 linux-arm-kernel@lists.infradead.org, james.morse@arm.com,
	 suzuki.poulose@arm.com, oliver.upton@linux.dev,
	yuzenghui@huawei.com,  ricarkol@google.com, sveith@amazon.de,
	dwmw2@infradead.org
Subject: Re: [PATCH 15/16] KVM: arm64: selftests: Augment existing timer test to handle variable offsets
Date: Tue, 14 Mar 2023 17:47:01 +0000	[thread overview]
Message-ID: <gsnta60fcjje.fsf@coltonlewis-kvm.c.googlers.com> (raw)
In-Reply-To: <86o7owyj0a.wl-maz@kernel.org> (message from Marc Zyngier on Mon, 13 Mar 2023 11:43:01 +0000)

Marc Zyngier <maz@kernel.org> writes:

> On Fri, 10 Mar 2023 19:26:47 +0000,
> Colton Lewis <coltonlewis@google.com> wrote:

>> Marc Zyngier <maz@kernel.org> writes:

>> >> mvbbq9:/data/coltonlewis/ecv/arm64-obj/kselftest/kvm#
>> >> ./aarch64/arch_timer -O 0xffff
>> >> ==== Test Assertion Failure ====
>> >>    aarch64/arch_timer.c:239: false
>> >>    pid=48094 tid=48095 errno=4 - Interrupted system call
>> >>       1  0x4010fb: test_vcpu_run at arch_timer.c:239
>> >>       2  0x42a5bf: start_thread at pthread_create.o:0
>> >>       3  0x46845b: thread_start at clone.o:0
>> >>    Failed guest assert: xcnt >= cval at aarch64/arch_timer.c:151
>> >> values: 2500645901305, 2500645961845; 9939, vcpu 0; stage; 3; iter: 2

>> > The fun part is that you can see similar things without the series:

>> > ==== Test Assertion Failure ====
>> >    aarch64/arch_timer.c:239: false
>> >    pid=647 tid=651 errno=4 - Interrupted system call
>> >       1  0x00000000004026db: test_vcpu_run at arch_timer.c:239
>> >       2  0x00007fffb13cedd7: ?? ??:0
>> >       3  0x00007fffb1437e9b: ?? ??:0
>> >    Failed guest assert: config_iter + 1 == irq_iter at
>> > aarch64/arch_timer.c:188
>> > values: 2, 3; 0, vcpu 3; stage; 4; iter: 3

>> > That's on a vanilla kernel (6.2-rc4) on an M1 with the test run
>> > without any argument in a loop. After a few iterations, it blows.

> I finally got to the bottom of that one. This is yet another case of
> the test making the assumption that spurious interrupts don't exist...

That's great!

> Here, the timer interrupt has been masked at the source, but the GIC
> (or its emulation) can be slow to retire it. So we take it again,
> spuriously, and account it as a true interrupt. None of the asserts in
> the timer handler fire because they only check the *previous* state.

> Eventually, the interrupt retires and we progress to the next
> iteration. But in the meantime, we have incremented the irq counter by
> the number of spurious events, and the test fails.

> The obvious fix is to check for the timer state in the handler and
> exit early if the timer interrupt is masked or the timer disabled.
> With that, I don't see these failures anymore.

> I've folded that into the patch that already deals with some spurious
> events.

I'll be looking at it and will keep in mind your questions about my
hardware should I find any issues. Yes it has ECV and CNTPOFF but no I
didn't try turning it off for this because my issue occured only when
setting a physical offset and that can't be done without ECV.