Re: [PATCH v2 10/19] iommufd/viommu: Add vdev_id helpers for IOMMU drivers

[Nicolin Chen]

On Mon, Oct 07, 2024 at 02:11:19PM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 07, 2024 at 09:36:18AM -0700, Nicolin Chen wrote:
> > On Mon, Oct 07, 2024 at 12:38:37PM -0300, Jason Gunthorpe wrote:
> > > On Fri, Oct 04, 2024 at 10:19:43PM -0700, Nicolin Chen wrote:
> > > > I tried exposing the struct iommufd_viommu to drivers, and was
> > > > able to drop a couple of helpers, except these two:
> > > > 
> > > > struct device *vdev_to_dev(struct iommufd_vdevice *vdev)
> > > > {
> > > > 	return vdev ? vdev->idev->dev : NULL;
> > > > } // Without it, we need to expose struct iommufd_device.
> > > > 
> > > > struct iommu_domain *
> > > > iommufd_viommu_to_parent_domain(struct iommufd_viommu *viommu)
> > > > {
> > > > 	if (!viommu || !viommu->hwpt)
> > > > 		return NULL;
> > > > 	return viommu->hwpt->common.domain;
> > > > } // Without it, we need to expose struct iommufd_hwpt_page.
> > > 
> > > It seems OK, there isn't really locking entanglements or performance
> > > path on this stuff?
> > 
> > -----
> > The typical use case of the first one is like:
> > 	dev = vdev_to_dev(xa_load(&viommu->vdevs, (unsigned long)vdev_id));
> > so I am asking for:
> > /* Caller should lock via viommu->vdevs_rwsem with proper permission */
> 
> Why would vdev_to_dev need that locking? The viommu cannot change hwpt
> during its lifecycle?

This is for vdev/dev v.s. hwpt. We need the lock for viommu's
vdev xarray.

Yet, giving a 2nd thought, I feel the lock would be useless if
a driver tries to refer the returned vdev (with this helper)
after the vdev object is destroyed..

We could only note something similar that caller must be aware
of the life cycle of vdev itself..

Nicolin