Re: arm-smmu-v3 sharing SID

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2023/5/18 21:04, Jean-Philippe Brucker wrote:
> On Thu, May 18, 2023 at 11:44:56AM +0000, Peng Fan wrote:
>>> Subject: Re: arm-smmu-v3 sharing SID
>>>
>>> On 2023-05-18 11:46, Peng Fan wrote:
>>>> Hi all,
>>>>
>>>> Current arm-smmu-v3 driver does not support sharing SID, If there are
>>>> two devices sharing one SID, the smmu-v3 driver will report "stream x
>>>> already in tree".
>>>>
>>>> We have an SOC that using smmu-v3, only supports limited SIDs. From my
>>>> understanding, the smmu-v3 hardware supports SID sharing, but linux
>>>> driver not support that.
>>>
>>> Ugh, we've always hoped that nobody would build such a thing, since
>>> SMMUv3 allows for plenty of stream IDs for any reasonable system (Arm's
>>> implementations support at least 24 bits), and aliasing at the StreamID level
>>> does rather compromise the usefulness.
>>
>> We only support max 64 SIDs. So I am thinking to share SID, since we
>> have lots of dma channels.
>>
>>>
>>>> I would like know is it ok to add support for sharing SID?
>>>> Something like to use common rb_add/find, not
>>>> smmu-v3 driver local rb tree add/find.
>>>
>>> Not sure what you're thinking there - the StreamID tree is still private to the
>>> SMMU instance either way, and the existing arm_smmu_find_master()
>>> function is ideal for arm_smmu_device_group() to detect aliasing devices
>>> and group them appropriately. Then you also need some way to skip
>>> updating STEs that have already been touched for a previous device in the
>>> group (basically generalising what arm_smmu_install_ste_for_dev()
>>> currently does for duplicate StreamIDs owned by one device).
>>
>> Thanks for sharing insights. BTW, would you plan to add the support?
>>
>>>
>>> I'm fairly confident in reasoning about this for basic .attach_dev purposes,
>>> since I did implement the equivalent for SMMUv2, but I'm not sure I even
>>> want to think about what it would mean for SVA, PASIDs and nesting...
> 
> We should disable all these things for multi-device groups, it's not worth
> the headache. I think we used to but I can't find it anywhere. That said,
> I guess only PCIe PRI (not upstream) absolutely won't work with multiple
> devices sharing a SID, since page responses are routed back by RID to the
> endpoint.

I second that we only support pasid features with singleton group.

Previously we have below explicit check and comment in
iommu_sva_bind_device():

         /*
          * To keep things simple, SVA currently doesn't support IOMMU 
groups
          * with more than one device. Existing SVA-capable systems are not
          * affected by the problems that required IOMMU groups (lack of ACS
          * isolation, device ID aliasing and other hardware issues).
          */
         if (iommu_group_device_count(group) != 1)
                 goto out_unlock;

This fact still holds true today, but the check has been moved to
pci_enable_pasid():

         if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
                 return -EINVAL;

And the implementation of iommu_attach/detach_device_pasid() is quite
confusing. The pasid domains are stored in the iommu_group. The
attach/detach_device_pasid interfaces iterate all devices in the group
and set pasid domain to each device of the group.

This creates an illusion that the iommu core still insists on supporting
pasid on multiple device groups.

> Stall and by extension SVA can't really be supported with multiple devices
> per SID either, but that's mainly a software complexity issue (we'd need
> to guess which device it comes from, or report the fault for all devices).
> PASID and nesting might work transparently due to iommu_group, though I'd
> rather not think about that either.
> 
> It may boil down to updating the smmu->streams structure to support
> multiple masters per stream (maybe simplify the driver first by moving to
> a xarray [1]). That would provide a refcount for each SID and allow to
> only write the STE once on setup and teardown. Since arm_smmu_find_master()
> is only used to handle I/O page faults (stall/PRI which we won't support
> in this case), it could return NULL for multi-master streams.
> 
> Thanks,
> Jean
> 
> [1] https://lore.kernel.org/linux-iommu/ecb3725c-27c4-944b-b42c-f4e293521f94@arm.com/
> 

Best regards,
baolu