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@xxxxxxxxxx>
> > > Reviewed-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > > Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx>
>
> [...]
>
> > > + 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.
