Re: [PATCH v6 21/24] iommu/amd: Add translation DTE and VFctrl TransDevID helpers

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

On 9/30/2026 2:53 PM, Suravee Suthikulpanit wrote:
> vIOMMU GPA->SPA walks use a per-vIOMMU TransDevID: VFCTRL guest-misc
> names the slot, and that host DTE points at the nest-parent v1 table.
> 
> Add amd_iommu_set_translate_dte() and amd_iommu_clear_translate_dte()
> to program that DTE, owned on amd_iommu_viommu.trans_dev_data rather
> than pci_seg->dev_data_xa. Rewrite it from amd_iommu_change_top() via
> pdom->viommu_list when the nest-parent table grows.
> 
> Add amd_iommu_update_vfctrl_mmio_translate_devid() to publish the
> TransDevID in VFCTRL guest-misc MMIO.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h       |   7 ++
>  drivers/iommu/amd/amd_iommu_types.h |   4 +
>  drivers/iommu/amd/iommu.c           | 153 ++++++++++++++++++++++++----
>  3 files changed, 142 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 6ac64524208b..4aede6f6d489 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -213,6 +213,13 @@ void amd_iommu_update_dte(struct amd_iommu *iommu,
>  			  struct iommu_dev_data *dev_data,
>  			  struct dev_table_entry *new);
>  
> +int amd_iommu_set_translate_dte(struct iommufd_viommu *viommu);
> +void amd_iommu_clear_translate_dte(struct amd_iommu *iommu,
> +				   struct iommu_dev_data *dev_data,
> +				   u16 trans_devid);
> +void amd_iommu_update_vfctrl_mmio_translate_devid(struct amd_iommu *iommu,
> +						  u16 gid, u32 trans_devid);
> +
>  static inline void
>  amd_iommu_make_clear_dte(struct amd_iommu *iommu, u16 devid,
>  			 struct dev_table_entry *new)
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 6b3a43952797..5f84f25e3f37 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -489,6 +489,7 @@ extern bool amdr_ivrs_remap_support;
>  /* VIOMMU stuff */
>  #define VIOMMU_VF_MMIO_ENTRY_SIZE		4096
>  #define VIOMMU_VFCTRL_MMIO_ENTRY_SIZE		64
> +#define VIOMMU_VFCTRL_GUEST_MISC_CONTROL_OFFSET	0x10
>  
>  /* Host ioremap/request_mem_region sizes for VF / VF_CNTL BARs */
>  #define VIOMMU_VF_MMIO_MAP_SIZE		0x10000000UL
> @@ -551,6 +552,9 @@ struct amd_iommu_viommu {
>  
>  	u64 *devid_table;
>  	u64 *domid_table;
> +	u16 trans_devid;
> +	/* Synthetic DTE; not stored in pci_seg->dev_data_xa */
> +	struct iommu_dev_data *trans_dev_data;
>  
>  	/*
>  	 * Serializes VFCTRL Guest DID map CONTROL0/CONTROL1 doorbells
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 65550b070a12..8943863105b6 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -148,11 +148,11 @@ static void write_dte_lower128(struct dev_table_entry *ptr, struct dev_table_ent
>   * So, only consider control bits related to DMA when updating the entry.
>   */
>  static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
> -			  struct dev_table_entry *new)
> +			  u16 devid, struct dev_table_entry *new)

Why this change? dev_data always contains devid right?

>  {
>  	unsigned long flags;
>  	struct dev_table_entry *dev_table = get_dev_table(iommu);
> -	struct dev_table_entry *ptr = &dev_table[dev_data->devid];
> +	struct dev_table_entry *ptr = &dev_table[devid];
>  
>  	spin_lock_irqsave(&dev_data->dte_lock, flags);

.../...

>   */
>  static void update_viommu_self_dte(struct protection_domain *pdom,
>  				   phys_addr_t top_paddr,
>  				   unsigned int top_level)
>  {
>  	struct pdom_iommu_info *pdom_iommu_info;
> +	struct amd_iommu_viommu *aviommu;
>  	unsigned long i;
>  
>  	lockdep_assert_held(&pdom->lock);
> @@ -2884,6 +2893,17 @@ static void update_viommu_self_dte(struct protection_domain *pdom,
>  		set_dte_entry(iommu, iommu->viommu_dev_data, top_paddr,
>  			      top_level);
>  	}
> +
> +	list_for_each_entry(aviommu, &pdom->viommu_list, pdom_list) {
> +		struct amd_iommu *iommu;
> +
> +		if (!aviommu->trans_dev_data)
> +			continue;
> +		iommu = container_of(aviommu->core.iommu_dev,
> +				     struct amd_iommu, iommu);
> +		set_dte_entry(iommu, aviommu->trans_dev_data, top_paddr,
> +			      top_level);
> +	}

These two are operating on different page table. So its better to have it in
separate function.

>  }
>  #else
>  static inline void update_viommu_self_dte(struct protection_domain *pdom,
> @@ -3463,6 +3483,95 @@ static bool amd_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>  	return true;
>  }
>  
> +#if IS_ENABLED(CONFIG_AMD_IOMMU_IOMMUFD)

why not move this functions to trans_devid.c ?

> +
> +void amd_iommu_update_vfctrl_mmio_translate_devid(struct amd_iommu *iommu,
> +						  u16 gid, u32 devid)
> +{
> +	writeq((devid & 0xFFFFULL) << 16,
> +	       VIOMMU_VFCTRL_MMIO_BASE(iommu, gid) +
> +	       VIOMMU_VFCTRL_GUEST_MISC_CONTROL_OFFSET);
> +}
> +
> +int amd_iommu_set_translate_dte(struct iommufd_viommu *viommu)
> +{
> +	struct amd_iommu_viommu *aviommu =
> +		container_of(viommu, struct amd_iommu_viommu, core);
> +	struct amd_iommu *iommu =
> +		container_of(viommu->iommu_dev, struct amd_iommu, iommu);
> +	struct protection_domain *pdom = aviommu->parent;
> +	u16 gid = aviommu->gid;
> +	u16 trans_devid = aviommu->trans_devid;
> +	struct iommu_dev_data *trans_dev_data;
> +	unsigned long flags;
> +
> +	if (!aviommu->trans_dev_data) {
> +		aviommu->trans_dev_data =
> +			amd_iommu_alloc_dev_data(trans_devid);
> +		if (!aviommu->trans_dev_data) {
> +			pr_err("%s: Failed to allocate dev_data for translate-device-id %#x\n",
> +			       __func__, trans_devid);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	trans_dev_data = aviommu->trans_dev_data;
> +	trans_dev_data->dev = NULL;
> +	trans_dev_data->domain = pdom;
> +	/*
> +	 * Leave off pdom->dev_list: this is not an IOMMU-API attach.
> +	 * Hold pdom->lock so change_top() cannot grow the nest-parent
> +	 * top between the sample and the DTE commit. Publish devid
> +	 * under that lock; it is the live TransDevID for
> +	 * update_viommu_self_dte(). If this vIOMMU is not yet on
> +	 * viommu_list, add it before dropping the lock so a concurrent
> +	 * increase_top() cannot miss the new DTE.
> +	 */
> +
> +	spin_lock_irqsave(&pdom->lock, flags);
> +	trans_dev_data->devid = trans_devid;
> +	set_dte_entry(iommu, trans_dev_data, 0, 0);
> +	if (list_empty(&aviommu->pdom_list))
> +		list_add(&aviommu->pdom_list, &pdom->viommu_list);

Already updated in amd_iommufd_viommu_init() ?

> +	spin_unlock_irqrestore(&pdom->lock, flags);
> +
> +	pr_debug("%s: gid=%#x, iommu_devid=%#x, devid=%#x\n",
> +		 __func__, gid, iommu->devid, trans_devid);
> +	return 0;
> +}
> +
> +static void amd_iommu_update_dte_slot(struct amd_iommu *iommu,
> +				      struct iommu_dev_data *dev_data,
> +				      u16 devid, struct dev_table_entry *new)
> +{> +	update_dte256(iommu, dev_data, devid, new);
> +	if (dev_data->dev)
> +		clone_aliases(iommu, dev_data->dev);

We know there is no alias device for translation dte right?

Actually why not call amd_iommu_update_dte() . It calls device_flush_dte(), but
for translation dte we are not setting ats_enabled flag.

-Vasant