Re: [PATCH v2 10/12] iommu/amd: Add support for nested domain allocation

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Oct 01, 2025 at 06:09:52AM +0000, Suravee Suthikulpanit wrote:

> @@ -857,6 +878,8 @@ struct iommu_dev_data {
>  	struct list_head list;		  /* For domain->dev_list */
>  	struct llist_node dev_data_list;  /* For global dev_data_list */
>  	struct protection_domain *domain; /* Domain the device is bound to */
> +	struct protection_domain *parent; /* Parent Domain the device is bound to */
> +	struct nested_domain *ndom;       /* Nested Domain the device is bound to */

These two should never be needed. domain should point to the nested
domain and it can get the other information from it.

> @@ -2620,7 +2623,16 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>  		if ((flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) &&
>  		    !amd_iommu_hd_support(iommu))
>  			break;
> -		return do_iommu_domain_alloc(dev, flags, PD_MODE_V1);
> +
> +		dom = do_iommu_domain_alloc(dev, flags, PD_MODE_V1);
> +		if (!IS_ERR_VALUE(dom) &&
> +		    (flags & IOMMU_HWPT_ALLOC_NEST_PARENT)) {
> +			struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +			dev_data->parent = to_pdomain(dom);

We talked about this many times already. Domain allocation DOES NOT
CHANGE the dev_data.

> +#define pr_fmt(fmt)     "AMD-Vi: " fmt
> +#define dev_fmt(fmt)    pr_fmt(fmt)

I'm not sure these make sense, you should not be using pr_fmt, use a
dev_XX on the iommu instance.

> +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);
> +
> +	/*
> +	 * Need to make sure size of struct iommu_hwpt_amd_guest and
> +	 * struct dev_table_entry are the same since it will be copied
> +	 * from one to the other later on.
> +	 */
> +	if (WARN_ON(sizeof(struct dev_table_entry) != sizeof(gdte)))
> +		return ERR_PTR(-EINVAL);

use static_assert for something like this.

> +
> +	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 (IS_ERR(ndom))
> +		return ERR_PTR(-ENOMEM);
> +
> +	ndom->domain.ops = &nested_domain_ops;
> +	ndom->domain.type = IOMMU_DOMAIN_NESTED;
> +	memcpy(&ndom->guest_dte, &gdte, sizeof(struct dev_table_entry));
> +
> +	/* Due to possible aliasing issue use per-device nested domain ID */
> +	ndom->id = amd_iommu_pdom_id_alloc();

I've forgotten the details, but doesn't the gdet have the virtual
domain ID in side it? Shouldn't this be going to the viommu struct and
mapping virtual domain IDs to physical ones so they can be shared if
the guest says it is safe to share them? I guess that is an
optimization, but it should have a note here explaining it.

Jason