On Thu, Aug 15, 2024 at 12:46:24PM -0700, Nicolin Chen wrote:
> On Thu, Aug 15, 2024 at 04:08:48PM -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 07, 2024 at 01:10:46PM -0700, Nicolin Chen wrote:
> >
> > > +int iommufd_viommu_set_vdev_id(struct iommufd_ucmd *ucmd)
> > > +{
> > > + struct iommu_viommu_set_vdev_id *cmd = ucmd->cmd;
> > > + struct iommufd_hwpt_nested *hwpt_nested;
> > > + struct iommufd_vdev_id *vdev_id, *curr;
> > > + struct iommufd_hw_pagetable *hwpt;
> > > + struct iommufd_viommu *viommu;
> > > + struct iommufd_device *idev;
> > > + int rc = 0;
> > > +
> > > + if (cmd->vdev_id > ULONG_MAX)
> > > + return -EINVAL;
> > > +
> > > + idev = iommufd_get_device(ucmd, cmd->dev_id);
> > > + if (IS_ERR(idev))
> > > + return PTR_ERR(idev);
> > > + hwpt = idev->igroup->hwpt;
> > > +
> > > + if (hwpt == NULL || hwpt->obj.type != IOMMUFD_OBJ_HWPT_NESTED) {
> > > + rc = -EINVAL;
> > > + goto out_put_idev;
> > > + }
> > > + hwpt_nested = container_of(hwpt, struct iommufd_hwpt_nested, common);
> >
> > This doesn't seem like a necessary check, the attached hwpt can change
> > after this is established, so this can't be an invariant we enforce.
> >
> > If you want to do 1:1 then somehow directly check if the idev is
> > already linked to a viommu.
>
> But idev can't link to a viommu without a proxy hwpt_nested?
Why not? The idev becomes linked to the viommu when the dev id is set
Unless we are also going to enforce the idev is always attached to a
nested then I don't think we need to check it here.
Things will definately not entirely work as expected if the vdev is
directly attached to the s2 or a blocking, but it won't harm anything.
> the stage-2 only configuration should have an identity hwpt_nested
> right?
Yes, that is the right way to use the API
> > It has to work by having the iommu driver directly access the xarray
> > and the entirely under the spinlock the iommu driver can translate the
> > vSID to the pSID and the let go and push the invalidation to HW. No
> > races.
>
> Maybe the iommufd_viommu_invalidate ioctl handler should hold that
> xa_lock around the viommu->ops->cache_invalidate, and then add lock
> assert in iommufd_viommu_find_device?
That doesn't seem like a great idea, you can't do copy_from_user under
a spinlock.
> > xa_lock(&viommu->vdev_ids);
> > vdev_id = xa_load(&viommu->vdev_ids, cmd->vdev_id);
> > if (!vdev_id || vdev_id->vdev_id != cmd->vdev_id (????) || vdev_id->dev != idev->dev)
> > err
> > __xa_erase(&viommu->vdev_ids, cmd->vdev_id);
> > xa_unlock((&viommu->vdev_ids);
>
> I've changed to xa_cmpxchg() in my local tree. Would it be simpler?
No, that is still not right, you can't take the vdev_id outside the
lock at all. Even for cmpxchng because the vdev_id could have been
freed and reallocated by another thread.
You must combine the validation of the vdev_id with the erase under a
single critical region.
Jason
