Christoph Hellwig
2021-May-06 12:38 UTC
[RFC PATCH V2 0/7] Do not read from descripto ring
On Thu, May 06, 2021 at 04:12:17AM -0400, Michael S. Tsirkin wrote:> Let's try for just a bit, won't make this window anyway: > > I have an old idea. Add a way to find out that unmap is a nop > (or more exactly does not use the address/length). > Then in that case even with DMA API we do not need > the extra data. Hmm?So we actually do have a check for that from the early days of the DMA API, but it only works at compile time: CONFIG_NEED_DMA_MAP_STATE. But given how rare configs without an iommu or swiotlb are these days it has stopped to be very useful. Unfortunately a runtime-version is not entirely trivial, but maybe if we allow for false positives we could do something like this bool dma_direct_need_state(struct device *dev) { /* some areas could not be covered by any map at all */ if (dev->dma_range_map) return false; if (force_dma_unencrypted(dev)) return false; if (dma_direct_need_sync(dev)) return false; return *dev->dma_mask == DMA_BIT_MASK(64); } bool dma_need_state(struct device *dev) { const struct dma_map_ops *ops = get_dma_ops(dev); if (dma_map_direct(dev, ops)) return dma_direct_need_state(dev); return ops->unmap_page || ops->sync_single_for_cpu || ops->sync_single_for_device; }
Michael S. Tsirkin
2021-May-14 11:13 UTC
[RFC PATCH V2 0/7] Do not read from descripto ring
On Thu, May 06, 2021 at 01:38:29PM +0100, Christoph Hellwig wrote:> On Thu, May 06, 2021 at 04:12:17AM -0400, Michael S. Tsirkin wrote: > > Let's try for just a bit, won't make this window anyway: > > > > I have an old idea. Add a way to find out that unmap is a nop > > (or more exactly does not use the address/length). > > Then in that case even with DMA API we do not need > > the extra data. Hmm? > > So we actually do have a check for that from the early days of the DMA > API, but it only works at compile time: CONFIG_NEED_DMA_MAP_STATE. > > But given how rare configs without an iommu or swiotlb are these days > it has stopped to be very useful. Unfortunately a runtime-version is > not entirely trivial, but maybe if we allow for false positives we > could do something like this > > bool dma_direct_need_state(struct device *dev) > { > /* some areas could not be covered by any map at all */ > if (dev->dma_range_map) > return false; > if (force_dma_unencrypted(dev)) > return false; > if (dma_direct_need_sync(dev)) > return false; > return *dev->dma_mask == DMA_BIT_MASK(64); > } > > bool dma_need_state(struct device *dev) > { > const struct dma_map_ops *ops = get_dma_ops(dev); > > if (dma_map_direct(dev, ops)) > return dma_direct_need_state(dev); > return ops->unmap_page || > ops->sync_single_for_cpu || ops->sync_single_for_device; > }Yea that sounds like a good idea. We will need to document that. Something like: /* * dma_need_state - report whether unmap calls use the address and length * @dev: device to guery * * This is a runtime version of CONFIG_NEED_DMA_MAP_STATE. * * Return the value indicating whether dma_unmap_* and dma_sync_* calls for the device * use the DMA state parameters passed to them. * The DMA state parameters are: scatter/gather list/table, address and * length. * * If dma_need_state returns false then DMA state parameters are * ignored by all dma_unmap_* and dma_sync_* calls, so it is safe to pass 0 for * address and length, and DMA_UNMAP_SG_TABLE_INVALID and * DMA_UNMAP_SG_LIST_INVALID for s/g table and length respectively. * If dma_need_state returns true then DMA state might * be used and so the actual values are required. */ And we will need DMA_UNMAP_SG_TABLE_INVALID and DMA_UNMAP_SG_LIST_INVALID as pointers to an empty global table and list for calls such as dma_unmap_sgtable that dereference pointers before checking they are used. Does this look good? The table/length variants are for consistency, virtio specifically does not use s/g at the moment, but it seems nicer than leaving users wonder what to do about these. Thoughts? Jason want to try implementing? -- MST