| Message ID | 20190430002952.18909-4-tmurphy@arista.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/amd: Convert the AMD iommu driver to the dma-iommu api | expand |
> static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, > - size_t size, int prot, struct iommu_domain *domain) > + size_t size, int prot, struct iommu_domain *domain, > + dma_addr_t dma_limit) Can we just call this dma_mask? > static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, > @@ -1250,7 +1251,8 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, > if (!msi_page) > return NULL; > > - iova = __iommu_dma_map(dev, msi_addr, size, prot, domain); > + iova = __iommu_dma_map(dev, msi_addr, size, prot, domain, > + dma_get_mask(dev)); Hmm, I don't think we need the DMA mask for the MSI mapping, this should probably always use a 64-bit mask. Or we could just untangle it from the DMA mapping fast path entire, something like: --- From 0debafc85174ca830f2e371ff8e8f7465bde3ad8 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Tue, 30 Apr 2019 07:06:23 -0400 Subject: iommu/dma: opencode __iommu_dma_map in iommu_dma_get_msi_page The MSI page mapping really is a little different from the normal DMA mappings and doesn't need to look at the DMA mask. Just open code it instead of trying to squeeze the behavior into the DMA path helpers. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/iommu/dma-iommu.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 58c35bab7626..2ac0df0879d7 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -358,11 +358,6 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain, struct iova_domain *iovad = &cookie->iovad; unsigned long shift, iova_len, iova = 0; - if (cookie->type == IOMMU_DMA_MSI_COOKIE) { - cookie->msi_iova += size; - return cookie->msi_iova - size; - } - shift = iova_shift(iovad); iova_len = size >> shift; /* @@ -397,10 +392,7 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie, { struct iova_domain *iovad = &cookie->iovad; - /* The MSI case is only ever cleaning up its most recent allocation */ - if (cookie->type == IOMMU_DMA_MSI_COOKIE) - cookie->msi_iova -= size; - else if (cookie->fq_domain) /* non-strict mode */ + if (cookie->fq_domain) /* non-strict mode */ queue_iova(iovad, iova_pfn(iovad, iova), size >> iova_shift(iovad), 0); else @@ -430,14 +422,10 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, { struct iommu_domain *domain = iommu_get_dma_domain(dev); struct iommu_dma_cookie *cookie = domain->iova_cookie; - size_t iova_off = 0; + size_t iova_off = iova_offset(&cookie->iovad, phys); dma_addr_t iova; - if (cookie->type == IOMMU_DMA_IOVA_COOKIE) { - iova_off = iova_offset(&cookie->iovad, phys); - size = iova_align(&cookie->iovad, size + iova_off); - } - + size = iova_align(&cookie->iovad, size + iova_off); iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev); if (!iova) return DMA_MAPPING_ERROR; @@ -1121,7 +1109,6 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, { struct iommu_dma_cookie *cookie = domain->iova_cookie; struct iommu_dma_msi_page *msi_page; - dma_addr_t iova; int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; size_t size = cookie_msi_granule(cookie); @@ -1134,16 +1121,16 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, if (!msi_page) return NULL; - iova = __iommu_dma_map(dev, msi_addr, size, prot); - if (iova == DMA_MAPPING_ERROR) + if (iommu_map(domain, cookie->msi_iova, msi_addr, size, prot)) goto out_free_page; INIT_LIST_HEAD(&msi_page->list); msi_page->phys = msi_addr; - msi_page->iova = iova; + msi_page->iova = cookie->msi_iova; list_add(&msi_page->list, &cookie->msi_page_list); - return msi_page; + cookie->msi_iova += size; + return msi_page; out_free_page: kfree(msi_page); return NULL;
On 30/04/2019 12:12, Christoph Hellwig wrote: >> static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, >> - size_t size, int prot, struct iommu_domain *domain) >> + size_t size, int prot, struct iommu_domain *domain, >> + dma_addr_t dma_limit) > > Can we just call this dma_mask? > >> static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, >> @@ -1250,7 +1251,8 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, >> if (!msi_page) >> return NULL; >> >> - iova = __iommu_dma_map(dev, msi_addr, size, prot, domain); >> + iova = __iommu_dma_map(dev, msi_addr, size, prot, domain, >> + dma_get_mask(dev)); > > Hmm, I don't think we need the DMA mask for the MSI mapping, this > should probably always use a 64-bit mask. If that were true then we wouldn't need DMA masks for regular mappings either. If we have to map the MSI doorbell at all, then we certainly have to place it at an IOVA that the relevant device is actually capable of addressing. Robin. > Or we could just untangle > it from the DMA mapping fast path entire, something like: > > --- > From 0debafc85174ca830f2e371ff8e8f7465bde3ad8 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Tue, 30 Apr 2019 07:06:23 -0400 > Subject: iommu/dma: opencode __iommu_dma_map in iommu_dma_get_msi_page > > The MSI page mapping really is a little different from the normal DMA > mappings and doesn't need to look at the DMA mask. Just open code > it instead of trying to squeeze the behavior into the DMA path helpers. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/iommu/dma-iommu.c | 27 +++++++-------------------- > 1 file changed, 7 insertions(+), 20 deletions(-) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 58c35bab7626..2ac0df0879d7 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -358,11 +358,6 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain, > struct iova_domain *iovad = &cookie->iovad; > unsigned long shift, iova_len, iova = 0; > > - if (cookie->type == IOMMU_DMA_MSI_COOKIE) { > - cookie->msi_iova += size; > - return cookie->msi_iova - size; > - } > - > shift = iova_shift(iovad); > iova_len = size >> shift; > /* > @@ -397,10 +392,7 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie, > { > struct iova_domain *iovad = &cookie->iovad; > > - /* The MSI case is only ever cleaning up its most recent allocation */ > - if (cookie->type == IOMMU_DMA_MSI_COOKIE) > - cookie->msi_iova -= size; > - else if (cookie->fq_domain) /* non-strict mode */ > + if (cookie->fq_domain) /* non-strict mode */ > queue_iova(iovad, iova_pfn(iovad, iova), > size >> iova_shift(iovad), 0); > else > @@ -430,14 +422,10 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, > { > struct iommu_domain *domain = iommu_get_dma_domain(dev); > struct iommu_dma_cookie *cookie = domain->iova_cookie; > - size_t iova_off = 0; > + size_t iova_off = iova_offset(&cookie->iovad, phys); > dma_addr_t iova; > > - if (cookie->type == IOMMU_DMA_IOVA_COOKIE) { > - iova_off = iova_offset(&cookie->iovad, phys); > - size = iova_align(&cookie->iovad, size + iova_off); > - } > - > + size = iova_align(&cookie->iovad, size + iova_off); > iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev); > if (!iova) > return DMA_MAPPING_ERROR; > @@ -1121,7 +1109,6 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, > { > struct iommu_dma_cookie *cookie = domain->iova_cookie; > struct iommu_dma_msi_page *msi_page; > - dma_addr_t iova; > int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; > size_t size = cookie_msi_granule(cookie); > > @@ -1134,16 +1121,16 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, > if (!msi_page) > return NULL; > > - iova = __iommu_dma_map(dev, msi_addr, size, prot); > - if (iova == DMA_MAPPING_ERROR) > + if (iommu_map(domain, cookie->msi_iova, msi_addr, size, prot)) > goto out_free_page; > > INIT_LIST_HEAD(&msi_page->list); > msi_page->phys = msi_addr; > - msi_page->iova = iova; > + msi_page->iova = cookie->msi_iova; > list_add(&msi_page->list, &cookie->msi_page_list); > - return msi_page; > > + cookie->msi_iova += size; > + return msi_page; > out_free_page: > kfree(msi_page); > return NULL; >
On Tue, Apr 30, 2019 at 12:27:02PM +0100, Robin Murphy wrote: > > Hmm, I don't think we need the DMA mask for the MSI mapping, this > > should probably always use a 64-bit mask. > > If that were true then we wouldn't need DMA masks for regular mappings > either. If we have to map the MSI doorbell at all, then we certainly have to > place it at an IOVA that the relevant device is actually capable of > addressing. Well, as shown by the patch below we don't even look at the DMA mask for the MSI page - we just allocate from bottom to top.
On 30/04/2019 12:32, Christoph Hellwig wrote: > On Tue, Apr 30, 2019 at 12:27:02PM +0100, Robin Murphy wrote: >>> Hmm, I don't think we need the DMA mask for the MSI mapping, this >>> should probably always use a 64-bit mask. >> >> If that were true then we wouldn't need DMA masks for regular mappings >> either. If we have to map the MSI doorbell at all, then we certainly have to >> place it at an IOVA that the relevant device is actually capable of >> addressing. > > Well, as shown by the patch below we don't even look at the DMA mask > for the MSI page - we just allocate from bottom to top. In the trivial cookie for unmanaged domains, yes, but in that case the responsibility is on VFIO to provide a suitable (i.e. sub-32-bit) address range for that cookie in the first place. In the managed case, allocation uses the streaming mask via iommu_dma_get_msi_page() calling __iommu_dma_map(). Admittedly the mask can then get overlooked when reusing an existing mapping, which strictly could pose a problem if you have multiple devices with incompatible masks in the same group (and such that the PCI stuff doesn't already mitigate it), but that's such an obscure corner case that I'm reticent to introduce the complication to handle it until it's actually proven necessary. Robin.
Just to make this clear, I won't apply Christoph's patch (the one in this email thread) and instead the only change I will make is to rename dma_limit to dma_mask. On Tue, Apr 30, 2019 at 1:05 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 30/04/2019 12:32, Christoph Hellwig wrote: > > On Tue, Apr 30, 2019 at 12:27:02PM +0100, Robin Murphy wrote: > >>> Hmm, I don't think we need the DMA mask for the MSI mapping, this > >>> should probably always use a 64-bit mask. > >> > >> If that were true then we wouldn't need DMA masks for regular mappings > >> either. If we have to map the MSI doorbell at all, then we certainly have to > >> place it at an IOVA that the relevant device is actually capable of > >> addressing. > > > > Well, as shown by the patch below we don't even look at the DMA mask > > for the MSI page - we just allocate from bottom to top. > > In the trivial cookie for unmanaged domains, yes, but in that case the > responsibility is on VFIO to provide a suitable (i.e. sub-32-bit) > address range for that cookie in the first place. In the managed case, > allocation uses the streaming mask via iommu_dma_get_msi_page() calling > __iommu_dma_map(). Admittedly the mask can then get overlooked when > reusing an existing mapping, which strictly could pose a problem if you > have multiple devices with incompatible masks in the same group (and > such that the PCI stuff doesn't already mitigate it), but that's such an > obscure corner case that I'm reticent to introduce the complication to > handle it until it's actually proven necessary. > > Robin.
On Mon, May 06, 2019 at 06:56:13PM +0100, Tom Murphy wrote: > Just to make this clear, I won't apply Christoph's patch (the one in > this email thread) and instead the only change I will make is to > rename dma_limit to dma_mask. Sounds good for now.
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index c18f74ad1e8b..df03104978d7 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -436,7 +436,8 @@ static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr, } static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, - size_t size, int prot, struct iommu_domain *domain) + size_t size, int prot, struct iommu_domain *domain, + dma_addr_t dma_limit) { struct iommu_dma_cookie *cookie = domain->iova_cookie; size_t iova_off = 0; @@ -447,7 +448,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, size = iova_align(&cookie->iovad, size + iova_off); } - iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev); + iova = iommu_dma_alloc_iova(domain, size, dma_limit, dev); if (!iova) return DMA_MAPPING_ERROR; @@ -490,7 +491,7 @@ static void *iommu_dma_alloc_contiguous(struct device *dev, size_t size, return NULL; *dma_handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot, - iommu_get_dma_domain(dev)); + iommu_get_dma_domain(dev), dev->coherent_dma_mask); if (*dma_handle == DMA_MAPPING_ERROR) { if (!dma_release_from_contiguous(dev, page, count)) __free_pages(page, page_order); @@ -760,7 +761,7 @@ static void *iommu_dma_alloc_pool(struct device *dev, size_t size, *dma_handle = __iommu_dma_map(dev, page_to_phys(page), size, dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs), - iommu_get_domain_for_dev(dev)); + iommu_get_domain_for_dev(dev), dev->coherent_dma_mask); if (*dma_handle == DMA_MAPPING_ERROR) { dma_free_from_pool(vaddr, PAGE_ALIGN(size)); return NULL; @@ -850,7 +851,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page, dma_handle =__iommu_dma_map(dev, phys, size, dma_info_to_prot(dir, coherent, attrs), - iommu_get_dma_domain(dev)); + iommu_get_dma_domain(dev), dma_get_mask(dev)); if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC) && dma_handle != DMA_MAPPING_ERROR) arch_sync_dma_for_device(dev, phys, size, dir); @@ -1065,7 +1066,7 @@ static dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys, return __iommu_dma_map(dev, phys, size, dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO, - iommu_get_dma_domain(dev)); + iommu_get_dma_domain(dev), dma_get_mask(dev)); } static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, @@ -1250,7 +1251,8 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, if (!msi_page) return NULL; - iova = __iommu_dma_map(dev, msi_addr, size, prot, domain); + iova = __iommu_dma_map(dev, msi_addr, size, prot, domain, + dma_get_mask(dev)); if (iova == DMA_MAPPING_ERROR) goto out_free_page;
Use the dev->coherent_dma_mask when allocating in the dma-iommu ops api. Signed-off-by: Tom Murphy <tmurphy@arista.com> --- drivers/iommu/dma-iommu.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)