The problem about arm64: io: Relax implicit barriers in default I/O accessors

The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Frank Li]

Will Deacon:

	Our a test case is failure at 8QM platform(arm64).  USB transfer failure if run with GPU stress test.
	I found it related with your below change. 
	
commit 22ec71615d824f4f11d38d0e55a88d8956b7e45f
Author: Will Deacon <will@kernel.org>
Date:   Fri Jun 7 15:48:58 2019 +0100

    arm64: io: Relax implicit barriers in default I/O accessors

    The arm64 implementation of the default I/O accessors requires barrier
    instructions to satisfy the memory ordering requirements documented in
    memory-barriers.txt [1], which are largely derived from the behaviour of
    I/O accesses on x86.
 
drivers/usb/host/xhci-ring.c

static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
                unsigned int ep_index, unsigned int stream_id, int start_cycle,
                struct xhci_generic_trb *start_trb)
{
        /*
         * Pass all the TRBs to the hardware at once and make sure this write
         * isn't reordered.
         */
        wmb();
        if (start_cycle)
                start_trb->field[3] |= cpu_to_le32(start_cycle);
        else
                start_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
        xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
}

	If I added wmb() before xhci_ring_ep_doorbell, the problem gone.  Writel include io_wmb, which map into dma_wmb(). 
	
	1. write ddr
	2. writel
		2a. io_wmb(),   dmb(oshst)
		2b, write usb register
	3. usb dma read ddr.

	
	Internal bus fabric only guarantee the order for the same AXID.  1 write ddr may be slow.  USB register get data before 1 because GPU occupy ddr now.  So USB DMA start read from ddr and get old dma descriptor data and find not ready yet, then missed door bell. 

	If do 2-3 times doorbell, problem also gone.

	So I think dmb(oshst) is not enough for writel. 

       A writeX() by the CPU to the peripheral will first wait for the
        completion of all prior CPU writes to memory. For example, this ensures
        that writes by the CPU to an outbound DMA buffer allocated by
        dma_alloc_coherent() will be visible to a DMA engine when the CPU writes
        to its MMIO control register to trigger the transfer.

	
Best regards
Frank Li

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Frank Li]

> -----Original Message-----
> From: Frank Li
> Sent: Monday, June 14, 2021 5:42 PM
> To: Will Deacon <will@kernel.org>
> Cc: Shenwei Wang <shenwei.wang@nxp.com>; Han Xu <han.xu@nxp.com>;
> Nitin Garg <nitin.garg@nxp.com>; Jason Liu <jason.hui.liu@nxp.com>; linux-
> arm-kernel@lists.infradead.org; Zhi Li <lznuaa@gmail.com>
> Subject: The problem about arm64: io: Relax implicit barriers in default I/O
> accessors

Added Catalin. 

> 
> Will Deacon:
> 
> 	Our a test case is failure at 8QM platform(arm64).  USB transfer
> failure if run with GPU stress test.
> 	I found it related with your below change.
> 
> commit 22ec71615d824f4f11d38d0e55a88d8956b7e45f
> Author: Will Deacon <will@kernel.org>
> Date:   Fri Jun 7 15:48:58 2019 +0100
> 
>     arm64: io: Relax implicit barriers in default I/O accessors
> 
>     The arm64 implementation of the default I/O accessors requires barrier
>     instructions to satisfy the memory ordering requirements documented in
>     memory-barriers.txt [1], which are largely derived from the behaviour of
>     I/O accesses on x86.
> 
> drivers/usb/host/xhci-ring.c
> 
> static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
>                 unsigned int ep_index, unsigned int stream_id, int start_cycle,
>                 struct xhci_generic_trb *start_trb)
> {
>         /*
>          * Pass all the TRBs to the hardware at once and make sure this write
>          * isn't reordered.
>          */
>         wmb();
>         if (start_cycle)
>                 start_trb->field[3] |= cpu_to_le32(start_cycle);
>         else
>                 start_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
>         xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
> }
> 
> 	If I added wmb() before xhci_ring_ep_doorbell, the problem gone.
> Writel include io_wmb, which map into dma_wmb().
> 
> 	1. write ddr
> 	2. writel
> 		2a. io_wmb(),   dmb(oshst)
> 		2b, write usb register
> 	3. usb dma read ddr.
> 
> 
> 	Internal bus fabric only guarantee the order for the same AXID.  1
> write ddr may be slow.  USB register get data before 1 because GPU occupy
> ddr now.  So USB DMA start read from ddr and get old dma descriptor data
> and find not ready yet, then missed door bell.
> 
> 	If do 2-3 times doorbell, problem also gone.
> 
> 	So I think dmb(oshst) is not enough for writel.
> 
>        A writeX() by the CPU to the peripheral will first wait for the
>         completion of all prior CPU writes to memory. For example, this ensures
>         that writes by the CPU to an outbound DMA buffer allocated by
>         dma_alloc_coherent() will be visible to a DMA engine when the CPU
> writes
>         to its MMIO control register to trigger the transfer.
> 
> 
> Best regards
> Frank Li
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Frank Li]

> 
> > -----Original Message-----
> > From: Frank Li
> > Sent: Monday, June 14, 2021 5:42 PM
> > To: Will Deacon <will@kernel.org>
> > Cc: Shenwei Wang <shenwei.wang@nxp.com>; Han Xu <han.xu@nxp.com>;
> > Nitin Garg <nitin.garg@nxp.com>; Jason Liu <jason.hui.liu@nxp.com>;
> linux-
> > arm-kernel@lists.infradead.org; Zhi Li <lznuaa@gmail.com>
> > Subject: The problem about arm64: io: Relax implicit barriers in default I/O
> > accessors
> 
> Added Catalin.
[Frank Li] sorry, corrected catalin's address
> 
> >
> > Will Deacon:
> >
> > 	Our a test case is failure at 8QM platform(arm64).  USB transfer
> > failure if run with GPU stress test.
> > 	I found it related with your below change.
> >
> > commit 22ec71615d824f4f11d38d0e55a88d8956b7e45f
> > Author: Will Deacon <will@kernel.org>
> > Date:   Fri Jun 7 15:48:58 2019 +0100
> >
> >     arm64: io: Relax implicit barriers in default I/O accessors
> >
> >     The arm64 implementation of the default I/O accessors requires barrier
> >     instructions to satisfy the memory ordering requirements documented in
> >     memory-barriers.txt [1], which are largely derived from the behaviour of
> >     I/O accesses on x86.
> >
> > drivers/usb/host/xhci-ring.c
> >
> > static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
> >                 unsigned int ep_index, unsigned int stream_id, int start_cycle,
> >                 struct xhci_generic_trb *start_trb)
> > {
> >         /*
> >          * Pass all the TRBs to the hardware at once and make sure this write
> >          * isn't reordered.
> >          */
> >         wmb();
> >         if (start_cycle)
> >                 start_trb->field[3] |= cpu_to_le32(start_cycle);
> >         else
> >                 start_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
> >         xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
> > }
> >
> > 	If I added wmb() before xhci_ring_ep_doorbell, the problem gone.
> > Writel include io_wmb, which map into dma_wmb().
> >
> > 	1. write ddr
> > 	2. writel
> > 		2a. io_wmb(),   dmb(oshst)
> > 		2b, write usb register
> > 	3. usb dma read ddr.
> >
> >
> > 	Internal bus fabric only guarantee the order for the same AXID.  1
> > write ddr may be slow.  USB register get data before 1 because GPU occupy
> > ddr now.  So USB DMA start read from ddr and get old dma descriptor data
> > and find not ready yet, then missed door bell.
> >
> > 	If do 2-3 times doorbell, problem also gone.
> >
> > 	So I think dmb(oshst) is not enough for writel.
> >
> >        A writeX() by the CPU to the peripheral will first wait for the
> >         completion of all prior CPU writes to memory. For example, this
> ensures
> >         that writes by the CPU to an outbound DMA buffer allocated by
> >         dma_alloc_coherent() will be visible to a DMA engine when the CPU
> > writes
> >         to its MMIO control register to trigger the transfer.
> >
> >
> > Best regards
> > Frank Li

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Catalin Marinas]

On Mon, Jun 14, 2021 at 10:41:38PM +0000, Frank Li wrote:
> commit 22ec71615d824f4f11d38d0e55a88d8956b7e45f
> Author: Will Deacon <will@kernel.org>
> Date:   Fri Jun 7 15:48:58 2019 +0100
> 
>     arm64: io: Relax implicit barriers in default I/O accessors
> 
>     The arm64 implementation of the default I/O accessors requires barrier
>     instructions to satisfy the memory ordering requirements documented in
>     memory-barriers.txt [1], which are largely derived from the behaviour of
>     I/O accesses on x86.
[...]
> 	If I added wmb() before xhci_ring_ep_doorbell, the problem gone.
> 	Writel include io_wmb, which map into dma_wmb(). 
> 	
> 	1. write ddr
> 	2. writel
> 		2a. io_wmb(),   dmb(oshst)
> 		2b, write usb register
> 	3. usb dma read ddr.
> 
> 	
> 	Internal bus fabric only guarantee the order for the same AXID.
> 	1 write ddr may be slow.  USB register get data before 1 because
> 	GPU occupy ddr now.  So USB DMA start read from ddr and get old
> 	dma descriptor data and find not ready yet, then missed door
> 	bell. 

That's a complex topic, Will should have a better answer. I'll try some
thought exercise below introducing a hypothetical second CPU.

From Will's commit above w.r.t. other-multi-copy atomicity:

      1. A write arriving at an endpoint shared between multiple CPUs is
         visible to all CPUs

      2. A write that is visible to all CPUs is also visible to all other
         observers in the shareability domain

So (1) would be the write to the USB device which is also an observer in
the system (of the DDR writes). (2) refers to the write to the DDR.

If we have CPU0 writing to DDR, followed by DMB and the write to the USB
device, a CPU1 observing the write to the USB device would also observe
the write to DDR (with a DMB between them). Since the USB device is an
observer and the system is multi-copy atomic, the USB should also
observe the CPU0 write to the DDR if CPU1 observed it.

CPU1 can only observe the write to the USB device via an access to that
USB device (e.g. a register read). Such access probably goes through
some serialisation point and the DMB on CPU0 ensures that the prior
write to DDR is visible. Now, a CPU1 read from the USB device cannot
affect the DMA access that the USB device started to the DDR, so we can
take it out of the equation. However, this means that the hardware
should ensure such ordering USB DMA ordering otherwise it wouldn't be
multi-copy atomic (or our understanding of it).

Either the hardware doesn't match the memory model or our reasoning is
incorrect (both are possible ;)).

I wonder whether we can look at this in a different way: the USB device
doing a "speculative" access to the DDR before the write to USB is
globally observable. There isn't a way to fix it in the USB device since
it does not observe the write to its register, so we are left with
having to guarantee the completion of the write to the DDR before
informing the USB about it.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Will Deacon]

On Wed, Jun 16, 2021 at 07:40:23PM +0100, Catalin Marinas wrote:
> On Mon, Jun 14, 2021 at 10:41:38PM +0000, Frank Li wrote:
> > commit 22ec71615d824f4f11d38d0e55a88d8956b7e45f
> > Author: Will Deacon <will@kernel.org>
> > Date:   Fri Jun 7 15:48:58 2019 +0100
> > 
> >     arm64: io: Relax implicit barriers in default I/O accessors
> > 
> >     The arm64 implementation of the default I/O accessors requires barrier
> >     instructions to satisfy the memory ordering requirements documented in
> >     memory-barriers.txt [1], which are largely derived from the behaviour of
> >     I/O accesses on x86.
> [...]
> > 	If I added wmb() before xhci_ring_ep_doorbell, the problem gone.
> > 	Writel include io_wmb, which map into dma_wmb(). 
> > 	
> > 	1. write ddr
> > 	2. writel
> > 		2a. io_wmb(),   dmb(oshst)
> > 		2b, write usb register
> > 	3. usb dma read ddr.
> > 
> > 	
> > 	Internal bus fabric only guarantee the order for the same AXID.
> > 	1 write ddr may be slow.  USB register get data before 1 because
> > 	GPU occupy ddr now.  So USB DMA start read from ddr and get old
> > 	dma descriptor data and find not ready yet, then missed door
> > 	bell. 
> 
> That's a complex topic, Will should have a better answer. I'll try some
> thought exercise below introducing a hypothetical second CPU.

It would also be helpful to know a bit more about the hardware:

  - What is the "internal bus fabric"?
  - Can you be more specific about the AxIDs? I can't tell how that
    correlates back to code running on the CPU.
  - Is the device cache coherent?
  - What memory types are used to map the DDR and the USB register on the
    CPU? (I got lost in the indirection)

Also, do you know which part of the data appears to be stale when the device
reads it?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Will Deacon]

On Thu, Jun 17, 2021 at 06:25:28PM +0100, Will Deacon wrote:
> On Thu, Jun 17, 2021 at 10:27:44AM +0100, Catalin Marinas wrote:
> > On Wed, Jun 16, 2021 at 02:24:39PM -0500, Zhi Li wrote:
> > > On Wed, Jun 16, 2021 at 2:18 PM Frank Li <frank.li@nxp.com> wrote:
> > > > Will Deacon wrote:
> > > > > It would also be helpful to know a bit more about the hardware:
> > > > >
> > > > >   - What is the "internal bus fabric"?
> > > 
> > > > Look like ARM call as "Interconnect",  Multi AXI master and multi AXI slave
> > > > connected together. 
> > > 
> > > I  drawed simplified bus structure. 
> > >  
> > >         ┌──────┐ ┌────┐
> > >         │ A53  │ │A72 │
> > >         └───┬──┘ └─┬──┘
> > >             │      │
> > >         ┌───▼──────▼──┐
> > >         │    CCI400   │
> > >         └─────┬───────┘
> > >               │   1 (a)write to ddr (normal uncached memory)
> > >               │   DMB OSHST
> > >               │   2 (b)write to usb register(device, nGnRE)
> > >         ┌─────▼───────────────────────┐       ┌───────────┐
> > >         │                             ◄───────┤   GPU     │
> > >         │     Bus fabric              │       │           │
> > >         └────────────────────────────┬┘       └───────────┘
> > > 3 (b) reach usb   ▲ 4 usb read   ▲   │ 6.(a)reach
> > >          │        │   ddr        │   │
> > >       ┌──▼────────┴─┐            │   │
> > >       │             │            │   │
> > >       │  USB        │      5.usb │   │
> > >       │             │      read  │   │
> > >       └─────────────┘            │   │
> > >                                ┌─┴───▼─┐
> > >                                │       │
> > >                                │ DDR   │
> > >                                │       │
> > >                                └───────┘
> > 
> > Since you sent an HTML message, it was rejected by the list server. The
> > above is a plain-text rendition by w3m (and changed barrier() to DMB
> > OSHST).
> > 
> > Is the DMB propagated to the bus fabric? IIUC, our logic is that if the
> > write (b) to USB is observable by, let's say, the GPU, the same GPU
> > should also observe the write (a) to DDR. Since the write (a) to DDR is
> > globally observable, the USB device read at (4) should also observe it
> > (well, we may be wrong).
> 
> It's pretty rare for barriers to propagate onto the fabric -- usually the
> CPU just orders everything based on acknowledgements. If the CCI gives the
> write response for the non-cacheable write I could see that causing an issue
> if the bus fabric can then reorder accesses, but then I would argue that's a
> broken system because simple ring buffers in non-cacheable memory would fail
> for peripherals hooking into the bus fabric (i.e. dma_*mb() would be
> broken). I think it would also mean that DSB doesn't necessarily fix the
> issue, it probably just makes it less likely because it takes longer to
> get the device write out after the acknowledgement -- ndelay() would achieve
> the same effect :)
> 
> Frank -- what happens if you try either DMB SY, or DMB OSH (without the ST)
> in writel()?

Also, digging into the A72 TRM there are a bunch of configuration signals
in this area; see SYSBARDISABLE and BROADCASTOUTER, for example.

Does the failure happen on both a53 and a72, or only on one CPU type?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Will Deacon]

On Thu, Jun 17, 2021 at 10:27:44AM +0100, Catalin Marinas wrote:
> On Wed, Jun 16, 2021 at 02:24:39PM -0500, Zhi Li wrote:
> > On Wed, Jun 16, 2021 at 2:18 PM Frank Li <frank.li@nxp.com> wrote:
> > > Will Deacon wrote:
> > > > It would also be helpful to know a bit more about the hardware:
> > > >
> > > >   - What is the "internal bus fabric"?
> > 
> > > Look like ARM call as "Interconnect",  Multi AXI master and multi AXI slave
> > > connected together. 
> > 
> > I  drawed simplified bus structure. 
> >  
> >         ┌──────┐ ┌────┐
> >         │ A53  │ │A72 │
> >         └───┬──┘ └─┬──┘
> >             │      │
> >         ┌───▼──────▼──┐
> >         │    CCI400   │
> >         └─────┬───────┘
> >               │   1 (a)write to ddr (normal uncached memory)
> >               │   DMB OSHST
> >               │   2 (b)write to usb register(device, nGnRE)
> >         ┌─────▼───────────────────────┐       ┌───────────┐
> >         │                             ◄───────┤   GPU     │
> >         │     Bus fabric              │       │           │
> >         └────────────────────────────┬┘       └───────────┘
> > 3 (b) reach usb   ▲ 4 usb read   ▲   │ 6.(a)reach
> >          │        │   ddr        │   │
> >       ┌──▼────────┴─┐            │   │
> >       │             │            │   │
> >       │  USB        │      5.usb │   │
> >       │             │      read  │   │
> >       └─────────────┘            │   │
> >                                ┌─┴───▼─┐
> >                                │       │
> >                                │ DDR   │
> >                                │       │
> >                                └───────┘
> 
> Since you sent an HTML message, it was rejected by the list server. The
> above is a plain-text rendition by w3m (and changed barrier() to DMB
> OSHST).
> 
> Is the DMB propagated to the bus fabric? IIUC, our logic is that if the
> write (b) to USB is observable by, let's say, the GPU, the same GPU
> should also observe the write (a) to DDR. Since the write (a) to DDR is
> globally observable, the USB device read at (4) should also observe it
> (well, we may be wrong).

It's pretty rare for barriers to propagate onto the fabric -- usually the
CPU just orders everything based on acknowledgements. If the CCI gives the
write response for the non-cacheable write I could see that causing an issue
if the bus fabric can then reorder accesses, but then I would argue that's a
broken system because simple ring buffers in non-cacheable memory would fail
for peripherals hooking into the bus fabric (i.e. dma_*mb() would be
broken). I think it would also mean that DSB doesn't necessarily fix the
issue, it probably just makes it less likely because it takes longer to
get the device write out after the acknowledgement -- ndelay() would achieve
the same effect :)

Frank -- what happens if you try either DMB SY, or DMB OSH (without the ST)
in writel()?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: The problem about arm64: io: Relax implicit barriers in default I/O accessors

[Catalin Marinas]

On Wed, Jun 16, 2021 at 02:24:39PM -0500, Zhi Li wrote:
> On Wed, Jun 16, 2021 at 2:18 PM Frank Li <frank.li@nxp.com> wrote:
> > Will Deacon wrote:
> > > It would also be helpful to know a bit more about the hardware:
> > >
> > >   - What is the "internal bus fabric"?
> 
> > Look like ARM call as "Interconnect",  Multi AXI master and multi AXI slave
> > connected together. 
> 
> I  drawed simplified bus structure. 
>  
>         ┌──────┐ ┌────┐
>         │ A53  │ │A72 │
>         └───┬──┘ └─┬──┘
>             │      │
>         ┌───▼──────▼──┐
>         │    CCI400   │
>         └─────┬───────┘
>               │   1 (a)write to ddr (normal uncached memory)
>               │   DMB OSHST
>               │   2 (b)write to usb register(device, nGnRE)
>         ┌─────▼───────────────────────┐       ┌───────────┐
>         │                             ◄───────┤   GPU     │
>         │     Bus fabric              │       │           │
>         └────────────────────────────┬┘       └───────────┘
> 3 (b) reach usb   ▲ 4 usb read   ▲   │ 6.(a)reach
>          │        │   ddr        │   │
>       ┌──▼────────┴─┐            │   │
>       │             │            │   │
>       │  USB        │      5.usb │   │
>       │             │      read  │   │
>       └─────────────┘            │   │
>                                ┌─┴───▼─┐
>                                │       │
>                                │ DDR   │
>                                │       │
>                                └───────┘

Since you sent an HTML message, it was rejected by the list server. The
above is a plain-text rendition by w3m (and changed barrier() to DMB
OSHST).

Is the DMB propagated to the bus fabric? IIUC, our logic is that if the
write (b) to USB is observable by, let's say, the GPU, the same GPU
should also observe the write (a) to DDR. Since the write (a) to DDR is
globally observable, the USB device read at (4) should also observe it
(well, we may be wrong).

So while the bus fabric could ensure the ordering of the DDR write (a)
and the USB write (b) from the perspective of a third observer (the
GPU), I don't see how it can force it from the USB perspective as it
cannot observe the write (b) to its registers.

Replacing the DMB with the DSB forces the write (a) to reach the DDR on
your platform.

Will, any better idea of why it goes wrong?

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel