linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Don Dutile <ddutile@redhat.com>
To: Logan Gunthorpe <logang@deltatee.com>,
	linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-block@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-mm@kvack.org, iommu@lists.linux-foundation.org
Cc: "Stephen Bates" <sbates@raithlin.com>,
	"Christoph Hellwig" <hch@lst.de>,
	"Dan Williams" <dan.j.williams@intel.com>,
	"Jason Gunthorpe" <jgg@ziepe.ca>,
	"Christian König" <christian.koenig@amd.com>,
	"John Hubbard" <jhubbard@nvidia.com>,
	"Matthew Wilcox" <willy@infradead.org>,
	"Daniel Vetter" <daniel.vetter@ffwll.ch>,
	"Jakowski Andrzej" <andrzej.jakowski@intel.com>,
	"Minturn Dave B" <dave.b.minturn@intel.com>,
	"Jason Ekstrand" <jason@jlekstrand.net>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	"Xiong Jianxin" <jianxin.xiong@intel.com>,
	"Bjorn Helgaas" <helgaas@kernel.org>,
	"Ira Weiny" <ira.weiny@intel.com>,
	"Robin Murphy" <robin.murphy@arm.com>
Subject: Re: [PATCH 11/16] iommu/dma: Support PCI P2PDMA pages in dma-iommu map_sg
Date: Tue, 11 May 2021 12:06:28 -0400	[thread overview]
Message-ID: <6003ed3d-5969-4201-3cbb-3bcf84385541@redhat.com> (raw)
In-Reply-To: <20210408170123.8788-12-logang@deltatee.com>

On 4/8/21 1:01 PM, Logan Gunthorpe wrote:
> When a PCI P2PDMA page is seen, set the IOVA length of the segment
> to zero so that it is not mapped into the IOVA. Then, in finalise_sg(),
> apply the appropriate bus address to the segment. The IOVA is not
> created if the scatterlist only consists of P2PDMA pages.
>
> Similar to dma-direct, the sg_mark_pci_p2pdma() flag is used to
> indicate bus address segments. On unmap, P2PDMA segments are skipped
> over when determining the start and end IOVA addresses.
>
> With this change, the flags variable in the dma_map_ops is
> set to DMA_F_PCI_P2PDMA_SUPPORTED to indicate support for
> P2PDMA pages.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
So, this code prevents use of p2pdma using an IOMMU, which wasn't checked and
short-circuited by other checks to use dma-direct?

So my overall comment to this code & related comments is that it should be sprinkled
with notes like "doesn't support IOMMU" and / or "TODO" when/if IOMMU is to be supported.
Or, if IOMMU-based p2pdma isn't supported in these routines directly, where/how they will be supported?

> ---
>   drivers/iommu/dma-iommu.c | 66 ++++++++++++++++++++++++++++++++++-----
>   1 file changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index af765c813cc8..ef49635f9819 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -20,6 +20,7 @@
>   #include <linux/mm.h>
>   #include <linux/mutex.h>
>   #include <linux/pci.h>
> +#include <linux/pci-p2pdma.h>
>   #include <linux/swiotlb.h>
>   #include <linux/scatterlist.h>
>   #include <linux/vmalloc.h>
> @@ -864,6 +865,16 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
>   		sg_dma_address(s) = DMA_MAPPING_ERROR;
>   		sg_dma_len(s) = 0;
>   
> +		if (is_pci_p2pdma_page(sg_page(s)) && !s_iova_len) {
> +			if (i > 0)
> +				cur = sg_next(cur);
> +
> +			pci_p2pdma_map_bus_segment(s, cur);
> +			count++;
> +			cur_len = 0;
> +			continue;
> +		}
> +
>   		/*
>   		 * Now fill in the real DMA data. If...
>   		 * - there is a valid output segment to append to
> @@ -961,10 +972,12 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	struct iova_domain *iovad = &cookie->iovad;
>   	struct scatterlist *s, *prev = NULL;
>   	int prot = dma_info_to_prot(dir, dev_is_dma_coherent(dev), attrs);
> +	struct dev_pagemap *pgmap = NULL;
> +	enum pci_p2pdma_map_type map_type;
>   	dma_addr_t iova;
>   	size_t iova_len = 0;
>   	unsigned long mask = dma_get_seg_boundary(dev);
> -	int i;
> +	int i, ret = 0;
>   
>   	if (static_branch_unlikely(&iommu_deferred_attach_enabled) &&
>   	    iommu_deferred_attach(dev, domain))
> @@ -993,6 +1006,31 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   		s_length = iova_align(iovad, s_length + s_iova_off);
>   		s->length = s_length;
>   
> +		if (is_pci_p2pdma_page(sg_page(s))) {
> +			if (sg_page(s)->pgmap != pgmap) {
> +				pgmap = sg_page(s)->pgmap;
> +				map_type = pci_p2pdma_map_type(pgmap, dev,
> +							       attrs);
> +			}
> +
> +			switch (map_type) {
> +			case PCI_P2PDMA_MAP_BUS_ADDR:
> +				/*
> +				 * A zero length will be ignored by
> +				 * iommu_map_sg() and then can be detected
> +				 * in __finalise_sg() to actually map the
> +				 * bus address.
> +				 */
> +				s->length = 0;
> +				continue;

> +			case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE:
> +				break;
So, this 'short-circuits' the use of the IOMMU, silently?
This seems ripe for users to enable IOMMU for secure computing reasons, and using/enabling p2pdma,
and not realizing that it isn't as secure as 1+1=2  appears to be.
If my understanding is wrong, please point me to the Documentation or code that corrects this mis-understanding.  I could have missed a warning when both are enabled in a past patch set.
Thanks.
--dd
> +			default:
> +				ret = -EREMOTEIO;
> +				goto out_restore_sg;
> +			}
> +		}
> +
>   		/*
>   		 * Due to the alignment of our single IOVA allocation, we can
>   		 * depend on these assumptions about the segment boundary mask:
> @@ -1015,6 +1053,9 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   		prev = s;
>   	}
>   
> +	if (!iova_len)
> +		return __finalise_sg(dev, sg, nents, 0);
> +
>   	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
>   	if (!iova)
>   		goto out_restore_sg;
> @@ -1032,13 +1073,13 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	iommu_dma_free_iova(cookie, iova, iova_len, NULL);
>   out_restore_sg:
>   	__invalidate_sg(sg, nents);
> -	return 0;
> +	return ret;
>   }
>   
>   static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>   		int nents, enum dma_data_direction dir, unsigned long attrs)
>   {
> -	dma_addr_t start, end;
> +	dma_addr_t end, start = DMA_MAPPING_ERROR;
>   	struct scatterlist *tmp;
>   	int i;
>   
> @@ -1054,14 +1095,22 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>   	 * The scatterlist segments are mapped into a single
>   	 * contiguous IOVA allocation, so this is incredibly easy.
>   	 */
> -	start = sg_dma_address(sg);
> -	for_each_sg(sg_next(sg), tmp, nents - 1, i) {
> +	for_each_sg(sg, tmp, nents, i) {
> +		if (sg_is_pci_p2pdma(tmp)) {
> +			sg_unmark_pci_p2pdma(tmp);
> +			continue;
> +		}
>   		if (sg_dma_len(tmp) == 0)
>   			break;
> -		sg = tmp;
> +
> +		if (start == DMA_MAPPING_ERROR)
> +			start = sg_dma_address(tmp);
> +
> +		end = sg_dma_address(tmp) + sg_dma_len(tmp);
>   	}
> -	end = sg_dma_address(sg) + sg_dma_len(sg);
> -	__iommu_dma_unmap(dev, start, end - start);
> +
> +	if (start != DMA_MAPPING_ERROR)
> +		__iommu_dma_unmap(dev, start, end - start);
>   }
>   
overall, fiddling with the generic dma-iommu code instead of using a dma-ops-based, p2pdma function that has it carved out and separated/refactored out to be cleaner seems less complicated, but I'm guessing you tried that and it was too complicated to do?
--dd

>   static dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
> @@ -1254,6 +1303,7 @@ static unsigned long iommu_dma_get_merge_boundary(struct device *dev)
>   }
>   
>   static const struct dma_map_ops iommu_dma_ops = {
> +	.flags			= DMA_F_PCI_P2PDMA_SUPPORTED,
wait, it's a const that's always turned on?
shouldn't the define for this flag be 0 for non-p2pdma configs?

>   	.alloc			= iommu_dma_alloc,
>   	.free			= iommu_dma_free,
>   	.alloc_pages		= dma_common_alloc_pages,


  parent reply	other threads:[~2021-05-11 16:06 UTC|newest]

Thread overview: 99+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 17:01 [PATCH 00/16] Add new DMA mapping operation for P2PDMA Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 01/16] PCI/P2PDMA: Pass gfp_mask flags to upstream_bridge_distance_warn() Logan Gunthorpe
2021-05-02  3:58   ` John Hubbard
2021-05-03 15:57     ` Logan Gunthorpe
2021-05-03 18:17       ` John Hubbard
2021-05-03 18:20         ` Logan Gunthorpe
2021-05-03 18:23           ` John Hubbard
2021-05-03 18:24         ` Christoph Hellwig
2021-05-11 16:05     ` Don Dutile
2021-05-11 16:12       ` Logan Gunthorpe
2021-05-11 16:23         ` Don Dutile
2021-04-08 17:01 ` [PATCH 02/16] PCI/P2PDMA: Avoid pci_get_slot() which sleeps Logan Gunthorpe
2021-05-02  5:35   ` John Hubbard
2021-05-03 16:08     ` Logan Gunthorpe
2021-05-03 18:20       ` John Hubbard
2021-05-03 18:25       ` Christoph Hellwig
2021-05-11 16:05     ` Don Dutile
2021-05-11 16:16       ` Logan Gunthorpe
2021-05-11 16:05   ` Don Dutile
2021-05-11 16:14     ` Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 03/16] PCI/P2PDMA: Attempt to set map_type if it has not been set Logan Gunthorpe
2021-05-02 19:58   ` John Hubbard
2021-05-03 16:17     ` Logan Gunthorpe
2021-05-03 18:22       ` John Hubbard
2021-05-03 18:35       ` Christoph Hellwig
2021-05-03 18:46         ` Logan Gunthorpe
2021-05-11 16:05     ` Don Dutile
2021-04-08 17:01 ` [PATCH 04/16] PCI/P2PDMA: Refactor pci_p2pdma_map_type() to take pagmap and device Logan Gunthorpe
2021-05-02 20:41   ` John Hubbard
2021-05-03 16:30     ` Logan Gunthorpe
2021-05-03 18:31       ` John Hubbard
2021-05-03 18:56         ` Logan Gunthorpe
2021-05-03 21:54           ` John Hubbard
2021-05-03 22:57             ` Jason Gunthorpe
2021-05-03 23:40               ` John Hubbard
2021-04-08 17:01 ` [PATCH 05/16] dma-mapping: Introduce dma_map_sg_p2pdma() Logan Gunthorpe
2021-04-27 19:22   ` Jason Gunthorpe
2021-04-27 22:49     ` Logan Gunthorpe
2021-04-27 19:31   ` Jason Gunthorpe
2021-04-27 22:55     ` Logan Gunthorpe
2021-04-27 23:01       ` Jason Gunthorpe
2021-05-03 18:28         ` Christoph Hellwig
2021-05-03 18:31           ` Logan Gunthorpe
2021-05-02 21:23   ` John Hubbard
2021-05-03 16:38     ` Logan Gunthorpe
2021-05-11 16:05   ` Don Dutile
2021-04-08 17:01 ` [PATCH 06/16] lib/scatterlist: Add flag for indicating P2PDMA segments in an SGL Logan Gunthorpe
2021-05-02 22:34   ` John Hubbard
2021-04-08 17:01 ` [PATCH 07/16] PCI/P2PDMA: Make pci_p2pdma_map_type() non-static Logan Gunthorpe
2021-05-02 22:44   ` John Hubbard
2021-05-03 16:39     ` Logan Gunthorpe
2021-05-11 16:06   ` Don Dutile
2021-05-11 16:17     ` Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 08/16] PCI/P2PDMA: Introduce helpers for dma_map_sg implementations Logan Gunthorpe
2021-05-02 22:52   ` John Hubbard
2021-05-03  0:50   ` John Hubbard
2021-05-03 17:15     ` Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 09/16] dma-direct: Support PCI P2PDMA pages in dma-direct map_sg Logan Gunthorpe
2021-04-27 19:33   ` Jason Gunthorpe
2021-04-27 19:40     ` Jason Gunthorpe
2021-04-27 22:56       ` Logan Gunthorpe
2021-05-02 23:28   ` John Hubbard
2021-05-02 23:32     ` John Hubbard
2021-05-03 17:06       ` Logan Gunthorpe
2021-05-03 16:55     ` Logan Gunthorpe
2021-05-04  0:12       ` John Hubbard
2021-05-03 17:04     ` Logan Gunthorpe
2021-05-04  0:01       ` John Hubbard
2021-04-08 17:01 ` [PATCH 10/16] dma-mapping: Add flags to dma_map_ops to indicate PCI P2PDMA support Logan Gunthorpe
2021-05-03  0:32   ` John Hubbard
2021-05-03 17:09     ` Logan Gunthorpe
2021-05-11 16:06   ` Don Dutile
2021-05-11 16:19     ` Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 11/16] iommu/dma: Support PCI P2PDMA pages in dma-iommu map_sg Logan Gunthorpe
2021-04-27 19:43   ` Jason Gunthorpe
2021-04-27 22:59     ` Logan Gunthorpe
2021-05-03  1:14   ` John Hubbard
2021-05-06 23:59     ` Logan Gunthorpe
2021-05-11 16:06   ` Don Dutile [this message]
2021-05-11 16:35     ` Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 12/16] nvme-pci: Check DMA ops when indicating support for PCI P2PDMA Logan Gunthorpe
2021-05-03  1:29   ` John Hubbard
2021-05-03 17:17     ` Logan Gunthorpe
2021-05-04  0:17       ` John Hubbard
2021-04-08 17:01 ` [PATCH 13/16] nvme-pci: Convert to using dma_map_sg_p2pdma for p2pdma pages Logan Gunthorpe
2021-05-03  1:34   ` John Hubbard
2021-05-03 17:19     ` Logan Gunthorpe
2021-05-04  0:26       ` John Hubbard
2021-04-08 17:01 ` [PATCH 14/16] nvme-rdma: Ensure dma support when using p2pdma Logan Gunthorpe
2021-04-27 19:47   ` Jason Gunthorpe
2021-04-27 22:59     ` Logan Gunthorpe
2021-05-03  1:37   ` John Hubbard
2021-04-08 17:01 ` [PATCH 15/16] RDMA/rw: use dma_map_sg_p2pdma() Logan Gunthorpe
2021-04-08 17:01 ` [PATCH 16/16] PCI/P2PDMA: Remove pci_p2pdma_[un]map_sg() Logan Gunthorpe
2021-04-27 19:28 ` [PATCH 00/16] Add new DMA mapping operation for P2PDMA Jason Gunthorpe
2021-04-27 20:21   ` John Hubbard
2021-04-27 20:48     ` Dan Williams
2021-05-02  1:22 ` John Hubbard
2021-05-11 16:05 ` Don Dutile

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6003ed3d-5969-4201-3cbb-3bcf84385541@redhat.com \
    --to=ddutile@redhat.com \
    --cc=andrzej.jakowski@intel.com \
    --cc=christian.koenig@amd.com \
    --cc=dan.j.williams@intel.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dave.b.minturn@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hch@lst.de \
    --cc=helgaas@kernel.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=ira.weiny@intel.com \
    --cc=jason@jlekstrand.net \
    --cc=jgg@ziepe.ca \
    --cc=jhubbard@nvidia.com \
    --cc=jianxin.xiong@intel.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=logang@deltatee.com \
    --cc=robin.murphy@arm.com \
    --cc=sbates@raithlin.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).