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

[John Kacur <jkacur@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]



On Tue, 14 Sep 2021, nsaenzju@redhat.com 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