Re: [PATCH v6 22/24] iommu/amd: Add translate-device-id alloc/free with vIOMMU owner

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

On 9/30/2026 2:53 PM, Suravee Suthikulpanit wrote:
> Extend the per-segment translate-device-id pool to record the owning
> struct amd_iommu_viommu in the xarray.  Add amd_iommu_trans_devid_alloc()
> and amd_iommu_trans_devid_free() to allocate ids from last_bdf down
> to 0 so each id indexes the device and alias tables, then return them
> on teardown.  Free loads the xarray slot and erases it only when the
> owner matches @aviommu.

AI (Claude Opus 5.5) generated comment (seems correct, copy-pasting as is)


The pool marks a RID as used only if it went through
amd_iommu_probe_device() (plus its PCI DMA aliases) or is the IOMMU
itself.  Are there RIDs the hardware uses that never reach
probe_device()?  For example:

  - IOAPIC/HPET RIDs from IVRS special device entries


With this patch there are two per-segment xarrays indexed by the same
thing, a device table slot: pci_seg->dev_data_xa and
pci_seg->trans_devid_xa.  The translate dev_data and the IOMMU's own
dev_data are kept out of dev_data_xa, and that breaks the DTE locking.
The interrupt remapping code finds the DTE lock by devid:

        static void set_dte_irq_entry(struct amd_iommu *iommu, u16 devid,
                                      struct irq_remap_table *table)
        {
                ...
                struct iommu_dev_data *dev_data = search_dev_data(iommu, devid);

                if (dev_data)
                        spin_lock(&dev_data->dte_lock);

                new = READ_ONCE(dte->data[2]);
                ...
                WRITE_ONCE(dte->data[2], new);

For a translate slot search_dev_data() returns NULL, so this
read-modify-write of data[2] runs without a lock while
write_dte_upper128() may be updating the same word under
trans_dev_data->dte_lock.  That can happen as soon as the pool hands
out a RID that also has an IRQ remapping table, such as an IOAPIC or
HPET RID that is not reserved today.

Could the two xarrays be merged into a single per-segment "DTE slot
owner" table? Something like:

  empty                       free, the translate allocator may use it
  xa_mk_value(RESERVED)       used by hardware but has no dev_data
                              (IVRS IOAPIC/HPET/ACPIHID, IVHD entries
                              not yet enumerated)
  struct iommu_dev_data *     the driver programs this DTE; an owner
                              field says host device, alias, IOMMU
                              self, or vIOMMU translate (with the
                              aviommu pointer)


My comment:

I did some analysis and may be having single xa_array makes it easy to maintain.
Any free slot in dev_data_xa is free for translation DTE. We can have additional
flag in dev_data to track the translation devid vs actual devid. So that we can
add checks to catch allocaton/free bugs. What do you think?


-Vasant