RE: [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年1月5日 19:17
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Penny Zheng
> <Penny.Zheng@xxxxxxx>; Jiamei Xie <Jiamei.Xie@xxxxxxx>; nd
> <nd@xxxxxxx>
> Subject: Re: [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for 
> Arm64
> 
> Hi Wei,
> 
> On 05/01/2021 07:19, Wei Chen wrote:
> > As the dicsussion [1] in mailing list. We'd better to have
> 
> I would say "Per the discussion [1] on the ...". I would also suggest to
> replace the "." with ",".
> 
> > a barrier after reading CNTPCT in get_cycles. If there is
> > not any barrier there. When get_cycles being used in some
> > seqlock critical context in the future, the seqlock can be
> > speculated potentially.
> 
> This comment seems a little off because we don't have seqlock on Xen. I
> think it would be best if you re-use the Linux commit as it would be
> clear that this is a backport.
> 
> Something like:
> 
> "Import commit .... from Linux:
> 
> <entire commit message indented by one>
> 
> While we are not aware of such use in Xen, it would be best to add the
> barrier to avoid any suprise."
> "
> 

Yes, that would be better. I will add it in next version. Thanks.

> >
> > In order to reduce the impact of new barrier, we perfer to
> > use enforce order instead of ISB [2].
> >
> > Currently, enforce order is not applied to arm32 as this is
> > not done in Linux at the date of this patch. If this is done
> > in Linux it will need to be also done in Xen.
> >
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2020-
> 12/msg00181.html
> > [2] https://lkml.org/lkml/2020/3/13/645
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/include/asm-arm/time.h | 43
> ++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
> > index 5c4529ebb5..3ef4e7cd3d 100644
> > --- a/xen/include/asm-arm/time.h
> > +++ b/xen/include/asm-arm/time.h
> > @@ -11,9 +11,26 @@
> >
> >   typedef uint64_t cycles_t;
> >
> > -static inline cycles_t get_cycles(void)
> > +/*
> > + * Ensure that reads of the counter are treated the same as memory reads
> > + * for the purposes of ordering by subsequent memory barriers.
> > + */
> 
> The comment is before the #ifdef which suggests the ordering is required
> for Arm as well. I would suggest to either mention that oddity or move
> the comment after the #ifdef.
> 
> > +#if defined(CONFIG_ARM_64)
> > +#define read_cntpct_enforce_ordering(val) do { \
> > +    u64 tmp, _val = (val);                     \
> 
> Please use uint64_t here.

Got it.

> 
> > +                                               \
> > +    asm volatile(                              \
> > +    "eor %0, %1, %1\n"                         \
> > +    "add %0, sp, %0\n"                         \
> > +    "ldr xzr, [%0]"                            \
> > +    : "=r" (tmp) : "r" (_val));                \
> > +} while (0)
> > +#else
> > +#define read_cntpct_enforce_ordering(val) do {} while (0)
> > +#endif
> > +
> > +static inline cycles_t read_cntpct_stable(void)
> >   {
> > -    isb();
> >       /*
> >        * ARM_WORKAROUND_858921: Cortex-A73 (all versions) counter read
> >        * can return a wrong value when the counter crosses a 32bit boundary.
> > @@ -36,6 +53,28 @@ static inline cycles_t get_cycles(void)
> >       }
> >   }
> >
> > +static inline cycles_t get_cycles(void)
> > +{
> > +    cycles_t cnt;
> > +
> > +    isb();
> > +    cnt = read_cntpct_stable();
> > +
> > +    /*
> > +     * If there is not any barrier here. When get_cycles being used in
> > +     * some seqlock critical context in the future, the seqlock can be
> > +     * speculated potentially.
> > +     *
> > +     * To prevent seqlock from being speculated silently, we add a barrier
> > +     * here defensively. Normally, we just need an ISB here is enough, but
> > +     * considering the minimum performance cost. We prefer to use enforce
> > +     * order here.
> > +     */
> 
> We don't use seqlock in Xen, so this comment looks quite confusing.. I
> think the comment on top of reach_cntpct_enforce_ordering() is
> sufficient, so I would drop this one. The alternative is to find a way
> to make the comment more Xen focused.
> 
> Although, I don't have a good suggestion so far.
> 

Ok, I will drop it.

> > +    read_cntpct_enforce_ordering(cnt);
> > +
> > +    return cnt;
> > +}
> > +
> >   /* List of timer's IRQ */
> >   enum timer_ppi
> >   {
> >
> 
> Cheers,
> 
> --
> Julien Grall