RE: [PATCH] xen/arm: Add Cortex-A73 erratum 858921 workaround

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Resuming this old thread.

On Fri, 13 Nov 2020, Wei Chen wrote:
> > Hi,
> > 
> > On 09/11/2020 08:21, Penny Zheng wrote:
> > > CNTVCT_EL0 or CNTPCT_EL0 counter read in Cortex-A73 (all versions)
> > > might return a wrong value when the counter crosses a 32bit boundary.
> > >
> > > Until now, there is no case for Xen itself to access CNTVCT_EL0,
> > > and it also should be the Guest OS's responsibility to deal with
> > > this part.
> > >
> > > But for CNTPCT, there exists several cases in Xen involving reading
> > > CNTPCT, so a possible workaround is that performing the read twice,
> > > and to return one or the other depending on whether a transition has
> > > taken place.
> > >
> > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > 
> > Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> > 
> > On a related topic, do we need a fix similar to Linux commit
> > 75a19a0202db "arm64: arch_timer: Ensure counter register reads occur
> > with seqlock held"?
> > 
> 
> I take a look at this Linux commit, it seems to prevent the seq-lock to be
> speculated.  Using an enforce ordering instead of ISB after the read counter
> operation seems to be for performance reasons.
> 
> I have found that you had placed an ISB before read counter in get_cycles
> to prevent counter value to be speculated. But you haven't placed the second
> ISB after reading. Is it because we haven't used the get_cycles in seq lock
> critical context (Maybe I didn't find the right place)? So should we need to 
> fix it
> now, or you prefer to fix it now for future usage?

Looking at the call sites, it doesn't look like we need any ISB after
get_cycles as it is not used in any critical context. There is also a
data dependency with the value returned by it.

So I am thinking we don't need any fix. At most we need an in-code comment?