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.

Agreed. Thanks for clarification.

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.

Yes. The small routine would be like:
 (1) Lock/get idev, dev->iommu, and dev->iommu->list
 (2) Increase the refcount at dev_iommu
 (3) Save the dev_iommu to viommu
 (4) Unlock/put those in (1).

Thank you
Nicolin