From: David Stevens <stevensd@chromium.org> To: Will Deacon <will@kernel.org> Cc: Robin Murphy <robin.murphy@arm.com>, Joerg Roedel <joro@8bytes.org>, Lu Baolu <baolu.lu@linux.intel.com>, Tom Murphy <murphyt7@tcd.ie>, iommu@lists.linux-foundation.org, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 1/4] dma-iommu: fix sync_sg with swiotlb Date: Thu, 5 Aug 2021 12:40:40 +0900 [thread overview] Message-ID: <CAD=HUj6BoCZ31Dd-jeWKKbmt-jSWXiS2ZLwrFg40FFdPm0y7cA@mail.gmail.com> (raw) In-Reply-To: <20210802133037.GB28547@willie-the-truck> On Mon, Aug 2, 2021 at 10:30 PM Will Deacon <will@kernel.org> wrote: > > On Fri, Jul 09, 2021 at 12:34:59PM +0900, David Stevens wrote: > > From: David Stevens <stevensd@chromium.org> > > > > The is_swiotlb_buffer function takes the physical address of the swiotlb > > buffer, not the physical address of the original buffer. The sglist > > contains the physical addresses of the original buffer, so for the > > sync_sg functions to work properly when a bounce buffer might have been > > used, we need to use iommu_iova_to_phys to look up the physical address. > > This is what sync_single does, so call that function on each sglist > > segment. > > > > The previous code mostly worked because swiotlb does the transfer on map > > and unmap. However, any callers which use DMA_ATTR_SKIP_CPU_SYNC with > > sglists or which call sync_sg would not have had anything copied to the > > bounce buffer. > > > > Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers") > > Signed-off-by: David Stevens <stevensd@chromium.org> > > --- > > drivers/iommu/dma-iommu.c | 26 +++++++++++++------------- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > > index 7bcdd1205535..eac65302439e 100644 > > --- a/drivers/iommu/dma-iommu.c > > +++ b/drivers/iommu/dma-iommu.c > > @@ -811,14 +811,14 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev, > > if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev)) > > return; > > > > - for_each_sg(sgl, sg, nelems, i) { > > - if (!dev_is_dma_coherent(dev)) > > - arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir); > > - > > - if (is_swiotlb_buffer(sg_phys(sg))) > > + if (dev_is_untrusted(dev)) > > + for_each_sg(sgl, sg, nelems, i) > > + iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg), > > + sg->length, dir); > > + else > > + for_each_sg(sgl, sg, nelems, i) > > swiotlb_sync_single_for_cpu(dev, sg_phys(sg), > > sg->length, dir); > > Doesn't this skip arch_sync_dma_for_cpu() for non-coherent trusted devices? Whoops, this was supposed to be a call to arch_sync_dma_for_cpu, not to swiotlb_sync_single_for_cpu. Similar to the sync_sg_for_device case. > Why not skip the extra dev_is_untrusted(dev) call here and just call > iommu_dma_sync_single_for_cpu() for each entry regardless? iommu_dma_sync_single_for_cpu calls iommu_iova_to_phys to translate the dma_addr_t to a phys_addr_t. Since the physical address is readily available, I think it's better to avoid that extra work. > Will
next prev parent reply other threads:[~2021-08-05 3:40 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-09 3:34 [PATCH v2 0/4] Fixes for dma-iommu swiotlb bounce buffers David Stevens 2021-07-09 3:34 ` [PATCH v2 1/4] dma-iommu: fix sync_sg with swiotlb David Stevens 2021-08-02 13:30 ` Will Deacon 2021-08-05 3:40 ` David Stevens [this message] 2021-07-09 3:35 ` [PATCH v2 2/4] dma-iommu: fix arch_sync_dma for map " David Stevens 2021-08-02 13:40 ` Will Deacon 2021-08-02 13:50 ` Will Deacon 2021-07-09 3:35 ` [PATCH v2 3/4] dma-iommu: pass SKIP_CPU_SYNC to swiotlb unmap David Stevens 2021-08-02 13:54 ` Will Deacon 2021-08-05 5:26 ` David Stevens 2021-08-06 11:38 ` Will Deacon 2021-07-09 3:35 ` [PATCH v2 4/4] dma-iommu: Check CONFIG_SWIOTLB more broadly David Stevens
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='CAD=HUj6BoCZ31Dd-jeWKKbmt-jSWXiS2ZLwrFg40FFdPm0y7cA@mail.gmail.com' \ --to=stevensd@chromium.org \ --cc=baolu.lu@linux.intel.com \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=linux-kernel@vger.kernel.org \ --cc=murphyt7@tcd.ie \ --cc=robin.murphy@arm.com \ --cc=will@kernel.org \ --subject='Re: [PATCH v2 1/4] dma-iommu: fix sync_sg with swiotlb' \ /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
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).