[v1,0/5] Add PASID support to SMMUv3 unmanaged domains

  Hi all,

This patch series refactors the arm-smmu-v3 driver and implements the
set_dev_pasid functionality for DMA and UNMANAGED iommu domains. As part
of this effort, we also refactor the arm-smmu-v3 driver such that each
iommu domain represent a single address space. In particular, stage 1
domains hold a single ContextDescriptor instead of the entire STE entry.

The refactor is arguably valuable independently from the set_dev_pasid
feature since an iommu_domain is conceptually closer to a single address
space than an entire STE. In addition this unlocks some nice clean-up of
the arm SVA implementation which today piggybacks SVA domains on the
"primary" domain.

This patch series makes some changes to SVA and PCIe, but I haven't
tested those features. The cd table allocations could also be further
optimized for masters that don't support multiple context.  However, the
SMMUv3 Nested translation patch series has me worried about this
change. At a glance, it seems like this refactor conflicts with its
proposed uAPI. Is this refactor no longer an appropriate clean-up or
path forward for set_dev_pasid support? Or could a uAPI that only
exposes a single CD instead of the entire STE be an appropriate fit for
the nesting use cases?

Thanks,
Michael Shavit

Link: https://lore.kernel.org/all/CAKHBV24u9b-=cJCF=Kt=3B4hynOyNr6gmi0F6zpO6b1mHc0Z7g@mail.gmail.com
Link: https://lore.kernel.org/all/cover.1683688960.git.nicolinc@nvidia.com/

Michael Shavit (5):
  iommu/arm-smmu-v3: Move cdtable to arm_smmu_master
  iommu/arm-smmu-v3: Add has_stage1 field
  iommu/arm-smmu-v3: Simplify arm_smmu_enable_ats
  iommu/arm-smmu-v3: Keep track of attached ssids
  iommu/arm-smmu-v3: Implement set_dev_pasid

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  46 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 432 ++++++++++++------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  44 +-
 3 files changed, 344 insertions(+), 178 deletions(-)
  

On Thu, May 11, 2023 at 04:50:47AM +0800, Michael Shavit wrote:
> Hi all,
> 
> This patch series refactors the arm-smmu-v3 driver and implements the
> set_dev_pasid functionality for DMA and UNMANAGED iommu domains. As part
> of this effort, we also refactor the arm-smmu-v3 driver such that each
> iommu domain represent a single address space. In particular, stage 1
> domains hold a single ContextDescriptor instead of the entire STE entry.

I'm not sure what you mean "holds" a ContextDescriptor?

Ideally the iommu_domain should only hold a pointer to some table
top. Depending on the domain type this would be a S1 IOPTE table, S2
IOPTE table or a CD table. Plus the non-table domains like IDENTITY
and blocked.

Logically when an iommu_domain is attached to a device or a PASID a
STE or CD is generated from the iommu_domain's configuration but the
iommu_domain doesn't "hold" it

When a kernel-owned CD table is used it should be held someplace else,
certianly not in the iommu_domain. Logically as a per-device
structure, but maybe with optimizations for sharing.

> The refactor is arguably valuable independently from the set_dev_pasid
> feature since an iommu_domain is conceptually closer to a single address
> space than an entire STE. In addition this unlocks some nice clean-up of
> the arm SVA implementation which today piggybacks SVA domains on the
> "primary" domain.

I always thought of this as sort of a pre-calculation of the STE, that
gets cached in the iommu_domain? Not sure the pre-calculation is that
valuable though..
 
> path forward for set_dev_pasid support? Or could a uAPI that only
> exposes a single CD instead of the entire STE be an appropriate fit for
> the nesting use cases?

The uAPI is to create an iommu_domain that holds a CD Table Top
located in user memory, it cannot deviate from this. These kinds of
iommu_domain's can only be pointed at by STEs.

Again it doesnt really "hold" the STE, but we can compute a STE that
points to the SD Table that it does hold.

Other than this, it is good to take this project on, getting PASID
support aligned with the new API is something that needs to be done
here!

Thanks,
Jason
  

> Logically when an iommu_domain is attached to a device or a PASID a
> STE or CD is generated from the iommu_domain's configuration but the
> iommu_domain doesn't "hold" it
>

Ah yes, I was using iommu domain and arm_smmu_domain interchangeably
here since there's a 1:1 mapping between the two. In the current
smmuv3 implementation, arm_smmu_domain holds the s1cfg structure which
represents the s1 portion of an STE.

> I always thought of this as sort of a pre-calculation of the STE, that
> gets cached in the iommu_domain? Not sure the pre-calculation is that
> valuable though..

I think we can consider the case where an iommu domain is attached to
multiple masters as a form of pre-calculation and resource sharing.
When attaching an iommu domain that has been attached before, its
arm_smmu_domain contains an already finalized STE configuration that
can be immediately written to the smmu. I don't think there's anything
specific to SVA about this behavior however, SVA will do the same
amount of work whether the cd table is owned by some special iommu
domain or by the arm_smmu_master (since we require that special iommu
domain be attached to the master and disallow detaching it).

> Ideally the iommu_domain should only hold a pointer to some table
top. Depending on the domain type this would be a S1 IOPTE table, S2
IOPTE table or a CD table. Plus the non-table domains like IDENTITY
and blocked.

Gotcha. So this patch series should be less aggressive, but is
probably still workable with the nested domain patch series:
1. For (stage 1) unmanaged/dma and sva domains, arm_smmu_domains
should hold a single CD. For the nested domain series, arm_smmu_domain
can alternatively hold an entire s1cfg.
2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
used by unmanaged/dma and sva domains attached to this master.
3. arm_smmu_master also holds a pointer to the live s1cfg, which may
either points to its owned s1cfg, or the nested domain's s1cfg, or
null (bypass or stage2)







> > path forward for set_dev_pasid support? Or could a uAPI that only
> > exposes a single CD instead of the entire STE be an appropriate fit for
> > the nesting use cases?
>
> The uAPI is to create an iommu_domain that holds a CD Table Top
> located in user memory, it cannot deviate from this. These kinds of
> iommu_domain's can only be pointed at by STEs.
>
> Again it doesnt really "hold" the STE, but we can compute a STE that
> points to the SD Table that it does hold.
>
> Other than this, it is good to take this project on, getting PASID
> support aligned with the new API is something that needs to be done
> here!
>
> Thanks,
> Jason
  

On Thu, May 11, 2023 at 11:52:41AM +0800, Michael Shavit wrote:
> > Logically when an iommu_domain is attached to a device or a PASID a
> > STE or CD is generated from the iommu_domain's configuration but the
> > iommu_domain doesn't "hold" it
> >
> 
> Ah yes, I was using iommu domain and arm_smmu_domain interchangeably
> here since there's a 1:1 mapping between the two. In the current
> smmuv3 implementation, arm_smmu_domain holds the s1cfg structure which
> represents the s1 portion of an STE.

I mean to include the arm_smmu_domain too..

Generally this sort of seemed OK in the code, I just wouldn't use the
word 'hold' - the iommu_domain may cache a computed STE or CD value
but that the actual steering or context tables are held else where

ie you insert an iommu_domain into a steering or context table, it
does not 'hold' a table entry.

> specific to SVA about this behavior however, SVA will do the same
> amount of work whether the cd table is owned by some special iommu
> domain or by the arm_smmu_master (since we require that special iommu
> domain be attached to the master and disallow detaching it).

The CD table for SVA definately should not be part of an iommu_domain,
moving it to the master seems reasonable.
 
> Gotcha. So this patch series should be less aggressive, but is
> probably still workable with the nested domain patch series:
> 1. For (stage 1) unmanaged/dma and sva domains, arm_smmu_domains
> should hold a single CD. For the nested domain series, arm_smmu_domain
> can alternatively hold an entire s1cfg.

These are just pre computed values the can help when inserting the
iommu_domain into a steering or CD table

> 2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
> used by unmanaged/dma and sva domains attached to this master.

The arm_smmu_master's cd table can be inserted into a steering table

> 3. arm_smmu_master also holds a pointer to the live s1cfg, which may
> either points to its owned s1cfg, or the nested domain's s1cfg, or
> null (bypass or stage2)

The steering table either points to the CD table owned by the
arm_smmu_master, a S1 domain held by an iommu_domain, or a S1 & CD
table owned by userspace represented by a special nested iommu_domain
and its internal parent.

If a kernel owned S2 it attached then the S1 points at the CD table
owned by the arm_smmu_master and the CD table points to the S2, same
as if there was PASID (IIRC, from memory I don't have the spec here
right now)

Think about it in terms of what object owns the table and what other
object(s) are inserted into the the table. Nothing "holds" a table
entry.

Jason
  

On 2023-05-11 05:33, Jason Gunthorpe wrote:
> On Thu, May 11, 2023 at 11:52:41AM +0800, Michael Shavit wrote:
>>> Logically when an iommu_domain is attached to a device or a PASID a
>>> STE or CD is generated from the iommu_domain's configuration but the
>>> iommu_domain doesn't "hold" it
>>>
>>
>> Ah yes, I was using iommu domain and arm_smmu_domain interchangeably
>> here since there's a 1:1 mapping between the two. In the current
>> smmuv3 implementation, arm_smmu_domain holds the s1cfg structure which
>> represents the s1 portion of an STE.
> 
> I mean to include the arm_smmu_domain too..
> 
> Generally this sort of seemed OK in the code, I just wouldn't use the
> word 'hold' - the iommu_domain may cache a computed STE or CD value
> but that the actual steering or context tables are held else where
> 
> ie you insert an iommu_domain into a steering or context table, it
> does not 'hold' a table entry.
> 
>> specific to SVA about this behavior however, SVA will do the same
>> amount of work whether the cd table is owned by some special iommu
>> domain or by the arm_smmu_master (since we require that special iommu
>> domain be attached to the master and disallow detaching it).
> 
> The CD table for SVA definately should not be part of an iommu_domain,
> moving it to the master seems reasonable.
>   
>> Gotcha. So this patch series should be less aggressive, but is
>> probably still workable with the nested domain patch series:
>> 1. For (stage 1) unmanaged/dma and sva domains, arm_smmu_domains
>> should hold a single CD. For the nested domain series, arm_smmu_domain
>> can alternatively hold an entire s1cfg.
> 
> These are just pre computed values the can help when inserting the
> iommu_domain into a steering or CD table

Right, a stage 1 domain still logically owns the *contents* of a 
corresponding CD, however in this design it can no longer own a physical 
CD structure, because now every device attached to the same domain must 
maintain its own distinct copy.

>> 2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
>> used by unmanaged/dma and sva domains attached to this master.
> 
> The arm_smmu_master's cd table can be inserted into a steering table

Not sure what you mean there... STE.S1ContextPtr is essentially just a 
pointer to an array of CD structures (which only contains 1 element when 
PASIDs aren't enabled), so every master must own its own CD table 
directly. There is no viable indirection if you want the abstraction to 
bear any relation to reality.

>> 3. arm_smmu_master also holds a pointer to the live s1cfg, which may
>> either points to its owned s1cfg, or the nested domain's s1cfg, or
>> null (bypass or stage2)
> 
> The steering table either points to the CD table owned by the
> arm_smmu_master, a S1 domain held by an iommu_domain, or a S1 & CD
> table owned by userspace represented by a special nested iommu_domain
> and its internal parent.
> 
> If a kernel owned S2 it attached then the S1 points at the CD table
> owned by the arm_smmu_master and the CD table points to the S2, same
> as if there was PASID (IIRC, from memory I don't have the spec here
> right now)

Nope, CDs *only* represent stage 1 domains, stage 2 configuration is a 
property of the STE, i.e. is directly connected to the arm_smmu_master.

Thanks,
Robin.
  

On Thu, May 11, 2023 at 01:33:58PM +0100, Robin Murphy wrote:
> > > 2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
> > > used by unmanaged/dma and sva domains attached to this master.
> > 
> > The arm_smmu_master's cd table can be inserted into a steering table
> 
> Not sure what you mean there... STE.S1ContextPtr is essentially just a
> pointer to an array of CD structures (which only contains 1 element when
> PASIDs aren't enabled), so every master must own its own CD table directly.
> There is no viable indirection if you want the abstraction to bear any
> relation to reality.

Yes, this is what I mean. Whenever we need a kernel owned CD table it
comes from the smmu master and is inserted into the steering table
owned by the arm_smmu_device.

"Insert" is just the usual verb we tend to use when talking about
these kinds of structures. Ie a PTE is inserted into a page table and
points at a page - a page table doesn't hold a PTE owned by the page.

So we have, basically, three kinds of tables, Steering/CD/IOPTE, they
are owned by their respective objects
arm_smmu_device/arm_smmu_master/arm_smmu_domain

And we insert pointers from Steering -> CD -> IOPTE as appropriate.

The only case a CD table is not in the arm_smmu_master is for nesting,
but we can still say that the nesting domain is inserted into the
steering table.

Jason