[PATCH] uboot-lantiq: fix out of bounds cache invalidate
Daniel Schwierzeck
daniel.schwierzeck at gmail.com
Thu Nov 11 13:09:59 PST 2021
Am Mittwoch, dem 10.11.2021 um 21:59 +0100 schrieb Mathias Kresin:
> 11/3/21 2:43 PM, Daniel Schwierzeck:
> > Am Dienstag, dem 02.11.2021 um 23:35 +0100 schrieb Mathias Kresin:
> > > If u-boot is compiled with gcc 10+ the relevant part of the
> > > memory is
> > > used as following:
> > >
> > > variable BootFile is at address: 0x83ffe4ec
> > > variable data is at address: 0x83ffdee2
> > > variable data has len: 0x600
> > >
> > > Within invalidate_dcache_range() the cache covering the data
> > > variable
> > > should be invalidated (0x83ffdee2 till 0x83ffdef2).
> > >
> > > Due to dcache line size of 32 and some alignment correction, the
> > > actual
> > > memory range for which the cache is invalidated is from
> > > 0x83ffdee0
> > > till
> > > 0x83ffe500, which includes the location of the BootFile variable
> > > as
> > > well.
> > >
> > > Afterwards the BootFile variable is 0. The issue can be observed
> > > by
> > > using tftpboot, which always uses the default/generated filename
> > > instead
> > > of the specified one.
> > >
> > > To fix the issue, align the memory addresses properly before
> > > passing
> > > them to invalidate_dcache_range(). The misalignment can be
> > > observed
> > > with
> > > older gcc versions as well but the BootFile variable is placed
> > > more
> > > than
> > > 0x1000 bytes after the data variable and doesn't get null'ed.
> > >
> > > Fixes: FS#4113
> > >
> > > Signed-off-by: Mathias Kresin <dev at kresin.me>
> > > ---
> > > ...q-fix-out-of-bounds-cache-invalidate.patch | 48
> > > +++++++++++++++++++
> > > 1 file changed, 48 insertions(+)
> > > create mode 100644 package/boot/uboot-lantiq/patches/0031-dma-
> > > lantiq-fix-out-of-bounds-cache-invalidate.patch
> > >
> > > diff --git a/package/boot/uboot-lantiq/patches/0031-dma-lantiq-
> > > fix-
> > > out-of-bounds-cache-invalidate.patch b/package/boot/uboot-
> > > lantiq/patches/0031-dma-lantiq-fix-out-of-bounds-cache-
> > > invalidate.patch
> > > new file mode 100644
> > > index 0000000000..5a2bf39441
> > > --- /dev/null
> > > +++ b/package/boot/uboot-lantiq/patches/0031-dma-lantiq-fix-out-
> > > of-
> > > bounds-cache-invalidate.patch
> > > @@ -0,0 +1,48 @@
> > > +From f10ffb65865d5bfa5679293b13e027e0474df1fe Mon Sep 17
> > > 00:00:00
> > > 2001
> > > +From: Mathias Kresin <dev at kresin.me>
> > > +Date: Tue, 2 Nov 2021 21:24:29 +0100
> > > +Subject: [PATCH] dma: lantiq: fix out of bounds cache invalidate
> > > +
> > > +If u-boot is compiled with gcc 10+ the relevant part of the
> > > memory
> > > is
> > > +used as following:
> > > +
> > > +variable BootFile is at address: 0x83ffe4ec
> > > +variable data is at address: 0x83ffdee2
> > > +variable data has len: 0x600
> > > +
> > > +Within invalidate_dcache_range() the cache covering the data
> > > variable
> > > +should be invalidated (0x83ffdee2 till 0x83ffdef2).
> > > +
> > > +Due to dcache line size of 32 and some alignment correction, the
> > > actual
> > > +memory range for which the cache is invalidated is from
> > > 0x83ffdee0
> > > till
> > > +0x83ffe500, which includes the location of the BootFile variable
> > > as
> > > +well.
> > > +
> > > +Afterwards the BootFile variable is 0. The issue can be observed
> > > by
> > > +using tftpboot, which always uses the default/generated filename
> > > instead
> > > +of the specified one.
> > > +
> > > +To fix the issue, align the memory addresses properly before
> > > passing
> > > +them to invalidate_dcache_range(). The misalignment can be
> > > observed
> > > with
> > > +older gcc versions as well but the BootFile variable is placed
> > > more
> > > than
> > > +0x1000 bytes after the data variable and doesn't get null'ed.
> > > +
> > > +Signed-off-by: Mathias Kresin <dev at kresin.me>
> > > +---
> > > + drivers/dma/lantiq_dma.c | 4 ++--
> > > + 1 file changed, 2 insertions(+), 2 deletions(-)
> > > +
> > > +--- a/drivers/dma/lantiq_dma.c
> > > ++++ b/drivers/dma/lantiq_dma.c
> > > +@@ -122,9 +122,9 @@ static inline void ltq_dma_dcache_wb_inv
> > > +
> > > + static inline void ltq_dma_dcache_inv(const void *ptr, size_t
> > > size)
> > > + {
> > > +- unsigned long addr = (unsigned long) ptr;
> > > ++ unsigned long addr = (unsigned long) ptr &
> > > ~(ARCH_DMA_MINALIGN
> > > - 1);
> > > +
> > > +- invalidate_dcache_range(addr, addr + size);
> > > ++ invalidate_dcache_range(addr, ALIGN(addr + size,
> > > ARCH_DMA_MINALIGN));
> > > + }
> >
> > actually invalidate_dcache_range() already aligns start and end
> > address
> > to CONFIG_SYS_CACHELINE_SIZE which is the same as
> > ARCH_DMA_MINALIGN.
> >
> > In mainline we fixed the cache functions to always issue a sync()
> > after
> > the cache ops to wait for all HW transactions to be complete. The
> > Lantiq DMA driver only does this in ltq_dma_dcache_wb_inv() but not
> > in ltq_dma_dcache_inv(). Maybe adding a ltq_dma_sync() there would
> > also
> > fix your problem?
> >
> > > +
> > > + void ltq_dma_init(void)
>
> Hey Daniel,
>
> a had an even closer look at what is going on here. It all starts in
> the
> soc netdev driver (here lantiq_danube_etop.c).
>
> NetRxPackets[] points to cache line size aligned addresses. In
> ltq_eth_rx_packet_align() the address NetRxPackets[] points to is
> increased by LTQ_ETH_IP_ALIGN and the resulting not cache aligned
> address is used further on. While doing so, the len/size is never
> updated.
>
> The "not cache aligned address" + len/size for a cache aligned
> address
> is passed to invalidate_dcache_range(). Hence,
> invalidate_dcache_range()
> invalidates the next 32 bit as well, which causes the issue.
>
> variable BootFile is at address: 0x83ffe12c
> NetRxPackets[] points to 0x83ffdb20 (len is 0x600)
> data points to: 0x83ffdb22 (len is 0x600)
>
> ltq_dma_dcache_inv: 0x83ffdb22 (for len 0x600)
> invalidate_dcache_range: 0x83ffdb20 to 0x83ffe120 (size: 32)
> invalidate_dcache_range: 0x83ffdb20 to 0x83ffdb40 (Bootfile:
> a.bin)
> ...
> invalidate_dcache_range: 0x83ffe100 to 0x83ffe120 (Bootfile:
> a.bin)
> invalidate_dcache_range: 0x83ffe120 to 0x83ffe140 (Bootfile: )
>
> I see two possible approaches to fix the issue.
>
> 1. Do not move the pointer by LTQ_ETH_IP_ALIGN:
>
> To my own surprise Ethernet still works. But I've no idea why the
> pointer has to be moved by LTQ_ETH_IP_ALIGN nor why it still works
> if
> the pointer isn't moved. I'm quite sure the pointer is moved for a
> reason, which I do not get yet.
>
> 2. fix the alignment in ltq_dma_dcache_inv() as in my proposed change
>
> In the end, the pointer/startaddress gets corrected to the value
> it
> should be (0x83ffdb22 => 0x83ffdb20), the size matches again and
> only
> the expected cache range gets invalidated.
>
> Any recommendation which approach is the more sane one? Since I
> don't
> understand the purpose of LTQ_ETH_IP_ALIGN, I tend to go with 2.,
> but
> with an updated commit message.
the alignment of the IP header was inspired by Linux. In Linux you want
to have the IP header for RX packets aligned to a 16 byte boundary
respectively cache-line boundary because most modifications are made
there. Thus the packet buffer prepared for RX DNA is offset by 2 bytes
(due to ethernet header size of 14 bytes). The byte-offset feature of
Lantiq DMA can deal with such an offset. Actually this IP header
alignment doesn't do anything useful in U-Boot or improves performance.
I can't remember why I used that ;)
The U-Boot DMA driver already calculates this byte offset
automatically, that's why it doesn't make a difference whether you use
the LTQ_ETH_IP_ALIGN or not. But if I look at ltq_dma_tx_map()
and ltq_dma_rx_map(), the start address passed
to ltq_dma_dcache_wb_inv() is incorrect. This should be:
offset = dma_addr % ltq_dma_burst_align(dev->rx_burst_len);
ltq_dma_dcache_wb_inv(data - offset, len);
desc->addr = dma_addr - offset;
Then the start address passed to flush_dcache_range() is always aligned
to 32, 64 or 128 bytes dependent on configured DMA burst size. The end
address should be automatically aligned to ARCH_MIN_DMAALIGN by
flush_dcache_range(). Just aligning the start address to
ARCH_DMA_MINALIGN as you suggested doesn't consider the DMA burst size
which is configured by the DMA client. It randomly works in your case
because lantiq_danube_etop.c passes LTQ_DMA_BURST_2WORDS
to ltq_dma_register().
>
> Mathias
--
- Daniel
More information about the openwrt-devel
mailing list