On Fri, Mar 31, 2023 at 12:38:45PM +0200, Arnd Bergmann wrote: > On Fri, Mar 31, 2023, at 11:35, Russell King (Oracle) wrote: > > On Fri, Mar 31, 2023 at 10:07:28AM +0100, Russell King (Oracle) wrote: > >> On Mon, Mar 27, 2023 at 02:13:11PM +0200, Arnd Bergmann wrote: > >> > From: Arnd Bergmann <arnd@xxxxxxxx> > >> > > >> > Most ARM CPUs can have write-back caches and that require > >> > cache management to be done in the dma_sync_*_for_device() > >> > operation. This is typically done in both writeback and > >> > writethrough mode. > >> > > >> > The cache-v4.S (arm720/740/7tdmi/9tdmi) and cache-v4wt.S > >> > (arm920t, arm940t) implementations are the exception here, > >> > and only do the cache management after the DMA is complete, > >> > in the dma_sync_*_for_cpu() operation. > >> > > >> > Change this for consistency with the other platforms. This > >> > should have no user visible effect. > >> > >> NAK... > >> > >> The reason we do cache management _after_ is to ensure that there > >> is no stale data. The kernel _has_ (at the very least in the past) > >> performed DMA to data structures that are embedded within other > >> data structures, resulting in cache lines being shared. If one of > >> those cache lines is touched while DMA is progressing, then we > >> must to cache management _after_ the DMA operation has completed. > >> Doing it before is no good. > > What I'm trying to address here is the inconsistency between > implementations. If we decide that we always want to invalidate > after FROM_DEVICE, I can do that as part of the series, but then > I have to change most of the other arm implementations. Why? First thing to say is that DMA to buffers where the cache lines are shared with data the CPU may be accessing need to be outlawed - they are a recipe for data corruption - always have been. Sadly, some folk don't see it that way because of a passed "x86 just works and we demand that all architectures behave like x86!" attitude. The SCSI sense buffer has historically been a big culpret for that. For WT, FROM_DEVICE, invalidating after DMA is the right thing to do, because we want to ensure that the DMA'd data is properly readable upon completion of the DMA. If overlapping cache lines have been touched while DMA is progressing, and we invalidate before DMA, then the cache will contain stale data that will remain in the cache after DMA has completed. Invalidating a WT cache does not destroy any data, so is safe to do. So the safest approach is to invalidate after DMA has completed in this instance. For WB, FROM_DEVICE, we have the problem of dirty cache lines which we have to get rid of. For the overlapping cache lines, we have to clean those before DMA begins to ensure that data written to the non-DMA-buffer part is preserved. All other cache lines need to be invalidated before DMA begins to ensure that writebacks do not corrupt data from the device. Hence why it's different. And hence why the ARM implementation is based around buffer ownership. And hence why they're called dma_map_area()/dma_unmap_area() rather than the cache operations themselves. This is an intentional change, one that was done when ARMv6 came along. > OTOH, most machines that are actually in use today (armv6+, > powerpc, later mips, microblaze, riscv, nios2) also have to > deal with speculative accesses, so they end up having to > invalidate or flush both before and after a DMA_FROM_DEVICE > and DMA_BIDIRECTIONAL. Again, these are implementation details of the cache, and this is precisely why having the map/unmap interface is so much better than having generic code explicitly call "clean" and "invalidate" interfaces into arch code. If we treat everything as a speculative cache, then we're doing needless extra work for those caches that aren't speculative. So, ARM would have to step through every cache line for every DMA buffer at 32-byte intervals performing cache maintenance whether the cache is speculative or not. That is expensive, and hurts performance. I put a lot of thought into this when I updated the ARM DMA implementation when we started seeing these different cache types particularly when ARMv6 came along. I really don't want that work wrecked. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
