Re: [PATCH v2 2/3] KVM: arm64: selftests: add arch_timer_edge_cases

[Marc Zyngier <maz@kernel.org>]

On 2022-03-17 06:44, Oliver Upton wrote:
> On Wed, Mar 16, 2022 at 09:51:26PM -0700, Ricardo Koller wrote:
>> Add an arch_timer edge-cases selftest. For now, just add some basic
>> sanity checks, and some stress conditions (like waiting for the timers
>> while re-scheduling the vcpu). The next commit will add the actual 
>> edge
>> case tests.
>> 
>> This test fails without a867e9d0cc1 "KVM: arm64: Don't miss pending
>> interrupts for suspended vCPU".
>> 
>> Reviewed-by: Reiji Watanabe <reijiw@google.com>
>> Reviewed-by: Raghavendra Rao Ananta <rananta@google.com>
>> Signed-off-by: Ricardo Koller <ricarkol@google.com>

[...]

>> +		asm volatile("wfi\n"
>> +			     "msr daifclr, #2\n"
>> +			     /* handle IRQ */
> 
> I believe an isb is owed here (DDI0487G.b D1.13.4). Annoyingly, I am
> having a hard time finding the same language in the H.a revision of the
> manual :-/

D1.3.6 probably is what you are looking for.

"Context synchronization event" is the key phrase to remember
when grepping through the ARM ARM. And yes, the new layout is
a nightmare (as if we really needed an additional 2800 pages...).

> 
>> +			     "msr daifset, #2\n"
>> +			     : : : "memory");
>> +	}
>> +}

[...]

>> +	/* tval should keep down-counting from 0 to -1. */
>> +	SET_COUNTER(DEF_CNT, test_args.timer);
>> +	timer_set_tval(test_args.timer, 0);
>> +	if (use_sched)
>> +		USERSPACE_SCHEDULE();
>> +	/* We just need 1 cycle to pass. */
>> +	isb();
> 
> Somewhat paranoid, but:
> 
> If the CPU retires the ISB much more quickly than the counter ticks, 
> its
> possible that you could observe an invalid TVAL even with a valid
> implementation.

Worse than that:

- ISB doesn't need to take any time at all. It just needs to ensure
   that everything is synchronised. Depending on how the CPU is built,
   this can come for free.

- There is no relation between the counter ticks and CPU cycles.

> What if you spin waiting for CNT to increment before the assertion? 
> Then
> you for sureknow (and can tell by reading the test) that the
> implementation is broken.

That's basically the only way to implement this. You can't rely
on any other event.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm