Re: [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  wrote:
>> >> From: Stefan Agner 
>> >>
>> >> Flush loaded data cacheline aligned. This avoids warnings such as
>> >> CACHE: Misaligned operation at range [8100, 816d0fa8]
>> >>
>> >> Signed-off-by: Stefan Agner 
>> >> ---
>> >
>> > 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  wrote:
> >> From: Stefan Agner 
> >>
> >> Flush loaded data cacheline aligned. This avoids warnings such as
> >> CACHE: Misaligned operation at range [8100, 816d0fa8]
> >>
> >> Signed-off-by: Stefan Agner 
> >> ---
> > 
> > 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


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  wrote:
>> Hi Joe,
>>
>> On 3 August 2016 at 17:18, Joe Hershberger  wrote:
>>> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner  wrote:
 From: Stefan Agner 

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

 Signed-off-by: Stefan Agner 
 ---
>>>
>>> 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

Hi Simon,

On Wed, Aug 3, 2016 at 8:17 PM, Simon Glass  wrote:
> Hi Joe,
>
> On 3 August 2016 at 17:18, Joe Hershberger  wrote:
>> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner  wrote:
>>> From: Stefan Agner 
>>>
>>> Flush loaded data cacheline aligned. This avoids warnings such as
>>> CACHE: Misaligned operation at range [8100, 816d0fa8]
>>>
>>> Signed-off-by: Stefan Agner 
>>> ---
>>
>> 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  wrote:
>> From: Stefan Agner 
>>
>> Flush loaded data cacheline aligned. This avoids warnings such as
>> CACHE: Misaligned operation at range [8100, 816d0fa8]
>>
>> Signed-off-by: Stefan Agner 
>> ---
> 
> 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@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Simon Glass]

Hi Joe,

On 3 August 2016 at 17:18, Joe Hershberger  wrote:
> On Tue, Aug 2, 2016 at 2:20 AM, Stefan Agner  wrote:
>> From: Stefan Agner 
>>
>> Flush loaded data cacheline aligned. This avoids warnings such as
>> CACHE: Misaligned operation at range [8100, 816d0fa8]
>>
>> Signed-off-by: Stefan Agner 
>> ---
>
> 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

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

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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Simon Glass]

On 2 August 2016 at 01:20, Stefan Agner  wrote:
> From: Stefan Agner 
>
> Flush loaded data cacheline aligned. This avoids warnings such as
> CACHE: Misaligned operation at range [8100, 816d0fa8]
>
> Signed-off-by: Stefan Agner 
> ---
> 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 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Fabio Estevam]

Hi Stefan,

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

This fixes the cache warnings:

Tested-by: Fabio Estevam 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Stefan Agner]

From: Stefan Agner 

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

Signed-off-by: Stefan Agner 
---
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot