Date:   Wed, 20 Nov 2019 12:16:37 +0100
From:   Christoph Hellwig <hch@....de>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Christoph Hellwig <hch@....de>, iommu@...ts.linux-foundation.org,
        Alexey Kardashevskiy <aik@...abs.ru>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: generic DMA bypass flag

On Tue, Nov 19, 2019 at 05:41:58PM +0000, Robin Murphy wrote:
> Is that a problem though? It's not safe in general to rewrite the default 
> domain willy-nilly,

Well.  Can you look at what intel-iommu does right now so that we can
sort that out first?

> so if it's a concern that drivers get stuck having to 
> use a translation domain if they do something dumb like:
>
> 	if (!dma_set_mask(DMA_BIT_MASK(32))
> 		dma_set_mask(DMA_BIT_MASK(64));
>
> then the simple solution is "don't do that" - note that this doesn't affect 
> overriding of the default 32-bit mask, because we don't use the driver API 
> to initialise those.

>>  And we had a couple drivers playing
>> interesting games there.
>
> If the games you're worried about are stuff like:
>
> 	dma_set_mask(dev, DMA_BIT_MASK(64));
> 	high_buf = dma_alloc_coherent(dev, ...);
> 	dma_set_mask(dev, DMA_BIT_MASK(32));
> 	low_buf = dma_alloc_coherent(dev, ...);
>
> then iommu_need_mapping() already ensures that will end spectacularly 
> badly. Unless we can somehow log when a mask has been "committed" by a 
> mapping operation, I don't think any kind of opportunistic bypass mechanism 
> is ever not going to blow up that case.

The prime example I had in mind is sata_nv.c.  This first does
a

	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

and does a dma_alloc_coherent for buffers that have "legacy" descriptors.
Then does a

	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));

and allocates the other coherent buffers than can be 64-bit capable.
and then actually overrides the streaming dma mask to 32-bit if
there is an ATAPI device attached for which it has some limitations,
or otherwise keeps the 64-bit mask in a somewhat odd way.  But
this device actually can't be used with intel IOMMU as it is integrted
into the nforce chipsets.

But looking through this mess I'm tempted to agree with you that if
anyone is messing with the mask to first set it to 32-bit and then
back they can live with the default domain and iommu overhead..

>>  FYI, this is the current intel-iommu
>> WIP conversion to the dma bypass flag:
>>
>> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-bypass
>
> Having thought a bit more, I guess my idea does end up with one slightly 
> ugly corner wherein dma_direct_supported() has to learn to look for an 
> IOMMU default domain and try iommu_dma_supported() before saying no, even 
> if it's clean everywhere else.

Yes, that actually is my main worry.  The "upgrading" from dma_iommu_ops
to direct / NULL is pretty easy and clean, it is the other way that is
a mess.

> The bypass flag is more 'balanced' in terms 
> of being equally invasive everywhere and preserving abstraction a bit 
> better. Plus I think it might let us bring back the default assignment of 
> dma_dummy_ops, which I do like the thought of :D

I was hoping to get rid of dma_dummy_ops once we've killed off the last
leftovers of allowing DMA with a NULL dma_mask or *dma_mask and just
reject all DMA operations in that case.

> Either way, making sure that the fundamental bypass decision is correct and 
> robust is still far more important than the implementation details.

So maybe we should massage intel-iommu toward that first.  Let me and Lu
know what makes sense to improve.

Powered by blists - more mailing lists