[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Flush loaded data cacheline aligned. This avoids warnings such as
CACHE: Misaligned operation at range [81000000, 816d0fa8]

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---
Why do we actually have to flush caches after load? It seems to
have worked so far despite the caches did not get flushed (due to
missalignment).

--
Stefan

 cmd/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/net.c b/cmd/net.c
index b2f3c7b..540daeb 100644
--- a/cmd/net.c
+++ b/cmd/net.c
@@ -244,7 +244,7 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
 	}
 
 	/* flush cache */
-	flush_cache(load_addr, size);
+	flush_cache(load_addr, ALIGN(size, CONFIG_SYS_CACHELINE_SIZE));
 
 	bootstage_mark(BOOTSTAGE_ID_NET_LOADED);
 
-- 
2.9.0

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Fabio Estevam]

Hi Stefan,

On Tue, Aug 2, 2016 at 4:20 AM, Stefan Agner <stefan@agner.ch> wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Flush loaded data cacheline aligned. This avoids warnings such as
> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>

This fixes the cache warnings:

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Simon Glass]

On 2 August 2016 at 01:20, Stefan Agner <stefan@agner.ch> wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Flush loaded data cacheline aligned. This avoids warnings such as
> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> Why do we actually have to flush caches after load? It seems to
> have worked so far despite the caches did not get flushed (due to
> missalignment).
>
> --
> Stefan
>
>  cmd/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Joe Hershberger]

On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Flush loaded data cacheline aligned. This avoids warnings such as
> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---

This was already rejected once.
http://lists.denx.de/pipermail/u-boot/2012-April/121564.html

> Why do we actually have to flush caches after load? It seems to
> have worked so far despite the caches did not get flushed (due to
> missalignment).

I'm not sure that we do, but it's been there since as far back as the
git history goes. Maybe Wolfgang has memory of a reason.

-Joe

> --
> Stefan
>
>  cmd/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmd/net.c b/cmd/net.c
> index b2f3c7b..540daeb 100644
> --- a/cmd/net.c
> +++ b/cmd/net.c
> @@ -244,7 +244,7 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
>         }
>
>         /* flush cache */
> -       flush_cache(load_addr, size);
> +       flush_cache(load_addr, ALIGN(size, CONFIG_SYS_CACHELINE_SIZE));
>
>         bootstage_mark(BOOTSTAGE_ID_NET_LOADED);
>
> --
> 2.9.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Simon Glass]

Hi Joe,

On 3 August 2016 at 17:18, Joe Hershberger <joe.hershberger@gmail.com> wrote:
> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Flush loaded data cacheline aligned. This avoids warnings such as
>> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
>
> This was already rejected once.
> http://lists.denx.de/pipermail/u-boot/2012-April/121564.html
>
>> Why do we actually have to flush caches after load? It seems to
>> have worked so far despite the caches did not get flushed (due to
>> missalignment).
>
> I'm not sure that we do, but it's been there since as far back as the
> git history goes. Maybe Wolfgang has memory of a reason.

I think this is the correct solution. If you load something to memory
you have to flush the cache. If someone is brave enough to load an
image that finishes immediately before something where there is data
that will be overwritten by the flush, then they deserve some pain.

Anytime we flush the cache we have to do it according the rules of the
cache. One of those rules is that cache lines are larger than one
byte. So either we don't flush or we do, and if we do, we must align.

Regards,
Simon

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Stefan Agner]

On 2016-08-03 16:18, Joe Hershberger wrote:
> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Flush loaded data cacheline aligned. This avoids warnings such as
>> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
> 
> This was already rejected once.
> http://lists.denx.de/pipermail/u-boot/2012-April/121564.html

Oh I see, and in the end the message was converted to a debug() call, in
essence turning the whole problem back under a stone... :-)

FWIW, I largely support Mike Frysinger's position in that discussion,
and think it should be fine to flush these extra bytes...

> 
>> Why do we actually have to flush caches after load? It seems to
>> have worked so far despite the caches did not get flushed (due to
>> missalignment).

Btw, I need to correct myself here: flush_dcache ultimately calls
v7_dcache_clean_inval_range, which does not bail out in the unaligned
case. Instead, afaict, it flushes until mva < stop, hence flushes the
last line anyway...

> 
> I'm not sure that we do, but it's been there since as far back as the
> git history goes. Maybe Wolfgang has memory of a reason.

The only reason why it might be preferable to have loaded stuff in main
memory is with nowadays heterogeneous architectures (e.g. i.MX 7 with
its secondary M4 core). In those cases we need to flush caches since
those two cores are no cache coherent. However, I don't think that all
load commands are doing the flush, so not sure if can rely on that
today...

--
Stefan

> 
> -Joe
> 
>> --
>> Stefan
>>
>>  cmd/net.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/cmd/net.c b/cmd/net.c
>> index b2f3c7b..540daeb 100644
>> --- a/cmd/net.c
>> +++ b/cmd/net.c
>> @@ -244,7 +244,7 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
>>         }
>>
>>         /* flush cache */
>> -       flush_cache(load_addr, size);
>> +       flush_cache(load_addr, ALIGN(size, CONFIG_SYS_CACHELINE_SIZE));
>>
>>         bootstage_mark(BOOTSTAGE_ID_NET_LOADED);
>>
>> --
>> 2.9.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Joe Hershberger]

Hi Simon,

On Wed, Aug 3, 2016 at 8:17 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On 3 August 2016 at 17:18, Joe Hershberger <joe.hershberger@gmail.com> wrote:
>> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> Flush loaded data cacheline aligned. This avoids warnings such as
>>> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>>>
>>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>> ---
>>
>> This was already rejected once.
>> http://lists.denx.de/pipermail/u-boot/2012-April/121564.html
>>
>>> Why do we actually have to flush caches after load? It seems to
>>> have worked so far despite the caches did not get flushed (due to
>>> missalignment).
>>
>> I'm not sure that we do, but it's been there since as far back as the
>> git history goes. Maybe Wolfgang has memory of a reason.
>
> I think this is the correct solution. If you load something to memory
> you have to flush the cache. If someone is brave enough to load an
> image that finishes immediately before something where there is data
> that will be overwritten by the flush, then they deserve some pain.

I'm not at all worried about the overwrite case.

> Anytime we flush the cache we have to do it according the rules of the
> cache. One of those rules is that cache lines are larger than one
> byte. So either we don't flush or we do, and if we do, we must align.

For sure... I was just wondering why we need this flush at all.
Stefan's comment as to why seems reasonable now, but that reasoning
surely wasn't a concern pre-2000. Maybe some early arch needed it for
a different reason? Maybe someone was planning to DMA directly from
the memory that was loaded, but in that case it seems like the code
starting the DMA would ensure the flush, not the code that is
memcpy'ing from somewhere.

Cheers,
-Joe

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Stefan Agner]

On 2016-08-04 09:14, Joe Hershberger wrote:
> Hi Simon,
> 
> On Wed, Aug 3, 2016 at 8:17 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Joe,
>>
>> On 3 August 2016 at 17:18, Joe Hershberger <joe.hershberger@gmail.com> wrote:
>>> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>
>>>> Flush loaded data cacheline aligned. This avoids warnings such as
>>>> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>>>>
>>>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>>> ---
>>>
>>> This was already rejected once.
>>> http://lists.denx.de/pipermail/u-boot/2012-April/121564.html
>>>
>>>> Why do we actually have to flush caches after load? It seems to
>>>> have worked so far despite the caches did not get flushed (due to
>>>> missalignment).
>>>
>>> I'm not sure that we do, but it's been there since as far back as the
>>> git history goes. Maybe Wolfgang has memory of a reason.
>>
>> I think this is the correct solution. If you load something to memory
>> you have to flush the cache. If someone is brave enough to load an
>> image that finishes immediately before something where there is data
>> that will be overwritten by the flush, then they deserve some pain.
> 
> I'm not at all worried about the overwrite case.
> 
>> Anytime we flush the cache we have to do it according the rules of the
>> cache. One of those rules is that cache lines are larger than one
>> byte. So either we don't flush or we do, and if we do, we must align.
> 
> For sure... I was just wondering why we need this flush at all.
> Stefan's comment as to why seems reasonable now, but that reasoning
> surely wasn't a concern pre-2000. Maybe some early arch needed it for
> a different reason? Maybe someone was planning to DMA directly from
> the memory that was loaded, but in that case it seems like the code
> starting the DMA would ensure the flush, not the code that is
> memcpy'ing from somewhere.

Just to clarify my point on this:
Non cache coherent (peripheral) CPU's was the only reason I could think
of, but I don't think it is a reason to actually do it.

IMHO it should be handled the same way as DMA gets handled: if a non
cache coherent CPU is about to get started, that code should make sure
to handle the cache flush... (e.g. the bootaux command in the i.MX 7
case).

--
Stefan

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Tom Rini]

On Wed, Aug 03, 2016 at 07:43:24PM -0700, Stefan Agner wrote:
> On 2016-08-03 16:18, Joe Hershberger wrote:
> > On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
> >> From: Stefan Agner <stefan.agner@toradex.com>
> >>
> >> Flush loaded data cacheline aligned. This avoids warnings such as
> >> CACHE: Misaligned operation at range [81000000, 816d0fa8]
> >>
> >> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> >> ---
> > 
> > This was already rejected once.
> > http://lists.denx.de/pipermail/u-boot/2012-April/121564.html
> 
> Oh I see, and in the end the message was converted to a debug() call, in
> essence turning the whole problem back under a stone... :-)
> 
> FWIW, I largely support Mike Frysinger's position in that discussion,
> and think it should be fine to flush these extra bytes...

I re-read most of that thread last-night and yes, it seems like rather
than solving the problem we just put it into a debug().  But if I
followed Mike's point right, we shouldn't be having a debug there
either, the flushes are fine?  And as to Marek's point there about
driver bugs, it's perhaps only sub-optimal rather than a broken thing?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160804/d8641a35/attachment.sig>

[U-Boot] [PATCH] cmd: net: flush cache cacheline aligned

[Stefan Agner]

On 2016-08-04 10:46, Tom Rini wrote:
> On Wed, Aug 03, 2016 at 07:43:24PM -0700, Stefan Agner wrote:
>> On 2016-08-03 16:18, Joe Hershberger wrote:
>> > On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner <stefan@agner.ch> wrote:
>> >> From: Stefan Agner <stefan.agner@toradex.com>
>> >>
>> >> Flush loaded data cacheline aligned. This avoids warnings such as
>> >> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>> >>
>> >> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> >> ---
>> >
>> > This was already rejected once.
>> > http://lists.denx.de/pipermail/u-boot/2012-April/121564.html
>>
>> Oh I see, and in the end the message was converted to a debug() call, in
>> essence turning the whole problem back under a stone... :-)
>>
>> FWIW, I largely support Mike Frysinger's position in that discussion,
>> and think it should be fine to flush these extra bytes...
> 
> I re-read most of that thread last-night and yes, it seems like rather
> than solving the problem we just put it into a debug().  But if I
> followed Mike's point right, we shouldn't be having a debug there
> either, the flushes are fine?  And as to Marek's point there about
> driver bugs, it's perhaps only sub-optimal rather than a broken thing?

[added Mike to CC]

It seems that back then another cache driver was affected
(arch/arm/cpu/arm926ejs/cache.c).

I don't agree with Mike: We definitely should warn if a cache flush is
unaligned. Hence I think Siomons change is the right thing to do. Not
sure if all cache driver behave the same, but the ARMv7 cache driver
(arch/arm/cpu/armv7/cache_v7.c) flushes a unaligned line. This might not
be an issue in that particular case in cmd/net.c (since there are likely
no critical data immediately following the loaded binary), but it
definitely can be an issue if we flush something on the heap: Assume we
have an array of 32 byte long DMA descriptors, and the cache line is 64
bytes. If a driver tries to flush one descriptor only,  we would always
silently flush a second DMA descriptor... If that other descriptor is
updated by the DMA controller, we would run into an issue. So if a
driver flushes cache line unaligned, we should warn!

Since the cache driver anyway flushes cache aligned, the only thing this
patch changes is actually to be explicit about that... And compared to
the patch above, this patch uses CONFIG_SYS_CACHELINE_SIZE, which makes
it clear that this is not a DMA alignment issue but a cache line
alignment issue...

So I vote for either explicitly flush cache aligned (what this patch is
doing) or remove that flush entirely...

--
Stefan

[U-Boot] cmd: net: flush cache cacheline aligned

[Tom Rini]

On Tue, Aug 02, 2016 at 12:20:28AM -0700, Stefan Agner wrote:

> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Flush loaded data cacheline aligned. This avoids warnings such as
> CACHE: Misaligned operation at range [81000000, 816d0fa8]
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> Why do we actually have to flush caches after load? It seems to
> have worked so far despite the caches did not get flushed (due to
> missalignment).

Joe, would you prefer to just drop this flush, given the rest of the
thread?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160814/ae9ac3c5/attachment.sig>

[U-Boot] cmd: net: flush cache cacheline aligned

[Stefan Agner]

On 2016-08-14 13:06, Tom Rini wrote:
> On Tue, Aug 02, 2016 at 12:20:28AM -0700, Stefan Agner wrote:
> 
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Flush loaded data cacheline aligned. This avoids warnings such as
>> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>> Why do we actually have to flush caches after load? It seems to
>> have worked so far despite the caches did not get flushed (due to
>> missalignment).
> 
> Joe, would you prefer to just drop this flush, given the rest of the
> thread?  Thanks!


Just found Diana Craciuns (added to CC) commit
99ffccbd3e5b7bc715e2eed6ea6d36f4020b56d8 ("Flush cache after the OS
image is loaded into the memory.") which adds a flush_cache in
cmd_bootm.c:

>--snip
Flush cache after the OS image is loaded into the memory.

Since we are loading an executable image into memory we need flush it
out of the cache to possible maintain coherence on CPUs with split
instruction and data caches.  We do this for other executable image
loading command.

On PowerPC once we do this we no longer need to explicitly flush the
dcache on multi-core systems in the BOOTM_STATE_OS_PREP phase.  We now
treat the BOOTM_STATE_OS_PREP as a no-op to maintain backwards
compatibility with the bootm subcommand.
<--snip

That might be actually the case in this situation too, although I feel
this should be taken care of before kernel entry. At least on ARM,
caches get flushed before jumping to the kernel. Is PowerPC different in
that matter?

--
Stefan

[U-Boot] cmd: net: flush cache cacheline aligned

[Joe Hershberger]

On Sun, Aug 14, 2016 at 3:06 PM, Tom Rini <trini@konsulko.com> wrote:
> On Tue, Aug 02, 2016 at 12:20:28AM -0700, Stefan Agner wrote:
>
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Flush loaded data cacheline aligned. This avoids warnings such as
>> CACHE: Misaligned operation at range [81000000, 816d0fa8]
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>> Why do we actually have to flush caches after load? It seems to
>> have worked so far despite the caches did not get flushed (due to
>> missalignment).
>
> Joe, would you prefer to just drop this flush, given the rest of the
> thread?  Thanks!

That's what I'd prefer. I'm willing to wait for a bit more discussion. though.

Cheers,
-Joe