Re: [PATCH v7 13/16] iommufd/viommu: Select vIOMMU operations before allocation

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Oct 12, 2026 at 03:07:57PM +0530, Aneesh Kumar K.V wrote:
> > Currently, amd_iommufd_get_viommu_ops() unconditionally returns the ops
> > structure without checking the type argument. If a user requests an arbitrary
> > type like IOMMU_VIOMMU_TYPE_ARM_SMMUV3 on an AMD device, the core iommufd
> > framework might track the vIOMMU under a mismatched architectural type
> > identifier.
> >
> > The ARM and selftest backends appear to validate this argument against their
> > supported hardware types before returning the operations.
> >
> 
> No vIOMMU type has been added for any IOMMU other than Arm. The
> documentation also considers IOMMU_VIOMMU_TYPE_DEFAULT reserved.

This is a bug in the AMD driver we need to fix, send a patch? It
should only accept DEFAULT, not every type.

> >> @@ -73,5 +79,7 @@ static void amd_iommufd_viommu_destroy(struct iommufd_viommu *viommu)
> >>   * struct iommufd_viommu_ops - vIOMMU specific operations
> >>   */
> >>  static const struct iommufd_viommu_ops amd_viommu_ops = {
> >> +	.get_viommu_size = amd_iommufd_get_viommu_size,
> >> +	.viommu_init = amd_iommufd_viommu_init,
> >>  	.destroy = amd_iommufd_viommu_destroy,
> >>  };
> >
> > [Severity: High]
> > Is the alloc_domain_nested callback missing from this operations structure?
> >
> > The commit message and comments in amd_iommu_alloc_domain_nested() indicate
> > it should be assigned to the viommu operations. However, it is omitted here.
> >
> > If a user attempts to allocate an AMD nested domain using IOMMU_HWPT_ALLOC,
> > won't the core framework reject it with -EOPNOTSUPP because
> > viommu->ops->alloc_domain_nested is NULL?
> >
> 
> It is unclear whether nested domain support is fully integrated for AMD.
> I see amd_iommu_alloc_domain_nested(), but cannot find any callers.

There is a series on the list to complete more of this, it is fine the
way you have it.


> >> diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
> >> index f3d5b5a7eb4a4..e95138ae71d5b 100644
> >> --- a/drivers/iommu/iommufd/viommu.c
> >> +++ b/drivers/iommu/iommufd/viommu.c
> > [ ... ]
> >> @@ -103,16 +105,12 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
> >>  	 */
> >>  	viommu->iommu_dev = iommu_dev;
> >>  
> >> -	rc = ops->viommu_init(viommu, hwpt_paging->common.domain,
> >> +	rc = ops->viommu_init(viommu, idev->dev,
> >> +			      hwpt_paging->common.domain,
> >>  			      user_data.len ? &user_data : NULL);
> >
> > [Severity: Critical]
> > Is there a missing check to ensure the domain is owned by the same driver
> > before passing it to viommu_init?
> 
> The viommu_init callback needs to perform this check. For example, the
> Arm implementation does:

No, the check is missing. If a domain is being used then the core code
needs to check it, not the driver.

When the ops is first obtained it should be checked along with the
domain to be sure it is compatible.

The NULL parent domain on the TSM path should be enforced in the core
code since there is nothing to check there.

Jason