Messages in this thread Patch in this message | | | Date | Thu, 3 Aug 2023 14:52:26 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v5 4/6] iommu/arm-smmu-v3: Implement set_dev_pasid |
| |
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
| |