Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabell...@kernel.org> > Sent: 2020年11月26日 8:00 > To: Wei Chen <wei.c...@arm.com> > Cc: Julien Grall <jul...@xen.org>; Penny Zheng <penny.zh...@arm.com>; xen- > de...@lists.xenproject.org; sstabell...@kernel.org; Andre Przywara > <andre.przyw...@arm.com>; Bertrand Marquis <bertrand.marq...@arm.com>; > Kaly Xin <kaly....@arm.com>; nd <n...@arm.com> > Subject: RE: [PATCH] xen/arm: Add Cortex-A73 erratum 858921 workaround > > 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.zh...@arm.com> > > > > > > Acked-by: Julien Grall <jgr...@amazon.com> > > > > > > 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?
I agree with you to add an in-code comment. It will remind us in future when we use the get_cycles in critical context. Adding it now will probably only lead to meaningless performance degradation. Because Xen may never use it in a similar scenario. BTW: Can we send a patch that just contains a pure comment : ) Regards, Wei Chen
