uboot-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.

Fixes: FS#4113

Signed-off-by: Mathias Kresin <dev@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

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@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@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@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)

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@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@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@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.

Mathias

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@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@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@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