Re: [RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Thu, Mar 05, 2015 at 11:16:28AM +0000, Robin Murphy wrote:
> Hi Laura,
> 
> On 05/03/15 00:19, Laura Abbott wrote:
> [...]
> >>Consider that the IOMMU's page table walker is a DMA master in its own
> >right, and that is the device you're mapping the page tables for.
> >Therefore your IOMMU driver needs to have access to the struct device
> >of the IOMMU itself to use for the page-table-related mappings. Also,
> >be sure to set the IOMMU's DMA mask correctly to prevent SWIOTLB bounce
> >buffers being created in the process (which as I've found generally ends
> >in disaster).
> >>
> >>>          And normally, we always need do cache maintenance only for some
> >>>bytes in the pagetable but not whole a page. Then is there a easy way to
> >>>do the cache maintenance?
> >>
> >>For a noncoherent device, dma_map_single() will end up calling
> >__dma_map_area() with the page offset and size of the original request, so
> >the updated part gets flushed by VA, and the rest of the page isn't touched
> >if it doesn't need to be. On the other hand if the page tables were
> >allocated with dma_alloc_coherent() in the first place, then just calling
> >dma_sync_single_for_device() for the updated region should suffice.

That's wrong.  dma_sync_single_*() is not permitted to be called on
coherently allocated memory.  Where coherent memory needs to be remapped,
dma_sync_single_*() will panic the kernel.

If it's in coherent memory, all you should need is the appropriate
memory barrier to ensure that the DMA agent can see the writes.

> >Where exactly would you call the dma_unmap? It seems a bit strange to
> >be repeatedly calling dma_map and never calling dma_unmap. I don't see it
> >explicitly forbidden in the docs anywhere to do this but it seems like
> >it would be violating the implicit handoff of dma_map/dma_unmap.
> 
> I think ideally you'd call dma_map_page when you first create the page
> table, dma_sync_single_for_device on any update, and dma_unmap_page when you
> tear it down, and you'd also use the appropriate DMA addresses everywhere
> instead of physical addresses.

No.

dma_map_page()			ownership changes CPU->DMA
dma_sync_single_for_cpu()	ownership changes DMA->CPU
dma_sync_single_for_device()	ownership changes CPU->DMA
dma_unmap_page()		ownership changes DMA->CPU

It's invalid to miss out the pairing that give those ownership changes.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.