[PATCH 2/2] arm64/arch_timer: replace arch_counter_enforce_ordering() with isb

[Pingfan Liu]

On Tue, Mar 30, 2021 at 12:05:53PM +0100, Will Deacon wrote:
Hi Will,

Thank you for kindly review.

I have several questions haunting on this issue. Hope further help from you.

> On Tue, Mar 30, 2021 at 06:57:19PM +0800, Pingfan Liu wrote:
> > The description of getting counter value is not very clear. [1]
> > 'mrs Xt, cntpct' may execute out of program order, either forward or
> > backward.
> > 
> > Now taking a look at this group of getting counter routines. All of them
> > are called from sched_clock(). And there is an isb to protect forward
> > speculation. But there is no isb for the backward speculation.
> > 
> > The current code enforces read dependency instructions anchored on
> > getting counter. But it is not enough to protect against other no
> > dependency instructions, and even function call can not prevent the
> > speculation between getting counter and them.
> 
> Which "no dependency instructions"?
> 
In fact, I have two questions here. 

-1. let me start with the backward speculation.
No such instructions inside this function, but combining the caller and callee,
it will look like:
00000000000002e0 <sched_clock>:
...
 318:   f9400e60        ldr     x0, [x19, #24]
 31c:   d63f0000        blr     x0
					0000000000000030 <arch_counter_get_cntpct>:
					      30:       d503233f        paciasp
					      34:       d5033fdf        isb
					      38:       d53be020        mrs     x0, cntpct_el0
					      3c:       ca000001        eor     x1, x0, x0
					      40:       8b2163e1        add     x1, sp, x1
					      44:       f940003f        ldr     xzr, [x1]
					      48:       d50323bf        autiasp
					      4c:       d65f03c0        ret
		
 320:   29441261        ldp     w1, w4, [x19, #32]
...
 33c:   cb060000        sub     x0, x0, x6

There is no 'instruction sync semantics' applied on 'ret', so
arch_counter_get_cntpct() may return to 320 before its instrutions are finished
in pipeline, and then new instructions are fetched into pipeline.

In theory (no idea about better material than [1]), any instruction touching
x0, but have no data dependency anchored on getting cntpct will take the risk
of speculation with getting cntpct. Can this happen?  (I admit there is no such
instruction in disasemble, but does it vary due to compiler?)

Making an analogy to isb at the entry. If this speculation is not possible, could
it be eliminated?


-2. Is arch_counter_enforce_ordering() needed?
Since the return value of arch_counter_get_cntpct() is always read, and
naturally, this read op will observe the read-barrier.
Here this instruction is "33c:   cb060000        sub     x0, x0, x6"


> > 
> > Replacing arch_counter_enforce_ordering() with isb to achieve the aim.
> > 
> > [1]: AArch64 Programmer's Guides Generic Timer:  3.1. Count and frequency
> 
> I wouldn't trust that guide as far as I can throw it.
> 
> Please describe the problem you're trying to solve, and hopefully I can
> help. ISB is an expensive instruction so we need a good justification to
> add it here (i.e. an example of why the current scheme is not correct).
> 
Yes, ISB is expensive, and original, I consider whether it can be eliminated
totally, but finally run to this reverse conclusion.

On the contrary, if this kind of speculation is not real, plus the fact that
there is already a natural read on getting cntvct. Can the second isb in
__arch_get_hw_counter() just be replaced with compiler barrier,


Thanks,

Pingfan

---
In case of reference

00000000000002e0 <sched_clock>:
 2e0:   d503233f        paciasp
 2e4:   a9bc7bfd        stp     x29, x30, [sp, #-64]!
 2e8:   910003fd        mov     x29, sp
 2ec:   a90153f3        stp     x19, x20, [sp, #16]
 2f0:   90000014        adrp    x20, 0 <jiffy_sched_clock_read>
 2f4:   91000294        add     x20, x20, #0x0
 2f8:   a90363f7        stp     x23, x24, [sp, #48]
 2fc:   91002297        add     x23, x20, #0x8
 300:   52800518        mov     w24, #0x28                      // #40
 304:   a9025bf5        stp     x21, x22, [sp, #32]
 308:   b9400296        ldr     w22, [x20]
 30c:   120002d5        and     w21, w22, #0x1
 310:   9bb87eb5        umull   x21, w21, w24
 314:   8b1502f3        add     x19, x23, x21
 318:   f9400e60        ldr     x0, [x19, #24]
 31c:   d63f0000        blr     x0
 320:   29441261        ldp     w1, w4, [x19, #32]
 324:   f8756ae3        ldr     x3, [x23, x21]
 328:   a9409666        ldp     x6, x5, [x19, #8]
 32c:   d50339bf        dmb     ishld
 330:   b9400282        ldr     w2, [x20]
 334:   6b16005f        cmp     w2, w22
 338:   54fffe81        b.ne    308 <sched_clock+0x28>  // b.any
 33c:   cb060000        sub     x0, x0, x6
 340:   2a0103e1        mov     w1, w1
 344:   8a050000        and     x0, x0, x5
 348:   a94153f3        ldp     x19, x20, [sp, #16]
 34c:   9b017c00        mul     x0, x0, x1
 350:   a9425bf5        ldp     x21, x22, [sp, #32]
 354:   9ac42400        lsr     x0, x0, x4
 358:   8b030000        add     x0, x0, x3
 35c:   a94363f7        ldp     x23, x24, [sp, #48]
 360:   a8c47bfd        ldp     x29, x30, [sp], #64
 364:   d50323bf        autiasp
 368:   d65f03c0        ret