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

[Ricardo Koller <ricarkol@google.com>]

On Thu, Mar 17, 2022 at 08:52:38AM +0000, Marc Zyngier wrote:
> 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 :-/

Got it, adding it. Saw that there is a similar pattern in the kernel and
it has an ISB in the middle.

> 
> 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.

Good point.

> 
> > 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.

The next commit fixes this (by spinning on the counter). Will move it
here.

> 
> Thanks,
> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...

Thank you both for the review.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm