Re: [PATCH RFC v6 5/5] iommu/arm-smmu-v3: Wire up shared-SID STE ordering and feature gating

[Robin Murphy <robin.murphy@arm.com>]

On 13/10/2026 12:54 am, Nicolin Chen wrote:
> On Mon, Oct 12, 2026 at 09:23:32PM +0800, Peng Fan wrote:
>>> +static bool arm_smmu_skip_shared_ste(struct arm_smmu_device *smmu,
>>> +				     u32 sid, bool is_abort)
>>> +{
>>> +	struct arm_smmu_stream *stream;
>>> +
>>> +	lockdep_assert_held(&smmu->streams_mutex);
>>> +
>>> +	stream = arm_smmu_find_stream(smmu, sid);
>>> +	if (!stream || list_empty(&stream->shared_masters))
>>> +		return false;
>>> +
>>> +	if (is_abort) {
>>> +		stream->ste_installed = false;
>>> +		return true;
>>> +	}
>>> +	if (stream->ste_installed)
>>> +		return true;
>>> +	stream->ste_installed = true;
>>> +	return false;
>>
>> sashiko has comments: https://sashiko.dev/#/patchset/20261011-smmu-shared-sid-v6-0-cb76b89dc612%40nxp.com
>>
>> We just need to support a simple case that platform devices(single SID) which
>> could share there SIDs, such as MMC-0 and MMC-1 share one SID.
>>
>> Shared-SID masters are restricted to platform devices sharing a single SID.
>> SVA, IOPF, and vSMMU nesting are all rejected for shared-SID masters (patch 5),
>> so VFIO passthrough and direct domain replacement scenarios do not apply.
>> The only domain transitions are the default DMA domain attach at probe time
>> and ABORT on unbind. Or I may miss something.
>   
> Domain can be switched between DMA and IDENTITY via sysfs.
> 
> I think there should be a way to support that. But, without a concrete
> use case, it might be easier to add this into the attach_dev callbacks
> (except arm_smmu_attach_dev_blocked):
> 
> 	if (arm_smmu_master_shares_sid(master) &&
> 	    old_domain && old_domain != domain &&
> 	    old_domain != &arm_smmu_blocked_domain)
> 		return -EBUSY;
> 
> And Sashiko's second remark does reveal a real bug:
> "Additionally, when devices are detached from a domain, is_abort is true.
>   arm_smmu_skip_shared_ste() clears ste_installed but unconditionally returns
>   true for all masters. Will this cause the ABORT STE to never be written to
>   hardware?"
> 
> Given that your use case always group SID-sharing masters in the same
> iommu_group and attach/replace a domain on those masters together, I
> think a simpler solution can be to always let stream->master install
> the shared STE.
> 
> One pitfall would be the release domain's attach_dev, which would go
> through arm_smmu_attach_dev_blocked() just like a blocked domain. So,
> it has to skip correctly if the shared stream->master == master. This
> means we might have to add a separate arm_smmu_attach_dev_release().

Hmm, but in fact why are we worrying about skipping things at all? Yes 
originally trying to rewrite a "live" STE was a problem, but since 
Jason's redesign, isn't arm_smmu_write_ste() just going to see that the 
"new" entry is identical to what's already there and happily do nothing?

> And I realized that there is another problem in your design: the CD
> table is still allocated per master; see arm_smmu_alloc_cd_ptr(). If
> STE is shared, its CD table must be shared too.

If we're not going to support PASID for shared streams (which I suggest 
we don't...) than we shouldn't strictly need to worry about CD tables 
anyway. So rather than having to special-case CD table sharing for 
shared streams, perhaps it might make sense to generalise back to a 
"default" per-domain CD for non-PASID use, which can then get switched 
for the private per-stream cd_table if and when a device does enable PASID?

Thanks,
Robin.

> You'd need to redirect all the master->cd_table with something like:
> 
> static struct arm_smmu_master *arm_smmu_cd_owner(struct arm_smmu_master *master)
> {
> 	if (arm_smmu_master_shares_sid(master)) {
> 		int i;
> 
> 		for (i = 0; i < master->num_streams; i++) {
> 			struct arm_smmu_stream *stream = master->streams[i];
> 
> 			if (!list_empty(&stream->shared_masters))
> 				return stream->master;
> 		}
> 	}
> 
> 	return master;
> }
> 
>>> @@ -4360,9 +4391,11 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
>> I am thinking to simplify reject if stall_enabled is true:
>>   			if (list_empty(&stream->shared_masters)) {
>> +				if (stream->master->stall_enabled) {
>> +					dev_warn(master->dev,
>> +						 "Shared StreamID 0x%x rejected: existing master has stall enabled\n",
>> +						 sid);
>> +					ret = -EBUSY;
>> +					break;
>> +				}
>   
> I'd move it upwards: next to the existing "num_streams != 1" guard.
> 
> Nicolin