Re: [PATCH 3/5] kvx: Implement dma handling primitives

[Jules Maselbas <jmaselbas@kalray.eu>]

Hi Lucas and Ahmad,

On Tue, Mar 02, 2021 at 11:14:09AM +0100, Lucas Stach wrote:
> Am Dienstag, dem 02.03.2021 um 09:37 +0100 schrieb Ahmad Fatoum:
> > Hello Jules, Yann,
> > 
> > On 01.03.21 16:58, Jules Maselbas wrote:
> > > From: Yann Sionneau <ysionneau@kalray.eu>
> > Some comments inline. I am not a cache cohereny expert, so take
> > it with a grain of salt.
> > 
> > > +static inline void *dma_alloc_coherent(size_t size, dma_addr_t *dma_handle)
> > > +{
> > > +	void *ret = xmemalign(PAGE_SIZE, size);
> > > +
> > > +	if (dma_handle)
> > > +		*dma_handle = (dma_addr_t)(uintptr_t)ret;
> > > +
> > > +	return ret;
> > > +}
> > 
> > This would imply that the CPU barebox is booting is coherent with all
> > 
> > devices that barebox needs to access. Is that the case?
> > 
> > (See below)
> > 
This is bogus, memory is not coherent with all devices, this should be
handled by the mmu, which is currently not supported in our barebox port.
Using this can lead to coherency issues. We can either drop this
function, so that is leads to an error at link time, or add a call to
BUG for a runtime error.

Right now we aren't using any driver that require dma_alloc_coherent,
but we use drivers that requires dma_alloc and dma_map_single instead.

> > > +/*
> > > + * The implementation of arch should follow the following rules:
> > > + *		map		for_cpu		for_device	unmap
> > > + * TO_DEV	writeback	none		writeback	none
> > > + * FROM_DEV	invalidate	invalidate(*)	invalidate	invalidate(*)
> > > + * BIDIR	writeback	invalidate	writeback	invalidate
> > > + *
> > > + * (*) - only necessary if the CPU speculatively prefetches.
> > > + *
> > > + * (see https://lkml.org/lkml/2018/5/18/979)
> > > + */
> > > +
> > > +void dma_sync_single_for_device(dma_addr_t addr, size_t size,
> > > +				enum dma_data_direction dir)
> > > +{
> > > +	switch (dir) {
> > > +	case DMA_FROM_DEVICE:
> > > +		kvx_dcache_invalidate_mem_area(addr, size);
> 
> Why do you need to explicitly invalidate, but not flush? Even if the
> CPU speculatively prefetches, the coherency protocol should make sure
> to invalidate the speculatively loaded lines, right?
Since we don't have a coherent memory, here we need to invalidate L1
dcache to let the CPU see deivce's writes in memory.
Also every write goes through the cache, flush is not required.

> 
> > > +		break;
> > > +	case DMA_TO_DEVICE:
> > > +	case DMA_BIDIRECTIONAL:
> > > +		/* allow device to read buffer written by CPU */
> > > +		wmb();
> > 
> > If the interconnect was indeed coherent, like dma_alloc_coherent
> > above hints, you wouldn't need any barriers here..?
> 
> Coherency does not imply strict ordering, so the barriers are in fact
> correct, as the CPU write buffers and/or the interconnect can still
> change the ordering of the writes as seen by a remote observer.


Best,
Jules 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox