RE: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu
From: Laurentiu Tudor
Date: Fri Apr 19 2019 - 14:16:42 EST
> -----Original Message-----
> From: Robin Murphy <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
> > 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
> > 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
> > 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