[PATCH 1/2] ARM: SMMU: add devices attached to the SMMU to an IOMMU group

[PATCH 1/2] ARM: SMMU: add devices attached to the SMMU to an IOMMU group

[Antonios Motakis]

IOMMU groups are expected by certain users of the IOMMU API,
e.g. VFIO. Add new devices found by the SMMU driver to an IOMMU
group to satisfy those users.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0f45a48..8b71332 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1502,6 +1502,8 @@ static int arm_smmu_add_device(struct device *dev)
 {
 	struct arm_smmu_device *child, *parent, *smmu;
 	struct arm_smmu_master *master = NULL;
+	struct iommu_group *group;
+	int ret;
 
 	spin_lock(&arm_smmu_devices_lock);
 	list_for_each_entry(parent, &arm_smmu_devices, list) {
@@ -1534,13 +1536,27 @@ static int arm_smmu_add_device(struct device *dev)
 	if (!master)
 		return -ENODEV;
 
+	group = iommu_group_get(dev);
+
+	if (!group) {
+		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);
 	dev->archdata.iommu = smmu;
-	return 0;
+
+	return ret;
 }
 
 static void arm_smmu_remove_device(struct device *dev)
 {
 	dev->archdata.iommu = NULL;
+	iommu_group_remove_device(dev);
 }
 
 static struct iommu_ops arm_smmu_ops = {
-- 
1.8.1.2

[PATCH 2/2] ARM: SMMU: return NULL on error in arm_smmu_iova_to_phys

[Antonios Motakis]

The return value of arm_smmu_iova_to_phys is directly passed to the
user of the IOMMU API via iommu_iova_to_phys; however the ARM SMMU
driver returns -EINVAL on error, which is not consistent with the
rest of the drivers implementing the IOMMU API. VFIO also relies on
the call returning NULL when a page has not been mapped already.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 8b71332..fe81b20 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1480,10 +1480,7 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
 
 err_unlock:
 	spin_unlock(&smmu_domain->lock);
-	dev_warn(smmu->dev,
-		 "invalid (corrupt?) page tables detected for iova 0x%llx\n",
-		 (unsigned long long)iova);
-	return -EINVAL;
+	return NULL;
 }
 
 static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
-- 
1.8.1.2

Re: [PATCH 2/2] ARM: SMMU: return NULL on error in arm_smmu_iova_to_phys

[Will Deacon]

On Fri, Oct 11, 2013 at 02:24:47PM +0100, Antonios Motakis wrote:
> The return value of arm_smmu_iova_to_phys is directly passed to the
> user of the IOMMU API via iommu_iova_to_phys; however the ARM SMMU
> driver returns -EINVAL on error, which is not consistent with the
> rest of the drivers implementing the IOMMU API. VFIO also relies on
> the call returning NULL when a page has not been mapped already.
> 
> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 8b71332..fe81b20 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1480,10 +1480,7 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
>  
>  err_unlock:
>  	spin_unlock(&smmu_domain->lock);
> -	dev_warn(smmu->dev,
> -		 "invalid (corrupt?) page tables detected for iova 0x%llx\n",
> -		 (unsigned long long)iova);
> -	return -EINVAL;
> +	return NULL;

Why are you removing the warning message?

Will

Re: [PATCH 2/2] ARM: SMMU: return NULL on error in arm_smmu_iova_to_phys

[Antonios Motakis]

On Mon, Oct 14, 2013 at 2:48 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Oct 11, 2013 at 02:24:47PM +0100, Antonios Motakis wrote:
>> The return value of arm_smmu_iova_to_phys is directly passed to the
>> user of the IOMMU API via iommu_iova_to_phys; however the ARM SMMU
>> driver returns -EINVAL on error, which is not consistent with the
>> rest of the drivers implementing the IOMMU API. VFIO also relies on
>> the call returning NULL when a page has not been mapped already.
>>
>> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
>> ---
>>  drivers/iommu/arm-smmu.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 8b71332..fe81b20 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1480,10 +1480,7 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
>>
>>  err_unlock:
>>       spin_unlock(&smmu_domain->lock);
>> -     dev_warn(smmu->dev,
>> -              "invalid (corrupt?) page tables detected for iova 0x%llx\n",
>> -              (unsigned long long)iova);
>> -     return -EINVAL;
>> +     return NULL;
>
> Why are you removing the warning message?

VFIO will exercise this code path every time when mapping DMA memory.
This is normal and VFIO *expects* the function to fail - it is only if
the function succeeds that VFIO needs to back down from the DMA
mapping and fail.

This means that there would be a warning every time a VFIO user maps
some memory for DMA use, even though nothing went wrong.

>
> Will

Re: [PATCH 2/2] ARM: SMMU: return NULL on error in arm_smmu_iova_to_phys

[Will Deacon]

On Mon, Oct 14, 2013 at 04:17:51PM +0100, Antonios Motakis wrote:
> On Mon, Oct 14, 2013 at 2:48 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Fri, Oct 11, 2013 at 02:24:47PM +0100, Antonios Motakis wrote:
> >> The return value of arm_smmu_iova_to_phys is directly passed to the
> >> user of the IOMMU API via iommu_iova_to_phys; however the ARM SMMU
> >> driver returns -EINVAL on error, which is not consistent with the
> >> rest of the drivers implementing the IOMMU API. VFIO also relies on
> >> the call returning NULL when a page has not been mapped already.
> >>
> >> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> >> ---
> >>  drivers/iommu/arm-smmu.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 8b71332..fe81b20 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -1480,10 +1480,7 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
> >>
> >>  err_unlock:
> >>       spin_unlock(&smmu_domain->lock);
> >> -     dev_warn(smmu->dev,
> >> -              "invalid (corrupt?) page tables detected for iova 0x%llx\n",
> >> -              (unsigned long long)iova);
> >> -     return -EINVAL;
> >> +     return NULL;
> >
> > Why are you removing the warning message?
> 
> VFIO will exercise this code path every time when mapping DMA memory.
> This is normal and VFIO *expects* the function to fail - it is only if
> the function succeeds that VFIO needs to back down from the DMA
> mapping and fail.
> 
> This means that there would be a warning every time a VFIO user maps
> some memory for DMA use, even though nothing went wrong.

Ok, in which case it might be worth reworking arm_smmu_iova_to_phys to treat
{pgd,pud,pmd,pte}_none different from {pgd,pud,pmd,pte}_bad.

Will

Re: [PATCH 2/2] ARM: SMMU: return NULL on error in arm_smmu_iova_to_phys

[Will Deacon]

On Mon, Oct 14, 2013 at 06:09:48PM +0100, Will Deacon wrote:
> On Mon, Oct 14, 2013 at 04:17:51PM +0100, Antonios Motakis wrote:
> > On Mon, Oct 14, 2013 at 2:48 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > > On Fri, Oct 11, 2013 at 02:24:47PM +0100, Antonios Motakis wrote:
> > VFIO will exercise this code path every time when mapping DMA memory.
> > This is normal and VFIO *expects* the function to fail - it is only if
> > the function succeeds that VFIO needs to back down from the DMA
> > mapping and fail.
> > 
> > This means that there would be a warning every time a VFIO user maps
> > some memory for DMA use, even though nothing went wrong.
> 
> Ok, in which case it might be worth reworking arm_smmu_iova_to_phys to treat
> {pgd,pud,pmd,pte}_none different from {pgd,pud,pmd,pte}_bad.

Just an FYI, but I realised the page table locking is broken in my driver
(hold a spinlock over a non-atomic allocation; looks like the exynos guys do
this too), so I've just reworked a bunch of iova_to_phys and incorporated this
change as part of that.

Cheers,

Will

Re: [PATCH 1/2] ARM: SMMU: add devices attached to the SMMU to an IOMMU group

[Will Deacon]

Hi Antonios,

On Fri, Oct 11, 2013 at 02:24:46PM +0100, Antonios Motakis wrote:
> IOMMU groups are expected by certain users of the IOMMU API,
> e.g. VFIO. Add new devices found by the SMMU driver to an IOMMU
> group to satisfy those users.

Cheers for looking at this: VFIO has been on my TODO list for some time.

> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 0f45a48..8b71332 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1502,6 +1502,8 @@ static int arm_smmu_add_device(struct device *dev)
>  {
>  	struct arm_smmu_device *child, *parent, *smmu;
>  	struct arm_smmu_master *master = NULL;
> +	struct iommu_group *group;
> +	int ret;
>  
>  	spin_lock(&arm_smmu_devices_lock);
>  	list_for_each_entry(parent, &arm_smmu_devices, list) {
> @@ -1534,13 +1536,27 @@ static int arm_smmu_add_device(struct device *dev)
>  	if (!master)
>  		return -ENODEV;
>  
> +	group = iommu_group_get(dev);

I'm not especially familiar with IOMMU groups (I understand them as the
minimum translation granularity, which would mean single StreamID for the
ARM SMMU), but under what circumstances would you expect to receive a
non-NULL group here? I can't see any other code adding devices to groups
(outside of other drivers)...

Will

Re: [PATCH 1/2] ARM: SMMU: add devices attached to the SMMU to an IOMMU group

[Antonios Motakis]

On Mon, Oct 14, 2013 at 2:48 PM, Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Antonios,
>
> On Fri, Oct 11, 2013 at 02:24:46PM +0100, Antonios Motakis wrote:
> > IOMMU groups are expected by certain users of the IOMMU API,
> > e.g. VFIO. Add new devices found by the SMMU driver to an IOMMU
> > group to satisfy those users.
>
> Cheers for looking at this: VFIO has been on my TODO list for some time.
>
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 0f45a48..8b71332 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1502,6 +1502,8 @@ static int arm_smmu_add_device(struct device *dev)
> >  {
> >       struct arm_smmu_device *child, *parent, *smmu;
> >       struct arm_smmu_master *master = NULL;
> > +     struct iommu_group *group;
> > +     int ret;
> >
> >       spin_lock(&arm_smmu_devices_lock);
> >       list_for_each_entry(parent, &arm_smmu_devices, list) {
> > @@ -1534,13 +1536,27 @@ static int arm_smmu_add_device(struct device *dev)
> >       if (!master)
> >               return -ENODEV;
> >
> > +     group = iommu_group_get(dev);
>
> I'm not especially familiar with IOMMU groups (I understand them as the
> minimum translation granularity, which would mean single StreamID for the
> ARM SMMU), but under what circumstances would you expect to receive a
> non-NULL group here? I can't see any other code adding devices to groups
> (outside of other drivers)...
>
Hi Will,

You are right, only other IOMMU drivers will add a device to a group.
There was a discussion about this when I posted a similar patch for
the Exynos System MMU driver, see
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185675.html

The idea is to check in the case of add_device() being called multiple
times, which is not the case most of the time, but still a sane
safeguard.

>
> Will

Re: [PATCH 1/2] ARM: SMMU: add devices attached to the SMMU to an IOMMU group

[Will Deacon]

On Mon, Oct 14, 2013 at 04:13:15PM +0100, Antonios Motakis wrote:
> On Mon, Oct 14, 2013 at 2:48 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index 0f45a48..8b71332 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -1502,6 +1502,8 @@ static int arm_smmu_add_device(struct device *dev)
> > >  {
> > >       struct arm_smmu_device *child, *parent, *smmu;
> > >       struct arm_smmu_master *master = NULL;
> > > +     struct iommu_group *group;
> > > +     int ret;
> > >
> > >       spin_lock(&arm_smmu_devices_lock);
> > >       list_for_each_entry(parent, &arm_smmu_devices, list) {
> > > @@ -1534,13 +1536,27 @@ static int arm_smmu_add_device(struct device *dev)
> > >       if (!master)
> > >               return -ENODEV;
> > >
> > > +     group = iommu_group_get(dev);
> >
> > I'm not especially familiar with IOMMU groups (I understand them as the
> > minimum translation granularity, which would mean single StreamID for the
> > ARM SMMU), but under what circumstances would you expect to receive a
> > non-NULL group here? I can't see any other code adding devices to groups
> > (outside of other drivers)...
> >
> 
> You are right, only other IOMMU drivers will add a device to a group.
> There was a discussion about this when I posted a similar patch for
> the Exynos System MMU driver, see
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185675.html
> 
> The idea is to check in the case of add_device() being called multiple
> times, which is not the case most of the time, but still a sane
> safeguard.

Ok, but it feels a bit weird. The current code (arm_smmu_add_device)
basically does a bunch of sanity checking against the DT data in order to
find where the master sits in the device topology. Then it updates
dev->archdata.iommu to point at the relevant SMMU instance.

So, the interesting case is where the device was previously associated with
a *different* IOMMU. In that case, the current code clobbers the iommu field
with the new smmu, whereas the new code could end up getting very confused
with respect to IOMMU groups.

A better way is probably to check that dev->archdata.iommu is NULL before we
assign to it. If not, then spit out a warning and return an error. That
would also mean you could get rid of the group get/put calls.

Will

Re: [PATCH 1/2] ARM: SMMU: add devices attached to the SMMU to an IOMMU group

[Antonios Motakis]

On Mon, Oct 14, 2013 at 7:07 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Mon, Oct 14, 2013 at 04:13:15PM +0100, Antonios Motakis wrote:
>> On Mon, Oct 14, 2013 at 2:48 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> > > index 0f45a48..8b71332 100644
>> > > --- a/drivers/iommu/arm-smmu.c
>> > > +++ b/drivers/iommu/arm-smmu.c
>> > > @@ -1502,6 +1502,8 @@ static int arm_smmu_add_device(struct device *dev)
>> > >  {
>> > >       struct arm_smmu_device *child, *parent, *smmu;
>> > >       struct arm_smmu_master *master = NULL;
>> > > +     struct iommu_group *group;
>> > > +     int ret;
>> > >
>> > >       spin_lock(&arm_smmu_devices_lock);
>> > >       list_for_each_entry(parent, &arm_smmu_devices, list) {
>> > > @@ -1534,13 +1536,27 @@ static int arm_smmu_add_device(struct device *dev)
>> > >       if (!master)
>> > >               return -ENODEV;
>> > >
>> > > +     group = iommu_group_get(dev);
>> >
>> > I'm not especially familiar with IOMMU groups (I understand them as the
>> > minimum translation granularity, which would mean single StreamID for the
>> > ARM SMMU), but under what circumstances would you expect to receive a
>> > non-NULL group here? I can't see any other code adding devices to groups
>> > (outside of other drivers)...
>> >
>>
>> You are right, only other IOMMU drivers will add a device to a group.
>> There was a discussion about this when I posted a similar patch for
>> the Exynos System MMU driver, see
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185675.html
>>
>> The idea is to check in the case of add_device() being called multiple
>> times, which is not the case most of the time, but still a sane
>> safeguard.
>
> Ok, but it feels a bit weird. The current code (arm_smmu_add_device)
> basically does a bunch of sanity checking against the DT data in order to
> find where the master sits in the device topology. Then it updates
> dev->archdata.iommu to point at the relevant SMMU instance.
>
> So, the interesting case is where the device was previously associated with
> a *different* IOMMU. In that case, the current code clobbers the iommu field
> with the new smmu, whereas the new code could end up getting very confused
> with respect to IOMMU groups.
>
> A better way is probably to check that dev->archdata.iommu is NULL before we
> assign to it. If not, then spit out a warning and return an error. That
> would also mean you could get rid of the group get/put calls.
>

Good point, this is a better way to handle this. I'll respin based on this.

> Will