Re: [PATCH v2 2/3] oslat: Add aarch64 support

[Punit Agrawal <punitagrawal@gmail.com>]

nsaenzju@redhat.com 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

[...]