linux-kernel - Re: [PATCH v2 2/2] dma: add IOMMU static calls with clear default ops

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240718034854.GA31912@lst.de>
Date: Thu, 18 Jul 2024 05:48:54 +0200
From: Christoph Hellwig <hch@....de>
To: Leon Romanovsky <leon@...nel.org>
Cc: Christoph Hellwig <hch@....de>, Robin Murphy <robin.murphy@....com>,
	Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Leon Romanovsky <leonro@...dia.com>,
	Easwar Hariharan <eahariha@...ux.microsoft.com>,
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
	Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH v2 2/2] dma: add IOMMU static calls with clear default
 ops

On Wed, Jul 17, 2024 at 03:37:11PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@...dia.com>
> 
> Most of the arch DMA ops (which often, but not  always, involve
> some sort of IOMMU) are using the same DMA operations. These DMA
> operations are default ones implemented in drivers/iomem/dma-iommu.c.

I'm not sure I agree with the exact wording.  There's tons of arch DMA
ops not in any way tied to dma-iommu (or the iommu subsystem, but not
dma-iommu like the arm32 ones), but for all modern platforms dma-iommu
is what actually matters.

> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> Signed-off-by: Leon Romanovsky <leon@...nel.org>

A double-signoff for the same person is a bit weird, isn't it?

>  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> @@ -1756,10 +1733,10 @@ void iommu_setup_dma_ops(struct device *dev)
>  	if (iommu_is_dma_domain(domain)) {
>  		if (iommu_dma_init_domain(domain, dev))
>  			goto out_err;
> +		dev->dma_iommu = true;
> +	} else if (dev->dma_iommu) {
>  		/* Clean up if we've switched *from* a DMA domain */
> +		dev->dma_iommu = false;
>  	}

Strictly speaking we can no remove the if part of the else if above.
Or reword this a bit to:

	dev->dma_iommu = iommu_is_dma_domain(domain);
	if (dev->dma_iommu && iommu_dma_init_domain(domain, dev))
		goto out_err;

> diff --git a/include/linux/device.h b/include/linux/device.h
> index ace039151cb8..7fa1e40b617a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -822,6 +822,9 @@ struct device {
>  #ifdef CONFIG_DMA_NEED_SYNC
>  	bool			dma_skip_sync:1;
>  #endif
> +#ifdef CONFIG_IOMMU_DMA
> +	bool			dma_iommu : 1;
> +#endif

The kerneldoc above should be updated to describe this field.
Please also add the maintainers of this file to the next round.

> +static inline bool dma_is_default_iommu(struct device *dev)
> +{
> +#ifdef CONFIG_IOMMU_DMA
> +	return dev->dma_iommu;
> +#else
> +	return false;
> +#endif
> +}

The normal style would be to move the ifdefs outside the helper
function.  Also maybe name this use_dma_iommu?

>  static bool dma_go_direct(struct device *dev, dma_addr_t mask,
>  		const struct dma_map_ops *ops)
>  {
> +	WARN_ON_ONCE(ops && dma_is_default_iommu(dev));

I'd prefer to keep this out of the fast path and only do it in
say dma_set_mask.  And fail the call while we're at it.

> +	if (likely(!ops) && !dma_is_default_iommu(dev))

The likely should cover both conditions.

>  		return true;
> +
>  #ifdef CONFIG_DMA_OPS_BYPASS
> +	WARN_ON_ONCE(dev->dma_ops_bypass && dma_is_default_iommu(dev));
>  	if (dev->dma_ops_bypass)

Let's skip this and think about it if we ever use the bypass for
something that is not powerpc with it's own dma ops.

> @@ -323,8 +346,12 @@ void dma_unmap_resource(struct device *dev, dma_addr_t addr, size_t size,
>  	const struct dma_map_ops *ops = get_dma_ops(dev);
>  
>  	BUG_ON(!valid_dma_direction(dir));
> -	if (!dma_map_direct(dev, ops) && ops->unmap_resource)
> -		ops->unmap_resource(dev, addr, size, dir, attrs);
> +	if (!dma_map_direct(dev, ops)) {
> +		if (dma_is_default_iommu(dev))
> +			iommu_dma_unmap_resource(dev, addr, size, dir, attrs);
> +		else if (ops->unmap_resource)
> +			ops->unmap_resource(dev, addr, size, dir, attrs);
> +	}

I'd prefer to order this as:

	if (dma_map_direct(dev, ops))
		; /* nothing to do: uncached and no swiotlb */
	else if (use_dma_iommu(dev))
		iommu_dma_unmap_resource(dev, addr, size, dir, attrs);
	else if (ops->unmap_resource)
		ops->unmap_resource(dev, addr, size, dir, attrs);

> +	return (!ops || dma_is_default_iommu(dev));

dma_is_default_iommu implies !ops, so the second condition is
redundant.

>  EXPORT_SYMBOL_GPL(dma_opt_mapping_size);
> @@ -888,7 +943,12 @@ unsigned long dma_get_merge_boundary(struct device *dev)
>  {
>  	const struct dma_map_ops *ops = get_dma_ops(dev);
>  
> +	if (!ops)
> +		return 0;	/* can't merge */
> +	if (dma_is_default_iommu(dev))
> +		return iommu_dma_get_merge_boundary(dev);

The second check needs to move up here.