diff mbox series

[v4,10/19] iommu: Add set_platform_dma_ops iommu ops

Message ID 20230104125725.271850-11-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Retire detach_dev callback |

Commit Message

Baolu Lu Jan. 4, 2023, 12:57 p.m. UTC 
  When VFIO finishes assigning a device to user space and calls
iommu_group_release_dma_owner() to return the device to kernel, the IOMMU
core will attach the default domain to the device. Unfortunately, some
IOMMU drivers don't support default domain, hence in the end, the core
calls .detach_dev instead.

This adds set_platform_dma_ops iommu ops to make it clear that what it
does is returning control back to the platform DMA ops.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h |  4 ++++
 drivers/iommu/iommu.c | 23 +++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

Comments

Jason Gunthorpe Jan. 4, 2023, 1:17 p.m. UTC | #1 
On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index de91dd88705b..4e35a9f94873 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
> +{
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +
> +	if (!ops->set_platform_dma_ops)
> +		return -EINVAL;
> +
> +	ops->set_platform_dma_ops(dev);
> +	return 0;
> +}
> +
>  static int __iommu_group_set_domain(struct iommu_group *group,
>  				    struct iommu_domain *new_domain)
>  {
> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>  	 * platform specific behavior.
>  	 */
>  	if (!new_domain) {
> -		if (WARN_ON(!group->domain->ops->detach_dev))
> -			return -EINVAL;

This should still have the WARN_ON..

if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)

Jason
Baolu Lu Jan. 5, 2023, 5:58 a.m. UTC | #2 
Hi Jason,

On 2023/1/4 21:17, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:
> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index de91dd88705b..4e35a9f94873 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>>   	return 0;
>>   }
>>   
>> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
>> +{
>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>> +
>> +	if (!ops->set_platform_dma_ops)
>> +		return -EINVAL;
>> +
>> +	ops->set_platform_dma_ops(dev);
>> +	return 0;
>> +}
>> +
>>   static int __iommu_group_set_domain(struct iommu_group *group,
>>   				    struct iommu_domain *new_domain)
>>   {
>> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>>   	 * platform specific behavior.
>>   	 */
>>   	if (!new_domain) {
>> -		if (WARN_ON(!group->domain->ops->detach_dev))
>> -			return -EINVAL;
> This should still have the WARN_ON..
> 
> if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)

This has been implicitly included in the code.

iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver
doesn't support set_platform_dma_ops (otherwise always return success).
Then, the domain->ops->detach_dev is required and a WARN_ON was there.

         if (!new_domain) {
                 ret = __iommu_group_for_each_dev(group, NULL,
                                 iommu_group_do_set_platform_dma);
                 if (ret) {
                         if (WARN_ON(!group->domain->ops->detach_dev))
                                 return -EINVAL;
                         __iommu_group_for_each_dev(group, group->domain,
                                 iommu_group_do_detach_device);
                 }
                 group->domain = NULL;
                 return 0;
         }

Perhaps I should add a comment to explain this?

--
Best regards,
baolu
Jason Gunthorpe Jan. 5, 2023, 1:15 p.m. UTC | #3 
On Thu, Jan 05, 2023 at 01:58:42PM +0800, Baolu Lu wrote:
> Hi Jason,
> 
> On 2023/1/4 21:17, Jason Gunthorpe wrote:
> > On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:
> > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index de91dd88705b..4e35a9f94873 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
> > >   	return 0;
> > >   }
> > > +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
> > > +{
> > > +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> > > +
> > > +	if (!ops->set_platform_dma_ops)
> > > +		return -EINVAL;
> > > +
> > > +	ops->set_platform_dma_ops(dev);
> > > +	return 0;
> > > +}
> > > +
> > >   static int __iommu_group_set_domain(struct iommu_group *group,
> > >   				    struct iommu_domain *new_domain)
> > >   {
> > > @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
> > >   	 * platform specific behavior.
> > >   	 */
> > >   	if (!new_domain) {
> > > -		if (WARN_ON(!group->domain->ops->detach_dev))
> > > -			return -EINVAL;
> > This should still have the WARN_ON..
> > 
> > if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)
> 
> This has been implicitly included in the code.
> 
> iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver
> doesn't support set_platform_dma_ops (otherwise always return success).
> Then, the domain->ops->detach_dev is required and a WARN_ON was there.
> 
>         if (!new_domain) {
>                 ret = __iommu_group_for_each_dev(group, NULL,
>                                 iommu_group_do_set_platform_dma);
>                 if (ret) {
>                         if (WARN_ON(!group->domain->ops->detach_dev))
>                                 return -EINVAL;
>                         __iommu_group_for_each_dev(group, group->domain,
>                                 iommu_group_do_detach_device);
>                 }
>                 group->domain = NULL;
>                 return 0;
>         }
> 
> Perhaps I should add a comment to explain this?

But you delete this later when you remove this.

I think testing the op directly is much clearer, get rid of the whole
ret and EINVAL thinig:

if (dev_iommu_ops(dev)->set_platform_dma_ops)
   __iommu_group_for_each_dev(group, NULL,
                                 iommu_group_do_set_platform_dma); //	 Can't fail!
else if (group->domain->ops->detach_dev)
     __iommu_group_for_each_dev(group, group->domain,
                                 iommu_group_do_detach_device);
else
   WARN(true)

Jason
Baolu Lu Jan. 6, 2023, 6:07 a.m. UTC | #4 
On 1/5/23 9:15 PM, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2023 at 01:58:42PM +0800, Baolu Lu wrote:
>> Hi Jason,
>>
>> On 2023/1/4 21:17, Jason Gunthorpe wrote:
>>> On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:
>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index de91dd88705b..4e35a9f94873 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>>>>    	return 0;
>>>>    }
>>>> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
>>>> +{
>>>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>>>> +
>>>> +	if (!ops->set_platform_dma_ops)
>>>> +		return -EINVAL;
>>>> +
>>>> +	ops->set_platform_dma_ops(dev);
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int __iommu_group_set_domain(struct iommu_group *group,
>>>>    				    struct iommu_domain *new_domain)
>>>>    {
>>>> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>>>>    	 * platform specific behavior.
>>>>    	 */
>>>>    	if (!new_domain) {
>>>> -		if (WARN_ON(!group->domain->ops->detach_dev))
>>>> -			return -EINVAL;
>>> This should still have the WARN_ON..
>>>
>>> if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)
>> This has been implicitly included in the code.
>>
>> iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver
>> doesn't support set_platform_dma_ops (otherwise always return success).
>> Then, the domain->ops->detach_dev is required and a WARN_ON was there.
>>
>>          if (!new_domain) {
>>                  ret = __iommu_group_for_each_dev(group, NULL,
>>                                  iommu_group_do_set_platform_dma);
>>                  if (ret) {
>>                          if (WARN_ON(!group->domain->ops->detach_dev))
>>                                  return -EINVAL;
>>                          __iommu_group_for_each_dev(group, group->domain,
>>                                  iommu_group_do_detach_device);
>>                  }
>>                  group->domain = NULL;
>>                  return 0;
>>          }
>>
>> Perhaps I should add a comment to explain this?
> But you delete this later when you remove this.
> 
> I think testing the op directly is much clearer, get rid of the whole
> ret and EINVAL thinig:
> 
> if (dev_iommu_ops(dev)->set_platform_dma_ops)
>     __iommu_group_for_each_dev(group, NULL,
>                                   iommu_group_do_set_platform_dma); //	 Can't fail!
> else if (group->domain->ops->detach_dev)
>       __iommu_group_for_each_dev(group, group->domain,
>                                   iommu_group_do_detach_device);
> else
>     WARN(true)

Above looks good to me. Thanks! I have updated this part of code like
below:

@@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct 
iommu_group *group,
          * platform specific behavior.
          */
         if (!new_domain) {
-               if (WARN_ON(!group->domain->ops->detach_dev))
-                       return -EINVAL;
-               __iommu_group_for_each_dev(group, group->domain,
-                                          iommu_group_do_detach_device);
+               struct group_device *grp_dev;
+
+               grp_dev = list_first_entry(&group->devices,
+                                          struct group_device, list);
+
+               if (dev_iommu_ops(grp_dev->dev)->set_platform_dma_ops)
+                       __iommu_group_for_each_dev(group, NULL,
+                                       iommu_group_do_set_platform_dma);
+               else if (group->domain->ops->detach_dev)
+                       __iommu_group_for_each_dev(group, group->domain,
+                                       iommu_group_do_detach_device);
+               else
+                       WARN_ON_ONCE(1);
+
                 group->domain = NULL;
                 return 0;
         }

--
Best regards,
baolu
Jason Gunthorpe Jan. 6, 2023, 2:26 p.m. UTC | #5 
On Fri, Jan 06, 2023 at 02:07:32PM +0800, Baolu Lu wrote:

> Above looks good to me. Thanks! I have updated this part of code like
> below:
> 
> @@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct
> iommu_group *group,
>          * platform specific behavior.
>          */
>         if (!new_domain) {
> -               if (WARN_ON(!group->domain->ops->detach_dev))
> -                       return -EINVAL;
> -               __iommu_group_for_each_dev(group, group->domain,
> -                                          iommu_group_do_detach_device);
> +               struct group_device *grp_dev;
> +
> +               grp_dev = list_first_entry(&group->devices,
> +                                          struct group_device, list);

It seems OK - I hope we naturally can't ever get in a situation where
a group has disjoint iommu drivers.

Jason
Baolu Lu Jan. 7, 2023, 2:48 a.m. UTC | #6 
On 1/6/2023 10:26 PM, Jason Gunthorpe wrote:
> On Fri, Jan 06, 2023 at 02:07:32PM +0800, Baolu Lu wrote:
> 
>> Above looks good to me. Thanks! I have updated this part of code like
>> below:
>>
>> @@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct
>> iommu_group *group,
>>           * platform specific behavior.
>>           */
>>          if (!new_domain) {
>> -               if (WARN_ON(!group->domain->ops->detach_dev))
>> -                       return -EINVAL;
>> -               __iommu_group_for_each_dev(group, group->domain,
>> -                                          iommu_group_do_detach_device);
>> +               struct group_device *grp_dev;
>> +
>> +               grp_dev = list_first_entry(&group->devices,
>> +                                          struct group_device, list);
> It seems OK - I hope we naturally can't ever get in a situation where
> a group has disjoint iommu drivers.

The final code after cleanup looks like below. We will WARN_ON the lack
of callback in the iommu_group_do_set_platform_dma() helper.

2152 static int iommu_group_do_set_platform_dma(struct device *dev, void 
*data)
2153 {
2154         const struct iommu_ops *ops = dev_iommu_ops(dev);
2155
2156         if (!WARN_ON(!ops->set_platform_dma_ops))
2157                 ops->set_platform_dma_ops(dev);
2158
2159         return 0;
2160 }
2161
2162 static int __iommu_group_set_domain(struct iommu_group *group,
2163                                     struct iommu_domain *new_domain)
2164 {
2165         int ret;
2166
2167         if (group->domain == new_domain)
2168                 return 0;
2169
2170         /*
2171          * New drivers should support default domains, so 
set_platform_dma()
2172          * op will never be called. Otherwise the NULL domain 
represents some
2173          * platform specific behavior.
2174          */
2175         if (!new_domain) {
2176                 __iommu_group_for_each_dev(group, NULL,
2177 
iommu_group_do_set_platform_dma);
2178                 group->domain = NULL;
2179                 return 0;
2180         }

How do you like this?

--
Best regards,
baolu
Jason Gunthorpe Jan. 9, 2023, 12:17 p.m. UTC | #7 
On Sat, Jan 07, 2023 at 10:48:31AM +0800, Baolu Lu wrote:
> On 1/6/2023 10:26 PM, Jason Gunthorpe wrote:
> > On Fri, Jan 06, 2023 at 02:07:32PM +0800, Baolu Lu wrote:
> > 
> > > Above looks good to me. Thanks! I have updated this part of code like
> > > below:
> > > 
> > > @@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct
> > > iommu_group *group,
> > >           * platform specific behavior.
> > >           */
> > >          if (!new_domain) {
> > > -               if (WARN_ON(!group->domain->ops->detach_dev))
> > > -                       return -EINVAL;
> > > -               __iommu_group_for_each_dev(group, group->domain,
> > > -                                          iommu_group_do_detach_device);
> > > +               struct group_device *grp_dev;
> > > +
> > > +               grp_dev = list_first_entry(&group->devices,
> > > +                                          struct group_device, list);
> > It seems OK - I hope we naturally can't ever get in a situation where
> > a group has disjoint iommu drivers.
> 
> The final code after cleanup looks like below. We will WARN_ON the lack
> of callback in the iommu_group_do_set_platform_dma() helper.

Yeah OK

Jason
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa22..7b3e3775b069 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -228,6 +228,9 @@  struct iommu_iotlb_gather {
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
  *                  group and attached to the groups domain
+ * @set_platform_dma_ops: Returning control back to the platform DMA ops. This op
+ *                        is to support old IOMMU drivers, new drivers should use
+ *                        default domains, and the common IOMMU DMA ops.
  * @device_group: find iommu group for a particular device
  * @get_resv_regions: Request list of reserved regions for a device
  * @of_xlate: add OF master IDs to iommu grouping
@@ -256,6 +259,7 @@  struct iommu_ops {
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
 	void (*probe_finalize)(struct device *dev);
+	void (*set_platform_dma_ops)(struct device *dev);
 	struct iommu_group *(*device_group)(struct device *dev);
 
 	/* Request/Free a list of reserved regions for a device */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705b..4e35a9f94873 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2163,6 +2163,17 @@  static int iommu_group_do_detach_device(struct device *dev, void *data)
 	return 0;
 }
 
+static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+	if (!ops->set_platform_dma_ops)
+		return -EINVAL;
+
+	ops->set_platform_dma_ops(dev);
+	return 0;
+}
+
 static int __iommu_group_set_domain(struct iommu_group *group,
 				    struct iommu_domain *new_domain)
 {
@@ -2177,10 +2188,14 @@  static int __iommu_group_set_domain(struct iommu_group *group,
 	 * platform specific behavior.
 	 */
 	if (!new_domain) {
-		if (WARN_ON(!group->domain->ops->detach_dev))
-			return -EINVAL;
-		__iommu_group_for_each_dev(group, group->domain,
-					   iommu_group_do_detach_device);
+		ret = __iommu_group_for_each_dev(group, NULL,
+				iommu_group_do_set_platform_dma);
+		if (ret) {
+			if (WARN_ON(!group->domain->ops->detach_dev))
+				return -EINVAL;
+			__iommu_group_for_each_dev(group, group->domain,
+				iommu_group_do_detach_device);
+		}
 		group->domain = NULL;
 		return 0;
 	}