RE: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu

[Laurentiu Tudor]

Hi Robin,

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: Friday, March 29, 2019 4:51 PM
> 
> On 29/03/2019 14:00, laurentiu.tudor@xxxxxxx wrote:
> > From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> >   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);
> >