On Wed, Sep 04, 2024 at 08:37:07PM -0300, Jason Gunthorpe wrote:
On Wed, Sep 04, 2024 at 10:29:26AM -0700, Nicolin Chen wrote:
On Wed, Sep 04, 2024 at 01:26:21PM -0300, Jason Gunthorpe wrote:
On Sun, Sep 01, 2024 at 10:27:09PM -0700, Nicolin Chen wrote:
On Sun, Sep 01, 2024 at 10:39:17AM +0800, Baolu Lu wrote:
On 2024/8/28 0:59, Nicolin Chen wrote:
+int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) +{
struct iommu_viommu_alloc *cmd = ucmd->cmd;struct iommufd_hwpt_paging *hwpt_paging;struct iommufd_viommu *viommu;struct iommufd_device *idev;int rc;if (cmd->flags)return -EOPNOTSUPP;idev = iommufd_get_device(ucmd, cmd->dev_id);Why does a device reference count is needed here? When is this reference count released after the VIOMMU is allocated?
Hmm, it was used to get dev->iommu->iommu_dev to pin the VIOMMU to a physical IOMMU instance (in v1). Jason suggested to remove that, yet I didn't realize that this idev is now completely useless.
With that being said, a parent HWPT could be shared across VIOMUs allocated for the same VM. So, I think we do need a dev pointer to know which physical instance the VIOMMU allocates for, especially for a driver-managed VIOMMU.
Eventually you need a way to pin the physical iommu, without pinning any idevs. Not sure how best to do that
Just trying to clarify "without pinning any idevs", does it mean we shouldn't pass in an idev_id to get dev->iommu->iommu_dev?
From userspace we have no choice but to use an idev_id to locate the physical iommu
But since we want to support hotplug it is rather problematic if that idev is permanently locked down.
Otherwise, iommu_probe_device_lock and iommu_device_lock in the iommu.c are good enough to lock dev->iommu and iommu->list. And I think we just need an iommu helper refcounting the dev_iommu (or iommu_device) as we previously discussed.
If you have a ref on an idev then the iommu_dev has to be stable, so you can just incr some refcount and then drop the idev stuff.
Looks like a refcount could only WARN on an unbalanced iommu_dev in iommu_device_unregister() and iommu_device_unregister_bus(), either of which returns void so no way of doing a retry. And their callers would also likely free the entire memory of the driver-level struct where iommu_dev usually locates.. I feel it gets less meaningful to add the refcount if the lifecycle cannot be guaranteed.
You mentioned that actually only the iommufd selftest might hit such a corner case, so perhaps we should do something in the selftest code v.s. the iommu core. What do you think?
Thanks Nicolin