diff mbox series

[v12,07/17] iommu: Try to allocate blocking domain when probing device

Message ID 20220826121141.50743-8-baolu.lu@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series iommu: SVA and IOPF refactoring | expand

Commit Message

Baolu Lu Aug. 26, 2022, 12:11 p.m. UTC 
Allocate the blocking domain when probing devices if the driver supports
blocking domain allocation. Otherwise, revert to the previous behavior,
that is, use UNMANAGED domain instead when the blocking domain is needed.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Tested-by: Tony Zhu <tony.zhu@intel.com>
---
 drivers/iommu/iommu.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

Jason Gunthorpe Aug. 26, 2022, 2:52 p.m. UTC | #1 
On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote:
> Allocate the blocking domain when probing devices if the driver supports
> blocking domain allocation. Otherwise, revert to the previous behavior,
> that is, use UNMANAGED domain instead when the blocking domain is needed.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Tested-by: Tony Zhu <tony.zhu@intel.com>
> ---
>  drivers/iommu/iommu.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)

This seems like a lot of overhead to allocate these things for every
group?

Why not add a simple refcount on the blocking domain instead and
allocate the domain on the pasid attach like we do for ownership?

Jason
Baolu Lu Aug. 29, 2022, 3:40 a.m. UTC | #2 
On 2022/8/26 22:52, Jason Gunthorpe wrote:
> On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote:
>> Allocate the blocking domain when probing devices if the driver supports
>> blocking domain allocation. Otherwise, revert to the previous behavior,
>> that is, use UNMANAGED domain instead when the blocking domain is needed.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
>> Tested-by: Tony Zhu<tony.zhu@intel.com>
>> ---
>>   drivers/iommu/iommu.c | 29 +++++++++++++++++------------
>>   1 file changed, 17 insertions(+), 12 deletions(-)
> This seems like a lot of overhead to allocate these things for every
> group?
> 
> Why not add a simple refcount on the blocking domain instead and
> allocate the domain on the pasid attach like we do for ownership?

I am working towards implementing static instance of blocking domain for
each IOMMU driver, and then, there's no much overhead to allocate it in
the probing device path.

Best regards,
baolu
Jason Gunthorpe Aug. 29, 2022, 5:27 p.m. UTC | #3 
On Mon, Aug 29, 2022 at 11:40:24AM +0800, Baolu Lu wrote:
> On 2022/8/26 22:52, Jason Gunthorpe wrote:
> > On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote:
> > > Allocate the blocking domain when probing devices if the driver supports
> > > blocking domain allocation. Otherwise, revert to the previous behavior,
> > > that is, use UNMANAGED domain instead when the blocking domain is needed.
> > > 
> > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> > > Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
> > > Tested-by: Tony Zhu<tony.zhu@intel.com>
> > > ---
> > >   drivers/iommu/iommu.c | 29 +++++++++++++++++------------
> > >   1 file changed, 17 insertions(+), 12 deletions(-)
> > This seems like a lot of overhead to allocate these things for every
> > group?
> > 
> > Why not add a simple refcount on the blocking domain instead and
> > allocate the domain on the pasid attach like we do for ownership?
> 
> I am working towards implementing static instance of blocking domain for
> each IOMMU driver, and then, there's no much overhead to allocate it in
> the probing device path.

Well, I thought about that and I don't think we can get
there in a short order. Would rather you progress this series without
getting entangled in such a big adventure

Jason
Baolu Lu Aug. 30, 2022, 1:46 a.m. UTC | #4 
On 2022/8/30 01:27, Jason Gunthorpe wrote:
> On Mon, Aug 29, 2022 at 11:40:24AM +0800, Baolu Lu wrote:
>> On 2022/8/26 22:52, Jason Gunthorpe wrote:
>>> On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote:
>>>> Allocate the blocking domain when probing devices if the driver supports
>>>> blocking domain allocation. Otherwise, revert to the previous behavior,
>>>> that is, use UNMANAGED domain instead when the blocking domain is needed.
>>>>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
>>>> Tested-by: Tony Zhu<tony.zhu@intel.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 29 +++++++++++++++++------------
>>>>    1 file changed, 17 insertions(+), 12 deletions(-)
>>> This seems like a lot of overhead to allocate these things for every
>>> group?
>>>
>>> Why not add a simple refcount on the blocking domain instead and
>>> allocate the domain on the pasid attach like we do for ownership?
>>
>> I am working towards implementing static instance of blocking domain for
>> each IOMMU driver, and then, there's no much overhead to allocate it in
>> the probing device path.
> 
> Well, I thought about that and I don't think we can get
> there in a short order.

Yes. Fair enough.

> Would rather you progress this series without
> getting entangled in such a big adventure

Agreed. I will drop this patch and add below code in the iommu
interface:

--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3219,6 +3219,26 @@ int iommu_attach_device_pasid(struct iommu_domain 
*domain,
                 return -ENODEV;

         mutex_lock(&group->mutex);
+
+       /*
+        * The underlying IOMMU driver needs to support blocking domain
+        * allocation and the callback to block DMA transactions with a
+        * specific PASID.
+        */
+       if (!group->blocking_domain) {
+               group->blocking_domain = __iommu_domain_alloc(dev->bus,
+                               IOMMU_DOMAIN_BLOCKED);
+               if (!group->blocking_domain) {
+                       ret = -ENODEV;
+                       goto out_unlock;
+               }
+       }
+
+       if (!group->blocking_domain->ops->set_dev_pasid) {
+               ret = -EOPNOTSUPP;
+               goto out_unlock;
+       }
+
         curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, 
GFP_KERNEL);
         if (curr) {
                 ret = xa_err(curr) ? : -EBUSY;

Currently both ARM SMMUv3 and VT-d drivers use static blocking domain.
Hence I didn't use a refcount for blocking domain release here.

Best regards,
baolu
Jason Gunthorpe Aug. 30, 2022, 1:29 p.m. UTC | #5 
On Tue, Aug 30, 2022 at 09:46:01AM +0800, Baolu Lu wrote:
> On 2022/8/30 01:27, Jason Gunthorpe wrote:
> > On Mon, Aug 29, 2022 at 11:40:24AM +0800, Baolu Lu wrote:
> > > On 2022/8/26 22:52, Jason Gunthorpe wrote:
> > > > On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote:
> > > > > Allocate the blocking domain when probing devices if the driver supports
> > > > > blocking domain allocation. Otherwise, revert to the previous behavior,
> > > > > that is, use UNMANAGED domain instead when the blocking domain is needed.
> > > > > 
> > > > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> > > > > Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
> > > > > Tested-by: Tony Zhu<tony.zhu@intel.com>
> > > > > ---
> > > > >    drivers/iommu/iommu.c | 29 +++++++++++++++++------------
> > > > >    1 file changed, 17 insertions(+), 12 deletions(-)
> > > > This seems like a lot of overhead to allocate these things for every
> > > > group?
> > > > 
> > > > Why not add a simple refcount on the blocking domain instead and
> > > > allocate the domain on the pasid attach like we do for ownership?
> > > 
> > > I am working towards implementing static instance of blocking domain for
> > > each IOMMU driver, and then, there's no much overhead to allocate it in
> > > the probing device path.
> > 
> > Well, I thought about that and I don't think we can get
> > there in a short order.
> 
> Yes. Fair enough.
> 
> > Would rather you progress this series without
> > getting entangled in such a big adventure
> 
> Agreed. I will drop this patch and add below code in the iommu
> interface:
> 
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3219,6 +3219,26 @@ int iommu_attach_device_pasid(struct iommu_domain
> *domain,
>                 return -ENODEV;
> 
>         mutex_lock(&group->mutex);
> +
> +       /*
> +        * The underlying IOMMU driver needs to support blocking domain
> +        * allocation and the callback to block DMA transactions with a
> +        * specific PASID.
> +        */
> +       if (!group->blocking_domain) {
> +               group->blocking_domain = __iommu_domain_alloc(dev->bus,
> +                               IOMMU_DOMAIN_BLOCKED);
> +               if (!group->blocking_domain) {
> +                       ret = -ENODEV;
> +                       goto out_unlock;
> +               }
> +       }
> +
> +       if (!group->blocking_domain->ops->set_dev_pasid) {
> +               ret = -EOPNOTSUPP;
> +               goto out_unlock;
> +       }
> +
>         curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
> GFP_KERNEL);
>         if (curr) {
>                 ret = xa_err(curr) ? : -EBUSY;
> 
> Currently both ARM SMMUv3 and VT-d drivers use static blocking domain.
> Hence I didn't use a refcount for blocking domain release here.

I don't think that works in the general case, you can't just destroy
what is in group->blocking_domain..

Maybe all of this is just the good reason to go to a simple
device->ops->remove_dev_pasid() callback and forget about blocking
domain here.

Jason
Baolu Lu Aug. 31, 2022, 1:49 a.m. UTC | #6 
On 8/30/22 9:29 PM, Jason Gunthorpe wrote:
> On Tue, Aug 30, 2022 at 09:46:01AM +0800, Baolu Lu wrote:
>> On 2022/8/30 01:27, Jason Gunthorpe wrote:
>>> On Mon, Aug 29, 2022 at 11:40:24AM +0800, Baolu Lu wrote:
>>>> On 2022/8/26 22:52, Jason Gunthorpe wrote:
>>>>> On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote:
>>>>>> Allocate the blocking domain when probing devices if the driver supports
>>>>>> blocking domain allocation. Otherwise, revert to the previous behavior,
>>>>>> that is, use UNMANAGED domain instead when the blocking domain is needed.
>>>>>>
>>>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>>>> Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
>>>>>> Tested-by: Tony Zhu<tony.zhu@intel.com>
>>>>>> ---
>>>>>>     drivers/iommu/iommu.c | 29 +++++++++++++++++------------
>>>>>>     1 file changed, 17 insertions(+), 12 deletions(-)
>>>>> This seems like a lot of overhead to allocate these things for every
>>>>> group?
>>>>>
>>>>> Why not add a simple refcount on the blocking domain instead and
>>>>> allocate the domain on the pasid attach like we do for ownership?
>>>>
>>>> I am working towards implementing static instance of blocking domain for
>>>> each IOMMU driver, and then, there's no much overhead to allocate it in
>>>> the probing device path.
>>>
>>> Well, I thought about that and I don't think we can get
>>> there in a short order.
>>
>> Yes. Fair enough.
>>
>>> Would rather you progress this series without
>>> getting entangled in such a big adventure
>>
>> Agreed. I will drop this patch and add below code in the iommu
>> interface:
>>
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3219,6 +3219,26 @@ int iommu_attach_device_pasid(struct iommu_domain
>> *domain,
>>                  return -ENODEV;
>>
>>          mutex_lock(&group->mutex);
>> +
>> +       /*
>> +        * The underlying IOMMU driver needs to support blocking domain
>> +        * allocation and the callback to block DMA transactions with a
>> +        * specific PASID.
>> +        */
>> +       if (!group->blocking_domain) {
>> +               group->blocking_domain = __iommu_domain_alloc(dev->bus,
>> +                               IOMMU_DOMAIN_BLOCKED);
>> +               if (!group->blocking_domain) {
>> +                       ret = -ENODEV;
>> +                       goto out_unlock;
>> +               }
>> +       }
>> +
>> +       if (!group->blocking_domain->ops->set_dev_pasid) {
>> +               ret = -EOPNOTSUPP;
>> +               goto out_unlock;
>> +       }
>> +
>>          curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
>> GFP_KERNEL);
>>          if (curr) {
>>                  ret = xa_err(curr) ? : -EBUSY;
>>
>> Currently both ARM SMMUv3 and VT-d drivers use static blocking domain.
>> Hence I didn't use a refcount for blocking domain release here.
> 
> I don't think that works in the general case, you can't just destroy
> what is in group->blocking_domain..

If I understand you correctly, we can't just free the blocking domain
and forget about whether this domain is still set on any device?

> 
> Maybe all of this is just the good reason to go to a simple
> device->ops->remove_dev_pasid() callback and forget about blocking
> domain here.

Do you mean rolling back to what we did in v10?

--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -262,6 +262,8 @@ struct iommu_ops {
   * struct iommu_domain_ops - domain specific operations
   * @attach_dev: attach an iommu domain to a device
   * @detach_dev: detach an iommu domain from a device
+ * @set_dev_pasid: set an iommu domain to a pasid of device
+ * @block_dev_pasid: block pasid of device from using iommu domain
   * @map: map a physically contiguous memory region to an iommu domain
   * @map_pages: map a physically contiguous set of pages of the same 
size to
   *             an iommu domain.
@@ -282,6 +284,10 @@ struct iommu_ops {
  struct iommu_domain_ops {
         int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
         void (*detach_dev)(struct iommu_domain *domain, struct device 
*dev);
+       int (*set_dev_pasid)(struct iommu_domain *domain, struct device 
*dev,
+                            ioasid_t pasid);
+       void (*block_dev_pasid)(struct iommu_domain *domain, struct 
device *dev,
+                               ioasid_t pasid);

Best regards,
baolu
Jason Gunthorpe Aug. 31, 2022, 2:10 p.m. UTC | #7 
On Wed, Aug 31, 2022 at 09:49:44AM +0800, Baolu Lu wrote:
> > Maybe all of this is just the good reason to go to a simple
> > device->ops->remove_dev_pasid() callback and forget about blocking
> > domain here.
> 
> Do you mean rolling back to what we did in v10?

Yeah, but it shouldn't be a domain_op, removing a pasid is a device op

Just 

remove_dev_pasid(struct device *dev, ioasid_t pasid)

Jason
Baolu Lu Sept. 1, 2022, 10:44 a.m. UTC | #8 
On 2022/8/31 22:10, Jason Gunthorpe wrote:
> On Wed, Aug 31, 2022 at 09:49:44AM +0800, Baolu Lu wrote:
>>> Maybe all of this is just the good reason to go to a simple
>>> device->ops->remove_dev_pasid() callback and forget about blocking
>>> domain here.
>>
>> Do you mean rolling back to what we did in v10?
> 
> Yeah, but it shouldn't be a domain_op, removing a pasid is a device op
> 
> Just
> 
> remove_dev_pasid(struct device *dev, ioasid_t pasid)

It's clear now. Thanks!

How about below iommu_attach/detach_device_pasid() code?

By the way, how about naming it "block_dev_pasid(dev, pasid)"?

+static int __iommu_set_group_pasid(struct iommu_domain *domain,
+				   struct iommu_group *group, ioasid_t pasid)
+{
+	struct group_device *device;
+	int ret = 0;
+
+	list_for_each_entry(device, &group->devices, list) {
+		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static void __iommu_remove_group_pasid(struct iommu_group *group,
+				       ioasid_t pasid)
+{
+	struct group_device *device;
+	const struct iommu_ops *ops;
+
+	list_for_each_entry(device, &group->devices, list) {
+		ops = dev_iommu_ops(device->dev);
+		ops->remove_dev_pasid(device->dev, pasid);
+	}
+}
+
+/*
+ * iommu_attach_device_pasid() - Attach a domain to pasid of device
+ * @domain: the iommu domain.
+ * @dev: the attached device.
+ * @pasid: the pasid of the device.
+ *
+ * Return: 0 on success, or an error.
+ */
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid)
+{
+	struct iommu_group *group;
+	void *curr;
+	int ret;
+
+	if (!domain->ops->set_dev_pasid)
+		return -EOPNOTSUPP;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return -ENODEV;
+
+	mutex_lock(&group->mutex);
+	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
+	if (curr) {
+		ret = xa_err(curr) ? : -EBUSY;
+		goto out_unlock;
+	}
+
+	ret = __iommu_set_group_pasid(domain, group, pasid);
+	if (ret) {
+		__iommu_remove_group_pasid(group, pasid);
+		xa_erase(&group->pasid_array, pasid);
+	}
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
+
+/*
+ * iommu_detach_device_pasid() - Detach the domain from pasid of device
+ * @domain: the iommu domain.
+ * @dev: the attached device.
+ * @pasid: the pasid of the device.
+ *
+ * The @domain must have been attached to @pasid of the @dev with
+ * iommu_attach_device_pasid().
+ */
+void iommu_detach_device_pasid(struct iommu_domain *domain, struct 
device *dev,
+			       ioasid_t pasid)
+{
+	struct iommu_group *group = iommu_group_get(dev);
+
+	mutex_lock(&group->mutex);
+	__iommu_remove_group_pasid(group, pasid);
+	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
+	mutex_unlock(&group->mutex);
+
+	iommu_group_put(group);
+}
+EXPORT_SYMBOL_GPL(iommu_detach_device_pasid);

+ * @remove_dev_pasid: Remove any translation configurations of a specific
+ *                    pasid, so that any DMA transactions with this pasid
+ *                    will be blocked by the hardware.
   * @pgsize_bitmap: bitmap of all possible supported page sizes
   * @owner: Driver module providing these ops
   */
@@ -256,6 +259,7 @@ struct iommu_ops {
  			     struct iommu_page_response *msg);

  	int (*def_domain_type)(struct device *dev);
+	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);

Best regards,
baolu
Jason Gunthorpe Sept. 2, 2022, 12:48 p.m. UTC | #9 
On Thu, Sep 01, 2022 at 06:44:10PM +0800, Baolu Lu wrote:
> On 2022/8/31 22:10, Jason Gunthorpe wrote:
> > On Wed, Aug 31, 2022 at 09:49:44AM +0800, Baolu Lu wrote:
> > > > Maybe all of this is just the good reason to go to a simple
> > > > device->ops->remove_dev_pasid() callback and forget about blocking
> > > > domain here.
> > > 
> > > Do you mean rolling back to what we did in v10?
> > 
> > Yeah, but it shouldn't be a domain_op, removing a pasid is a device op
> > 
> > Just
> > 
> > remove_dev_pasid(struct device *dev, ioasid_t pasid)
> 
> It's clear now. Thanks!
> 
> How about below iommu_attach/detach_device_pasid() code?

I think this is probably the right thing

> By the way, how about naming it "block_dev_pasid(dev, pasid)"?

set/remove is a better pairing that set/block

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2c9488d966ab..e985f4d5895f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -318,6 +318,10 @@  int iommu_probe_device(struct device *dev)
 	mutex_lock(&group->mutex);
 	iommu_alloc_default_domain(group, dev);
 
+	/* Try to allocate a blocking domain */
+	group->blocking_domain = __iommu_domain_alloc(dev->bus,
+						      IOMMU_DOMAIN_BLOCKED);
+
 	/*
 	 * If device joined an existing group which has been claimed, don't
 	 * attach the default domain.
@@ -1778,6 +1782,10 @@  int bus_iommu_probe(struct bus_type *bus)
 		/* Try to allocate default domain */
 		probe_alloc_default_domain(bus, group);
 
+		/* Try to allocate blocking domain */
+		group->blocking_domain =
+				__iommu_domain_alloc(bus, IOMMU_DOMAIN_BLOCKED);
+
 		if (!group->default_domain) {
 			mutex_unlock(&group->mutex);
 			continue;
@@ -3165,18 +3173,15 @@  static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 	if (group->blocking_domain)
 		return 0;
 
-	group->blocking_domain =
-		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
-	if (!group->blocking_domain) {
-		/*
-		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
-		 * create an empty domain instead.
-		 */
-		group->blocking_domain = __iommu_domain_alloc(
-			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
-		if (!group->blocking_domain)
-			return -EINVAL;
-	}
+	/*
+	 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
+	 * create an empty domain instead.
+	 */
+	group->blocking_domain = __iommu_domain_alloc(dev->dev->bus,
+						      IOMMU_DOMAIN_UNMANAGED);
+	if (!group->blocking_domain)
+		return -EINVAL;
+
 	return 0;
 }