| Message ID | 20190329140014.8126-3-laurentiu.tudor@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisites for NXP LS104xA SMMU enablement | expand |
On 29/03/2019 14:00, laurentiu.tudor@nxp.com wrote: > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > 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 <laurentiu.tudor@nxp.com> > --- > 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 <linux/iommu.h> > > 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 = &pdev->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); >
On Fri, Mar 29, 2019 at 9:03 AM <laurentiu.tudor@nxp.com> wrote: > > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > 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 <laurentiu.tudor@nxp.com> > --- > 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 <linux/iommu.h> > > 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 = &pdev->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 >
Hi Robin, > -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: Friday, March 29, 2019 4:51 PM > > On 29/03/2019 14:00, laurentiu.tudor@nxp.com wrote: > > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > > > 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 <laurentiu.tudor@nxp.com> > > --- > > 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 <linux/iommu.h> > > > > 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 = &pdev->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); > >
Hi Leo, > -----Original Message----- > From: Li Yang [mailto:leoyang.li@nxp.com] > Sent: Friday, March 29, 2019 11:16 PM > > On Fri, Mar 29, 2019 at 9:03 AM <laurentiu.tudor@nxp.com> wrote: > > > > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > > > 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 <laurentiu.tudor@nxp.com> > > --- > > 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 <linux/iommu.h> > > > > 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 = &pdev->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 > >
Hi Robin, > -----Original Message----- > From: Robin Murphy <robin.murphy@arm.com> > Sent: Friday, March 29, 2019 4:51 PM > > On 29/03/2019 14:00, laurentiu.tudor@nxp.com wrote: > > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > > > 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 <laurentiu.tudor@nxp.com> > > --- > > 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 <linux/iommu.h> > > > > 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 = &pdev->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
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 <linux/iommu.h> 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 = &pdev->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); + } + bm_set_memory(fbpr_a, fbpr_sz); err_irq = platform_get_irq(pdev, 0);