Re: [PATCH v2] xen/amd/iommu: record the IOMMU for alias DeviceIDs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.10.2026 00:59, Stefano Stabellini wrote:
> add_ivrs_mapping_entry() marks an alias DeviceID valid and may give it
> an interrupt remapping table, but records the IOMMU only for the device
> using the alias.  If no IVHD entry names the alias DeviceID itself,
> amd_iommu_setup_device_table() passes a NULL IOMMU to
> amd_iommu_set_intremap_table(), which dereferences it at boot.

Also possibly amd_iommu_alloc_intremap_table(), as it looks.

> Record the IOMMU for the alias too, as was done before the interrupt
> remapping rework.  Nothing relies on it being NULL: the alias is then
> handled like any other DeviceID.  Its remapping table is freed on
> cleanup and dumped by the 'V' key, a device at the alias DeviceID can
> be added instead of failing with -ENODEV, and reserved ranges, already
> recorded for the alias as well, are reported once under it.  A NULL
> check at the crash site instead would leave the alias device table
> entry without interrupt remapping.

What about find_iommu_for_device(), for example? That does check .iommu
to be non-NULL, and hence behavior there is being altered by the change.
Maybe to the better, but that's not obvious to me.
amd_iommu_get_reserved_device_memory() has a similar, related check.

> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -104,6 +104,10 @@ static void __init add_ivrs_mapping_entry(
>  
>      /* Assign IOMMU hardware. */
>      ivrs_mappings[bdf].iommu = iommu;
> +
> +    /* Requests using the alias go through the same IOMMU. */
> +    if ( !ivrs_mappings[alias_id].iommu )
> +        ivrs_mappings[alias_id].iommu = iommu;

Comment and code are out of sync as long as the assignment is conditional.
Question is which way this needs adjusting, and hence why you added the
conditional in the first place. Which extends to other aspects of this
function, when called with an alias_id (or bdf) which was previously seen:
Should we reject such (early in the function)? Should later data override
earlier one?

Perhaps what we want here is

    /* Requests using the alias go through the same IOMMU. */
    if ( ivrs_mappings[alias_id].iommu )
        ASSERT(ivrs_mappings[alias_id].iommu == iommu);
    ivrs_mappings[alias_id].iommu = iommu;

?

Furthermore, if there's an issue with .iommu, why would there not be the
same issue with the other fields set by this function? (Else if some of
the fields can be relied upon only when
ivrs_mappings[n].dte_requestor_id == n, then I think this would need
writing down somewhere.) See e.g. amd_iommu_setup_domain_device(): It
uses get_dma_requestor_id() to determine DTE and ivrs_dev, and passes
that ivrs_dev to use_ats(). If this is an alias, ivrs_dev->block_ats
won't be properly set.

Jan