Re: [PATCH v2 3/8] iommu/vt-d: Implement device_pasid domain attach ops

Jason Gunthorpe
2022-03-15T14:33:30Z

On Mon, Mar 14, 2022 at 10:07:07PM -0700, Jacob Pan wrote:
> + /*
> + * Each domain could have multiple devices attached with shared or per
> + * device PASIDs. At the domain level, we keep track of unique PASIDs and
> + * device user count.
> + * E.g. If a domain has two devices attached, device A has PASID 0, 1;
> + * device B has PASID 0, 2. Then the domain would have PASID 0, 1, 2.
> + */

A 2d array of xarray's seems like a poor data structure for this task.

AFACIT this wants to store a list of (device, pasid) tuples, so a
simple linked list, 1d xarray vector or a red black tree seems more
appropriate..

> + if (entry) {
> + pinfo = entry;
> + } else {
> + pinfo = kzalloc(sizeof(*pinfo), GFP_ATOMIC);
> + if (!pinfo)
> + return -ENOMEM;
> + pinfo->pasid = pasid;
> + /* Store the new PASID info in the per domain array */
> + ret = xa_err(__xa_store(&dmar_domain->pasids, pasid, pinfo,
> + GFP_ATOMIC));
> + if (ret)
> + goto xa_store_err;
> + }
> + /* Store PASID in per device-domain array, this is for tracking devTLB */
> + ret = xa_err(xa_store(&info->pasids, pasid, pinfo, GFP_ATOMIC));
> + if (ret)
> + goto xa_store_err;
> +
> + atomic_inc(&pinfo->users);
> + xa_unlock(&dmar_domain->pasids);
> +
> + return 0;
> +
> +xa_store_err:
> + xa_unlock(&dmar_domain->pasids);
> + spin_lock_irqsave(&iommu->lock, flags);
> + intel_pasid_tear_down_entry(iommu, dev, pasid, false);
> + spin_unlock_irqrestore(&iommu->lock, flags);
> +
> + if (!atomic_read(&pinfo->users)) {
> + __xa_erase(&dmar_domain->pasids, pasid);

This isn't locked right

Jason