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