Re: [PATCH] iommu/dma: Handle MSI mappings separately
On Mon, Jul 29, 2019 at 04:32:38PM +0100, Robin Murphy wrote: > drivers/iommu/dma-iommu.c | 17 ++--- > 1 file changed, 10 insertions(+), 7 deletions(-) Applied to iommu/fixes, thanks Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Handle MSI mappings separately
On Tue, Jul 30, 2019 at 11:43:25AM +0100, Robin Murphy wrote:
> > Hmm. I remember proposing this patch and you didn't like it because
> > we could also have msis for a !IOMMU_DMA_IOVA_COOKIE cookie type.
> > Or did we talk past each other?
>
> Do you have a pointer? That sparks the vaguest of memories, but I can't seem
> to turn anything up in my inbox. If that was my objection, though, it sounds
> like your patch was probably trying to go a step or two further than this
> one.
I can't find anything either. This must have been a git tree I passed
around to you before posting it.
> > Note that if this change turns out to be valid we should also
> > clean up the iommu_dma_free_iova() side.
>
> We're not touching the iommu_dma_{alloc,free}_iova() path here; those are
> designed to cope with both types of cookie, and I think that's a reasonable
> abstraction to keep. This is just getting rid of the asymmetry - and now bug
> - caused by trying to keep the MSI page flow going through a special case in
> __iommu_dma_map() despite that having evolved into a more specific DMA
> domain fastpath (there's no corresponding unmap special case since MSI
> mappings just persist and get recycled until the domain is destroyed).
Ok, that might have been the issue with my earlier patch..
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Handle MSI mappings separately
On 30/07/2019 07:28, Christoph Hellwig wrote:
On Mon, Jul 29, 2019 at 04:32:38PM +0100, Robin Murphy wrote:
MSI pages must always be mapped into a device's *current* domain, which
*might* be the default DMA domain, but might instead be a VFIO domain
with its own MSI cookie. This subtlety got accidentally lost in the
streamlining of __iommu_dma_map(), but rather than reintroduce more
complexity and/or special-casing, it turns out neater to just split this
path out entirely.
Since iommu_dma_get_msi_page() already duplicates much of what
__iommu_dma_map() does, it can easily just make the allocation and
mapping calls directly as well. That way we can further streamline the
helper back to exclusively operating on DMA domains.
Fixes: b61d271e59d7 ("iommu/dma: Move domain lookup into
__iommu_dma_{map,unmap}")
Reported-by: Shameer Kolothum
Reported-by: Andre Przywara
Signed-off-by: Robin Murphy
Hmm. I remember proposing this patch and you didn't like it because
we could also have msis for a !IOMMU_DMA_IOVA_COOKIE cookie type.
Or did we talk past each other?
Do you have a pointer? That sparks the vaguest of memories, but I can't
seem to turn anything up in my inbox. If that was my objection, though,
it sounds like your patch was probably trying to go a step or two
further than this one.
Note that if this change turns out to be valid we should also
clean up the iommu_dma_free_iova() side.
We're not touching the iommu_dma_{alloc,free}_iova() path here; those
are designed to cope with both types of cookie, and I think that's a
reasonable abstraction to keep. This is just getting rid of the
asymmetry - and now bug - caused by trying to keep the MSI page flow
going through a special case in __iommu_dma_map() despite that having
evolved into a more specific DMA domain fastpath (there's no
corresponding unmap special case since MSI mappings just persist and get
recycled until the domain is destroyed).
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Handle MSI mappings separately
On Mon, Jul 29, 2019 at 04:32:38PM +0100, Robin Murphy wrote:
> MSI pages must always be mapped into a device's *current* domain, which
> *might* be the default DMA domain, but might instead be a VFIO domain
> with its own MSI cookie. This subtlety got accidentally lost in the
> streamlining of __iommu_dma_map(), but rather than reintroduce more
> complexity and/or special-casing, it turns out neater to just split this
> path out entirely.
>
> Since iommu_dma_get_msi_page() already duplicates much of what
> __iommu_dma_map() does, it can easily just make the allocation and
> mapping calls directly as well. That way we can further streamline the
> helper back to exclusively operating on DMA domains.
>
> Fixes: b61d271e59d7 ("iommu/dma: Move domain lookup into
> __iommu_dma_{map,unmap}")
> Reported-by: Shameer Kolothum
> Reported-by: Andre Przywara
> Signed-off-by: Robin Murphy
Hmm. I remember proposing this patch and you didn't like it because
we could also have msis for a !IOMMU_DMA_IOVA_COOKIE cookie type.
Or did we talk past each other?
Note that if this change turns out to be valid we should also
clean up the iommu_dma_free_iova() side.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Handle MSI mappings separately
On 2019-07-29 16:32, Robin Murphy wrote:
MSI pages must always be mapped into a device's *current* domain,
which
*might* be the default DMA domain, but might instead be a VFIO domain
with its own MSI cookie. This subtlety got accidentally lost in the
streamlining of __iommu_dma_map(), but rather than reintroduce more
complexity and/or special-casing, it turns out neater to just split
this
path out entirely.
Since iommu_dma_get_msi_page() already duplicates much of what
__iommu_dma_map() does, it can easily just make the allocation and
mapping calls directly as well. That way we can further streamline
the
helper back to exclusively operating on DMA domains.
Fixes: b61d271e59d7 ("iommu/dma: Move domain lookup into
__iommu_dma_{map,unmap}")
Reported-by: Shameer Kolothum
Reported-by: Andre Przywara
Signed-off-by: Robin Murphy
With this patch, my TX2 is back in business with a bnx2x device passed
to a guest. FWIW:
Tested-by: Marc Zyngier
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Handle MSI mappings separately
On Mon, 29 Jul 2019 16:32:38 +0100
Robin Murphy wrote:
Hi,
> MSI pages must always be mapped into a device's *current* domain, which
> *might* be the default DMA domain, but might instead be a VFIO domain
> with its own MSI cookie. This subtlety got accidentally lost in the
> streamlining of __iommu_dma_map(), but rather than reintroduce more
> complexity and/or special-casing, it turns out neater to just split this
> path out entirely.
>
> Since iommu_dma_get_msi_page() already duplicates much of what
> __iommu_dma_map() does, it can easily just make the allocation and
> mapping calls directly as well. That way we can further streamline the
> helper back to exclusively operating on DMA domains.
>
> Fixes: b61d271e59d7 ("iommu/dma: Move domain lookup into
> __iommu_dma_{map,unmap}")
> Reported-by: Shameer Kolothum
> Reported-by: Andre Przywara
> Signed-off-by: Robin Murphy
Thanks, that indeed fixes the pass through problem for me, the NVMe and SATA
controller can now happily receive MSIs again.
Tested-by: Andre Przywara
Cheers,
Andre.
> ---
> drivers/iommu/dma-iommu.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index a7f9c3edbcb2..6441197a75ea 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -459,13 +459,11 @@ 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;
> + struct iova_domain *iovad = >iovad;
> + size_t iova_off = iova_offset(iovad, phys);
> dma_addr_t iova;
>
> - if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> - iova_off = iova_offset(>iovad, phys);
> - size = iova_align(>iovad, size + iova_off);
> - }
> + size = iova_align(iovad, size + iova_off);
>
> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> if (!iova)
> @@ -1147,16 +1145,21 @@ 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)
> + iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> + if (!iova)
> goto out_free_page;
>
> + if (iommu_map(domain, iova, msi_addr, size, prot))
> + goto out_free_iova;
> +
> INIT_LIST_HEAD(_page->list);
> msi_page->phys = msi_addr;
> msi_page->iova = iova;
> list_add(_page->list, >msi_page_list);
> return msi_page;
>
> +out_free_iova:
> + iommu_dma_free_iova(cookie, iova, size);
> out_free_page:
> kfree(msi_page);
> return NULL;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] iommu/dma: Handle MSI mappings separately
> -Original Message-
> From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org]
> On Behalf Of Robin Murphy
> Sent: 29 July 2019 16:33
> To: j...@8bytes.org
> Cc: m...@kernel.org; iommu@lists.linux-foundation.org; Shameerali Kolothum
> Thodi ;
> linux-arm-ker...@lists.infradead.org; Andre Przywara
>
> Subject: [PATCH] iommu/dma: Handle MSI mappings separately
>
> MSI pages must always be mapped into a device's *current* domain, which
> *might* be the default DMA domain, but might instead be a VFIO domain
> with its own MSI cookie. This subtlety got accidentally lost in the
> streamlining of __iommu_dma_map(), but rather than reintroduce more
> complexity and/or special-casing, it turns out neater to just split this
> path out entirely.
>
> Since iommu_dma_get_msi_page() already duplicates much of what
> __iommu_dma_map() does, it can easily just make the allocation and
> mapping calls directly as well. That way we can further streamline the
> helper back to exclusively operating on DMA domains.
>
> Fixes: b61d271e59d7 ("iommu/dma: Move domain lookup into
> __iommu_dma_{map,unmap}")
> Reported-by: Shameer Kolothum
> Reported-by: Andre Przywara
> Signed-off-by: Robin Murphy
Thanks. This fixes the vf assignment issue on D06 as well. FWIW,
Tested-by: Shameer Kolothum
Shameer
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/dma: Handle MSI mappings separately
MSI pages must always be mapped into a device's *current* domain, which
*might* be the default DMA domain, but might instead be a VFIO domain
with its own MSI cookie. This subtlety got accidentally lost in the
streamlining of __iommu_dma_map(), but rather than reintroduce more
complexity and/or special-casing, it turns out neater to just split this
path out entirely.
Since iommu_dma_get_msi_page() already duplicates much of what
__iommu_dma_map() does, it can easily just make the allocation and
mapping calls directly as well. That way we can further streamline the
helper back to exclusively operating on DMA domains.
Fixes: b61d271e59d7 ("iommu/dma: Move domain lookup into
__iommu_dma_{map,unmap}")
Reported-by: Shameer Kolothum
Reported-by: Andre Przywara
Signed-off-by: Robin Murphy
---
drivers/iommu/dma-iommu.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index a7f9c3edbcb2..6441197a75ea 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -459,13 +459,11 @@ 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;
+ struct iova_domain *iovad = >iovad;
+ size_t iova_off = iova_offset(iovad, phys);
dma_addr_t iova;
- if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
- iova_off = iova_offset(>iovad, phys);
- size = iova_align(>iovad, size + iova_off);
- }
+ size = iova_align(iovad, size + iova_off);
iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
if (!iova)
@@ -1147,16 +1145,21 @@ 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)
+ iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
+ if (!iova)
goto out_free_page;
+ if (iommu_map(domain, iova, msi_addr, size, prot))
+ goto out_free_iova;
+
INIT_LIST_HEAD(_page->list);
msi_page->phys = msi_addr;
msi_page->iova = iova;
list_add(_page->list, >msi_page_list);
return msi_page;
+out_free_iova:
+ iommu_dma_free_iova(cookie, iova, size);
out_free_page:
kfree(msi_page);
return NULL;
--
2.21.0.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
