Hi, On Tue, Jan 17, 2017 at 10:54 AM, Marek Vasut <ma...@denx.de> wrote: > On 01/17/2017 10:51 AM, Jean-Jacques Hiblot wrote: >> >> >> On 17/01/2017 10:38, Marek Vasut wrote: >>> On 01/17/2017 10:35 AM, Jean-Jacques Hiblot wrote: >>>> >>>> On 17/01/2017 10:15, Marek Vasut wrote: >>>>> On 01/17/2017 10:08 AM, Jean-Jacques Hiblot wrote: >>>>>> On 16/01/2017 20:33, Marek Vasut wrote: >>>>>>> On 01/16/2017 08:16 PM, Jean-Jacques Hiblot wrote: >>>>>>>> On 16/01/2017 17:00, Marek Vasut wrote: >>>>>>>>> On 01/16/2017 02:29 PM, Jean-Jacques Hiblot wrote: >>>>>>>>>> Tom, Marek >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>>> At the moment, whenever an unaligned address is used in cache >>>>>>>>>> operations >>>>>>>>>> (invalidate_dcache_range, or flush_dcache_range), the whole >>>>>>>>>> request is >>>>>>>>>> discarded for am926ejs. for armV7 or armV8 only the aligned >>>>>>>>>> part is >>>>>>>>>> maintained. This is probably what is causing the bug addressed in >>>>>>>>>> 8133f43d1cd. There are a lot of unaligned buffers used in DMA >>>>>>>>>> operations >>>>>>>>>> and for all of them, we're possibly handling the cached partially >>>>>>>>>> or not >>>>>>>>>> at all. I've seen this when using the environment from a file >>>>>>>>>> stored in >>>>>>>>>> a FAT partition. commit 8133f43d1cd addresses this by using a >>>>>>>>>> bounce >>>>>>>>>> buffer at the FAT level but it's only one of many cases. >>>>>>>>>> >>>>>>>>>> I think we can do better with unaligned cache operations: >>>>>>>>>> >>>>>>>>>> * flush (writeback + invalidate): Suppose we use address p >>>>>>>>>> which is >>>>>>>>>> unaligned, flush_dcache_range() can do the writeback+invalidate >>>>>>>>>> on the >>>>>>>>>> whole range [p & ~(line_sz - 1); p + length | (line_sz - 1)]. >>>>>>>>>> There >>>>>>>>>> should no problem with that since writeback can happen at any >>>>>>>>>> point in >>>>>>>>>> time. >>>>>>>>>> >>>>>>>>>> * invalidation >>>>>>>>>> >>>>>>>>>> It is a bit trickier. here is a pseudo-code: >>>>>>>>>> invalidate_dcache_range(p,length) >>>>>>>>>> { >>>>>>>>>> write_back_invalidate(first line) >>>>>>>>>> write_back_invalidate(last line) >>>>>>>>>> invalidate(all other lines) >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> Here again this should work fine IF invalidate_dcache_range() is >>>>>>>>>> called >>>>>>>>>> BEFORE the DMA operation (again the writeback can happen at >>>>>>>>>> time so >>>>>>>>>> it's >>>>>>>>>> valid do it here). Calling it only AFTER the operation, may >>>>>>>>>> corrupt >>>>>>>>>> the >>>>>>>>>> data written by the DMA with old data from CPU. This how I used to >>>>>>>>>> handle unaligned buffers in some other projects. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> There is however one loophole: a data sitting in the first or the >>>>>>>>>> last >>>>>>>>>> line is accessed before the memory is updated by the DMA, then the >>>>>>>>>> first/line will be corrupted. But it's not highly probable as this >>>>>>>>>> data >>>>>>>>>> would have to be used in parallel of the DMA (interrupt handling, >>>>>>>>>> SMP?, >>>>>>>>>> dma mgt related variable). So it's not perfect but it would >>>>>>>>>> still be >>>>>>>>>> better than we have today. >>>>>>>>> Or just fix all the code which complains about unaligned buffers, >>>>>>>>> done. >>>>>>>>> That's the way to go without all the complications above. >>>>>>>> It's not that complex, but it's not perfect. We would need to >>>>>>>> keep the >>>>>>>> same warning as we have now, but it would make it work in more >>>>>>>> cases. >>>>>>> The warning is there for that exact reason -- to inform you >>>>>>> something's >>>>>>> wrong. >>>>>>> >>>>>>>> Tracking every possible unaligned buffer that gets invalidated is >>>>>>>> not a >>>>>>>> trivial job. Most of the time the buffer is allocated in a upper >>>>>>>> layer >>>>>>>> and passed down to a driver via layers like network stack, block >>>>>>>> layer >>>>>>>> etc.And in many cases, the warning will come and go depending on >>>>>>>> how the >>>>>>>> variable aligned on the stack or the heap. >>>>>>> I didn't observe this much in fact. I usually see the buffers >>>>>>> coming it >>>>>>> aligned or being allocated in drivers. Also, I think that's why >>>>>>> the RC >>>>>>> cycle is there, so we can test the next release and fix these issues. >>>>>> It's not commonly seen but I came across it some times. >>>>>> >>>>>> Here are two examples: >>>>>> >>>>>> Network: >>>>>> U-Boot 2016.09-rc1-00087-gd40ff0a (Jul 27 2016 - 10:04:33 -0500) >>>>>> CPU : DRA752-HS ES2.0 >>>>>> Model: TI DRA742 >>>>>> [...] >>>>>> Booting from network ... >>>>>> cpsw Waiting for PHY auto negotiation to complete.... done >>>>>> link up on port 0, speed 1000, full duplex >>>>>> BOOTP broadcast 1 >>>>>> CACHE: Misaligned operation at range [dffecb40, dffecc96] >>>>>> CACHE: Misaligned operation at range [dffed140, dffed17e] >>>>>> BOOTP broadcast 2 >>>>>> [...] >>>>>> File transfer via NFS from server 10.0.1.26; our IP address is >>>>>> 128.247.83.128; sending through gateway 128.247.82.1 >>>>>> [...] >>>>>> Load address: 0x82000000 >>>>>> Loading: CACHE: Misaligned operation at range [dffebfc0, dffebfea] >>>>>> >>>>>> >>>>>> FAT: it has been fixed recently by using a bounce buffer in FAT >>>>>> code by >>>>>> "8133f43d1cd fs/fat/fat_write: Fix buffer alignments". >>>>>> I've also seen it a few years back on PowerPC platforms when >>>>>> accessing a >>>>>> USB storage. >>>>>> >>>>>> I'm sure that we could find more examples. I don't mean that they >>>>>> shouldn't be fixed, simply that we could still make things work in >>>>>> most >>>>>> cases at a low cost. >>>>>> The modifications I proposed do not change the behaviour of the >>>>>> code for >>>>>> aligned buffers, it just make it much more likely that it would work >>>>>> with unaligned buffers. I fail to see the reason why this wouldn't >>>>>> be a >>>>>> good thing. >>>>> It seems to me like "it kinda works, but it really doesn't sometimes >>>>> ...." , so it's encouraging bad practice as people will get used to >>>>> ignoring this warning because things "kinda work, in most cases". >>>> I see your point but the current situation is exactly like that: it >>>> works most of the time. >>> But in this case, it's a property of the hardware. With your patch, it's >>> also a property of the code which works "most of the time". You >>> even state that in the patch description. >> I'm not sure I understand your point. > > I'm referring to the last paragraph of the commit message, "There is > however one loophole..." > >> But as I said I'm not pushing. >> You're not keen to the idea and you're more knowledgeable than I am in >> u-boot, so be it. I'll drop the subject >> Thanks for taking time to discuss it. > > Let's wait for what the others have to say maybe ?
IMHO, a 100% reliable U-Boot is preferable, even if this means less performance. It does not seem like there would be a way of working around this loophole, so I would avoid this cache hack, even if the probability of getting an issue would be small. Maintaining only the aligned part of DMA buffers for cache operations as Jean-Jacques says is done for armv7/8 is also not perfect. This means that probably at least all the storage-related device drivers on ARM can be affected in some way by unaligned buffers. That would mean a lot of changes to move the buffer bounce mechanism from the FAT layer to the device driver layer. That's why I'd rather move it to the block layer. The architecture/driver combinations requiring it could add a flag or a configuration setting somewhere, so that the performance is not lowered when not needed. It would also only be applied if a misalignment is detected, in which case a debug() message could also be printed. >>>> I'm not pushing for the changes, I just wanted to let know that we could >>>> do better if we wish. >>> Replacing one problem with another which looks like it isn't problem >>> anymore (but still is) is not better IMO. >>> >>>>>> BTW the L2 uniphier cache implements this for flush and invalidation, >>>>>> and some architectures (pxa, blackfin, openrisc, etc.) do the flush() >>>>>> this way too. Best regards, Benoît _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot