diff mbox series

[1/2] iommu: Add static iommu_ops->release_domain

Message ID 20240223051302.177596-2-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Fix domain check on device release |

Commit Message

Baolu Lu Feb. 23, 2024, 5:13 a.m. UTC 
  The current device_release callback for individual iommu drivers does the
following:

1) Silent IOMMU DMA translation: It detaches any existing domain from the
   device and puts it into a blocking state (some drivers might use the
   identity state).
2) Resource release: It releases resources allocated during the
   device_probe callback and restores the device to its pre-probe state.

Step 1 is challenging for individual iommu drivers because each must check
if a domain is already attached to the device. Additionally, if a deferred
attach never occurred, the device_release should avoid modifying hardware
configuration regardless of the reason for its call.

To simplify this process, introduce a static release_domain within the
iommu_ops structure. It can be either a blocking or identity domain
depending on the iommu hardware. The iommu core will decide whether to
attach this domain before the device_release callback, eliminating the
need for repetitive code in various drivers.

Consequently, the device_release callback can focus solely on the opposite
operations of device_probe, including releasing all resources allocated
during that callback.

Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h |  1 +
 drivers/iommu/iommu.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

Comments

Tian, Kevin Feb. 27, 2024, 7:32 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Friday, February 23, 2024 1:13 PM
> 
> 
> +	/*
> +	 * If the iommu driver provides release_domain then the core code
> +	 * ensures that domain is attached prior to calling release_device.
> +	 * Drivers can use this to enforce a translation on the idle iommu.

'enforce a translation' is confusing in the context of blocking/identity
domain.

otherwise looks good to me:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Baolu Lu Feb. 28, 2024, 1:13 a.m. UTC | #2 
On 2/27/24 3:32 PM, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Friday, February 23, 2024 1:13 PM
>>
>>
>> +	/*
>> +	 * If the iommu driver provides release_domain then the core code
>> +	 * ensures that domain is attached prior to calling release_device.
>> +	 * Drivers can use this to enforce a translation on the idle iommu.
> 'enforce a translation' is confusing in the context of blocking/identity
> domain.

Blocking or identity domain is also kind of a translation from the
core's perspective. The core does not care what type of translation the
release_domain is; it just enforces this type of translation before
device release if the driver has specified one.

Best regards,
baolu
Tian, Kevin Feb. 28, 2024, 3:08 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, February 28, 2024 9:14 AM
> 
> On 2/27/24 3:32 PM, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@linux.intel.com>
> >> Sent: Friday, February 23, 2024 1:13 PM
> >>
> >>
> >> +	/*
> >> +	 * If the iommu driver provides release_domain then the core code
> >> +	 * ensures that domain is attached prior to calling release_device.
> >> +	 * Drivers can use this to enforce a translation on the idle iommu.
> > 'enforce a translation' is confusing in the context of blocking/identity
> > domain.
> 
> Blocking or identity domain is also kind of a translation from the
> core's perspective. The core does not care what type of translation the
> release_domain is; it just enforces this type of translation before
> device release if the driver has specified one.
> 

OK.

btw you may also want to update the following comment together:

	/*
	 * release_device() must stop using any attached domain on the device.
	 * If there are still other devices in the group they are not effected
	 * by this callback.
	 *
	 * The IOMMU driver must set the device to either an identity or
	 * blocking translation and stop using any domain pointer, as it is
	 * going to be freed.
	 */

all the purpose is to set the device to identity or blocking, either via attaching
to an explicit release_domain or implicitly by release_device().
Baolu Lu Feb. 28, 2024, 3:30 a.m. UTC | #4 
On 2/28/24 11:08 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, February 28, 2024 9:14 AM
>>
>> On 2/27/24 3:32 PM, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Friday, February 23, 2024 1:13 PM
>>>>
>>>>
>>>> +	/*
>>>> +	 * If the iommu driver provides release_domain then the core code
>>>> +	 * ensures that domain is attached prior to calling release_device.
>>>> +	 * Drivers can use this to enforce a translation on the idle iommu.
>>> 'enforce a translation' is confusing in the context of blocking/identity
>>> domain.
>>
>> Blocking or identity domain is also kind of a translation from the
>> core's perspective. The core does not care what type of translation the
>> release_domain is; it just enforces this type of translation before
>> device release if the driver has specified one.
>>
> 
> OK.
> 
> btw you may also want to update the following comment together:
> 
> 	/*
> 	 * release_device() must stop using any attached domain on the device.
> 	 * If there are still other devices in the group they are not effected
> 	 * by this callback.
> 	 *
> 	 * The IOMMU driver must set the device to either an identity or
> 	 * blocking translation and stop using any domain pointer, as it is
> 	 * going to be freed.
> 	 */
> 
> all the purpose is to set the device to identity or blocking, either via attaching
> to an explicit release_domain or implicitly by release_device().

Done.

Best regards,
baolu
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index de839fd01bb8..e3d9365b0fa9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -585,6 +585,7 @@  struct iommu_ops {
 	struct module *owner;
 	struct iommu_domain *identity_domain;
 	struct iommu_domain *blocked_domain;
+	struct iommu_domain *release_domain;
 	struct iommu_domain *default_domain;
 };
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 210dc7b4c8cf..fb06c3f47320 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -459,6 +459,18 @@  static void iommu_deinit_device(struct device *dev)
 
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
+	/*
+	 * If the iommu driver provides release_domain then the core code
+	 * ensures that domain is attached prior to calling release_device.
+	 * Drivers can use this to enforce a translation on the idle iommu.
+	 * Usually the global static blocked_domain is a good choice.
+	 *
+	 * Anyway, if a deferred attach never happened then the release
+	 * should still avoid touching any hardware configuration either.
+	 */
+	if (!dev->iommu->attach_deferred && ops->release_domain)
+		ops->release_domain->ops->attach_dev(ops->release_domain, dev);
+
 	/*
 	 * release_device() must stop using any attached domain on the device.
 	 * If there are still other devices in the group they are not effected