Re: [PATCH v5 05/17] dma-mapping: Provide an interface to allow allocate IOVA

[Leon Romanovsky <leon@kernel.org>]

On Tue, Jan 14, 2025 at 08:50:28PM +0000, Robin Murphy wrote:
> On 17/12/2024 1:00 pm, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > The existing .map_page() callback provides both allocating of IOVA
> > and linking DMA pages. That combination works great for most of the
> > callers who use it in control paths, but is less effective in fast
> > paths where there may be multiple calls to map_page().
> > 
> > These advanced callers already manage their data in some sort of
> > database and can perform IOVA allocation in advance, leaving range
> > linkage operation to be in fast path.
> > 
> > Provide an interface to allocate/deallocate IOVA and next patch
> > link/unlink DMA ranges to that specific IOVA.
> > 
> > In the new API a DMA mapping transaction is identified by a
> > struct dma_iova_state, which holds some recomputed information
> > for the transaction which does not change for each page being
> > mapped, so add a check if IOVA can be used for the specific
> > transaction.
> > 
> > The API is exported from dma-iommu as it is the only implementation
> > supported, the namespace is clearly different from iommu_* functions
> > which are not allowed to be used. This code layout allows us to save
> > function call per API call used in datapath as well as a lot of boilerplate
> > code.
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >   drivers/iommu/dma-iommu.c   | 74 +++++++++++++++++++++++++++++++++++++
> >   include/linux/dma-mapping.h | 49 ++++++++++++++++++++++++
> >   2 files changed, 123 insertions(+)
> > 
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 853247c42f7d..5906b47a300c 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -1746,6 +1746,80 @@ size_t iommu_dma_max_mapping_size(struct device *dev)
> >   	return SIZE_MAX;
> >   }
> > +/**
> > + * dma_iova_try_alloc - Try to allocate an IOVA space
> > + * @dev: Device to allocate the IOVA space for
> > + * @state: IOVA state
> > + * @phys: physical address
> > + * @size: IOVA size
> > + *
> > + * Check if @dev supports the IOVA-based DMA API, and if yes allocate IOVA space
> > + * for the given base address and size.
> > + *
> > + * Note: @phys is only used to calculate the IOVA alignment. Callers that always
> > + * do PAGE_SIZE aligned transfers can safely pass 0 here.
> > + *
> > + * Returns %true if the IOVA-based DMA API can be used and IOVA space has been
> > + * allocated, or %false if the regular DMA API should be used.
> > + */
> > +bool dma_iova_try_alloc(struct device *dev, struct dma_iova_state *state,
> > +		phys_addr_t phys, size_t size)
> > +{
> > +	struct iommu_domain *domain = iommu_get_dma_domain(dev);
> > +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
> > +	struct iova_domain *iovad = &cookie->iovad;
> > +	size_t iova_off = iova_offset(iovad, phys);
> > +	dma_addr_t addr;
> > +
> > +	memset(state, 0, sizeof(*state));
> > +	if (!use_dma_iommu(dev))
> > +		return false;
> 
> Can you guess why that return won't ever be taken?

I will move references to pointers after this check, but like Christoph
said, this "if ..." is taken a lot and we didn't see any issues with
inbox GCC versions.

> 
> > +	if (static_branch_unlikely(&iommu_deferred_attach_enabled) &&
> > +	    iommu_deferred_attach(dev, iommu_get_domain_for_dev(dev)))
> > +		return false;
> > +
> > +	if (WARN_ON_ONCE(!size))
> > +		return false;
> > +	if (WARN_ON_ONCE(size & DMA_IOVA_USE_SWIOTLB))
> 
> This looks weird. Why would a caller ever set an effectively-private flag in
> the first place? If it's actually supposed to be a maximum size check,
> please make it look like a maximum size check.

It is in-kernel API and the idea is to warn developers of something that
is not right and not perform defensive programming by doing size checks.

<...>

> > +/**
> > + * dma_iova_free - Free an IOVA space
> > + * @dev: Device to free the IOVA space for
> > + * @state: IOVA state
> > + *
> > + * Undoes a successful dma_try_iova_alloc().
> > + *
> > + * Note that all dma_iova_link() calls need to be undone first.  For callers
> > + * that never call dma_iova_unlink(), dma_iova_destroy() can be used instead
> > + * which unlinks all ranges and frees the IOVA space in a single efficient
> > + * operation.
> 
> That's only true if they *also* call dma_iova_link() in just the right way
> too.

We can update the comment section to any wording, feel free to propose.

> 
> > + */
> > +void dma_iova_free(struct device *dev, struct dma_iova_state *state)
> > +{
> > +	struct iommu_domain *domain = iommu_get_dma_domain(dev);
> > +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
> > +	struct iova_domain *iovad = &cookie->iovad;
> > +	size_t iova_start_pad = iova_offset(iovad, state->addr);
> > +	size_t size = dma_iova_size(state);
> > +
> > +	iommu_dma_free_iova(cookie, state->addr - iova_start_pad,
> > +			iova_align(iovad, size + iova_start_pad), NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(dma_iova_free);
> > +
> >   void iommu_setup_dma_ops(struct device *dev)
> >   {
> >   	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index b79925b1c433..55899d65668b 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -7,6 +7,8 @@
> >   #include <linux/dma-direction.h>
> >   #include <linux/scatterlist.h>
> >   #include <linux/bug.h>
> > +#include <linux/mem_encrypt.h>
> > +#include <linux/iommu.h>
> 
> Why are these being pulled in here?

It is rebase leftover.

> 
> >   /**
> >    * List of possible attributes associated with a DMA mapping. The semantics
> > @@ -72,6 +74,21 @@
> >   #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> > +struct dma_iova_state {
> > +	dma_addr_t addr;
> > +	size_t __size;
> > +};
> > +
> > +/*
> > + * Use the high bit to mark if we used swiotlb for one or more ranges.
> > + */
> > +#define DMA_IOVA_USE_SWIOTLB		(1ULL << 63)
> 
> This will give surprising results for 32-bit size_t (in fact I guess it
> might fire some build warnings already).

We got none, I will change to u64.

> 
> Thanks,
> Robin.

Thanks