RE: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu
Hi Robin, > -Original Message- > From: Robin Murphy > Sent: Friday, March 29, 2019 4:51 PM > > On 29/03/2019 14:00, laurentiu.tu...@nxp.com wrote: > > From: Laurentiu Tudor > > > > Add a one-to-one iommu mapping for bman private data memory (FBPR). > > This is required for BMAN to work without faults behind an iommu. > > > > Signed-off-by: Laurentiu Tudor > > --- > > drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c > b/drivers/soc/fsl/qbman/bman_ccsr.c > > index 7c3cc968053c..b209c79511bb 100644 > > --- a/drivers/soc/fsl/qbman/bman_ccsr.c > > +++ b/drivers/soc/fsl/qbman/bman_ccsr.c > > @@ -29,6 +29,7 @@ > >*/ > > > > #include "bman_priv.h" > > +#include > > > > u16 bman_ip_rev; > > EXPORT_SYMBOL(bman_ip_rev); > > @@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device > *pdev) > > int ret, err_irq; > > struct device *dev = >dev; > > struct device_node *node = dev->of_node; > > + struct iommu_domain *domain; > > struct resource *res; > > u16 id, bm_pool_cnt; > > u8 major, minor; > > @@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device > *pdev) > > > > dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz); > > > > + /* Create an 1-to-1 iommu mapping for FBPR area */ > > + domain = iommu_get_domain_for_dev(dev); > > If that's expected to be the default domain that you're grabbing, then > this is *incredibly* fragile. There's nothing to stop the IOVA that you > forcibly map from being automatically allocated later and causing some > other DMA mapping to fail noisily and unexpectedly. Furthermore, have > you tried this with "iommu.passthrough=1"? > > That said, I really don't understand what's going on here anyway :/ > > As far as I can tell from qbman_init_private_mem(), fbpr_a comes from > dma_alloc_coherent() and thus would already be a mapped IOVA - isn't > this the stuff that Roy converted to nicely use shared-dma-pool regions > a while ago? > Finally found some time to look into this, sorry for the delay. It seems that on the code path taken in our case (dma_alloc_coherent() -> dma_alloc_attrs() -> dma_alloc_from_dev_coherent() -> __dma_alloc_from_coherent()) there's no call into the iommu layer, thus no mapping in the smmu. I plan to come up with a RFC patch early next week so we have something concrete to discuss on. --- Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu
Hi Leo,
> -Original Message-
> From: Li Yang [mailto:leoyang...@nxp.com]
> Sent: Friday, March 29, 2019 11:16 PM
>
> On Fri, Mar 29, 2019 at 9:03 AM wrote:
> >
> > From: Laurentiu Tudor
> >
> > Add a one-to-one iommu mapping for bman private data memory (FBPR).
> > This is required for BMAN to work without faults behind an iommu.
> >
> > Signed-off-by: Laurentiu Tudor
> > ---
> > drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c
> b/drivers/soc/fsl/qbman/bman_ccsr.c
> > index 7c3cc968053c..b209c79511bb 100644
> > --- a/drivers/soc/fsl/qbman/bman_ccsr.c
> > +++ b/drivers/soc/fsl/qbman/bman_ccsr.c
> > @@ -29,6 +29,7 @@
> > */
> >
> > #include "bman_priv.h"
> > +#include
> >
> > u16 bman_ip_rev;
> > EXPORT_SYMBOL(bman_ip_rev);
> > @@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device
> *pdev)
> > int ret, err_irq;
> > struct device *dev = >dev;
> > struct device_node *node = dev->of_node;
> > + struct iommu_domain *domain;
> > struct resource *res;
> > u16 id, bm_pool_cnt;
> > u8 major, minor;
> > @@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device
> *pdev)
> >
> > dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz);
> >
> > + /* Create an 1-to-1 iommu mapping for FBPR area */
> > + domain = iommu_get_domain_for_dev(dev);
> > + if (domain) {
> > + ret = iommu_map(domain, fbpr_a, fbpr_a, fbpr_sz,
> > + IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
> > + if (ret)
> > + dev_warn(dev, "failed to iommu_map() %d\n",
> ret);
> > + }
>
> Like Robin has pointed out, could you explain why the mapping in this
> patch and other similar patches cannot be dealt with the dma APIs
> automatically? If the current bqman driver doesn't use the dma APIs
> correctly, we need to fix that instead of doing the mapping
> explicitly.
Please see my reply to Robin.
As a side comment, if we want to convert to dma api it will be interesting to
see how we'll deal with the qman portal devices as they require a smmu mapping
for an area of their register block (where stashing goes). Problem is they
treated as distinct devices, the address where they should dma cannot be
configured independently for each one of them. I think I can came up with a
proposal but probably will not look very pretty.
---
Best Regards, Laurentiu
> > +
> > bm_set_memory(fbpr_a, fbpr_sz);
> >
> > err_irq = platform_get_irq(pdev, 0);
> > --
> > 2.17.1
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu
Hi Robin,
> -Original Message-
> From: Robin Murphy [mailto:robin.mur...@arm.com]
> Sent: Friday, March 29, 2019 4:51 PM
>
> On 29/03/2019 14:00, laurentiu.tu...@nxp.com wrote:
> > From: Laurentiu Tudor
> >
> > Add a one-to-one iommu mapping for bman private data memory (FBPR).
> > This is required for BMAN to work without faults behind an iommu.
> >
> > Signed-off-by: Laurentiu Tudor
> > ---
> > drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c
> b/drivers/soc/fsl/qbman/bman_ccsr.c
> > index 7c3cc968053c..b209c79511bb 100644
> > --- a/drivers/soc/fsl/qbman/bman_ccsr.c
> > +++ b/drivers/soc/fsl/qbman/bman_ccsr.c
> > @@ -29,6 +29,7 @@
> >*/
> >
> > #include "bman_priv.h"
> > +#include
> >
> > u16 bman_ip_rev;
> > EXPORT_SYMBOL(bman_ip_rev);
> > @@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device
> *pdev)
> > int ret, err_irq;
> > struct device *dev = >dev;
> > struct device_node *node = dev->of_node;
> > + struct iommu_domain *domain;
> > struct resource *res;
> > u16 id, bm_pool_cnt;
> > u8 major, minor;
> > @@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device
> *pdev)
> >
> > dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz);
> >
> > + /* Create an 1-to-1 iommu mapping for FBPR area */
> > + domain = iommu_get_domain_for_dev(dev);
>
> If that's expected to be the default domain that you're grabbing, then
> this is *incredibly* fragile. There's nothing to stop the IOVA that you
> forcibly map from being automatically allocated later and causing some
> other DMA mapping to fail noisily and unexpectedly.
Agree here, we pretty much rely on luck with this implementation. As a side
note, I've also experimented using dma_map_resource() instead of directly
calling into iommu api and things worked fine, but see below ...
> Furthermore, have you tried this with "iommu.passthrough=1"?
Yes. The iommu_map() calls fail and the drivers issue warning messages, but
apart from that I don't see any issues.
> That said, I really don't understand what's going on here anyway :/
>
> As far as I can tell from qbman_init_private_mem(), fbpr_a comes from
> dma_alloc_coherent() and thus would already be a mapped IOVA - isn't
> this the stuff that Roy converted to nicely use shared-dma-pool regions
> a while ago?
I must say that I'm also unclear on this. The thing is that I don't get to see
a smmu mapping being created for the reserved memory as result of calling
dma_alloc_coherent(). IIRC, at the time when I looked at this I concluded that
the call to dma_alloc_coherent() simply returns the phys address of the
device's reserved memory without creating a smmu mapping to back it up. Maybe
my understanding was not correct or perhaps there's an issue with this
shared-dma-pool mechanism where instead of creating a mapping in the smmu and
return an IOVA it just returns the physical address of the reserved memory area.
---
Thanks & Best Regards, Laurentiu
>
> > + if (domain) {
> > + ret = iommu_map(domain, fbpr_a, fbpr_a, fbpr_sz,
> > + IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
> > + if (ret)
> > + dev_warn(dev, "failed to iommu_map() %d\n", ret);
> > + }
> > +
> > bm_set_memory(fbpr_a, fbpr_sz);
> >
> > err_irq = platform_get_irq(pdev, 0);
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu
On Fri, Mar 29, 2019 at 9:03 AM wrote:
>
> From: Laurentiu Tudor
>
> Add a one-to-one iommu mapping for bman private data memory (FBPR).
> This is required for BMAN to work without faults behind an iommu.
>
> Signed-off-by: Laurentiu Tudor
> ---
> drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c
> b/drivers/soc/fsl/qbman/bman_ccsr.c
> index 7c3cc968053c..b209c79511bb 100644
> --- a/drivers/soc/fsl/qbman/bman_ccsr.c
> +++ b/drivers/soc/fsl/qbman/bman_ccsr.c
> @@ -29,6 +29,7 @@
> */
>
> #include "bman_priv.h"
> +#include
>
> u16 bman_ip_rev;
> EXPORT_SYMBOL(bman_ip_rev);
> @@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device *pdev)
> int ret, err_irq;
> struct device *dev = >dev;
> struct device_node *node = dev->of_node;
> + struct iommu_domain *domain;
> struct resource *res;
> u16 id, bm_pool_cnt;
> u8 major, minor;
> @@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device *pdev)
>
> dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz);
>
> + /* Create an 1-to-1 iommu mapping for FBPR area */
> + domain = iommu_get_domain_for_dev(dev);
> + if (domain) {
> + ret = iommu_map(domain, fbpr_a, fbpr_a, fbpr_sz,
> + IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
> + if (ret)
> + dev_warn(dev, "failed to iommu_map() %d\n", ret);
> + }
Like Robin has pointed out, could you explain why the mapping in this
patch and other similar patches cannot be dealt with the dma APIs
automatically? If the current bqman driver doesn't use the dma APIs
correctly, we need to fix that instead of doing the mapping
explicitly.
> +
> bm_set_memory(fbpr_a, fbpr_sz);
>
> err_irq = platform_get_irq(pdev, 0);
> --
> 2.17.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu
On 29/03/2019 14:00, laurentiu.tu...@nxp.com wrote:
From: Laurentiu Tudor
Add a one-to-one iommu mapping for bman private data memory (FBPR).
This is required for BMAN to work without faults behind an iommu.
Signed-off-by: Laurentiu Tudor
---
drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c
b/drivers/soc/fsl/qbman/bman_ccsr.c
index 7c3cc968053c..b209c79511bb 100644
--- a/drivers/soc/fsl/qbman/bman_ccsr.c
+++ b/drivers/soc/fsl/qbman/bman_ccsr.c
@@ -29,6 +29,7 @@
*/
#include "bman_priv.h"
+#include
u16 bman_ip_rev;
EXPORT_SYMBOL(bman_ip_rev);
@@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device *pdev)
int ret, err_irq;
struct device *dev = >dev;
struct device_node *node = dev->of_node;
+ struct iommu_domain *domain;
struct resource *res;
u16 id, bm_pool_cnt;
u8 major, minor;
@@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device *pdev)
dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz);
+ /* Create an 1-to-1 iommu mapping for FBPR area */
+ domain = iommu_get_domain_for_dev(dev);
If that's expected to be the default domain that you're grabbing, then
this is *incredibly* fragile. There's nothing to stop the IOVA that you
forcibly map from being automatically allocated later and causing some
other DMA mapping to fail noisily and unexpectedly. Furthermore, have
you tried this with "iommu.passthrough=1"?
That said, I really don't understand what's going on here anyway :/
As far as I can tell from qbman_init_private_mem(), fbpr_a comes from
dma_alloc_coherent() and thus would already be a mapped IOVA - isn't
this the stuff that Roy converted to nicely use shared-dma-pool regions
a while ago?
Robin.
+ if (domain) {
+ ret = iommu_map(domain, fbpr_a, fbpr_a, fbpr_sz,
+ IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
+ if (ret)
+ dev_warn(dev, "failed to iommu_map() %d\n", ret);
+ }
+
bm_set_memory(fbpr_a, fbpr_sz);
err_irq = platform_get_irq(pdev, 0);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
