[v5,4/6] iommu/arm-smmu-v3: Implement set_dev_pasid
Commit Message
This change enables the use of the iommu_attach_dev_pasid API for
UNMANAGED domains. The primary use-case is to allow in-kernel users of
the iommu API to manage domains with PASID. This change also allows for
future support of pasid in the DMA api.
Signed-off-by: Michael Shavit <mshavit@google.com>
---
Changes in v5:
- Fix missing error value return in set_dev_pasid
- Fix issue where nr_attached_pasid_domains isn't updated when
arm_smmu_write_ctx_desc fails
- Fix missing free of the attached_domain node
- Split off the CD table refactor to separate patch series: https://lore.kernel.org/all/20230802163328.2623773-1-mshavit@google.com/
- Link to v4: https://lore.kernel.org/all/20230621063825.268890-1-mshavit@google.com/
- Remove districting change where a NULL master is passed to
arm_smmu_prepare_domain_for_smmu
Changes in v4:
- Fix build warning and error on patch 07. The error was introduced
during a v1->v2 rebase and hidden by patch 09 which removed the
offending line.
- Link to v3: https://lore.kernel.org/all/20230614154304.2860121-1-mshavit@google.com/
Changes in v3:
- Dropped the bulk of the SVA refactoring to re-work as a follow-up
series.
- Reworded cover letter to omit dropped changes.
- Rebased on 6.4 tip
- Link to v2: https://lore.kernel.org/all/20230606120854.4170244-1-mshavit@google.com/
Changes in v2:
- Reworded cover letter and commits based on v1 feedback.
- Split and reworked `iommu/arm-smmu-v3: Move cdtable to arm_smmu_master`
- Added SVA clean-up and refactor.
- A few other small bug fixes and cosmetics.
- Link to v1: https://lore.kernel.org/all/20230510205054.2667898-1-mshavit@google.com/
- Add missing atc invalidation when detaching with pasid
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 156 ++++++++++++++++++--
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
2 files changed, 141 insertions(+), 16 deletions(-)
Comments
On Thu, Aug 03, 2023 at 06:12:24PM +0800, Michael Shavit wrote:
> @@ -2448,21 +2476,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> return -EBUSY;
> }
>
> - arm_smmu_detach_dev(master);
> -
> - mutex_lock(&smmu_domain->init_mutex);
> -
> - if (!smmu_domain->smmu) {
> - smmu_domain->smmu = smmu;
> - ret = arm_smmu_domain_finalise(domain, master);
> - if (ret)
> - smmu_domain->smmu = NULL;
> - } else if (smmu_domain->smmu != smmu)
> - ret = -EINVAL;
> + /*
> + * Attaching a bypass or stage 2 domain would break any domains attached
> + * with pasid. Attaching an S1 domain should be feasible but requires
> + * more complicated logic to handle.
> + */
> + if (arm_smmu_master_has_pasid_domains(master)) {
> + dev_err(dev, "cannot attach - domain attached with pasid\n");
> + return -EBUSY;
> + }
Basically don't you just call set_dev_pased(pasid = 0) and it will do
that logic?
> +static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + int ret = 0;
> + unsigned long flags;
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> + struct arm_smmu_device *smmu;
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> + struct arm_smmu_attached_domain *attached_domain;
> + struct arm_smmu_master *master;
> +
> + if (!fwspec)
> + return -ENOENT;
fwspec drivers always have fwspecs, they don't need to check this.
> +
> + master = dev_iommu_priv_get(dev);
> + smmu = master->smmu;
> +
> + ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
> + if (ret)
> + return ret;
> +
> + if (pasid == 0) {
> + dev_err(dev, "pasid 0 is reserved for the device's primary domain\n");
> + return -ENODEV;
> + }
> +
> + if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) {
> + dev_err(dev, "set_dev_pasid only supports stage 1 domains\n");
don't print messages, iommufd can trigger these paths from userspace.
> @@ -2738,6 +2828,15 @@ static void arm_smmu_release_device(struct device *dev)
>
> if (WARN_ON(arm_smmu_master_sva_enabled(master)))
> iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> + if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
> + /*
> + * TODO: Do we need to handle this case?
> + * This requires a mechanism to obtain all the pasid domains
> + * that this master is attached to so that we can clean up the
> + * domain's attached_domain list.
> + */
> + }
Unnecessary, the core code should handle this, add a patch that does this:
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5b5cf74edc7e53..57cac1ffba1cfe 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -438,9 +438,16 @@ static void iommu_deinit_device(struct device *dev)
{
struct iommu_group *group = dev->iommu_group;
const struct iommu_ops *ops = dev_iommu_ops(dev);
+ struct iommu_domain *{pasid_domain;
+ unsigned long pasid;
lockdep_assert_held(&group->mutex);
+ xa_for_each(&group->pasid_array, pasid, pasid_domain) {
+ __iommu_remove_group_pasid(pasid);
+ xa_erase(&group->pasid_array, pasid);
+ }
+
iommu_device_unlink(dev->iommu->iommu_dev, dev);
/*
> @@ -2874,12 +2973,36 @@ static int arm_smmu_def_domain_type(struct device *dev)
> static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> {
> struct iommu_domain *domain;
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> + struct arm_smmu_domain *smmu_domain;
> + struct arm_smmu_attached_domain *attached_domain;
> + unsigned long flags;
>
> - domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
> + if (!master || pasid == 0)
> + return;
> +
> + domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
> if (WARN_ON(IS_ERR(domain)) || !domain)
> return;
I feel like we have made a mistake here to not pass in the current
domain from the core code, it already knows what the domain was..
> + if (domain->type == IOMMU_DOMAIN_SVA)
> + return arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
Yuk.
Jason
@@ -2388,6 +2388,11 @@ static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
return 0;
}
+static bool arm_smmu_master_has_pasid_domains(struct arm_smmu_master *master)
+{
+ return master->nr_attached_pasid_domains > 0;
+}
+
static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
{
struct pci_dev *pdev;
@@ -2423,6 +2428,25 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
arm_smmu_install_ste_for_dev(master);
}
+static int arm_smmu_prepare_domain_for_smmu(struct arm_smmu_device *smmu,
+ struct arm_smmu_domain *smmu_domain,
+ struct arm_smmu_master *master)
+{
+ int ret = 0;
+
+ mutex_lock(&smmu_domain->init_mutex);
+ if (!smmu_domain->smmu) {
+ smmu_domain->smmu = smmu;
+ ret = arm_smmu_domain_finalise(&smmu_domain->domain, master);
+ if (ret)
+ smmu_domain->smmu = NULL;
+ } else if (smmu_domain->smmu != smmu)
+ ret = -EINVAL;
+
+ mutex_unlock(&smmu_domain->init_mutex);
+ return ret;
+}
+
static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
{
int ret = 0;
@@ -2438,6 +2462,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
master = dev_iommu_priv_get(dev);
smmu = master->smmu;
+ ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
+ if (ret)
+ return ret;
+
/*
* Checking that SVA is disabled ensures that this device isn't bound to
* any mm, and can be safely detached from its old domain. Bonds cannot
@@ -2448,21 +2476,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
return -EBUSY;
}
- arm_smmu_detach_dev(master);
-
- mutex_lock(&smmu_domain->init_mutex);
-
- if (!smmu_domain->smmu) {
- smmu_domain->smmu = smmu;
- ret = arm_smmu_domain_finalise(domain, master);
- if (ret)
- smmu_domain->smmu = NULL;
- } else if (smmu_domain->smmu != smmu)
- ret = -EINVAL;
+ /*
+ * Attaching a bypass or stage 2 domain would break any domains attached
+ * with pasid. Attaching an S1 domain should be feasible but requires
+ * more complicated logic to handle.
+ */
+ if (arm_smmu_master_has_pasid_domains(master)) {
+ dev_err(dev, "cannot attach - domain attached with pasid\n");
+ return -EBUSY;
+ }
- mutex_unlock(&smmu_domain->init_mutex);
- if (ret)
- return ret;
+ arm_smmu_detach_dev(master);
/*
* The SMMU does not support enabling ATS with bypass. When the STE is
@@ -2500,6 +2524,72 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
return 0;
}
+static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ int ret = 0;
+ unsigned long flags;
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct arm_smmu_device *smmu;
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ struct arm_smmu_attached_domain *attached_domain;
+ struct arm_smmu_master *master;
+
+ if (!fwspec)
+ return -ENOENT;
+
+ master = dev_iommu_priv_get(dev);
+ smmu = master->smmu;
+
+ ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
+ if (ret)
+ return ret;
+
+ if (pasid == 0) {
+ dev_err(dev, "pasid 0 is reserved for the device's primary domain\n");
+ return -ENODEV;
+ }
+
+ if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) {
+ dev_err(dev, "set_dev_pasid only supports stage 1 domains\n");
+ return -EINVAL;
+ }
+
+ if (!master->cd_table.cdtab)
+ return -EBUSY;
+
+ attached_domain = kzalloc(sizeof(*attached_domain), GFP_KERNEL);
+ if (!attached_domain)
+ return -ENOMEM;
+
+ attached_domain->master = master;
+ attached_domain->domain = smmu_domain;
+ attached_domain->ssid = pasid;
+
+ /*
+ * arm_smmu_share_asid may update the cd's asid value and write the
+ * ctx_desc for every attached_domains in the list. There's a potential
+ * race here regardless of whether we first write the ctx_desc or
+ * first insert into the domain's list. Grabbing the asic_lock prevents
+ * SVA from changing the cd's ASID while the cd is being attached.
+ */
+ mutex_lock(&arm_smmu_asid_lock);
+ ret = arm_smmu_write_ctx_desc(master, pasid, &smmu_domain->cd);
+ if (ret) {
+ mutex_unlock(&arm_smmu_asid_lock);
+ kfree(attached_domain);
+ return ret;
+ }
+
+ spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+ list_add(&attached_domain->list, &smmu_domain->attached_ssids);
+ spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
+ mutex_unlock(&arm_smmu_asid_lock);
+
+ master->nr_attached_pasid_domains += 1;
+ return 0;
+}
+
static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t pgsize, size_t pgcount,
int prot, gfp_t gfp, size_t *mapped)
@@ -2738,6 +2828,15 @@ static void arm_smmu_release_device(struct device *dev)
if (WARN_ON(arm_smmu_master_sva_enabled(master)))
iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+ if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
+ /*
+ * TODO: Do we need to handle this case?
+ * This requires a mechanism to obtain all the pasid domains
+ * that this master is attached to so that we can clean up the
+ * domain's attached_domain list.
+ */
+ }
+
arm_smmu_detach_dev(master);
arm_smmu_disable_pasid(master);
arm_smmu_remove_master(master);
@@ -2874,12 +2973,36 @@ static int arm_smmu_def_domain_type(struct device *dev)
static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
{
struct iommu_domain *domain;
+ struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ struct arm_smmu_domain *smmu_domain;
+ struct arm_smmu_attached_domain *attached_domain;
+ unsigned long flags;
- domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
+ if (!master || pasid == 0)
+ return;
+
+ domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
if (WARN_ON(IS_ERR(domain)) || !domain)
return;
+ if (domain->type == IOMMU_DOMAIN_SVA)
+ return arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
- arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
+ smmu_domain = to_smmu_domain(domain);
+ mutex_lock(&arm_smmu_asid_lock);
+ spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+ list_for_each_entry(attached_domain, &smmu_domain->attached_ssids, list) {
+ if (attached_domain->master != master ||
+ attached_domain->ssid != pasid)
+ continue;
+ list_del(&attached_domain->list);
+ master->nr_attached_pasid_domains -= 1;
+ kfree(attached_domain);
+ break;
+ }
+ spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
+ arm_smmu_write_ctx_desc(master, pasid, NULL);
+ arm_smmu_atc_inv_master_ssid(master, pasid);
+ mutex_unlock(&arm_smmu_asid_lock);
}
static struct iommu_ops arm_smmu_ops = {
@@ -2899,6 +3022,7 @@ static struct iommu_ops arm_smmu_ops = {
.owner = THIS_MODULE,
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = arm_smmu_attach_dev,
+ .set_dev_pasid = arm_smmu_set_dev_pasid,
.map_pages = arm_smmu_map_pages,
.unmap_pages = arm_smmu_unmap_pages,
.flush_iotlb_all = arm_smmu_flush_iotlb_all,
@@ -713,6 +713,7 @@ struct arm_smmu_master {
bool iopf_enabled;
struct list_head bonds;
unsigned int ssid_bits;
+ unsigned int nr_attached_pasid_domains;
};
/* SMMU private data for an IOMMU domain */