* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group @ 2013-07-23 10:01 Antonios Motakis 2013-07-23 10:31 ` Inki Dae 2013-07-23 12:46 ` Sethi Varun-B16395 0 siblings, 2 replies; 10+ messages in thread From: Antonios Motakis @ 2013-07-23 10:01 UTC (permalink / raw) To: linux-arm-kernel 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@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(); + 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 10:01 [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group Antonios Motakis @ 2013-07-23 10:31 ` Inki Dae 2013-07-23 11:00 ` Antonios Motakis 2013-07-23 12:46 ` Sethi Varun-B16395 1 sibling, 1 reply; 10+ messages in thread From: Inki Dae @ 2013-07-23 10:31 UTC (permalink / raw) To: linux-arm-kernel > -----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@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. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 10:31 ` Inki Dae @ 2013-07-23 11:00 ` Antonios Motakis 2013-07-23 11:21 ` Inki Dae 0 siblings, 1 reply; 10+ messages in thread From: Antonios Motakis @ 2013-07-23 11:00 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 23, 2013 at 12:31 PM, Inki Dae <inki.dae@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@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. (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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 11:00 ` Antonios Motakis @ 2013-07-23 11:21 ` Inki Dae 2013-07-23 12:04 ` Antonios Motakis 2013-07-23 12:13 ` Cho KyongHo 0 siblings, 2 replies; 10+ messages in thread From: Inki Dae @ 2013-07-23 11:21 UTC (permalink / raw) To: linux-arm-kernel > -----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@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@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 > > (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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 11:21 ` Inki Dae @ 2013-07-23 12:04 ` Antonios Motakis 2013-07-23 12:59 ` Inki Dae 2013-07-23 12:13 ` Cho KyongHo 1 sibling, 1 reply; 10+ messages in thread From: Antonios Motakis @ 2013-07-23 12:04 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 23, 2013 at 1:21 PM, Inki Dae <inki.dae@samsung.com> wrote: > > >> -----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@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@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. But is this grouping imposed by the hardware in some way, or is it just the way you handle them in software? In the latter case, then what you are looking for are IOMMU domains (which can contain multiple groups). IOMMU groups describe something that is imposed by the hardware. > > 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? Generic IOMMU code (such as VFIO) will check for IOMMU groups to determine how the devices behind an IOMMU relate to each other. Returning a unique IOMMU group object per device provides this information - that each device can also be programmed separately. VFIO relies on this behavior - if it doesn't find any IOMMU group objects it will refuse to use the device. Additionally, this information can be seen by userspace. Being able to programmatically determine that there is a one to one relationship is certainly useful. I don't think the lack of IOMMU groups implies no grouping - I think it implies unknown grouping or an incomplete driver, so in my opinion VFIO is correct in refusing to work without IOMMU groups. So any future ARM support for VFIO, will also rely on IOMMU groups. > > Thanks, > Inki Dae > >> >> (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 >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 12:04 ` Antonios Motakis @ 2013-07-23 12:59 ` Inki Dae 0 siblings, 0 replies; 10+ messages in thread From: Inki Dae @ 2013-07-23 12:59 UTC (permalink / raw) To: linux-arm-kernel > -----Original Message----- > From: Antonios Motakis [mailto:a.motakis at virtualopensystems.com] > Sent: Tuesday, July 23, 2013 9:05 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; Alex > Williamson > Subject: Re: [PATCH] iommu/exynos: add devices attached to the System MMU > to an IOMMU group > > On Tue, Jul 23, 2013 at 1:21 PM, Inki Dae <inki.dae@samsung.com> wrote: > > > > > >> -----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@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@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. > > But is this grouping imposed by the hardware in some way, or is it > just the way you handle them in software? In the latter case, then > what you are looking for are IOMMU domains (which can contain multiple > groups). IOMMU groups describe something that is imposed by the > hardware. > That's true. That seems like out of my intention that we try to use the iommu group feature. > > > > 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? > > Generic IOMMU code (such as VFIO) will check for IOMMU groups to > determine how the devices behind an IOMMU relate to each other. > Returning a unique IOMMU group object per device provides this > information - that each device can also be programmed separately. VFIO > relies on this behavior - if it doesn't find any IOMMU group objects > it will refuse to use the device. > > Additionally, this information can be seen by userspace. Being able to > programmatically determine that there is a one to one relationship is > certainly useful. > I have read VFIO document file just now. The VFIO framework is similar to UIO that is used to write user space device driver. But the VFIO is more secure, and provides more featureful userspace driver environment than UIO. And the VFIO requires iommu group object. I didn't check this. Sorry about that. Thanks, Inki Dae > I don't think the lack of IOMMU groups implies no grouping - I think > it implies unknown grouping or an incomplete driver, so in my opinion > VFIO is correct in refusing to work without IOMMU groups. So any > future ARM support for VFIO, will also rely on IOMMU groups. > > > > > Thanks, > > Inki Dae > > > >> > >> (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 > >> > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 11:21 ` Inki Dae 2013-07-23 12:04 ` Antonios Motakis @ 2013-07-23 12:13 ` Cho KyongHo 2013-07-23 12:23 ` Antonios Motakis 1 sibling, 1 reply; 10+ messages in thread From: Cho KyongHo @ 2013-07-23 12:13 UTC (permalink / raw) To: linux-arm-kernel > -----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@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@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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 12:13 ` Cho KyongHo @ 2013-07-23 12:23 ` Antonios Motakis 2013-07-23 12:36 ` Cho KyongHo 0 siblings, 1 reply; 10+ messages in thread From: Antonios Motakis @ 2013-07-23 12:23 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 23, 2013 at 2:13 PM, Cho KyongHo <pullip.cho@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@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@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 >> > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 12:23 ` Antonios Motakis @ 2013-07-23 12:36 ` Cho KyongHo 0 siblings, 0 replies; 10+ messages in thread From: Cho KyongHo @ 2013-07-23 12:36 UTC (permalink / raw) To: linux-arm-kernel > -----Original Message----- > From: Antonios Motakis [mailto:a.motakis at virtualopensystems.com] > Sent: Tuesday, July 23, 2013 9:23 PM > > On Tue, Jul 23, 2013 at 2:13 PM, Cho KyongHo <pullip.cho@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@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@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. > 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 at vger.kernel.org > >> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group 2013-07-23 10:01 [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group Antonios Motakis 2013-07-23 10:31 ` Inki Dae @ 2013-07-23 12:46 ` Sethi Varun-B16395 1 sibling, 0 replies; 10+ messages in thread From: Sethi Varun-B16395 @ 2013-07-23 12:46 UTC (permalink / raw) To: linux-arm-kernel > -----Original Message----- > From: iommu-bounces at lists.linux-foundation.org [mailto:iommu- > bounces at lists.linux-foundation.org] On Behalf Of Antonios Motakis > Sent: Tuesday, July 23, 2013 3:32 PM > To: linux-arm-kernel at lists.infradead.org; iommu at lists.linux- > foundation.org; linux-samsung-soc at vger.kernel.org > Cc: Sachin Kamat; Jiri Kosina; open list; Wei Yongjun; Antonios Motakis; > Cho KyongHo; kvmarm at lists.cs.columbia.edu > 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@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(); > + 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, }; > [Sethi Varun-B16395] Do you support PCI devices? This assumption of one iommu group per pci device may not hold true in case of PCI devices. -Varun ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-07-23 12:59 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-23 10:01 [PATCH] iommu/exynos: add devices attached to the System MMU to an IOMMU group Antonios Motakis 2013-07-23 10:31 ` Inki Dae 2013-07-23 11:00 ` Antonios Motakis 2013-07-23 11:21 ` Inki Dae 2013-07-23 12:04 ` Antonios Motakis 2013-07-23 12:59 ` Inki Dae 2013-07-23 12:13 ` Cho KyongHo 2013-07-23 12:23 ` Antonios Motakis 2013-07-23 12:36 ` Cho KyongHo 2013-07-23 12:46 ` Sethi Varun-B16395
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).