On Tue, 14 Sep 2021, nsaenzju@xxxxxxxxxx wrote:
> 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
>
>
I already integrated and pushed your v2 patch, so you should pull the code
from upstream and then create a new patch to add the isb()
Thanks
John
