Re: [PATCH v6 17/24] iommu/amd: Program nested DTE and DomID map on attach

[Vasant Hegde <vasant.hegde@amd.com>]

On 9/30/2026 2:53 PM, Suravee Suthikulpanit wrote:
> On nested attach, the host DTE must enable vIOMMU and carry the guest
> device and guest IDs from the vDevice. Look up gdev_id with
> iommufd_viommu_get_vdev_id() and fail the attach if that lookup fails.
> 
> Program the Domain ID mapping table through VFCTRL so hardware can
> translate gdom_id to hdom_id. Write CONTROL1 before the DTE is live
> with DTE_VIOMMU_EN so guest DMA cannot resolve against the idle
> nest-parent map.
> 
> DomID CONTROL1 uses the same per-vIOMMU vfctrl_lock as DevID CONTROL0.
> Hold that lock across CONTROL1 and the DTE commit, and on last-ref
> nested_domain_free() take it before gdomid_array so idle restore
> cannot clobber a concurrent alloc+attach.
> 
> CONTROL1 WRITE uses the same doorbell as CONTROL0. Poll WRITE until
> it clears; iommu_completion_wait() does not drain the table DMA.
> 
> On the last nested_domain_free() for a gdom_id, restore the idle
> DomID map (nest parent, V=1) before returning hdom_id to the IDA.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 10 ++++
>  drivers/iommu/amd/amd_viommu.h      |  8 ++++
>  drivers/iommu/amd/nested.c          | 74 +++++++++++++++++++++++++----
>  drivers/iommu/amd/viommu.c          | 41 ++++++++++++++++
>  4 files changed, 125 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h


.../...

> diff --git a/drivers/iommu/amd/amd_viommu.h b/drivers/iommu/amd/amd_viommu.h
> index 1022ae20c170..30227a5e4bfa 100644
> --- a/drivers/iommu/amd/amd_viommu.h
> +++ b/drivers/iommu/amd/amd_viommu.h
> @@ -23,6 +23,9 @@ int amd_viommu_init_one(struct amd_iommu *iommu, struct amd_iommu_viommu *viommu
>  
>  void amd_viommu_uninit_one(struct amd_iommu *iommu, struct amd_iommu_viommu *viommu);
>  
> +void amd_viommu_domain_id_update_locked(struct amd_iommu_viommu *aviommu,
> +					u16 hdom_id, u16 gdom_id);
> +
>  void amd_viommu_set_device_mapping(struct amd_iommu_viommu *aviommu,
>  				   u16 hdev_id, u16 gdev_id);
>  #else
> @@ -51,6 +54,11 @@ static inline void amd_viommu_set_device_mapping(struct amd_iommu_viommu *aviomm
>  {
>  }
>  
> +static inline void amd_viommu_domain_id_update_locked(struct amd_iommu_viommu *aviommu,
> +						      u16 hdom_id, u16 gdom_id)
> +{
> +}
> +

Do we need this ? Because this func gets called only when IOMMUFD is enabled.


>  #endif /* CONFIG_AMD_IOMMU_IOMMUFD */
>  
>  #endif /* AMD_VIOMMU_H */
> diff --git a/drivers/iommu/amd/nested.c b/drivers/iommu/amd/nested.c
> index 8bb0d1596c6c..ecdcecdc0bb7 100644
> --- a/drivers/iommu/amd/nested.c
> +++ b/drivers/iommu/amd/nested.c
> @@ -10,6 +10,7 @@



.../...

>  }
>  
>  static int nested_attach_device(struct iommu_domain *dom, struct device *dev,
>  				struct iommu_domain *old)
>  {
>  	struct dev_table_entry new = {0};
> +	struct nested_domain *ndom = to_ndomain(dom);
>  	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;
> @@ -251,10 +268,23 @@ static int nested_attach_device(struct iommu_domain *dom, struct device *dev,
>  
>  	mutex_lock(&dev_data->mutex);
>  
> -	set_dte_nested(iommu, dom, dev_data, &new);
> +	ret = set_dte_nested(iommu, dom, dev_data, &new);
> +	if (ret)
> +		goto out_err;
>  
> +	/*
> +	 * Program gdom_id -> hdom_id before the DTE is live with
> +	 * DTE_VIOMMU_EN. Hold vfctrl_lock across both so a concurrent
> +	 * last-ref free cannot restore the idle map in between.
> +	 */
> +	mutex_lock(&ndom->viommu->vfctrl_lock);
> +	amd_viommu_domain_id_update_locked(ndom->viommu,
> +					   ndom->gdom_info->hdom_id,
> +					   ndom->gdom_id);
>  	amd_iommu_update_dte(iommu, dev_data, &new);

Shouldn't we keep update_dte outside vfctl_lock? We have dev_lock to cover dte
update.

Also as Jason commented we should probably move vfctrl update to hdom_id
allocation path.


> +	mutex_unlock(&ndom->viommu->vfctrl_lock);
>  
> +out_err:
>  	mutex_unlock(&dev_data->mutex);
>  
>  	return ret;
> @@ -267,10 +297,18 @@ static void nested_domain_free(struct iommu_domain *dom)
>  	struct nested_domain *ndom __free(kfree) = to_ndomain(dom);
>  	struct amd_iommu_viommu *aviommu = ndom->viommu;
>  
> +	/*
> +	 * vfctrl_lock then gdomid_array: keep the slot empty in the
> +	 * xarray until CONTROL1 is idle so a concurrent alloc+attach
> +	 * for this gdom_id cannot publish H2 and then lose it to this

In an ideal scenario guest/VMM will not re-use same gdom_id until control goes
from back free path right?

-Vasant