Re: [PATCH v2 02/19] iommufd/viommu: Add IOMMUFD_OBJ_VIOMMU and IOMMU_VIOMMU_ALLOC ioctl

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:

quoted

On Wed, Sep 04, 2024 at 01:26:21PM -0300, Jason Gunthorpe wrote:

quoted

On Sun, Sep 01, 2024 at 10:27:09PM -0700, Nicolin Chen wrote:

quoted

On Sun, Sep 01, 2024 at 10:39:17AM +0800, Baolu Lu wrote:

quoted

On 2024/8/28 0:59, Nicolin Chen wrote:

quoted

+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.

quoted

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