Re: [PATCH v1 05/16] iommufd/viommu: Add IOMMU_VIOMMU_SET/UNSET_VDEV_ID ioctl

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Aug 19, 2024 at 02:33:32PM -0300, Jason Gunthorpe wrote:
> On Thu, Aug 15, 2024 at 05:21:57PM -0700, Nicolin Chen wrote:
> 
> > > 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.
> > 
> > My view is that, the moment there is a VIOMMU object, that must
> > be a nested IOMMU case, so there must be a nested hwpt. Blocking
> > domain would be a hwpt_nested too (vSTE=Abort) as we previously
> > concluded.
> 
> I'm not sure other vendors can do that vSTE=Abort/Bypass thing though
> yet..
> 
> > Then, in a nested case, it feels odd that an idev is attached to
> > an S2 hwpt..
> >
> > That being said, I think we can still do that with validations:
> >  If idev->hwpt is nested, compare input viommu v.s idev->hwpt->viommu.
> >  If idev->hwpt is paging, compare input viommu->hwpt v.s idev->hwpt.
> 
> But again, if you don't contiguously validate those invariants in all
> the other attach paths it is sort of pointless to check them since the
> userspace can still violate things.

Hmm, would that be unsafe? I start to wonder if we should allow an
attach to viommu and put validations on that?

> > This complicates things overall especially with the VIRQ that has
> > involved interrupt context polling vdev_id, where semaphore/mutex
> > won't fit very well. Perhaps it would need a driver-level bottom
> > half routine to call those helpers with locks. I am glad that you
> > noticed the problem early.
> 
> I think you have to show the xarray to the driver and the driver can
> use the spinlock to access it safely. Keeping it hidden in the core
> code is causing all these locking problems.

Yea, I just figured that out... You have been right. I was able to
get rid of the locking problem with invalidation API. But then irq
became a headache as drivers would only know the dev pointer, so
everything that the dev could convert to would be unsafe as it can
not grab the idev/viommu locks until it converts.

Thanks
Nicolin