arm-smmu-v3 sharing SID

arm-smmu-v3 sharing SID

[Peng Fan]

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.

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.

Thanks,
Peng.

Re: arm-smmu-v3 sharing SID

[Robin Murphy]

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.

> 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).

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...

Thanks,
Robin.

RE: arm-smmu-v3 sharing SID

[Peng Fan]

> 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...

Smmu-v3 is a bit new to me, I have not look into it so far (:
If you could help, I would be appreciated. 

Thanks,
Peng.
> 
> Thanks,
> Robin.

Re: arm-smmu-v3 sharing SID

[Jean-Philippe Brucker]

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.

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/

Re: arm-smmu-v3 sharing SID

[Baolu Lu]

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

Re: arm-smmu-v3 sharing SID

[Jason Gunthorpe]

On Thu, May 18, 2023 at 12:33:25PM +0100, Robin Murphy wrote:

> 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...

IIRC we've already taken the approach elsewhere to block PASID if the
group is not singleton.

Jason