On Tue, Jul 28, 2015 at 04:39:23PM +0100, Robin Murphy wrote:
> On 28/07/15 11:17, Will Deacon wrote:
> >> + if (cfg->tlb->flush_pgtable)
> >
> > Why would you have both a dev and a flush callback? In which cases is the
> > DMA API insufficient?
>
> a) Bisectability given existing users.
You could still make it an if (dev) .. else if (flush_pgtable) ... though.
Then we could put the wmb() in the if () clause after the DMA-api calls
and remove the conditional once everybody has been ported over.
> >> +static void __arm_lpae_set_pte(arm_lpae_iopte *ptep, arm_lpae_iopte pte,
> >> + struct io_pgtable_cfg *cfg, void *cookie)
> >> +{
> >> + struct device *dev = cfg->iommu_dev;
> >> +
> >> + *ptep = pte;
> >> +
> >> + if (dev)
> >> + dma_sync_single_for_device(dev, __arm_lpae_dma(dev, ptep),
> >> + sizeof(pte), DMA_TO_DEVICE);
> >> + if (cfg->tlb->flush_pgtable)
> >> + cfg->tlb->flush_pgtable(ptep, sizeof(pte), cookie);
> >
> > Could we kill the flush_pgtable callback completely and just stick in a
> > dma_wmb() here? Ideally, we've have something like dma_store_release,
> > which we could use to set the parent page table entry, but that's left
> > as a future exercise ;)
>
> I couldn't convince myself that there wouldn't never be some weird case
> where an IOMMU driver still needs to do something funky for a coherent
> device, so I left it in. Given that the existing implementations are
> either dsb or nothing, however, I agree it may be worth taking out now
> and only bringing back later if proven necessary.
Yeah, let's keep it as simple as we can and avoid giving people callbacks
unless they actually need them.
> I would think we'd need at least wmb() though, since dma_wmb() only
> gives us a dmb on arm64; if the PTE is going from invalid to valid (i.e.
> no TLB involved), we'd have the normal cacheable write of the PTE
> potentially racing with an MMIO write after we return (the "do DMA with
> this address" command to the master) and I think we might need a dsb to
> avoid that - if the PTE write hasn't got far enough for the IOMMU to
> snoop it, the walk hits the stale invalid entry, aborts the incoming
> transaction and kills the device.
Yes, you're right. I was in a CPU-centric mindset and forgot that we can't
deal with transient faults when going from invalid -> valid for a device
mapping.
Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
