<arnd>
followed by moving the then common logic into shared code
<bjdooks>
yeah, i was going to send something for that, a couple of the flushes i think are meant to be invalidate in riscv
<bjdooks>
arnd: my issue is the dma-allocator mapping, if you use dma_alloc and expect the CMO to work, it still maps the memory as uncached on riscv
<bjdooks>
which makes the CMO code irrelevant
<arnd>
which dma_alloc variant?
<bjdooks>
any of them
<arnd>
dma_alloc_coherent() must map things as uncached if the DMA is non-coherent
<arnd>
if DMA is fully coherent, then you need no CMO
<arnd>
so I think that bit is correct
<bjdooks>
no
<bjdooks>
so i've faked a non-coherent system, done extensive testing, if you have a device marked noncohereht and do a dma-alloc on it, it does the CMOs but the page is marked as uncached.
<bjdooks>
the issue is is SVPBMT and ZICBOM exist, SVPBMT gets picked first for both ioremap() and dma-alloc
<bjdooks>
where you want SVPBMT for ioreamp() and ZICBOM for dma-alloc
<arnd>
help me out with the acronyms: what does SVPBMT mean, and how is it differnet from ZICBOM?
<bjdooks>
ZICBOM is the cahce management like clean/flush/inval
<bjdooks>
SVPBMT allows memory pages to be marked uncached/weakly-ordered
<arnd>
still confused. so ioremap() should generally require uncached/strongly-ordered, not uncached/weakly-ordered in order to guarantee ordering between individual mmio register accesses, but that is probably covered by the barriers in readl()/writel(), so that's fine.
<arnd>
the riscv_page_dmacoherent() function in the patch looks correct to me (assuming only ZICBOM is supported, not custom cache management operations)
<arnd>
at least as far as I can tell, this does the same as any other architecture: if the DMA is fully coherent, pgprot_dmacoherent() is regular cached mapping, and the CMOs are all nops
<conchuod>
arnd: are your changes there a strict requirement for Prabhakars series adding CMO stuff for Renesas/Andes stuff?
<arnd>
but if you have ZICBOM, then pgprot_dmacoherent() returns some variant of uncached memory and CMOs do the appropriate flushes
<bjdooks>
but if it isn't, then you get uncached pages
<bjdooks>
totally unached is stupid, then you don't need any ops as it is uncached
<bjdooks>
totally unached is stupid, then you don't need any cmo ops as it is uncached
<arnd>
bjdooks: I think you mix up the streaming mapping (dma_map_*) with the coherent mapping (dma_alloc_*)
<arnd>
CMO is only used for streaming mappings, but cannot work for coherent mapping
<arnd>
doing a flush on an uncached page should result in a CPU fault
<arnd>
(not sure if it does on zicbom, but it does on some other ones)
<bjdooks>
arnd: those instructions do not fault
<arnd>
ok
<arnd>
not a big deal, just makes it a little harder to find bugs in drivers that get the interface wrong
<bjdooks>
I still think riscv is doing it wrong
<arnd>
the typical example is a network driver using dma_alloc_coherent() to create a buffer for its descriptors that is uncached, and dma_map_sg() for the SKBs
<arnd>
In the descriptors, you need the individual accesses to be strictly ordered (first the data pointer, then the valid flag), which you cannot enforce on cached memory
<bjdooks>
so i'm fairly sure in that case, riscv with both svpbmt and zicbom will provide an uncached area and then do CMO ops, which seems strange when uncached should be more than sufficient there,
<arnd>
for the descriptor access, the fictional network driver in my example only does a dma_wmb() between the address write and the flag write.
<arnd>
if dma_wmb() turns into a CMO, that is indeed a bug
<arnd>
bjdooks: do you have a particular driver that you were looking at, or just the architecture code?
<conchuod>
bjdooks: oh, I think I got what you meant in your original message wrong. I didn't realise you meant your recent patches, saying "ping arnd" was for allowing CMO stuff from functions.
<bjdooks>
so I've been using a test driver i wrote for testing, that does dma_alloc() with differetn attributes and then uses dma_sync_single_for_cpu and dma_sync_single_for_device
<arnd>
ah, that makes sense. So you are using a broken testcase ;-)
<arnd>
dma_sync_single_for_cpu() is only defined on memory you got from dma_map_*()
<conchuod>
arnd: are your cross-arch changes a strict requirement for Prabhakars series adding CMO stuff for Renesas/Andes?
prabhakarlad has joined #riscv
<arnd>
conchuod: as far as I'm concerned, the strict requirement for new CMOs is that we come up with a sensible definition of what each dma operation should do
<arnd>
both the current riscv definition for ZICBOM and the version that prabhakarlad was adding are common across other architectures, but they are fundamentally at odds with one another, so the bit I'm interested in is making them do the same thing first
<conchuod>
Okay, that makes sense.
zjason` is now known as zjason
<arnd>
I think the most controversial bit is the question about DMA_BIDIRECTIONAL: powerpc started the flush/flush semantics a long time ago, and this has made it into parisc, microblaze and now riscv over time
mahk has quit [Ping timeout: 268 seconds]
<arnd>
the idea was to deal with a partially shared cache line at the beginning of the mapping, where one part of it is used by the CPU and another part is used by a device
<arnd>
having a flush in dma_map_*() here makes sense, as this means the device will see the data that was written by the CPU and the CPU doesn't lose any of its own data
mahk has joined #riscv
<arnd>
but in dma_unmap_*() there is absolutely no way to preserve both the data from the device and the CPU, if they concurrently write into the same cacheline
<arnd>
invalidate loses any new data written by the CPU, and flush loses data written by the device
BootLayer has joined #riscv
<conchuod>
That sounds like a topic for the Wills and Christophs of the world :)
<arnd>
absolutely. There are a number of easier changes to make where I hope we can easily agree
<arnd>
such as powerpc always doing the same thing for map and unmap, agaict that is just a historic artifact and changing it just makes it more efficient
<conchuod>
I won't hold my horses for this to be resolved soon so! I did like the idea of removing the ability to decide what op is called for what, if there's gonna end up being several methods for doing this on riscv, that approach sounds ideal.
Sos has joined #riscv
<bjdooks>
ok, in the case of a non-coherent device, then zicbom isn't going to cut it with dma_alloc as there's no way to sync the data or make it uncached
Sos has quit [Quit: Leaving]
<bjdooks>
ok, so one of the tests is dma_alloc_noncoherent and that if i read it correctly should require the dma-sync calls
<bjdooks>
^arnd ?
pecastro has joined #riscv
<arnd>
bjdooks: correct, though note that dma_alloc_noncoherent() is rarely used, it pretty much only exists for old MIPS and Itanium workstations from 25 years ago that had custom requirements
<arnd>
it's not even mentioned in Documentation/core-api/dma-api-howto.rst
<jrtc27>
why would anyone try to support a cache line being shared between dma and something else...
<jrtc27>
that's just broken by design
<jrtc27>
unless you have an architecture that guarantees partial writebacks
<arnd>
jrtc27: yes, that was pretty much my point. I think we have a couple of device drivers that did this in violation of the interface, and they worked fine on machines with coherent caches but caused bugs on certain machines
<arnd>
and then we had architecture maintainers trying to work around this without fully understanding the problem
<jrtc27>
perhaps you want a sanitiser mode where the cmo implementation zeroes out the partial ends...
<jrtc27>
(or some other junk pattenr)
<arnd>
right, I had already considered adding a WARN_ONCE(unaligned address or size)\
ldevulder has quit [Remote host closed the connection]
jacklsw has quit [Ping timeout: 265 seconds]
sh1r4s3 has joined #riscv
wingsorc has quit [Ping timeout: 246 seconds]
<arnd>
jrtc27: I wonder if KASAN could do even better here: mark whole cache line as invalid in dma_sync_*_for_device(..., DMA_FROM_DEVICE) but mark only the actual data as valid in dma_sync_*_for_cpu(..., DMA_FROM_DEVICE)
<arnd>
DMA_TO_DEVICE on partial cache lines is not harmful because there is no data corruption as long as the device only reads
sh1r4s3 has quit [Remote host closed the connection]
sh1r4s3 has joined #riscv
joev has quit [Ping timeout: 255 seconds]
joev has joined #riscv
Andre_Z has joined #riscv
sh1r4s3 has quit [Read error: Connection reset by peer]
sh1r4s3 has joined #riscv
joev has quit [Ping timeout: 255 seconds]
joev has joined #riscv
joev has quit [Ping timeout: 250 seconds]
joev has joined #riscv
joev has quit [Ping timeout: 250 seconds]
joev has joined #riscv
billchenchina- has joined #riscv
billchenchina- has quit [Remote host closed the connection]
billchenchina has joined #riscv
cwebber has joined #riscv
rneese has joined #riscv
Tenkawa has joined #riscv
Andre_Z has quit [Quit: Leaving.]
jmdaemon has quit [Ping timeout: 265 seconds]
ldevulder has joined #riscv
MaxGanzII has quit [Remote host closed the connection]
MaxGanzII has joined #riscv
billchenchina- has joined #riscv
billchenchina has quit [Ping timeout: 265 seconds]
elastic_dog has quit [Killed (zinc.libera.chat (Nickname regained by services))]
elastic_dog has joined #riscv
rurtty has joined #riscv
sh1r4s3_ has joined #riscv
sh1r4s3 has quit [Read error: Connection reset by peer]