nsaenzju@xxxxxxxxxx writes:
> 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.
Thanks - indeed that's the part I was referring to. Indeed the impact
will likely be low or none here but thought I'd mention the issue since
I noticed it while going through the patch.
Punit
[...]
