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

Tue Jul 23 08:23:23 EDT 2013

On Tue, Jul 23, 2013 at 2:13 PM, Cho KyongHo <pullip.cho at samsung.com> wrote:
>> -----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.

It is not clear to me why this is the case, can add_device be called
multiple times per device? I will take a look into this.

Anyway thanks for taking this into account.

>
> 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
>> > >
>