diff mbox series

[v1,2/8] iommu: Introduce a new iommu_group_replace_domain() API

Message ID a98e622f41d76b64f5a7d0c758d8bda5e8043013.1675320212.git.nicolinc@nvidia.com
State Superseded
Headers show
Series Add IO page table replacement support | expand

Commit Message

Nicolin Chen Feb. 2, 2023, 7:05 a.m. UTC 
qemu has a need to replace the translations associated with a domain
when the guest does large-scale operations like switching between an
IDENTITY domain and, say, dma-iommu.c.

Currently, it does this by replacing all the mappings in a single
domain, but this is very inefficient and means that domains have to be
per-device rather than per-translation.

Provide a high-level API to allow replacements of one domain with
another. This is similar to a detach/attach cycle except it doesn't
force the group to go to the blocking domain in-between.

By removing this forced blocking domain the iommu driver has the
opportunity to implement an atomic replacement of the domains to the
greatest extent its hardware allows.

Atomic replacement allows the qemu emulation of the viommu to be more
complete, as real hardware has this ability.

All drivers are already required to support changing between active
UNMANAGED domains when using their attach_dev ops.

This API is expected to be used by IOMMUFD, so add to the iommu-priv
header and mark it as IOMMUFD_INTERNAL.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu-priv.h |  4 ++++
 drivers/iommu/iommu.c      | 30 ++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

Comments

Tian, Kevin Feb. 3, 2023, 8:26 a.m. UTC | #1 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, February 2, 2023 3:05 PM
> 
> All drivers are already required to support changing between active
> UNMANAGED domains when using their attach_dev ops.

All drivers which don't have *broken* UNMANAGED domain?

> 
> +/**
> + * iommu_group_replace_domain - replace the domain that a group is
> attached to
> + * @new_domain: new IOMMU domain to replace with
> + * @group: IOMMU group that will be attached to the new domain
> + *
> + * This API allows the group to switch domains without being forced to go to
> + * the blocking domain in-between.
> + *
> + * If the attached domain is a core domain (e.g. a default_domain), it will act
> + * just like the iommu_attach_group().

I think you meant "the currently-attached domain", which implies a
'detached' state as you replied to Baolu.

> + */
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *new_domain)

what actual value does 'replace' give us? It's just a wrapper of
__iommu_group_set_domain() then calling it set_domain is
probably clearer. You can clarify the 'replace' behavior in the
comment.

> +{
> +	int ret;
> +
> +	if (!new_domain)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +	ret = __iommu_group_set_domain(group, new_domain);
> +	if (ret) {
> +		if (__iommu_group_set_domain(group, group->domain))
> +			__iommu_group_set_core_domain(group);
> +	}

Can you elaborate the error handling here? Ideally if
__iommu_group_set_domain() fails then group->domain shouldn't
be changed. Why do we need further housekeeping here?
Nicolin Chen Feb. 3, 2023, 5:45 p.m. UTC | #2 
On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:

> > +/**
> > + * iommu_group_replace_domain - replace the domain that a group is
> > attached to
> > + * @new_domain: new IOMMU domain to replace with
> > + * @group: IOMMU group that will be attached to the new domain
> > + *
> > + * This API allows the group to switch domains without being forced to go to
> > + * the blocking domain in-between.
> > + *
> > + * If the attached domain is a core domain (e.g. a default_domain), it will act
> > + * just like the iommu_attach_group().
> 
> I think you meant "the currently-attached domain", which implies a
> 'detached' state as you replied to Baolu.

Hmm, I don't see an implication, since we only have either
"the attached domain" or "a new domain" in the context?

With that being said, I can add "currently" in v2:
 * If the currently attached domain is a core domain (e.g. a default_domain),
 * it will act just like the iommu_attach_group().

Thanks
Nic
Tian, Kevin Feb. 6, 2023, 6:40 a.m. UTC | #3 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Saturday, February 4, 2023 1:45 AM
> 
> On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> 
> > > +/**
> > > + * iommu_group_replace_domain - replace the domain that a group is
> > > attached to
> > > + * @new_domain: new IOMMU domain to replace with
> > > + * @group: IOMMU group that will be attached to the new domain
> > > + *
> > > + * This API allows the group to switch domains without being forced to
> go to
> > > + * the blocking domain in-between.
> > > + *
> > > + * If the attached domain is a core domain (e.g. a default_domain), it will
> act
> > > + * just like the iommu_attach_group().
> >
> > I think you meant "the currently-attached domain", which implies a
> > 'detached' state as you replied to Baolu.
> 
> Hmm, I don't see an implication, since we only have either
> "the attached domain" or "a new domain" in the context?

probably just me reading it as "the newly attached domain". 😊

> 
> With that being said, I can add "currently" in v2:
>  * If the currently attached domain is a core domain (e.g. a default_domain),
>  * it will act just like the iommu_attach_group().
> 

this is clearer.
diff mbox series

Patch

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index 9e1497027cff..b546795a7e49 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -15,4 +15,8 @@  static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	 */
 	return dev->iommu->iommu_dev->ops;
 }
+
+extern int iommu_group_replace_domain(struct iommu_group *group,
+				      struct iommu_domain *new_domain);
+
 #endif /* __LINUX_IOMMU_PRIV_H */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a18b7f1a4e6e..c35da7a5c0d4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2151,6 +2151,36 @@  int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
+/**
+ * iommu_group_replace_domain - replace the domain that a group is attached to
+ * @new_domain: new IOMMU domain to replace with
+ * @group: IOMMU group that will be attached to the new domain
+ *
+ * This API allows the group to switch domains without being forced to go to
+ * the blocking domain in-between.
+ *
+ * If the attached domain is a core domain (e.g. a default_domain), it will act
+ * just like the iommu_attach_group().
+ */
+int iommu_group_replace_domain(struct iommu_group *group,
+			       struct iommu_domain *new_domain)
+{
+	int ret;
+
+	if (!new_domain)
+		return -EINVAL;
+
+	mutex_lock(&group->mutex);
+	ret = __iommu_group_set_domain(group, new_domain);
+	if (ret) {
+		if (__iommu_group_set_domain(group, group->domain))
+			__iommu_group_set_core_domain(group);
+	}
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, IOMMUFD_INTERNAL);
+
 static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);