[PATCH v2] iommu/dma: Respect IOMMU aperture when allocating
Where a device driver has set a 64-bit DMA mask to indicate the absence
of addressing limitations, we still need to ensure that we don't
allocate IOVAs beyond the actual input size of the IOMMU. The reported
aperture is the most reliable way we have of inferring that input
address size, so use that to enforce a hard upper limit where available.
Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
Signed-off-by: Robin Murphy
---
Now with bonus "not being completely broken" as well as the min()
change - seems the necessary prototype changes somehow wound up rebased
into a different patch :/
drivers/iommu/dma-iommu.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7d991c81c4fa..00c8a08d56e7 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -152,12 +152,15 @@ int dma_direction_to_prot(enum dma_data_direction dir,
bool coherent)
}
}
-static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
+static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
dma_addr_t dma_limit)
{
+ struct iova_domain *iovad = domain->iova_cookie;
unsigned long shift = iova_shift(iovad);
unsigned long length = iova_align(iovad, size) >> shift;
+ if (domain->geometry.force_aperture)
+ dma_limit = min(dma_limit, domain->geometry.aperture_end);
/*
* Enforce size-alignment to be safe - there could perhaps be an
* attribute to control this per-device, or at least per-domain...
@@ -315,7 +318,7 @@ struct page **iommu_dma_alloc(struct device *dev, size_t
size, gfp_t gfp,
if (!pages)
return NULL;
- iova = __alloc_iova(iovad, size, dev->coherent_dma_mask);
+ iova = __alloc_iova(domain, size, dev->coherent_dma_mask);
if (!iova)
goto out_free_pages;
@@ -387,7 +390,7 @@ dma_addr_t iommu_dma_map_page(struct device *dev, struct
page *page,
phys_addr_t phys = page_to_phys(page) + offset;
size_t iova_off = iova_offset(iovad, phys);
size_t len = iova_align(iovad, size + iova_off);
- struct iova *iova = __alloc_iova(iovad, len, dma_get_mask(dev));
+ struct iova *iova = __alloc_iova(domain, len, dma_get_mask(dev));
if (!iova)
return DMA_ERROR_CODE;
@@ -539,7 +542,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist
*sg,
prev = s;
}
- iova = __alloc_iova(iovad, iova_len, dma_get_mask(dev));
+ iova = __alloc_iova(domain, iova_len, dma_get_mask(dev));
if (!iova)
goto out_restore_sg;
--
2.8.1.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu/dma: Respect IOMMU aperture when allocating
On 09/08/16 16:36, Joerg Roedel wrote: > On Tue, Aug 09, 2016 at 04:23:18PM +0100, Robin Murphy wrote: >> Where a device driver has set a 64-bit DMA mask to indicate the absence >> of addressing limitations, we still need to ensure that we don't >> allocate IOVAs beyond the actual input size of the IOMMU. The reported >> aperture is the most reliable way we have of inferring that input >> address size, so use that to enforce a hard upper limit. >> >> Signed-off-by: Robin Murphy > > Also missing 'Fixes:' line. > >> --- >> >> This is the only other thing I currently have which could perhaps be >> considered a fix; otherwise I'll pull it into the PCI/SMMU series. >> >> drivers/iommu/dma-iommu.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >> index 7d991c81c4fa..092d781f5f35 100644 >> --- a/drivers/iommu/dma-iommu.c >> +++ b/drivers/iommu/dma-iommu.c >> @@ -158,6 +158,8 @@ static struct iova *__alloc_iova(struct iova_domain >> *iovad, size_t size, >> unsigned long shift = iova_shift(iovad); >> unsigned long length = iova_align(iovad, size) >> shift; >> >> +if (domain->geometry.force_aperture) >> +dma_limit &= domain->geometry.aperture_end; > > This might work, but is misleading as limits and aperture_end are > neither a mask nor a bitfield. It is more descriptive to use min() > here: > > dma_limit = min(dma_limit, omain->geometry.aperture_end); > Well, dma_limit is always a DMA mask, but I suppose you're right that an IOMMU driver could in theory set a wacky non-power-of-two aperture if it really wanted to. I'll respin... Thanks, Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu/dma: Respect IOMMU aperture when allocating
On Tue, Aug 09, 2016 at 04:23:18PM +0100, Robin Murphy wrote: > Where a device driver has set a 64-bit DMA mask to indicate the absence > of addressing limitations, we still need to ensure that we don't > allocate IOVAs beyond the actual input size of the IOMMU. The reported > aperture is the most reliable way we have of inferring that input > address size, so use that to enforce a hard upper limit. > > Signed-off-by: Robin Murphy Also missing 'Fixes:' line. > --- > > This is the only other thing I currently have which could perhaps be > considered a fix; otherwise I'll pull it into the PCI/SMMU series. > > drivers/iommu/dma-iommu.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 7d991c81c4fa..092d781f5f35 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -158,6 +158,8 @@ static struct iova *__alloc_iova(struct iova_domain > *iovad, size_t size, > unsigned long shift = iova_shift(iovad); > unsigned long length = iova_align(iovad, size) >> shift; > > + if (domain->geometry.force_aperture) > + dma_limit &= domain->geometry.aperture_end; This might work, but is misleading as limits and aperture_end are neither a mask nor a bitfield. It is more descriptive to use min() here: dma_limit = min(dma_limit, omain->geometry.aperture_end); ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/dma: Don't put uninitialised IOVA domains
On Tue, Aug 09, 2016 at 04:23:17PM +0100, Robin Murphy wrote:
> Due to the limitations of having to wait until we see a device's DMA
> restrictions before we know how we want an IOVA domain initialised,
> there is a window for error if a DMA ops domain is allocated but later
> freed without ever being used. In that case, init_iova_domain() was
> never called, so calling put_iova_domain() from iommu_put_dma_cookie()
> ends up trying to take an uninitialised lock and crashing.
>
> Make things robust by skipping the call unless the IOVA domain actually
> has been initialised, as we probably should have done from the start.
>
> Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
> Cc: sta...@vger.kernel.org
> Reported-by: Nate Watterson
> Reviewed-by: Nate Watterson
> Tested-by: Nate Watterson
> Reviewed-by: Eric Auger
> Tested-by: Eric Auger
> Signed-off-by: Robin Murphy
> ---
> drivers/iommu/dma-iommu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Wow, that was quick :)
Applied to iommu/fixes, thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/exynos: Modify error handling
On Mon, Aug 01, 2016 at 11:48:38AM +0530, Amitoj Kaur Chawla wrote:
> of_platform_device_create returns NULL on error so an IS_ERR test is
> incorrect here and a NULL check is required.
>
> The Coccinelle semantic patch used to make this change is as follows:
> @@
> expression e;
> @@
>
> e = of_platform_device_create(...);
> if(
> -IS_ERR(e)
> +!e
> )
> {
> <+...
> return
> - PTR_ERR(e)
> + -ENODEV
> ;
> ...+>
> }
>
> Signed-off-by: Amitoj Kaur Chawla
> ---
> drivers/iommu/exynos-iommu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Patch didn't apply cleanly against v4.8-rc1, but I fixed it up and it is
now in my tree, thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/2] iommu/dma: Respect IOMMU aperture when allocating
Where a device driver has set a 64-bit DMA mask to indicate the absence of addressing limitations, we still need to ensure that we don't allocate IOVAs beyond the actual input size of the IOMMU. The reported aperture is the most reliable way we have of inferring that input address size, so use that to enforce a hard upper limit. Signed-off-by: Robin Murphy --- This is the only other thing I currently have which could perhaps be considered a fix; otherwise I'll pull it into the PCI/SMMU series. drivers/iommu/dma-iommu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 7d991c81c4fa..092d781f5f35 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -158,6 +158,8 @@ static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size, unsigned long shift = iova_shift(iovad); unsigned long length = iova_align(iovad, size) >> shift; + if (domain->geometry.force_aperture) + dma_limit &= domain->geometry.aperture_end; /* * Enforce size-alignment to be safe - there could perhaps be an * attribute to control this per-device, or at least per-domain... -- 2.8.1.dirty ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/2] iommu/dma: Don't put uninitialised IOVA domains
Due to the limitations of having to wait until we see a device's DMA
restrictions before we know how we want an IOVA domain initialised,
there is a window for error if a DMA ops domain is allocated but later
freed without ever being used. In that case, init_iova_domain() was
never called, so calling put_iova_domain() from iommu_put_dma_cookie()
ends up trying to take an uninitialised lock and crashing.
Make things robust by skipping the call unless the IOVA domain actually
has been initialised, as we probably should have done from the start.
Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
Cc: sta...@vger.kernel.org
Reported-by: Nate Watterson
Reviewed-by: Nate Watterson
Tested-by: Nate Watterson
Reviewed-by: Eric Auger
Tested-by: Eric Auger
Signed-off-by: Robin Murphy
---
drivers/iommu/dma-iommu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 08a1e2f3690f..7d991c81c4fa 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -68,7 +68,8 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
if (!iovad)
return;
- put_iova_domain(iovad);
+ if (iovad->granule)
+ put_iova_domain(iovad);
kfree(iovad);
domain->iova_cookie = NULL;
}
--
2.8.1.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Don't put uninitialised IOVA domains
On 09/08/16 16:01, Joerg Roedel wrote: > On Wed, Jul 27, 2016 at 04:46:06PM +0100, Robin Murphy wrote: >> Due to the limitations of having to wait until we see a device's DMA >> restrictions before we know how we want an IOVA domain initialised, >> there is a window for error if a DMA ops domain is allocated but later >> freed without ever being used. In that case, init_iova_domain() was >> never called, so calling put_iova_domain() from iommu_put_dma_cookie() >> ends up trying to take an uninitialised lock and crashing. >> >> Make things robust by skipping the call unless the IOVA domain actually >> has been initialised, as we probably should have done from the start. >> > > Missing 'Fixes:' and probably 'Cc: stable' lines? > >> Reported-by: Nate Watterson >> Signed-off-by: Robin Murphy >> --- >> >> I'm not sure this warrants a cc stable, as with the code currently in >> mainline it's only at all likely if other things have already failed >> elsewhere in a manner they should not be expected to. > > Yes, I think this qualifies for stable. Please re-send with the Acks and > Reviewed-by lines too. I'll queue this in my fixes branch and send it > upstream asap. Will do, thanks! Robin. > > > Joerg > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH -next] iommu/amd: Fix error return code in irq_remapping_alloc()
On Thu, Jul 28, 2016 at 02:10:26AM +, Wei Yongjun wrote: > Fix to return a negative error code from the alloc_irq_index() error > handling case instead of 0, as done elsewhere in this function. > > Signed-off-by: Wei Yongjun Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH -next] iommu/amd: Fix non static symbol warning
On Thu, Jul 28, 2016 at 02:09:53AM +, Wei Yongjun wrote: > Fixes the following sparse warning: > > drivers/iommu/amd_iommu.c:106:1: warning: > symbol '__pcpu_scope_flush_queue' was not declared. Should it be static? > > Signed-off-by: Wei Yongjun Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Don't put uninitialised IOVA domains
On Wed, Jul 27, 2016 at 04:46:06PM +0100, Robin Murphy wrote: > Due to the limitations of having to wait until we see a device's DMA > restrictions before we know how we want an IOVA domain initialised, > there is a window for error if a DMA ops domain is allocated but later > freed without ever being used. In that case, init_iova_domain() was > never called, so calling put_iova_domain() from iommu_put_dma_cookie() > ends up trying to take an uninitialised lock and crashing. > > Make things robust by skipping the call unless the IOVA domain actually > has been initialised, as we probably should have done from the start. > Missing 'Fixes:' and probably 'Cc: stable' lines? > Reported-by: Nate Watterson > Signed-off-by: Robin Murphy > --- > > I'm not sure this warrants a cc stable, as with the code currently in > mainline it's only at all likely if other things have already failed > elsewhere in a manner they should not be expected to. Yes, I think this qualifies for stable. Please re-send with the Acks and Reviewed-by lines too. I'll queue this in my fixes branch and send it upstream asap. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/mediatek: Mark static functions in headers inline
From: Joerg Roedel
This was an oversight while merging these functions. Fix it.
Cc: Honghui Zhang
Fixes: 9ca340c98c0d ('iommu/mediatek: move the common struct into header file')
Signed-off-by: Joerg Roedel
---
drivers/iommu/mtk_iommu.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 9ed0a84..3dab13b 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -55,19 +55,19 @@ struct mtk_iommu_data {
boolenable_4GB;
};
-static int compare_of(struct device *dev, void *data)
+static inline int compare_of(struct device *dev, void *data)
{
return dev->of_node == data;
}
-static int mtk_iommu_bind(struct device *dev)
+static inline int mtk_iommu_bind(struct device *dev)
{
struct mtk_iommu_data *data = dev_get_drvdata(dev);
return component_bind_all(dev, &data->smi_imu);
}
-static void mtk_iommu_unbind(struct device *dev)
+static inline void mtk_iommu_unbind(struct device *dev)
{
struct mtk_iommu_data *data = dev_get_drvdata(dev);
--
2.6.6
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] iommu/ipmmu-vmsa: Hook up r8a7796 DT matching code
On Tuesday 09 Aug 2016 16:17:57 Laurent Pinchart wrote: > On Wednesday 08 Jun 2016 18:12:31 Magnus Damm wrote: > > On Wed, Jun 8, 2016 at 5:48 PM, Laurent Pinchart wrote: > >> On Wednesday 08 Jun 2016 09:04:17 Geert Uytterhoeven wrote: > >>> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart wrote: > > --- 0031/drivers/iommu/ipmmu-vmsa.c > > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-06-06 11:19:40.210607110 > > [snip] > > > @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r > >ipmmu_vmsa_iommu_of_setup); > > IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795", > >ipmmu_vmsa_iommu_of_setup); > > +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796", > > + ipmmu_vmsa_iommu_of_setup); > > How about a Gen3 generic compatible string in addition to the > SoC-specific ones ? > >>> > >>> Do we want to specify the number of utlbs here? > >>> Does it differ between r8a7795, r8a7796, and future members? > >> > >> It differs between IPMMU instances on a given SoC, so if we want to > >> specify it it should be a DT property. > > > > Can you please point out which documentation that says it varies with > > IPMMU instance? > > > > Based on IMUCTRn register description "H3-ES1" has 0-31 range while > > "Others" have 0-47. > > The maximum number of uTLBs is indeed the same according to that part of the > documentation, but not all uTLBs are available in all IPMMU instances. We > even have holes in the uTLB ranges, maybe a mask would be more appropriate. And the other option is of course to ignore that and accept any uTLB number, in which case we will rely on the IOMMU bus master node providing correct information. -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] iommu/ipmmu-vmsa: Hook up r8a7796 DT matching code
Hi Magnus, On Wednesday 08 Jun 2016 18:12:31 Magnus Damm wrote: > On Wed, Jun 8, 2016 at 5:48 PM, Laurent Pinchart wrote: > > On Wednesday 08 Jun 2016 09:04:17 Geert Uytterhoeven wrote: > >> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart wrote: > --- 0031/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-06-06 11:19:40.210607110 [snip] > @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r > ipmmu_vmsa_iommu_of_setup); > IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795", > ipmmu_vmsa_iommu_of_setup); > +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796", > + ipmmu_vmsa_iommu_of_setup); > >>> > >>> How about a Gen3 generic compatible string in addition to the > >>> SoC-specific ones ? > >> > >> Do we want to specify the number of utlbs here? > >> Does it differ between r8a7795, r8a7796, and future members? > > > > It differs between IPMMU instances on a given SoC, so if we want to > > specify it it should be a DT property. > > Can you please point out which documentation that says it varies with > IPMMU instance? > > Based on IMUCTRn register description "H3-ES1" has 0-31 range while > "Others" have 0-47. The maximum number of uTLBs is indeed the same according to that part of the documentation, but not all uTLBs are available in all IPMMU instances. We even have holes in the uTLB ranges, maybe a mask would be more appropriate. -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 02/11] genirq/msi: msi_compose wrapper
On Tue, 2 Aug 2016, Eric Auger wrote: > Currently the MSI message is composed by directly calling > irq_chip_compose_msi_msg and erased by setting the memory to zero. > > On some platforms, we will need to complexify this composition to > properly handle MSI emission through IOMMU. Also we will need to track > when the MSI message is erased. I just can't find how you do that. After applying the series the > + if (erase) > + memset(msg, 0, sizeof(*msg)); branch is still just a memset(). The wrapper is fine for the compose side, but having the extra argument just to wrap the memset() for no gain is silly. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
