| Message ID | 20230327121317.4081816-10-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dma-mapping: unify support for cache flushes | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD e45d6a52fe2b |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 1 and now 1 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 18 this patch: 18 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 18 this patch: 18 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Mon, Mar 27, 2023 at 02:13:05PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > For a DMA_BIDIRECTIONAL transfer, the caches have to be cleaned > first to let the device see data written by the CPU, and invalidated > after the transfer to let the CPU see data written by the device. > > riscv also invalidates the caches before the transfer, which does > not appear to serve any purpose. Rationale makes sense to me.. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks for working on all of this Arnd!
On Mon, Mar 27, 2023 at 1:16 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > For a DMA_BIDIRECTIONAL transfer, the caches have to be cleaned > first to let the device see data written by the CPU, and invalidated > after the transfer to let the CPU see data written by the device. > > riscv also invalidates the caches before the transfer, which does > not appear to serve any purpose. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/riscv/mm/dma-noncoherent.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Cheers, Prabhakar > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index 640f4c496d26..69c80b2155a1 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -25,7 +25,7 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > case DMA_BIDIRECTIONAL: > - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > + ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > default: > break; > -- > 2.39.2 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, 27 Mar 2023 05:13:05 PDT (-0700), arnd@kernel.org wrote: > From: Arnd Bergmann <arnd@arndb.de> > > For a DMA_BIDIRECTIONAL transfer, the caches have to be cleaned > first to let the device see data written by the CPU, and invalidated > after the transfer to let the CPU see data written by the device. > > riscv also invalidates the caches before the transfer, which does > not appear to serve any purpose. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/riscv/mm/dma-noncoherent.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index 640f4c496d26..69c80b2155a1 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -25,7 +25,7 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > case DMA_BIDIRECTIONAL: > - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > + ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > default: > break; Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
On Mon, Mar 27, 2023 at 8:15 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > For a DMA_BIDIRECTIONAL transfer, the caches have to be cleaned > first to let the device see data written by the CPU, and invalidated > after the transfer to let the CPU see data written by the device. > > riscv also invalidates the caches before the transfer, which does > not appear to serve any purpose. Yes, we can't guarantee the CPU pre-load cache lines randomly during dma working. But I've two purposes to keep invalidates before dma transfer: - We clearly tell the CPU these cache lines are invalid. The caching algorithm would use these invalid slots first instead of replacing valid ones. - Invalidating is very cheap. Actually, flush and clean have the same performance in our machine. So, how about: diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c index d919efab6eba..2c52fbc15064 100644 --- a/arch/riscv/mm/dma-noncoherent.c +++ b/arch/riscv/mm/dma-noncoherent.c @@ -22,8 +22,6 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); break; case DMA_FROM_DEVICE: - ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); - break; case DMA_BIDIRECTIONAL: ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); break; @@ -42,7 +40,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, break; case DMA_FROM_DEVICE: case DMA_BIDIRECTIONAL: /* I'm not sure all drivers have guaranteed cacheline alignment. If not, this inval would cause problems */ - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); + ALT_CMO_OP(inval, vaddr, size, riscv_cbom_block_size); break; default: break; > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/riscv/mm/dma-noncoherent.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index 640f4c496d26..69c80b2155a1 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -25,7 +25,7 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > case DMA_BIDIRECTIONAL: > - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > + ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > default: > break; > -- > 2.39.2 >
On Fri, May 5, 2023, at 07:47, Guo Ren wrote: > On Mon, Mar 27, 2023 at 8:15 PM Arnd Bergmann <arnd@kernel.org> wrote: >> >> riscv also invalidates the caches before the transfer, which does >> not appear to serve any purpose. > Yes, we can't guarantee the CPU pre-load cache lines randomly during > dma working. > > But I've two purposes to keep invalidates before dma transfer: > - We clearly tell the CPU these cache lines are invalid. The caching > algorithm would use these invalid slots first instead of replacing > valid ones. > - Invalidating is very cheap. Actually, flush and clean have the same > performance in our machine. The main purpose of the series was to get consistent behavior on all machines, so I really don't want a custom optimization on one architecture. You make a good point about cacheline reuse after invalidation, but if we do that, I'd suggest doing this across all architectures. > So, how about: > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index d919efab6eba..2c52fbc15064 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -22,8 +22,6 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > break; > case DMA_FROM_DEVICE: > - ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > - break; > case DMA_BIDIRECTIONAL: > ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > break; This is something we can consider. Unfortunately, this is something that no architecture (except pa-risc, which has other problems) does at the moment, so we'd probably need to have a proper debate about this. We already have two conflicting ways to handle DMA_FROM_DEVICE, either invalidate/invalidate, or clean/invalidate. I can see that flush/invalidate may be a sensible option as well, but I'd want to have that discussion after the series is complete, so we can come to a generic solution that has the same documented behavior across all architectures. In particular, if we end up moving arm64 and riscv back to the traditional invalidate/invalidate for DMA_FROM_DEVICE and document that driver must not rely on buffers getting cleaned before a partial DMA_FROM_DEVICE, the question between clean or flush becomes moot as well. > @@ -42,7 +40,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, > break; > case DMA_FROM_DEVICE: > case DMA_BIDIRECTIONAL: > /* I'm not sure all drivers have guaranteed cacheline > alignment. If not, this inval would cause problems */ > - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > + ALT_CMO_OP(inval, vaddr, size, riscv_cbom_block_size); > break; This is my original patch, and I would not mix it with the other change. The problem with non-aligned DMA_BIDIRECTIONAL buffers in is that both flush and inval would be wrong if you get simultaneous writes from device and cpu to the same cache line, so there is no way to win this. Using inval instead of flush would at least work if the CPU data in the cacheline is read-only from the CPU, so that seems better than something that is always wrong. The documented API is that sharing the cache line is not allowed at all, so anything that would observe a difference between the two is also a bug. One idea that we have considered already is that we could overwrite the unused bits of the cacheline with poison values and/or mark them as invalid using KASAN for debugging purposes, to find drivers that already violate this. Arnd
On Fri, May 5, 2023 at 9:19 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, May 5, 2023, at 07:47, Guo Ren wrote: > > On Mon, Mar 27, 2023 at 8:15 PM Arnd Bergmann <arnd@kernel.org> wrote: > > >> > >> riscv also invalidates the caches before the transfer, which does > >> not appear to serve any purpose. > > Yes, we can't guarantee the CPU pre-load cache lines randomly during > > dma working. > > > > But I've two purposes to keep invalidates before dma transfer: > > - We clearly tell the CPU these cache lines are invalid. The caching > > algorithm would use these invalid slots first instead of replacing > > valid ones. > > - Invalidating is very cheap. Actually, flush and clean have the same > > performance in our machine. > > The main purpose of the series was to get consistent behavior on > all machines, so I really don't want a custom optimization on > one architecture. You make a good point about cacheline reuse > after invalidation, but if we do that, I'd suggest doing this > across all architectures. Yes, invalidation of DMA_FROM_DEVICE-for_device is a proposal for all architectures. > > > So, how about: > > > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > > index d919efab6eba..2c52fbc15064 100644 > > --- a/arch/riscv/mm/dma-noncoherent.c > > +++ b/arch/riscv/mm/dma-noncoherent.c > > @@ -22,8 +22,6 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > > ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > > break; > > case DMA_FROM_DEVICE: > > - ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > > - break; > > case DMA_BIDIRECTIONAL: > > ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > > break; > > This is something we can consider. Unfortunately, this is something > that no architecture (except pa-risc, which has other problems) > does at the moment, so we'd probably need to have a proper debate > about this. > > We already have two conflicting ways to handle DMA_FROM_DEVICE, > either invalidate/invalidate, or clean/invalidate. I can see I vote to invalidate/invalidate. My key point is to let DMA_FROM_DEVICE-for_device invalidate, and DMA_BIDIRECTIONAL contains DMA_FROM_DEVICE. So I also agree: @@ -22,8 +22,6 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); break; case DMA_FROM_DEVICE: - ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); + ALT_CMO_OP(invalidate, vaddr, size, riscv_cbom_block_size); break; case DMA_BIDIRECTIONAL: ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); break; > that flush/invalidate may be a sensible option as well, but I'd > want to have that discussion after the series is complete, so > we can come to a generic solution that has the same documented > behavior across all architectures. Yes, I agree to unify them into a generic solution first. My proposal could be another topic in the future. For that purpose, I give Acked-by: Guo Ren <guoren@kernel.org> > > In particular, if we end up moving arm64 and riscv back to the > traditional invalidate/invalidate for DMA_FROM_DEVICE and > document that driver must not rely on buffers getting cleaned After invalidation, the cache lines are also cleaned, right? So why do we need to document it additionally? > before a partial DMA_FROM_DEVICE, the question between clean > or flush becomes moot as well. > > > @@ -42,7 +40,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, > > break; > > case DMA_FROM_DEVICE: > > case DMA_BIDIRECTIONAL: > > /* I'm not sure all drivers have guaranteed cacheline > > alignment. If not, this inval would cause problems */ > > - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); > > + ALT_CMO_OP(inval, vaddr, size, riscv_cbom_block_size); > > break; > > This is my original patch, and I would not mix it with the other > change. The problem with non-aligned DMA_BIDIRECTIONAL buffers in > is that both flush and inval would be wrong if you get simultaneous > writes from device and cpu to the same cache line, so there is > no way to win this. Using inval instead of flush would at least > work if the CPU data in the cacheline is read-only from the CPU, > so that seems better than something that is always wrong. If CPU data in the cacheline is read-only, the cacheline would never be dirty. Yes, It's always safe. Okay, I agree we must keep cache-line-aligned. I comment it here, just worry some dirty drivers couldn't work with the "invalid mechanism" because of the CPU data corruption, and device data in the cacheline is useless. > > The documented API is that sharing the cache line is not allowed > at all, so anything that would observe a difference between the > two is also a bug. One idea that we have considered already is > that we could overwrite the unused bits of the cacheline with > poison values and/or mark them as invalid using KASAN for debugging > purposes, to find drivers that already violate this. > > Arnd
On Sat, May 6, 2023, at 09:25, Guo Ren wrote: > On Fri, May 5, 2023 at 9:19 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> This is something we can consider. Unfortunately, this is something >> that no architecture (except pa-risc, which has other problems) >> does at the moment, so we'd probably need to have a proper debate >> about this. >> >> We already have two conflicting ways to handle DMA_FROM_DEVICE, >> either invalidate/invalidate, or clean/invalidate. I can see > I vote to invalidate/invalidate. > ... > >> that flush/invalidate may be a sensible option as well, but I'd >> want to have that discussion after the series is complete, so >> we can come to a generic solution that has the same documented >> behavior across all architectures. > Yes, I agree to unify them into a generic solution first. My proposal > could be another topic in the future. Right, I was explicitly trying to exclude that question from my series, and left it as an architecture specific Kconfig option based on the current behavior. >> In particular, if we end up moving arm64 and riscv back to the >> traditional invalidate/invalidate for DMA_FROM_DEVICE and >> document that driver must not rely on buffers getting cleaned > After invalidation, the cache lines are also cleaned, right? So why do > we need to document it additionally? I mentioned the debate in the cover letter, the full explanation is archived at https://lore.kernel.org/all/20220606152150.GA31568@willie-the-truck/ In short, the problem that is addressed here is leaking sensitive kernel data to user space or a device as in this sequence: 1. A DMA buffer is allocated in the kernel and contains stale data that is no longer needed but must not be exposed to untrusted userspace, i.e. encryption keys or user file pages 2. allocator uses memset() to clear out the buffer 3. buffer gets mapped into a device for DMA_FROM_DEVICE 4. writeback cache gets invalidated, uncovering the sensitive data by discarding the zeros 5. device returns less data than expected 6. buffer is unmapped 7. whole buffer is mapped or copied to user space Will added his patch for arm64 to prevent this scenario by using 'clean' instead of 'invalidate' in step 4, and the same behavior got copied to riscv but not most of the other architectures. The dma-mapping documentation does not say anything about this case, and an alternative approach would be to document that device drivers must watch out for short reads in step 5, or that kzalloc() should clean the cache in step 2. Both of these come at a cost as well. Arnd
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c index 640f4c496d26..69c80b2155a1 100644 --- a/arch/riscv/mm/dma-noncoherent.c +++ b/arch/riscv/mm/dma-noncoherent.c @@ -25,7 +25,7 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); break; case DMA_BIDIRECTIONAL: - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size); + ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); break; default: break;