Messages in this thread | | | From | Laurentiu Tudor <> | Subject | RE: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu | Date | Mon, 1 Apr 2019 11:04:52 +0000 |
| |
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); > >
| |