Re: [PATCH v6 24/24] iommu/amd: Relocate vIOMMU translate-device-id on PCI reserve

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

On 9/30/2026 2:53 PM, Suravee Suthikulpanit wrote:
> When PCI probe reserves a BDF already allocated to a vIOMMU, relocate
> that vIOMMU to a newly allocated translate-device-id before marking the
> BDF reserved for the device. Reserve holds the segment mutex and
> trylocks trans_devid_lock so the vIOMMU pointer stays valid without
> reversing the nested lock order. Reprogram translation DTE and VFctrl
> under trans_devid_lock; roll back the pool on failure.
> 
> Mark from_id reserved with a raw xa_store so ALLOCATED->RESERVED does
> not WARN on the non-NULL owner. On MMIO failure, restore from_id the
> same way (RESERVED->owner) instead of install_allocated.
> 
> Reuse the vIOMMU's trans_dev_data object. After programming the new
> slot, clear the old DTE without freeing the object.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  10 +-
>  drivers/iommu/amd/trans_devid.c     | 146 +++++++++++++++++++++++++++-
>  2 files changed, 152 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index c748bab27509..347c6954439d 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -558,11 +558,15 @@ struct amd_iommu_viommu {
>  


.../...

>  
> +	aviommu = trans_devid_xa_owner(entry);
> +	if (aviommu) {
> +		/*
> +		 * Pin aviommu with trylock while the mutex still covers
> +		 * the xarray pointer. A blocking lock here would invert
> +		 * the nested order (ABBA with destroy). Dropping the
> +		 * mutex first would UAF if destroy frees aviommu.
> +		 *
> +		 * No retry bound. cond_resched() avoids a soft lockup;
> +		 * progress is this owner dropping trans_devid_lock or
> +		 * freeing the slot.
> +		 */
> +		if (!mutex_trylock(&aviommu->trans_devid_lock)) {
> +			mutex_unlock(&pci_seg->trans_devid_mutex);
> +			cond_resched();
> +			goto retry;
> +		}

AI comment (looks genuine, hence copy-pasting as is)
This is an unbounded trylock/retry loop in amd_iommu_probe_device(),
which runs under iommu_probe_device_lock.  Would a single per-segment
mutex covering both the pool and TransDevID/DTE programming remove the
lock ordering problem, and with it the trylock?  vIOMMU create and
destroy are not hot paths.

The reservation also runs too late in probe.  amd_iommu_probe_device()
calls iommu_init_device() first, which goes through setup_aliases() ->
clone_aliases() -> clone_alias().  clone_alias() reads the device's own
DTE and copies it into every alias slot.  Only afterwards does
amd_iommu_trans_devid_reserve() run and relocate a vIOMMU that owns one
of these RIDs.  Two cases look wrong:

  - The device's own RID is a live translate slot.  The VM's translate
    DTE (GPA->SPA) is cloned into the alias slots, and the aliases keep
    pointing at the VM's address space until the default domain is
    attached.

  - An alias RID is a live translate slot.  find_dev_data() creates a
    host dev_data for it and clone_alias() overwrites the VM's
    translate DTE under the new dev_data's dte_lock, not
    trans_dev_data->dte_lock.  Relocation then clears old_id, which
    wipes the alias DTE the host just wrote.

Shouldn't the reservation (and any relocation) complete before
iommu_init_device()?


-Vasant