Hi Marek,

On 10/09/2015 10:49 AM, Ley Foon Tan wrote:

...

Is this an attempt at poor-mans' rounding ? I think you want to implment something like arch/arm/cpu/arm926ejs/cache.c check_cache_range() and NOT do any rounding here. The reason for that is that if you do rounding, you might accidentally corrupt a piece of memory which was just delivered via DMA before you did the flush.

The code above is to convert the address to dcache line size. arch/arm/cpu/arm926ejs/cache.c check_cache_range() will skip the cache flushing if it is unaligned to cache line size. I'm not sure how frequent U-boot access to non-aligned cache line size.

Thanks a lot for your looking into this, Ley Foon.

I think we take cache flushing in a different way to arm926ejs.

In nios2 driver programming, we would request all the DMA buffers be aligned to cache line. This is necessary to avoid the cache racing issue as you mention above.

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

In the copy_exception_trampoline() patch, both dcache and icache must be flushed at the exception target address. Though the flush range is only 12 bytes, which won't be aligned.

Thank you for your review.

Best regards, Thomas

Hi Marek,

On 10/09/2015 10:42 PM, Marek Vasut wrote:

...

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

I take a look into the cache flush operations in every arch of u-boot. It turns out that the arm926ejs is the only platform that does such cache line range check and skip. All other ARM and all other arch don't. And the cache flush in Linux don't.

Best regards, Thomas

On Saturday, October 10, 2015 at 08:32:09 AM, Thomas Chou wrote:

Hi Marek,

Hi,

On 10/10/2015 01:55 PM, Thomas Chou wrote:

...

Hi Marek,

On 10/09/2015 10:42 PM, Marek Vasut wrote:

...

...

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

I take a look into the cache flush operations in every arch of u-boot. It turns out that the arm926ejs is the only platform that does such cache line range check and skip. All other ARM and all other arch don't. And the cache flush in Linux don't.

+arm11, which is based on the same code.

I see your patch on this range check. I would prefer that the details of cache line configuration be kept inside the cache flush operators, and need not be exposed to drivers.

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

As drivers might be used by different arch with different cache implementation, L1,L2..etc. It is not good for drivers to adjust the flush range before passing to cache flush operators.

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

Best regards, Marek Vasut

On Sunday, October 11, 2015 at 02:38:35 AM, Thomas Chou wrote:

Hi Marek,

Hi,

On 10/11/2015 02:18 AM, Marek Vasut wrote:

...

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to allocate DMA buffers so that they are cache aligned.

That and include/memalign.h , which contains macros that are used to align variables.

...

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

I would suggest debug check should be left as for debug only. The definition of common functions should be kept as it is more important than coding style.

Uh yes, that's what arm926 cache functions do, they're debug only.

I debugged DMA issues a lot in the past until I realized the importance of aligned buffers. So there should be a developer's guideline.

For what exactly?

But it is even much more difficult when something you believed does not work as expected, what is taken as common sense. It will trap a lot of developers when they called your flush cache functions but was skipped just because, eg, the end of packets are not aligned which is usually the case.

This is good, it should bite them, because this is a bug. If, on the other hand, you will paper over such bugs by adding crap to the cache ops, there will be even worse bugs coming for you, like variables which are sitting in the same cacheline as your unaligned buffer that you want to invalidate or flush will possibly get trashed by such cache operation.

Consider this:

cacheline 0: [ variable A ; buffer B ......... ] cacheline 1: [ buffer B ......... ; Empty .... ]

Now you do the following:

1) Variable A = 0; 2) Flush buffer B (which is unaligned, so flush cacheline 0 and 1) 3) Start DMA to buffer B 4) Variable A = 1; 5) Check if DMA finished, it did 6) Invalidate buffer B ... oh, but it's unaligned, let's invalidate everything around it, which is cacheline 0 and 1. 7) What is the value of variable A ? Oh, it's fetched from memory and it's 0 there, even though we did set it to 1 ...

I would suggest that, with the best of my knowledge, please change the range check to a debug probe, and restore the cache flush functions to the common definition.

See above, does my example make it clear why we should never ever hide bugs in the cache ops code ?

Best regards, Marek Vasut

On Monday, October 12, 2015 at 02:34:16 AM, Thomas Chou wrote:

Hi Marek,

Hi!

On 10/11/2015 08:15 PM, Marek Vasut wrote:

...

On Sunday, October 11, 2015 at 02:38:35 AM, Thomas Chou wrote:

...

Hi Marek,

Hi,

...

On 10/11/2015 02:18 AM, Marek Vasut wrote:

...

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to allocate DMA buffers so that they are cache aligned.

That and include/memalign.h , which contains macros that are used to align variables.

Yes. It is malloc_cache_aligned(), which should be used to allocate DMA buffer. Thank you for the pointer.

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

...

...

...

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

I would suggest debug check should be left as for debug only. The definition of common functions should be kept as it is more important than coding style.

Uh yes, that's what arm926 cache functions do, they're debug only.

...

I debugged DMA issues a lot in the past until I realized the importance of aligned buffers. So there should be a developer's guideline.

For what exactly?

For u-boot, every DMA buffer must be allocated with malloc_cache_aligned(). Then there will be not variables and DMA buffers cache racing issues as you describe below.

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

...

...

But it is even much more difficult when something you believed does not work as expected, what is taken as common sense. It will trap a lot of developers when they called your flush cache functions but was skipped just because, eg, the end of packets are not aligned which is usually the case.

This is good, it should bite them, because this is a bug. If, on the other hand, you will paper over such bugs by adding crap to the cache ops, there will be even worse bugs coming for you, like variables which are sitting in the same cacheline as your unaligned buffer that you want to invalidate or flush will possibly get trashed by such cache operation.

Consider this:

cacheline 0: [ variable A ; buffer B ......... ] cacheline 1: [ buffer B ......... ; Empty .... ]

Now you do the following:

1. Variable A = 0;

2. Flush buffer B (which is unaligned, so flush cacheline 0 and 1)

3. Start DMA to buffer B

4. Variable A = 1;

5. Check if DMA finished, it did

6. Invalidate buffer B ... oh, but it's unaligned, let's invalidate

   everything around it, which is cacheline 0 and 1.

7. What is the value of variable A ? Oh, it's fetched from memory and

   it's 0 there, even though we did set it to 1 ...

...

I would suggest that, with the best of my knowledge, please change the range check to a debug probe, and restore the cache flush functions to the common definition.

See above, does my example make it clear why we should never ever hide bugs in the cache ops code ?

It is the drivers' responsibility to follow the guide line above. If there is such a bug, it is not the cache flush ops bug. It is a driver's bug. You may add a probe to show the bug from caller, but you may not call it a bug of cache ops and skip the flush. Given that it is quite common that the return of such cache ops is not checked, few (if not none) will ever know that the flush was skipped.

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

Best regards, Marek Vasut

On Monday, October 12, 2015 at 03:12:18 PM, Thomas Chou wrote:

Hi Marek,

On 10/12/2015 06:30 PM, Marek Vasut wrote:

...

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

Thanks for sharing this.

...

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

The same rule can be applied to buffer allocated on stack, with the macro you mentioned above. In all, cache line aware allocation on heap or on stack must be used for DMA buffer.

That's correct, they must be used. But sadly, this is not yet the case in all the drivers, which we need to rectify. And how best to rectify this than to scream when someone does such a thing, right ?

...

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

It won't be safer to flush nothing. Sooner or later the cache will be flushed due to data access, even if the cache flush ops is skip.

That is bad bad bad, that's even nastier. We really need to fix the drivers, not paper over it in the cache ops.

To solve problem like this, the only solution is to enforce the rule to allocate DMA buffer. It is wrong to skip the flush.

I absolutelly agree we need aligned allocations for DMA memory areas. But, we also shouldn't hide bugs. And I believe aligning the incorrect arguments to cache functions is not the way to go. We should check the arguments and if someone tries an unaligned cache op, we should scream. What do you think?

btw. I think you won't get way too many cache warnings nowadays and we can fix those few remaining way before the 2016.01 is out.

Best regards, Marek Vasut

Hi Marek,

On 10/12/2015 09:29 PM, Marek Vasut wrote:

On Monday, October 12, 2015 at 03:12:18 PM, Thomas Chou wrote:

...

Hi Marek,

On 10/12/2015 06:30 PM, Marek Vasut wrote:

...

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

Thanks for sharing this.

...

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

The same rule can be applied to buffer allocated on stack, with the macro you mentioned above. In all, cache line aware allocation on heap or on stack must be used for DMA buffer.

That's correct, they must be used. But sadly, this is not yet the case in all the drivers, which we need to rectify. And how best to rectify this than to scream when someone does such a thing, right ?

Given that there are not so many drivers using DMA in u-boot, as compared to Linux. I would suggest we can walk through and rectify the allocation of DMA buffers.

...

...

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

It won't be safer to flush nothing. Sooner or later the cache will be flushed due to data access, even if the cache flush ops is skip.

That is bad bad bad, that's even nastier. We really need to fix the drivers, not paper over it in the cache ops.

I know how bad it is, with over 35 years work with DMA. grin..

...

To solve problem like this, the only solution is to enforce the rule to allocate DMA buffer. It is wrong to skip the flush.

I absolutelly agree we need aligned allocations for DMA memory areas. But, we also shouldn't hide bugs. And I believe aligning the incorrect arguments to cache functions is not the way to go. We should check the arguments and if someone tries an unaligned cache op, we should scream. What do you think?

btw. I think you won't get way too many cache warnings nowadays and we can fix those few remaining way before the 2016.01 is out.

I would suggest the "cache alignment check and skip" be removed from cache flush ops, and say out the DMA buffer allocation rule loudly in README, and enforce it by guardianship. Please allow me to restate the reasons,

1. The cache flush ops are commonly used. Please refer to the "Cache and TLB Flushing Under Linux" doc, linux/Documentation/cachetlb.txt. Violating the defined interface is much worse than violating coding style. It will certainly impact the portability of u-boot. And might introduce more bug than resolve.

2. We all agree that enforcing DMA buffer allocation to cache aligned is the only real solution. Adding such "check and skip" to cache flush ops cannot prevent the flush or solve the problem.

3. Though the flush size of block device are usually aligned, the size of packet are not. Asking the packet drivers to adjust the flush size does not make sense. It is the job of cache flush ops. The debug probe should not override the original purpose. It should be spelled for common understanding.

It is free to your consideration. As it is free and open software. :)

Best regards, Thomas

Hi Marek,

On 10/17/2015 07:03 AM, Marek Vasut wrote:

...

I would suggest the "cache alignment check and skip" be removed from cache flush ops, and say out the DMA buffer allocation rule loudly in README, and enforce it by guardianship.

What exactly do you envision by this "guardianship" ?

I mean the reviews of custodians.

...

Please allow me to restate the reasons,

1. The cache flush ops are commonly used. Please refer to the "Cache and

TLB Flushing Under Linux" doc, linux/Documentation/cachetlb.txt. Violating the defined interface is much worse than violating coding style. It will certainly impact the portability of u-boot. And might introduce more bug than resolve.

I agree with this one.

...

2. We all agree that enforcing DMA buffer allocation to cache aligned is

the only real solution. Adding such "check and skip" to cache flush ops cannot prevent the flush or solve the problem.

We should probably check-scream-skip here.

...

3. Though the flush size of block device are usually aligned, the size

of packet are not. Asking the packet drivers to adjust the flush size does not make sense. It is the job of cache flush ops. The debug probe should not override the original purpose. It should be spelled for common understanding.

The socket buffer(s) should be aligned, so network packets should be fine.

While the start of socket buffer might be aligned, the size of the transfer might not for the send ops. It is depended on the net/tcp/ip packets size.

For example, with tftp, there is a lot of unaligned end of packets.

tftp d1000000 u-boot-dtb.bin

flush unaligned d7ff7020-d7ff704e [repeat ..]

So, such an alarm may be false. And such a skip can be bug.

In fact, for my own projects, I have changed the memory allocation to always cache aligned. And I rarely worry about it ever after.

I look at the net.c of u-boot. There are packets buffer allocated on BSS and stack. I would suggest avoid such programming, and use aligned memory allocation stead.

Best regards, Thomas

On Saturday, October 10, 2015 at 07:55:45 AM, Thomas Chou wrote:

Hi Marek,

Hi,

On 10/09/2015 10:42 PM, Marek Vasut wrote:

...

...

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

I take a look into the cache flush operations in every arch of u-boot. It turns out that the arm926ejs is the only platform that does such cache line range check and skip. All other ARM and all other arch don't.

Yes, everyone else doesn't do the checks and if there is unaligned cache flush/invalidation, that platform also suffers from various obscure and hard to debug errors. I submitted patch to add the same check for ARMv7, but it didn't receive attention :-( I should repost it I guess.

And the cache flush in Linux don't.

Because the buffers there are always correctly aligned during allocation.

Best regards, Marek Vasut