Re: [PATCH] iommu/arm-smmu-v3: Allow stream table to have nodes with the same ID

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Apr 11, 2025 at 01:10:29PM +0100, Robin Murphy wrote:

> This is adding support for StreamID aliasing between devices, and as such it
> is incomplete. It's not OK to just allow devices to arbitrarily rewrite each
> other's STEs,

Okay, yes, we should be checking the iommu_group before permitting two
devices to share the STE. That is an easy fix, see below

> Aliases can only be permitted within a group, which means
> arm_smmu_device_group() also has to check and account for them in the first
> place - note that that applies to PCI devices as well, because as
> soon as we

On this system the alias come from the PCI DMA alias support and
pci_device_group() is already correctly grouping things.

Aliases from other places, like the IORT, never did work..

> allow StreamID aliasing at all then we're inherently allowing RID->SID
> mappings to alias outside the PCI hierarchy in ways that pci_device_group()
> can't know about. It should work out basically the same as SMMUv2, just with
> the streams tree in place of the S2CR array.

You mean the logic in v2's arm_smmu_device_group() to consult the
stream map to select the group if the IORT is creating aliases? Yes it
could be done..

However, this is a significant regression fix and I think we can be
confident there are no IORT tables in the wild that have aliases or
they would already be broken.

How about we add the missing validation that the group is the same,
that is easy to do and should be there anyhow:

static int arm_smmu_streams_cmp_node(struct rb_node *lhs,
				     const struct rb_node *rhs)
{
	struct arm_smmu_stream *stream_lhs =
		&rb_entry(lhs, struct arm_smmu_stream, node);
	struct arm_smmu_stream *stream_rhs =
		rb_entry(rhs, struct arm_smmu_stream, node);

	if (stream_lhs->id < stream_rhs->id)
		return -1;
	if (stream_lhs->id > stream_rhs->id)
		return 1;

	/*
	 * The stream table can have multiple nodes with the same ID if there
	 * are DMA aliases. If multiple masters share the same iommu group then
	 * they can use the overlapping STEs within the group.
	 */
	if (stream_lhs->master->dev->iommu_group ==
	    stream_rhs->master->dev->iommu_group) {
		if (stream_lhs < stream_rhs)
			return -1;
		if (stream_lhs > stream_rhs)
			return 1;
	}
	return 0;
}

That change will narrow this patch to only enable PCI DMA aliases that
already generate the correct iommu groupings. Other sources of alising
that don't generate the right groupings will continue to fail as they
do today.

Then I propose continuing to wait for a user before adding support for
more alias scenarios to arm_smmu_device_group()?

> > +	/*
> > +	 * If there are DMA alises then there are multiple devices with the same
> > +	 * stream ID and we cannot reliably convert from SID to master.
> > +	 */
> > +	if (node->rb_left &&
> > +	    rb_entry(node->rb_left, struct arm_smmu_stream, node)->id == sid)
> > +		return NULL;
> > +	if (node->rb_right &&
> > +	    rb_entry(node->rb_right, struct arm_smmu_stream, node)->id == sid)
> > +		return NULL;
> 
> This doesn't really work - the whole mechanism needs to fundamentally change
> to mapping StreamIDs to groups rather than to devices. Then it's really up
> to individual callers what they want to do if the group has more than one
> device.

There are only two callers. One is using it to print the log message,
in this case it will fall back to the unknown stream ID path and still
print a log message. This could perhaps print the group ID # instead
of the raw stream ID but I wouldn't do that in a regression fix rc
patch.

The other is doing stall/future PRI, and I don't think we should be
doing iommu_group based fault reporting at all. Returning NULL
effectively disables it.

Jason