* [PATCH 1/2] xen/arm: Correct the coding style of get_cycles @ 2021-01-05 7:19 Wei Chen 2021-01-05 7:19 ` [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 Wei Chen 2021-01-05 11:04 ` [PATCH 1/2] xen/arm: Correct the coding style of get_cycles Julien Grall 0 siblings, 2 replies; 5+ messages in thread From: Wei Chen @ 2021-01-05 7:19 UTC (permalink / raw) To: wei.chen, xen-devel, sstabellini, julien Cc: Bertrand.Marquis, Penny.Zheng, Jiamei.Xie, nd It seems the arm inline function get_cycles has used 8 spaces for line indent since 2012. This patch correct them to 4 spaces and remove extra space between function name and bracket. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/include/asm-arm/time.h | 40 +++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h index 1b2c13614b..5c4529ebb5 100644 --- a/xen/include/asm-arm/time.h +++ b/xen/include/asm-arm/time.h @@ -11,29 +11,29 @@ typedef uint64_t cycles_t; -static inline cycles_t get_cycles (void) +static inline cycles_t get_cycles(void) { - isb(); + isb(); + /* + * ARM_WORKAROUND_858921: Cortex-A73 (all versions) counter read + * can return a wrong value when the counter crosses a 32bit boundary. + */ + if ( !check_workaround_858921() ) + return READ_SYSREG64(CNTPCT_EL0); + else + { /* - * ARM_WORKAROUND_858921: Cortex-A73 (all versions) counter read - * can return a wrong value when the counter crosses a 32bit boundary. + * A recommended workaround for erratum 858921 is to: + * 1- Read twice CNTPCT. + * 2- Compare bit[32] of the two read values. + * - If bit[32] is different, keep the old value. + * - If bit[32] is the same, keep the new value. */ - if ( !check_workaround_858921() ) - return READ_SYSREG64(CNTPCT_EL0); - else - { - /* - * A recommended workaround for erratum 858921 is to: - * 1- Read twice CNTPCT. - * 2- Compare bit[32] of the two read values. - * - If bit[32] is different, keep the old value. - * - If bit[32] is the same, keep the new value. - */ - cycles_t old, new; - old = READ_SYSREG64(CNTPCT_EL0); - new = READ_SYSREG64(CNTPCT_EL0); - return (((old ^ new) >> 32) & 1) ? old : new; - } + cycles_t old, new; + old = READ_SYSREG64(CNTPCT_EL0); + new = READ_SYSREG64(CNTPCT_EL0); + return (((old ^ new) >> 32) & 1) ? old : new; + } } /* List of timer's IRQ */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 2021-01-05 7:19 [PATCH 1/2] xen/arm: Correct the coding style of get_cycles Wei Chen @ 2021-01-05 7:19 ` Wei Chen 2021-01-05 11:17 ` Julien Grall 2021-01-05 11:04 ` [PATCH 1/2] xen/arm: Correct the coding style of get_cycles Julien Grall 1 sibling, 1 reply; 5+ messages in thread From: Wei Chen @ 2021-01-05 7:19 UTC (permalink / raw) To: wei.chen, xen-devel, sstabellini, julien Cc: Bertrand.Marquis, Penny.Zheng, Jiamei.Xie, nd As the dicsussion [1] in mailing list. We'd better to have 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. 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@arm.com> --- 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. + */ +#if defined(CONFIG_ARM_64) +#define read_cntpct_enforce_ordering(val) do { \ + u64 tmp, _val = (val); \ + \ + 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. + */ + read_cntpct_enforce_ordering(cnt); + + return cnt; +} + /* List of timer's IRQ */ enum timer_ppi { -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 2021-01-05 7:19 ` [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 Wei Chen @ 2021-01-05 11:17 ` Julien Grall 2021-01-05 13:45 ` Wei Chen 0 siblings, 1 reply; 5+ messages in thread From: Julien Grall @ 2021-01-05 11:17 UTC (permalink / raw) To: Wei Chen, xen-devel, sstabellini Cc: Bertrand.Marquis, Penny.Zheng, Jiamei.Xie, nd 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." " > > 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@arm.com> > --- > 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. > + \ > + 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. > + read_cntpct_enforce_ordering(cnt); > + > + return cnt; > +} > + > /* List of timer's IRQ */ > enum timer_ppi > { > Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 2021-01-05 11:17 ` Julien Grall @ 2021-01-05 13:45 ` Wei Chen 0 siblings, 0 replies; 5+ messages in thread From: Wei Chen @ 2021-01-05 13:45 UTC (permalink / raw) To: Julien Grall, xen-devel, sstabellini Cc: Bertrand Marquis, Penny Zheng, Jiamei Xie, nd Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年1月5日 19:17 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Penny Zheng > <Penny.Zheng@arm.com>; Jiamei Xie <Jiamei.Xie@arm.com>; nd > <nd@arm.com> > 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@arm.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] xen/arm: Correct the coding style of get_cycles 2021-01-05 7:19 [PATCH 1/2] xen/arm: Correct the coding style of get_cycles Wei Chen 2021-01-05 7:19 ` [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 Wei Chen @ 2021-01-05 11:04 ` Julien Grall 1 sibling, 0 replies; 5+ messages in thread From: Julien Grall @ 2021-01-05 11:04 UTC (permalink / raw) To: Wei Chen, xen-devel, sstabellini Cc: Bertrand.Marquis, Penny.Zheng, Jiamei.Xie, nd Hi Wei, On 05/01/2021 07:19, Wei Chen wrote: > It seems the arm inline function get_cycles has used 8 spaces for > line indent since 2012. This patch correct them to 4 spaces and > remove extra space between function name and bracket. > > Signed-off-by: Wei Chen <wei.chen@arm.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers, > --- > xen/include/asm-arm/time.h | 40 +++++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h > index 1b2c13614b..5c4529ebb5 100644 > --- a/xen/include/asm-arm/time.h > +++ b/xen/include/asm-arm/time.h > @@ -11,29 +11,29 @@ > > typedef uint64_t cycles_t; > > -static inline cycles_t get_cycles (void) > +static inline cycles_t get_cycles(void) > { > - isb(); > + isb(); > + /* > + * ARM_WORKAROUND_858921: Cortex-A73 (all versions) counter read > + * can return a wrong value when the counter crosses a 32bit boundary. > + */ > + if ( !check_workaround_858921() ) > + return READ_SYSREG64(CNTPCT_EL0); > + else > + { > /* > - * ARM_WORKAROUND_858921: Cortex-A73 (all versions) counter read > - * can return a wrong value when the counter crosses a 32bit boundary. > + * A recommended workaround for erratum 858921 is to: > + * 1- Read twice CNTPCT. > + * 2- Compare bit[32] of the two read values. > + * - If bit[32] is different, keep the old value. > + * - If bit[32] is the same, keep the new value. > */ > - if ( !check_workaround_858921() ) > - return READ_SYSREG64(CNTPCT_EL0); > - else > - { > - /* > - * A recommended workaround for erratum 858921 is to: > - * 1- Read twice CNTPCT. > - * 2- Compare bit[32] of the two read values. > - * - If bit[32] is different, keep the old value. > - * - If bit[32] is the same, keep the new value. > - */ > - cycles_t old, new; > - old = READ_SYSREG64(CNTPCT_EL0); > - new = READ_SYSREG64(CNTPCT_EL0); > - return (((old ^ new) >> 32) & 1) ? old : new; > - } > + cycles_t old, new; > + old = READ_SYSREG64(CNTPCT_EL0); > + new = READ_SYSREG64(CNTPCT_EL0); > + return (((old ^ new) >> 32) & 1) ? old : new; > + } > } > > /* List of timer's IRQ */ > -- Julien Grall ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-05 13:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-05 7:19 [PATCH 1/2] xen/arm: Correct the coding style of get_cycles Wei Chen 2021-01-05 7:19 ` [PATCH 2/2] xen/arm: Add defensive barrier in get_cycles for Arm64 Wei Chen 2021-01-05 11:17 ` Julien Grall 2021-01-05 13:45 ` Wei Chen 2021-01-05 11:04 ` [PATCH 1/2] xen/arm: Correct the coding style of get_cycles Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.