[PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group

[Cho KyongHo]

> -----Original Message-----
> From: Inki Dae [mailto:inki.dae at samsung.com]
> Sent: Tuesday, July 23, 2013 8:21 PM
> 
> > -----Original Message-----
> > From: Antonios Motakis [mailto:a.motakis at virtualopensystems.com]
> > Sent: Tuesday, July 23, 2013 8:00 PM
> > To: Inki Dae
> > Cc: Linux ARM Kernel; Linux IOMMU; Linux Samsung SOC; kvm-arm; Cho
> KyongHo;
> > Joerg Roedel; Sachin Kamat; Jiri Kosina; Wei Yongjun; open list
> > Subject: Re: [PATCH] iommu/exynos: add devices attached to the System MMU
> > to an IOMMU group
> >
> > On Tue, Jul 23, 2013 at 12:31 PM, Inki Dae <inki.dae at samsung.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: linux-samsung-soc-owner at vger.kernel.org [mailto:linux-samsung-
> > soc-
> > > > owner at vger.kernel.org] On Behalf Of Antonios Motakis
> > > > Sent: Tuesday, July 23, 2013 7:02 PM
> > > > To: linux-arm-kernel at lists.infradead.org;
> > > iommu at lists.linux-foundation.org;
> > > > linux-samsung-soc at vger.kernel.org
> > > > Cc: kvmarm at lists.cs.columbia.edu; Antonios Motakis; Cho KyongHo; Joerg
> > > > Roedel; Sachin Kamat; Jiri Kosina; Wei Yongjun; open list
> > > > Subject: [PATCH] iommu/exynos: add devices attached to the System MMU
> > to
> > > > an IOMMU group
> > > >
> > > > IOMMU groups are expected by certain users of the IOMMU API,
> > > > e.g. VFIO. Since each device is behind its own System MMU, we
> > > > can allocate a new IOMMU group for each device.
> > > >
> > > > This patch depends on Cho KyongHo's patch series titled "[PATCH v7
> > 00/12]
> > > > iommu/exynos: Fixes and Enhancements of System MMU driver with DT",
> > > > applied on a Linux 3.10.1 kernel. It has been tested on the Arndale
> > board.
> > > >
> > > > Signed-off-by: Antonios Motakis <a.motakis at virtualopensystems.com>
> > > > ---
> > > >  drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++++++
> > > >  1 file changed, 24 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-
> > iommu.c
> > > > index 51d43bb..9f39eaa 100644
> > > > --- a/drivers/iommu/exynos-iommu.c
> > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > @@ -1134,6 +1134,28 @@ static phys_addr_t
> > exynos_iommu_iova_to_phys(struct
> > > > iommu_domain *domain,
> > > >       return phys;
> > > >  }
> > > >
> > > > +static int exynos_iommu_add_device(struct device *dev)
> > > > +{
> > > > +     struct iommu_group *group;
> > > > +     int ret;
> > > > +
> > > > +     group = iommu_group_alloc();
> > >
> > > Is that correct? I don't see why you allocate a group object every time
> > > add_device callback is called. That doesn't have any meaning we have to
> > use
> > > iommu group feature. I think the implementation should be one more
> > devices
> > > per a group. So I guess a given device object should be wrapped by
> > higher
> > > device object than the given device object. For a good example, you can
> > > refer to intel-iommu.c file.
> >
> > With an Intel IOMMU it can be the case that 2 devices have to share
> > the same IOMMU mappings (i.e. you can't program them separately). With
> > the Exynos System MMU, there is always one System MMU per device, so
> > there is nothing stopping you from programming any 2 devices' mappings
> > differently. So yes, the right thing to do here is to have a one to
> > one relationship between devices and IOMMU groups.
> 
> In case of Exynos drm driver, a unified iommu mapping table is used for all
> devices (fimd, g2d, hdmi, fimc, gsc, rotator) based on drm so they use the
> same iommu mapping table even though they have each iommu hardware unit. And
> the iommu mapping table is just logical data structure for hardware
> translation process by each DMA. Actually, I am considering using iommu
> group feature for more generic implementation.
> 
> And one question. Why do you allocate a iommu group object if we should have
> one to one relationship between devices and iommu groups? In this case, is
> there any reason you have to use the iommu group object?
> 
> Thanks,
> Inki Dae
> 
Antonios,

Your patch always creates new iommu group whenever .add_device() is called,
which results in memory leak. You need to check if the given device is already
involved in an iommu group.

BTW, I will post new patchset in a few days.
It will not be such different from previous one and your patch
will be rebased on that in a trivial manner.

Regards,
Cho KyongHo

> >
> > (resending because of html mail)
> >
> > Cheers,
> > Antonios
> >
> > >
> > >
> > > Thanks,
> > > Inki Dae
> > >
> > > > +     if (IS_ERR(group)) {
> > > > +             dev_err(dev, "Failed to allocate IOMMU group\n");
> > > > +             return PTR_ERR(group);
> > > > +     }
> > > > +
> > > > +     ret = iommu_group_add_device(group, dev);
> > > > +     iommu_group_put(group);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static void exynos_iommu_remove_device(struct device *dev)
> > > > +{
> > > > +     iommu_group_remove_device(dev);
> > > > +}
> > > > +
> > > >  static struct iommu_ops exynos_iommu_ops = {
> > > >       .domain_init = &exynos_iommu_domain_init,
> > > >       .domain_destroy = &exynos_iommu_domain_destroy,
> > > > @@ -1142,6 +1164,8 @@ static struct iommu_ops exynos_iommu_ops = {
> > > >       .map = &exynos_iommu_map,
> > > >       .unmap = &exynos_iommu_unmap,
> > > >       .iova_to_phys = &exynos_iommu_iova_to_phys,
> > > > +     .add_device     = exynos_iommu_add_device,
> > > > +     .remove_device  = exynos_iommu_remove_device,
> > > >       .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
> > > >  };
> > > >
> > > > --
> > > > 1.8.1.2
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-
> > samsung-
> > > > soc" in
> > > > the body of a message to majordomo at vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >