Re: [PATCH v5 14/14] iommu/amd: Add support for nested domain attach/detach

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Nov 12, 2025 at 06:25:06PM +0000, Suravee Suthikulpanit wrote:
> Introduce set_dte_nested() to program guest translation settings in
> the host DTE when attaches the nested domain to a device.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/nested.c | 69 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/iommu/amd/nested.c b/drivers/iommu/amd/nested.c
> index 1bbcb16abecc..eeb5d9b3a58f 100644
> --- a/drivers/iommu/amd/nested.c
> +++ b/drivers/iommu/amd/nested.c
> @@ -153,6 +153,74 @@ amd_iommu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
>  	return ERR_PTR(ret);
>  }
>  
> +static void set_dte_nested(struct amd_iommu *iommu,
> +			   struct iommu_domain *dom,
> +			   struct iommu_dev_data *dev_data)
> +{
> +	struct protection_domain *parent;
> +	struct dev_table_entry new = {0};
> +	struct nested_domain *ndom = to_ndomain(dom);
> +	struct iommu_hwpt_amd_guest *gdte = &ndom->gdte;
> +	struct pt_iommu_amdv1_hw_info pt_info;
> +
> +	/*
> +	 * The nest parent domain is attached during the call to the
> +	 * struct iommu_ops.viommu_init(), which will be stored as part
> +	 * of the struct amd_iommu_viommu.parent.
> +	 */
> +	if (WARN_ON(!ndom->viommu || !ndom->viommu->parent))
> +		return;
> +
> +	parent = ndom->viommu->parent;
> +	amd_iommu_make_clear_dte(dev_data, &new);
> +
> +	/* Retrieve the current pagetable info via the IOMMU PT API. */
> +	pt_iommu_amdv1_hw_info(&parent->amdv1, &pt_info);
> +
> +	/*
> +	 * Use nested domain ID to program DTE.
> +	 * See amd_iommu_alloc_domain_nested().
> +	 */
> +	amd_iommu_set_dte_v1(dev_data, parent, ndom->gdom_info->hdom_id, &pt_info, &new);
> +
> +	/* Guest PPR */
> +	new.data[0] |= gdte->dte[0] & DTE_FLAG_PPR;
> +
> +	/* Guest translation stuff */
> +	new.data[0] |= gdte->dte[0] & (DTE_GLX | DTE_FLAG_GV | DTE_FLAG_GIOV);
> +
> +	/* GCR3 table */
> +	new.data[0] |= gdte->dte[0] & DTE_GCR3_14_12;
> +	new.data[1] |= gdte->dte[1] & (DTE_GCR3_30_15 | DTE_GCR3_51_31);
> +
> +	/* Guest paging mode */
> +	new.data[2] |= gdte->dte[2] & DTE_GPT_LEVEL_MASK;
> +
> +	amd_iommu_update_dte(iommu, dev_data, &new);

The functions should be consistent a "set" function should just set a
struct dev_table_entry. A set function should not call
amd_iommu_update_dte().

So either lift the amd_iommu_update_dte() (I prefer) or change the
function name?

> +}
> +
> +static int nested_attach_device(struct iommu_domain *dom, struct device *dev,
> +				struct iommu_domain *old)
> +{
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
> +	int ret = 0;
> +
> +	if (WARN_ON(dom->type != IOMMU_DOMAIN_NESTED))
> +		return -EINVAL;

This is not needed, the ops are for nesting they are only called by
nesting domain types.

> +	mutex_lock(&dev_data->mutex);
> +
> +	/* Setup DTE for nested translation and
> +	 * update the device table
> +	 */
> +	set_dte_nested(iommu, dom, dev_data);
> +
> +	mutex_unlock(&dev_data->mutex);

This needs to make sure there are not PASIDs enabled.

And similarly the PASID attach path needs to to check that a v1 or
blocking domain is on the rid not identiy, not nesting.

But overall this looks OK and I think the series a whole is looking
pretty good. If you fix these little things it can possibly make this
cycle?

Jason