Re: [PATCH v3 11/15] iommu/amd: Add support for nested domain allocation

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 09, 2025 at 11:57:51PM +0000, Suravee Suthikulpanit wrote:

> +/*
> + * Structure defining one entry in the device table
> + */
> +struct dev_table_entry {
> +	union {

There is no longer a reason to move this, so these hunks can be dropped?

> +/*
> + * This function is assigned to struct iommufd_viommu_ops.alloc_domain_nested()
> + * during the call to struct iommu_ops.viommu_init().
> + */
> +struct iommu_domain *
> +amd_iommu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
> +			      const struct iommu_user_data *user_data)
> +{
> +	int ret;
> +	struct iommu_hwpt_amd_guest gdte;
> +	struct nested_domain *ndom;
> +
> +	if (user_data->type != IOMMU_HWPT_DATA_AMD_GUEST)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	ret = iommu_copy_struct_from_user(&gdte, user_data,
> +					  IOMMU_HWPT_DATA_AMD_GUEST,
> +					  dte);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ndom = kzalloc(sizeof(*ndom), GFP_KERNEL);
> +	if (!ndom)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ndom->domain.ops = &nested_domain_ops;
> +	ndom->domain.type = IOMMU_DOMAIN_NESTED;
> +	memcpy(&ndom->gdte, &gdte, sizeof(gdte));

You can move the iommu_copy_struct_from_user() to here and avoid this
memcpy and stack allocation.

> +	/*
> +	 * Normally, when a guest has multiple pass-through devices,
> +	 * the IOMMU driver setup DTEs with the same stage-2 table and
> +	 * use the same host domain ID (hDomId). In case of nested translation,
> +	 * if the guest setup different stage-1 tables with same PASID,
> +	 * IOMMU would use the same TLB tag. This will results in TLB
> +	 * aliasing issue.

Yes, but if the guest does this then the guest is required to use
different gDomID's.

> +	 *
> +	 * Workaround the issue by allocating per-device hDomID for nested
> +	 * domain (i.e. ndom->id). This require per-device IOMMU TLB invalidation
> +	 * with corresponded hDomId on the host side when updating stage-2 table.
> +	 */

This is still missing the point - we cannot work around this with
unique hDomID's because when you implement invalidation there is only
one input gDomID and you need to map it to exactly one hDomID to push
the PASID invalidation.

The only reason it is barely acceptable now is because there is no
invalidation support!

The comment should be:

 The guest is assigning gDomIDs based on its own algorithm for managing
 cache tags of (DomID, PASID). Within a single viommu the S2 is used
 by all DTEs but we need to consistently map the gDomID to a single hDomID.
 This should be done by using an xarray in the viommu to keep track of
 the gDomID mapping. Since there is no invalidation support and no
 viommu yet just always use a unique hDomId for now.

 When the S2 is changed all the hDomIDs in the xarray need to have
 invalidations pushed.

Other than that it looks OK

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason