| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 23 Jul 2013 21:36:43 +0900
From: Cho KyongHo <pullip.cho@...sung.com>
To: 'Antonios Motakis' <a.motakis@...tualopensystems.com>
Cc: 'Inki Dae' <inki.dae@...sung.com>,
'Linux ARM Kernel' <linux-arm-kernel@...ts.infradead.org>,
'Linux IOMMU' <iommu@...ts.linux-foundation.org>,
'Linux Samsung SOC' <linux-samsung-soc@...r.kernel.org>,
'kvm-arm' <kvmarm@...ts.cs.columbia.edu>,
'Joerg Roedel' <joro@...tes.org>,
'Sachin Kamat' <sachin.kamat@...aro.org>,
'Jiri Kosina' <jkosina@...e.cz>,
'Wei Yongjun' <yongjun_wei@...ndmicro.com.cn>,
'open list' <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] iommu/exynos: add devices attached to the System MMU to an
IOMMU group
> -----Original Message-----
> From: Antonios Motakis [mailto:a.motakis@...tualopensystems.com]
> Sent: Tuesday, July 23, 2013 9:23 PM
>
> On Tue, Jul 23, 2013 at 2:13 PM, Cho KyongHo <pullip.cho@...sung.com> wrote:
> >> -----Original Message-----
> >> From: Inki Dae [mailto:inki.dae@...sung.com]
> >> Sent: Tuesday, July 23, 2013 8:21 PM
> >>
> >> > -----Original Message-----
> >> > From: Antonios Motakis [mailto:a.motakis@...tualopensystems.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@...sung.com> wrote:
> >> > >
> >> > >
> >> > >
> >> > > > -----Original Message-----
> >> > > > From: linux-samsung-soc-owner@...r.kernel.org [mailto:linux-samsung-
> >> > soc-
> >> > > > owner@...r.kernel.org] On Behalf Of Antonios Motakis
> >> > > > Sent: Tuesday, July 23, 2013 7:02 PM
> >> > > > To: linux-arm-kernel@...ts.infradead.org;
> >> > > iommu@...ts.linux-foundation.org;
> >> > > > linux-samsung-soc@...r.kernel.org
> >> > > > Cc: kvmarm@...ts.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@...tualopensystems.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.
>
Yes, the case you have mentioned.
Even though it must not happen with perfect device drivers,
IOMMU driver needs to care about it IMHO.
> 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@...r.kernel.org
> >> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> > >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists