| Message ID | 148885666392.28553.6653211200015255398.sendpatchset@little-apple (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Magnus, On Tue, Mar 7, 2017 at 4:17 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > From: Magnus Damm <damm+renesas@opensource.se> > > Not all architectures have an iommu member in their archdata, so > use #ifdefs support build with COMPILE_TEST on any architecture. to support > Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > Reviewed-by: Joerg Roedel <jroedel@suse.de> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- 0010/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2017-03-06 19:26:26.070607110 +0900 > @@ -72,6 +72,25 @@ static struct ipmmu_vmsa_domain *to_vmsa > return container_of(dom, struct ipmmu_vmsa_domain, io_domain); > } > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev) > +{ > + return dev->archdata.iommu; > +} > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p) > +{ > + dev->archdata.iommu = p; > +} > +#else #else /* compile testing */ > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev) > +{ > + return NULL; > +} > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p) > +{ > +} > +#endif Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 07/03/17 03:17, Magnus Damm wrote: > From: Magnus Damm <damm+renesas@opensource.se> > > Not all architectures have an iommu member in their archdata, so > use #ifdefs support build with COMPILE_TEST on any architecture. I have a feeling I might be repeating myself, but ipmmu_vmsa_archdata looks to be trivially convertible to iommu_fwspec, which I strongly encourage, not least because it would obviate bodges like this. Robin. > Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > Reviewed-by: Joerg Roedel <jroedel@suse.de> > --- > > Changes since V6: > - Updated patch to handle newly introduced functions in: > [PATCH v7 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops > > drivers/iommu/ipmmu-vmsa.c | 43 ++++++++++++++++++++++++++++++------------- > 1 file changed, 30 insertions(+), 13 deletions(-) > > --- 0010/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2017-03-06 19:26:26.070607110 +0900 > @@ -72,6 +72,25 @@ static struct ipmmu_vmsa_domain *to_vmsa > return container_of(dom, struct ipmmu_vmsa_domain, io_domain); > } > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev) > +{ > + return dev->archdata.iommu; > +} > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p) > +{ > + dev->archdata.iommu = p; > +} > +#else > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev) > +{ > + return NULL; > +} > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p) > +{ > +} > +#endif > + > #define TLB_LOOP_TIMEOUT 100 /* 100us */ > > /* ----------------------------------------------------------------------------- > @@ -543,7 +562,7 @@ static void ipmmu_domain_free(struct iom > static int ipmmu_attach_device(struct iommu_domain *io_domain, > struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); > struct ipmmu_vmsa_device *mmu = archdata->mmu; > struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain); > unsigned long flags; > @@ -588,7 +607,7 @@ static int ipmmu_attach_device(struct io > static void ipmmu_detach_device(struct iommu_domain *io_domain, > struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); > struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain); > unsigned int i; > > @@ -709,7 +728,7 @@ static int ipmmu_init_platform_device(st > archdata->utlbs = utlbs; > archdata->num_utlbs = num_utlbs; > archdata->dev = dev; > - dev->archdata.iommu = archdata; > + set_archdata(dev, archdata); > return 0; > > error: > @@ -729,12 +748,11 @@ static struct iommu_domain *ipmmu_domain > > static int ipmmu_add_device(struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata; > struct ipmmu_vmsa_device *mmu = NULL; > struct iommu_group *group; > int ret; > > - if (dev->archdata.iommu) { > + if (to_archdata(dev)) { > dev_warn(dev, "IOMMU driver already assigned to device %s\n", > dev_name(dev)); > return -EINVAL; > @@ -770,8 +788,7 @@ static int ipmmu_add_device(struct devic > * - Make the mapping size configurable ? We currently use a 2GB mapping > * at a 1GB offset to ensure that NULL VAs will fault. > */ > - archdata = dev->archdata.iommu; > - mmu = archdata->mmu; > + mmu = to_archdata(dev)->mmu; > if (!mmu->mapping) { > struct dma_iommu_mapping *mapping; > > @@ -799,7 +816,7 @@ error: > if (mmu) > arm_iommu_release_mapping(mmu->mapping); > > - dev->archdata.iommu = NULL; > + set_archdata(dev, NULL); > > if (!IS_ERR_OR_NULL(group)) > iommu_group_remove_device(dev); > @@ -809,7 +826,7 @@ error: > > static void ipmmu_remove_device(struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); > > arm_iommu_detach_device(dev); > iommu_group_remove_device(dev); > @@ -817,7 +834,7 @@ static void ipmmu_remove_device(struct d > kfree(archdata->utlbs); > kfree(archdata); > > - dev->archdata.iommu = NULL; > + set_archdata(dev, NULL); > } > > static const struct iommu_ops ipmmu_ops = { > @@ -874,7 +891,7 @@ static void ipmmu_domain_free_dma(struct > > static int ipmmu_add_device_dma(struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); > struct iommu_group *group; > > /* The device has been verified in xlate() */ > @@ -893,7 +910,7 @@ static int ipmmu_add_device_dma(struct d > > static void ipmmu_remove_device_dma(struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); > > spin_lock(&ipmmu_slave_devices_lock); > list_del(&archdata->list); > @@ -904,7 +921,7 @@ static void ipmmu_remove_device_dma(stru > > static struct device *ipmmu_find_sibling_device(struct device *dev) > { > - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); > struct ipmmu_vmsa_archdata *sibling_archdata = NULL; > bool found = false; > >
Hi Robin, On Wed, Mar 8, 2017 at 9:48 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 07/03/17 03:17, Magnus Damm wrote: >> From: Magnus Damm <damm+renesas@opensource.se> >> >> Not all architectures have an iommu member in their archdata, so >> use #ifdefs support build with COMPILE_TEST on any architecture. > > I have a feeling I might be repeating myself, but ipmmu_vmsa_archdata > looks to be trivially convertible to iommu_fwspec, which I strongly > encourage, not least because it would obviate bodges like this. Yeah, I think it should be possible to use iommu_fwspec for this purpose. The question is when to do it. =) I actually looked into it recently, but then realised that for this to work then due to code sharing I need to make use of iommu_fwspec on both 32-bit and 64-bit ARM. So it requires rework of the existing IPMMU for 32-bit ARM (including hairy legacy CONFIG_IOMMU_DMA=n code). I was actually thinking of doing some rework of 32-bit ARM IPMMU code anyway (I suspect iommu_device_* conversion caused breakage) and it probably has to happen on top of current -next. I would also like to start reducing burden of forward porting all these patches, and stirring up the ground does not really help much there... Cheers, / magnus
On 09/03/17 03:44, Magnus Damm wrote: > Hi Robin, > > On Wed, Mar 8, 2017 at 9:48 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> On 07/03/17 03:17, Magnus Damm wrote: >>> From: Magnus Damm <damm+renesas@opensource.se> >>> >>> Not all architectures have an iommu member in their archdata, so >>> use #ifdefs support build with COMPILE_TEST on any architecture. >> >> I have a feeling I might be repeating myself, but ipmmu_vmsa_archdata >> looks to be trivially convertible to iommu_fwspec, which I strongly >> encourage, not least because it would obviate bodges like this. > > Yeah, I think it should be possible to use iommu_fwspec for this > purpose. The question is when to do it. =) I'd actually be inclined to do it *before* any other major changes, as it would be pretty minimal given the current structure of the driver. But then I'm free to wilfully ignore the burden of maintaining patch stacks between both mainline and older trees ;) > I actually looked into it recently, but then realised that for this to > work then due to code sharing I need to make use of iommu_fwspec on > both 32-bit and 64-bit ARM. So it requires rework of the existing > IPMMU for 32-bit ARM (including hairy legacy CONFIG_IOMMU_DMA=n code). > I was actually thinking of doing some rework of 32-bit ARM IPMMU code > anyway (I suspect iommu_device_* conversion caused breakage) and it > probably has to happen on top of current -next. I would also like to > start reducing burden of forward porting all these patches, and > stirring up the ground does not really help much there... Note that iommu_fwspec can be used pretty much orthogonally to any of the core DMA ops support, so it shouldn't be as invasive as you might think. See 84672f192671 ("iommu/mediatek: Convert M4Uv1 to iommu_fwspec") as an example of an archdata-to-fwspec conversion of a driver which only supports 32-bit ARM, and notably borrows its master handling directly from the the IPMMU driver. Robin. > > Cheers, > > / magnus >
--- 0010/drivers/iommu/ipmmu-vmsa.c +++ work/drivers/iommu/ipmmu-vmsa.c 2017-03-06 19:26:26.070607110 +0900 @@ -72,6 +72,25 @@ static struct ipmmu_vmsa_domain *to_vmsa return container_of(dom, struct ipmmu_vmsa_domain, io_domain); } +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev) +{ + return dev->archdata.iommu; +} +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p) +{ + dev->archdata.iommu = p; +} +#else +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev) +{ + return NULL; +} +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p) +{ +} +#endif + #define TLB_LOOP_TIMEOUT 100 /* 100us */ /* ----------------------------------------------------------------------------- @@ -543,7 +562,7 @@ static void ipmmu_domain_free(struct iom static int ipmmu_attach_device(struct iommu_domain *io_domain, struct device *dev) { - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); struct ipmmu_vmsa_device *mmu = archdata->mmu; struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain); unsigned long flags; @@ -588,7 +607,7 @@ static int ipmmu_attach_device(struct io static void ipmmu_detach_device(struct iommu_domain *io_domain, struct device *dev) { - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain); unsigned int i; @@ -709,7 +728,7 @@ static int ipmmu_init_platform_device(st archdata->utlbs = utlbs; archdata->num_utlbs = num_utlbs; archdata->dev = dev; - dev->archdata.iommu = archdata; + set_archdata(dev, archdata); return 0; error: @@ -729,12 +748,11 @@ static struct iommu_domain *ipmmu_domain static int ipmmu_add_device(struct device *dev) { - struct ipmmu_vmsa_archdata *archdata; struct ipmmu_vmsa_device *mmu = NULL; struct iommu_group *group; int ret; - if (dev->archdata.iommu) { + if (to_archdata(dev)) { dev_warn(dev, "IOMMU driver already assigned to device %s\n", dev_name(dev)); return -EINVAL; @@ -770,8 +788,7 @@ static int ipmmu_add_device(struct devic * - Make the mapping size configurable ? We currently use a 2GB mapping * at a 1GB offset to ensure that NULL VAs will fault. */ - archdata = dev->archdata.iommu; - mmu = archdata->mmu; + mmu = to_archdata(dev)->mmu; if (!mmu->mapping) { struct dma_iommu_mapping *mapping; @@ -799,7 +816,7 @@ error: if (mmu) arm_iommu_release_mapping(mmu->mapping); - dev->archdata.iommu = NULL; + set_archdata(dev, NULL); if (!IS_ERR_OR_NULL(group)) iommu_group_remove_device(dev); @@ -809,7 +826,7 @@ error: static void ipmmu_remove_device(struct device *dev) { - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); arm_iommu_detach_device(dev); iommu_group_remove_device(dev); @@ -817,7 +834,7 @@ static void ipmmu_remove_device(struct d kfree(archdata->utlbs); kfree(archdata); - dev->archdata.iommu = NULL; + set_archdata(dev, NULL); } static const struct iommu_ops ipmmu_ops = { @@ -874,7 +891,7 @@ static void ipmmu_domain_free_dma(struct static int ipmmu_add_device_dma(struct device *dev) { - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); struct iommu_group *group; /* The device has been verified in xlate() */ @@ -893,7 +910,7 @@ static int ipmmu_add_device_dma(struct d static void ipmmu_remove_device_dma(struct device *dev) { - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); spin_lock(&ipmmu_slave_devices_lock); list_del(&archdata->list); @@ -904,7 +921,7 @@ static void ipmmu_remove_device_dma(stru static struct device *ipmmu_find_sibling_device(struct device *dev) { - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; + struct ipmmu_vmsa_archdata *archdata = to_archdata(dev); struct ipmmu_vmsa_archdata *sibling_archdata = NULL; bool found = false;