Re: [PATCH v5 4/6] iommu/arm-smmu-v3: Implement set_dev_pasid

Jason Gunthorpe
2023-08-03T17:52:42Z

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