On Tue, 2021-09-14 at 10:52 +0900, Punit Agrawal wrote:
> Hi Nicolas,
>
> Apologies for the late comment. I just caught up with the thread.
No worries, thanks for the input!
[...]
> > +static inline void frc(uint64_t *pval)
> > +{
> > + /*
> > + * This isb() is required to prevent that the counter value
> > + * is speculated.
> > + */
> > + __asm__ __volatile__("isb" : : : "memory");
> > + __asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
> > +
>
> Although the isb() ensures completion of instructions before the counter
> is read, I think there is still the problem of speculative execution of
> instructions after the counter read being moved forward. See the
> examples in Arm ARM DDI 0487F.b Section D11.2.2 "The Virtual counter"
>
> So from my understanding the problem would be something like below -
>
> isb()
> ... <- speculatively executed instructions from after the counter read
> mrs %0, cntvct_el0
>
> This would skew the counter value to a later point than what is intended
> - a following isb() would address the issue.
For the record, here's what the Arm ARM states:
Accesses to memory appearing in program order after the read of the counter
are executed before the counter has been read. [...]
To ensure that a memory access only occurs after a read of the counter, the
following sequence should be used:
MRS Xn, CNTVCT_EL0
ISB
LDR Xa, [Xb] ; this load will be executed after the timer has been read"
As stated in the commit description, I made sure the program logic can't suffer
from this. I hadn't thought of the timing angle though. I doubt we'll see any
difference, given we have a 1us granularity, but I don't mind adding it for the
sake of correctness.
@John Should I send a v3 of the series or a separate patch adding the extra
isb().
--
Nicolás Sáenz
