[v4,02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

  Except for Nested domains, arm_smmu_master will own the STEs that are
inserted into the arm_smmu_device's STE table.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 28 +++++++++++++--------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
 2 files changed, 18 insertions(+), 11 deletions(-)
  

Hi Michael,

On Wed, Jun 21, 2023 at 02:37:14PM +0800, Michael Shavit wrote:
 
> Except for Nested domains, arm_smmu_master will own the STEs that are
> inserted into the arm_smmu_device's STE table.

I think that the master still owns an STE when attached to a
nested domain. Though an IOMMU_DOMAIN_NESTED iommu_domain is
an opaque object to the STE in the guest, the host still has
a real STE for the nested configuration somewhere -- and it's
likely still to be owned by the master that's attached to the
opaque NESTED iommu_domain in the host kernel.

> -static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain)
> +static int arm_smmu_init_s1_cfg(struct arm_smmu_master *master,
> +                               struct arm_smmu_s1_cfg *cfg)

We here pass in an s1_cfg ptr because we expect someone else
rather than the master could own the s1_cfg?

But the final codeline by the end of this series seems that
only master owns an s1_cfg. So perhaps we could re-organize
the patches to clean this away, as the cfg always comes from
a master?

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 68d519f21dbd8..053cc14c23969 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -688,6 +688,7 @@ struct arm_smmu_master {
>         struct arm_smmu_domain          *domain;
>         struct list_head                domain_head;
>         struct arm_smmu_stream          *streams;
> +       struct arm_smmu_s1_cfg          owned_s1_cfg;

I am a bit confused by this naming. If only master would own
an s1_cfg, perhaps we can just make it "s1_cfg" and drop the
s1_cfg pointer in the next patch.

Thanks
Nicolin
  

On Thu, Jul 13, 2023 at 9:22 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
>
> > Except for Nested domains, arm_smmu_master will own the STEs that are
> > inserted into the arm_smmu_device's STE table.
>
> I think that the master still owns an STE when attached to a
> nested domain. Though an IOMMU_DOMAIN_NESTED iommu_domain is
> an opaque object to the STE in the guest, the host still has
> a real STE for the nested configuration somewhere -- and it's
> likely still to be owned by the master that's attached to the
> opaque NESTED iommu_domain in the host kernel.

> I am a bit confused by this naming. If only master would own
> an s1_cfg, perhaps we can just make it "s1_cfg" and drop the
> s1_cfg pointer in the next patch.

Could be that the naming is causing some confusion. This owned_s1_cfg
is very different from the s1_cfg set-up by Nested domains in your
patch series. It's better to think of it as the default s1_cfg used
for DMA/SVA/UNMANAGED domains. Because stage 1 domains represent a
single page table, it doesn't make sense for them to own an entire CD
table. In contrast, nested domains map an entire CD table and it
therefore makes sense for them to own the s1_cfg representing that
table.
Would renaming this as default_s1_cfg make more sense?
  

On Thu, Jul 13, 2023 at 04:34:56PM +0800, Michael Shavit wrote:
> On Thu, Jul 13, 2023 at 9:22 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> >
> > > Except for Nested domains, arm_smmu_master will own the STEs that are
> > > inserted into the arm_smmu_device's STE table.
> >
> > I think that the master still owns an STE when attached to a
> > nested domain. Though an IOMMU_DOMAIN_NESTED iommu_domain is
> > an opaque object to the STE in the guest, the host still has
> > a real STE for the nested configuration somewhere -- and it's
> > likely still to be owned by the master that's attached to the
> > opaque NESTED iommu_domain in the host kernel.
> 
> > I am a bit confused by this naming. If only master would own
> > an s1_cfg, perhaps we can just make it "s1_cfg" and drop the
> > s1_cfg pointer in the next patch.
> 
> Could be that the naming is causing some confusion. This owned_s1_cfg
> is very different from the s1_cfg set-up by Nested domains in your
> patch series. It's better to think of it as the default s1_cfg used
> for DMA/SVA/UNMANAGED domains. Because stage 1 domains represent a
> single page table, it doesn't make sense for them to own an entire CD
> table. In contrast, nested domains map an entire CD table and it
> therefore makes sense for them to own the s1_cfg representing that
> table.
> Would renaming this as default_s1_cfg make more sense?

It would make alot more sense if the STE value used by an unmanaged S1
domain was located in/near the unmanaged domain or called 'unmanaged
S1 STE' or something if it really has to be in the master. Why does
this even need to be stored, can't we compute it?

Notice that we have unmanaged domains that use a CD and SVA domains
typically would be on a CD, so it is a bit weird already.

I'd think the basic mental model should be to extract the STE from the
thing you intend to install. Either the default CD table, or from the
iommu_domain. ie some 'get STE from iommu_domain' function?

There shouldn't be a concept of a "default" or "owned" STE value..

Jason
  

On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> It would make alot more sense if the STE value used by an unmanaged S1
> domain was located in/near the unmanaged domain or called 'unmanaged
> S1 STE' or something if it really has to be in the master. Why does
> this even need to be stored, can't we compute it?

struct s1_cfg* and struct s2_cfg* are precisely what is used to
compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
will write the s1_cfg's CD table dma_pointer into the STE's
STRTAB_STE_0_CFG field. When neither are set, the STE fields are
written to enable bypass (or abort depending on the config).

> I'd think the basic mental model should be to extract the STE from the
> thing you intend to install. Either the default CD table, or from the
> iommu_domain. ie some 'get STE from iommu_domain' function?

I don't follow this. When we attach a domain with pasid (whether
through SVA or the set_dev_pasid API) , we don't want to install an
entirely new CD table. We want to write something (page-table pointer)
to a common CD table. Where should the s1_cfg which owns that common
table live? I thought we concluded that it should be owned by the
arm_smmu_master rather than any domain (to avoid dependencies between
domains a-la aux-domain). With this change, even attach_dev with a DMA
or UNMANAGED domain is now just preparing a single entry into this
common CD table.
  

On Fri, Jul 14, 2023 at 12:16 AM Michael Shavit <mshavit@google.com> wrote:
> domains a-la aux-domain). With this change, even attach_dev with a DMA
> or UNMANAGED domain is now just preparing a single entry into this
> common CD table.

I have to correct this part, arm-smmu-v3.c's attach_dev()
implementation does both: it writes to the pasid=0 entry of the
owned_s1_cfg's CD table, and then installs that CD table to the STE.
  

On Fri, Jul 14, 2023 at 12:16:16AM +0800, Michael Shavit wrote:
> On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > It would make alot more sense if the STE value used by an unmanaged S1
> > domain was located in/near the unmanaged domain or called 'unmanaged
> > S1 STE' or something if it really has to be in the master. Why does
> > this even need to be stored, can't we compute it?
> 
> struct s1_cfg* and struct s2_cfg* are precisely what is used to
> compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
> will write the s1_cfg's CD table dma_pointer into the STE's
> STRTAB_STE_0_CFG field. When neither are set, the STE fields are
> written to enable bypass (or abort depending on the config).

I guess I never really understood why these were precomputed and
stored at all. Even more confusing is why we need to keep pointers to
them anywhere? Compute the STE and CDE directly from the source data
when you need it?

eg If I want to install an IDENITY domain into a STE then I compute
the STE for identity and go ahead and do it.

> > I'd think the basic mental model should be to extract the STE from the
> > thing you intend to install. Either the default CD table, or from the
> > iommu_domain. ie some 'get STE from iommu_domain' function?
> 
> I don't follow this. When we attach a domain with pasid (whether
> through SVA or the set_dev_pasid API) , we don't want to install an
> entirely new CD table. 

The master object owns an optional CD table. If it is exsists it is
used by every domain that is attached to that master.

In the code flow there are two entry points to attach a domain, attach
to a PASID or attach to a RID.

For attach to PASID the code should always force the master to have a
CD table and then attach the domain to the CD table.

For attach to RID the code should do a bunch of checks and decide if
it should force the master to have a CD table and attach the domain to
that, or directly attach the domain to the STE.

When the master gains a CD table then the CD table object becomes
attached to the STE. In all cases we should be able to point to the
object the STE points at and don't need a cfg or pointer to cfg since
the object itself can provide the cfg.

In all cases when you go to compute a STE you figure out what object
is attached to it (CD or domain), compute the correct STE for that
object, the set it. Same for he CDE, query the correct CDE from the
iommu_domain when you attach it to the table.

There should be no such thing as a "default" STE, and I question if it
makes sense to even precompute the s1/s2_cfg values during finalize at
all..

> We want to write something (page-table pointer) to a common CD
> table. Where should the s1_cfg which owns that common table live? 

I would suggest a 'cd table struct' that as all the stuff related to
the CD table, including an API to cacluate the STE this CD table
requires. If not in actual code with a real struct, then in a logical
sense in that a chunk of the master struct is the "CD table".

> I thought we concluded that it should be owned by the
> arm_smmu_master rather than any domain (to avoid dependencies
> between domains a-la aux-domain). 

Yes, I'm not saying anything against that, just how and where the STE
and CDE values flow around.

Jason
  

On Thu, Jul 13, 2023 at 01:41:38PM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 14, 2023 at 12:16:16AM +0800, Michael Shavit wrote:
> > On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > It would make alot more sense if the STE value used by an unmanaged S1
> > > domain was located in/near the unmanaged domain or called 'unmanaged
> > > S1 STE' or something if it really has to be in the master. Why does
> > > this even need to be stored, can't we compute it?
> > 
> > struct s1_cfg* and struct s2_cfg* are precisely what is used to
> > compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
> > will write the s1_cfg's CD table dma_pointer into the STE's
> > STRTAB_STE_0_CFG field. When neither are set, the STE fields are
> > written to enable bypass (or abort depending on the config).
> 
> I guess I never really understood why these were precomputed and
> stored at all. Even more confusing is why we need to keep pointers to
> them anywhere? Compute the STE and CDE directly from the source data
> when you need it?
> 
> eg If I want to install an IDENITY domain into a STE then I compute
> the STE for identity and go ahead and do it.

I think it'd be clear if the master stores STE value directly to
get rid of s1_cfg/s2_cfg in the master struct. We fill in those
domain-related STE fields when the domain attaches to the master
and then call arm_smmu_write_strtab().

For struct arm_smmu_domain, it still has a union of a CD (for an
S1 domain) or an s2_cfg (for an S2 domain). Or we could select
a better naming for s2_cfg too, since s1_cfg is gone.

Does this match with your expectation?

> > > I'd think the basic mental model should be to extract the STE from the
> > > thing you intend to install. Either the default CD table, or from the
> > > iommu_domain. ie some 'get STE from iommu_domain' function?
> > 
> > I don't follow this. When we attach a domain with pasid (whether
> > through SVA or the set_dev_pasid API) , we don't want to install an
> > entirely new CD table. 
> 
> The master object owns an optional CD table. If it is exsists it is
> used by every domain that is attached to that master.
>
> In the code flow there are two entry points to attach a domain, attach
> to a PASID or attach to a RID.
> 
> For attach to PASID the code should always force the master to have a
> CD table and then attach the domain to the CD table.
> 
> For attach to RID the code should do a bunch of checks and decide if
> it should force the master to have a CD table and attach the domain to
> that, or directly attach the domain to the STE.

I think a master own a CD table in both cases, though the table
could have a single entry or multiple entries, depending on its
ssid_bits/cdmax value. And since a master own the CD table, we
could preallocate the CD table in arm_smmu_probe_device(), like
Michael does in this series. And at the same time, the s1ctxptr
field of the STE could be setup too. Then we insert the CD from
a domain into the CD table during the domain attaching.

Yet two cases that would waste the preallocated CD table:
1) If a master with ssid_bits=0 gets attached to an IDENITY S1
   domain, it sets CONFIG=BYPASS in the STE, which wastes the
   single-entry CD table, unlike currently the driver bypassing
   the allocation of a CD table at all.
2) When enabling nested translation, we should replace all the
   S1 fields in the STE with guest/user values. So, the kernel-
   level CD table (either single-entry or multi-entry) in the
   master struct will not be used. Although this seems to be
   unchanged since currently the driver wastes this too in the
   default domain?

With that, I still feel it clear and flexible. And I can simply
add my use case of attaching an IDENITY domain to the default
substream when having a multi-entry CD table.

Thanks
Nicolin
  

On Thu, Jul 13, 2023 at 12:54:47PM -0700, Nicolin Chen wrote:
> On Thu, Jul 13, 2023 at 01:41:38PM -0300, Jason Gunthorpe wrote:
> > On Fri, Jul 14, 2023 at 12:16:16AM +0800, Michael Shavit wrote:
> > > On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > > It would make alot more sense if the STE value used by an unmanaged S1
> > > > domain was located in/near the unmanaged domain or called 'unmanaged
> > > > S1 STE' or something if it really has to be in the master. Why does
> > > > this even need to be stored, can't we compute it?
> > > 
> > > struct s1_cfg* and struct s2_cfg* are precisely what is used to
> > > compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
> > > will write the s1_cfg's CD table dma_pointer into the STE's
> > > STRTAB_STE_0_CFG field. When neither are set, the STE fields are
> > > written to enable bypass (or abort depending on the config).
> > 
> > I guess I never really understood why these were precomputed and
> > stored at all. Even more confusing is why we need to keep pointers to
> > them anywhere? Compute the STE and CDE directly from the source data
> > when you need it?
> > 
> > eg If I want to install an IDENITY domain into a STE then I compute
> > the STE for identity and go ahead and do it.
> 
> I think it'd be clear if the master stores STE value directly to
> get rid of s1_cfg/s2_cfg in the master struct. We fill in those
> domain-related STE fields when the domain attaches to the master
> and then call arm_smmu_write_strtab().

Right, though master effectively records the STE when it writes it to
the steering table. If we need another copy I don't know.

> For struct arm_smmu_domain, it still has a union of a CD (for an
> S1 domain) or an s2_cfg (for an S2 domain). Or we could select
> a better naming for s2_cfg too, since s1_cfg is gone.

By spec it should be called CD and STE value, but frankly I like CDTE
and STE a little better (context descriptor table entry) as it evokes
a sense of similarity. I don't care for the words 's1_cfg' and
's2_cfg' to represent the CD and STE, that is pretty confusing now
that we have more uses for the CD and STE's than simple s1/s2 IO page
tables.

> I think a master own a CD table in both cases, though the table
> could have a single entry or multiple entries

Yes.

> depending on its ssid_bits/cdmax value. And since a master own the
> CD table, we could preallocate the CD table in
> arm_smmu_probe_device(), like Michael does in this series. And at
> the same time, the s1ctxptr field of the STE could be setup
> too. Then we insert the CD from a domain into the CD table during
> the domain attaching.

We insert the CDTE from the domain into the CD table, and generate a
STE for the CD table and insert it ino the Steering table.

> Yet two cases that would waste the preallocated CD table:
> 1) If a master with ssid_bits=0 gets attached to an IDENITY S1
>    domain, it sets CONFIG=BYPASS in the STE, which wastes the
>    single-entry CD table, unlike currently the driver bypassing
>    the allocation of a CD table at all.
> 2) When enabling nested translation, we should replace all the
>    S1 fields in the STE with guest/user values. So, the kernel-
>    level CD table (either single-entry or multi-entry) in the
>    master struct will not be used. Although this seems to be
>    unchanged since currently the driver wastes this too in the
>    default domain?

As we discussed in another thread dynamic resizing of the CD table
makes some sense. Eg keep it at one entry until PASID mode is enabled
then resize it to the max PASID bits.

But I view that as an improvement beyond here. It seems fine for now
to pre allocate the full CD table and waste the memory. PASID capable
devices are rare.

> With that, I still feel it clear and flexible. And I can simply
> add my use case of attaching an IDENITY domain to the default
> substream when having a multi-entry CD table.

Yes, with the above note about dynamically resizing the CD table, I
think we generally have the idea that programming the CD, eg by
installing an entry, can cause the CD tables's STE to (atomically)
change.

Eg to toggle the S1DSS bit if the PASID 0 is IDENTITY, or to resize
the table if we exceed the PASID space.

Longer term we'd also need a way to setup IDENTITY domains for !0
substreams as well too (eg for SIOV). It is too bad the CDTE doesn't
have a bit for bypass. I suppose it will need dummy 1:1 IO page tables
or something.

Jason
  

On Thu, Jul 13, 2023 at 08:48:32PM -0300, Jason Gunthorpe wrote:
 
> > For struct arm_smmu_domain, it still has a union of a CD (for an
> > S1 domain) or an s2_cfg (for an S2 domain). Or we could select
> > a better naming for s2_cfg too, since s1_cfg is gone.
> 
> By spec it should be called CD and STE value, but frankly I like CDTE
> and STE a little better (context descriptor table entry) as it evokes
> a sense of similarity. I don't care for the words 's1_cfg' and
> 's2_cfg' to represent the CD and STE, that is pretty confusing now
> that we have more uses for the CD and STE's than simple s1/s2 IO page
> tables.

We have STE and PTE, so CDTE sounds legit to me, though probably
it'd be safer by just following the spec? We can always add kdoc
and comments to make things clear.

@Michael,
Would it be possible for you to update a v5, following Jason's
suggestion overall? And I think we can have a smaller refactor
series first without set_dev_pasid, to have a common codeline
sooner for us to add new features, such as set_dev_pasid, the
use case of IDENTITY default substream, and the nesting series.
I will help testing with some pasid/non-pasid use cases too.

> > Yet two cases that would waste the preallocated CD table:
> > 1) If a master with ssid_bits=0 gets attached to an IDENITY S1
> >    domain, it sets CONFIG=BYPASS in the STE, which wastes the
> >    single-entry CD table, unlike currently the driver bypassing
> >    the allocation of a CD table at all.
> > 2) When enabling nested translation, we should replace all the
> >    S1 fields in the STE with guest/user values. So, the kernel-
> >    level CD table (either single-entry or multi-entry) in the
> >    master struct will not be used. Although this seems to be
> >    unchanged since currently the driver wastes this too in the
> >    default domain?
> 
> As we discussed in another thread dynamic resizing of the CD table
> makes some sense. Eg keep it at one entry until PASID mode is enabled
> then resize it to the max PASID bits.

I see.

> But I view that as an improvement beyond here. It seems fine for now
> to pre allocate the full CD table and waste the memory. PASID capable
> devices are rare.

Yea, that'd be easier for us to move forward other features :)

> > With that, I still feel it clear and flexible. And I can simply
> > add my use case of attaching an IDENITY domain to the default
> > substream when having a multi-entry CD table.
> 
> Yes, with the above note about dynamically resizing the CD table, I
> think we generally have the idea that programming the CD, eg by
> installing an entry, can cause the CD tables's STE to (atomically)
> change.
> 
> Eg to toggle the S1DSS bit if the PASID 0 is IDENTITY, or to resize
> the table if we exceed the PASID space.

Yea, we have enable_pasid() so probably can resize over there.

> Longer term we'd also need a way to setup IDENTITY domains for !0
> substreams as well too (eg for SIOV). It is too bad the CDTE doesn't
> have a bit for bypass. I suppose it will need dummy 1:1 IO page tables
> or something.

Oh. I haven't thought about that far. Noted it down.

Thanks
Nicolin
  

On Fri, Jul 14, 2023 at 12:41 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> The master object owns an optional CD table. If it is exsists it is
> used by every domain that is attached to that master.
>
> In the code flow there are two entry points to attach a domain, attach
> to a PASID or attach to a RID.
>
> For attach to PASID the code should always force the master to have a
> CD table and then attach the domain to the CD table.
>
> For attach to RID the code should do a bunch of checks and decide if
> it should force the master to have a CD table and attach the domain to
> that, or directly attach the domain to the STE.

 Yes. This is the current flow (except that we fail instead of forcing
when a CD table isn't already attached in the PASID flow).
owned_s1_cfg is simply a pre-allocated version of your optional CD
table.

> When the master gains a CD table then the CD table object becomes
> attached to the STE. In all cases we should be able to point to the
> object the STE points at and don't need a cfg or pointer to cfg since
> the object itself can provide the cfg.

Ok, practically speaking, are we just talking about reverting patch 3
and keeping a handle to the primary domain in arm_smmu_master? Instead
of directly accessing the currently active CD table using
arm_smmu_master->s1_cfg, you'd like set_dev_pasid() to look for it by
investigating what kind of domain is attached, and reaching to the
pre-alllocated/optional CD table otherwise if necessary?

> > We want to write something (page-table pointer) to a common CD
> > table. Where should the s1_cfg which owns that common table live?
>
> I would suggest a 'cd table struct' that as all the stuff related to
> the CD table, including an API to cacluate the STE this CD table
> requires. If not in actual code with a real struct, then in a logical
> sense in that a chunk of the master struct is the "CD table".

Sure, that's almost exactly what s1_cfg is today (with these patches)....
  * s1_cfg.arm_smmu_ctx_desc_cfg describes the CD table
  * s1_cfg.s1fmt and s1_cfg.s1cdmax describes attributes of that CD
table. These could technically be deduced-back from
arm_smmu_ctx_desc_cfg's l1_desc and num_l1_ents
  * s1_cfg.stall_enabled was introduced by patch 4 and in retrospect
does not belong there at all. It should have gone in arm_smmu_ctx_desc
instead (ignoring the whole should arm_smmu_ctx_desc even be
precomputed in the first place topic for a second).
  

On Fri, Jul 14, 2023 at 9:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> @Michael,
> Would it be possible for you to update a v5, following Jason's
> suggestion overall? And I think we can have a smaller refactor
> series first without set_dev_pasid, to have a common codeline
> sooner for us to add new features, such as set_dev_pasid, the
> use case of IDENTITY default substream, and the nesting series.
> I will help testing with some pasid/non-pasid use cases too.

Want to make sure I fully understand these last few messages first. At
a high level, we want:
1. arm_smmu_master is allowed to own a CD table, but not an
STE-precursor (s1_cfg/s2_cfg). The s1_cfg is practically already that,
and we can probably get there with minimal changes.
2. arm_smmu_master shouldn't point to the currently active CD table
(which may or may not be the one it owns) or STE-precursor as a
shortcut. All code should figure it out by looking at the master's
currently attached domain (functionality could be provided by helper).
3. arm_smmu_domain shouldn't pre-generate any STE-precursors. The
STE/CD for a domain should either be computed right when it is
written, or computed ahead of time and stored as a copy in the
smmu-domain.
Is that right?
  

On Fri, Jul 14, 2023 at 05:12:35PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > @Michael,
> > Would it be possible for you to update a v5, following Jason's
> > suggestion overall? And I think we can have a smaller refactor
> > series first without set_dev_pasid, to have a common codeline
> > sooner for us to add new features, such as set_dev_pasid, the
> > use case of IDENTITY default substream, and the nesting series.
> > I will help testing with some pasid/non-pasid use cases too.
> 
> Want to make sure I fully understand these last few messages first. At
> a high level, we want:
> 1. arm_smmu_master is allowed to own a CD table, but not an
> STE-precursor (s1_cfg/s2_cfg). The s1_cfg is practically already that,
> and we can probably get there with minimal changes.
> 2. arm_smmu_master shouldn't point to the currently active CD table
> (which may or may not be the one it owns) or STE-precursor as a
> shortcut. All code should figure it out by looking at the master's
> currently attached domain (functionality could be provided by helper).
> 3. arm_smmu_domain shouldn't pre-generate any STE-precursors. The
> STE/CD for a domain should either be computed right when it is
> written, or computed ahead of time and stored as a copy in the
> smmu-domain.
> Is that right?

To be honest, I'm having a hard time following the thread. I'd like to
avoid renaming things "for fun", so let's try to focus on the actual
restructuring initially. I'd also like to see what this "computing" of
the SMMU data structures actually looks like -- the information stored
in the 'arm_smmu_master' structure isn't redundant, so what's the problem?

If you all grok this, then I'm happy to wait for the patches, but I don't
want you to go away and spend lots of effort hacking something which doesn't
end up being what folks are asking for.

Will
  

On Fri, Jul 14, 2023 at 05:12:35PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > @Michael,
> > Would it be possible for you to update a v5, following Jason's
> > suggestion overall? And I think we can have a smaller refactor
> > series first without set_dev_pasid, to have a common codeline
> > sooner for us to add new features, such as set_dev_pasid, the
> > use case of IDENTITY default substream, and the nesting series.
> > I will help testing with some pasid/non-pasid use cases too.
> 
> Want to make sure I fully understand these last few messages first. At
> a high level, we want:
> 1. arm_smmu_master is allowed to own a CD table, but not an
> STE-precursor (s1_cfg/s2_cfg). The s1_cfg is practically already that,
> and we can probably get there with minimal changes.

Yah

> 2. arm_smmu_master shouldn't point to the currently active CD table
> (which may or may not be the one it owns) or STE-precursor as a
> shortcut. All code should figure it out by looking at the master's
> currently attached domain (functionality could be provided by
> helper).

I think that is close.

Most likely the master should have a pointer to an STE owning
iommu_domain. If that is !NULL then the master's CD table is not used
and the STE is computed from that iommu_domain. Essentially it says
that iommu_domain owns the entire RID.

Otherwise the STE comes from the master's CD table, and this means the
master is in SSID mode.

> 3. arm_smmu_domain shouldn't pre-generate any STE-precursors. The
> STE/CD for a domain should either be computed right when it is
> written, or computed ahead of time and stored as a copy in the
> smmu-domain.

I think this is cleaner since alot of the STE/CD calculation is either
constant or copying data from other places in the domain struct, but
maybe you'll find this is wrong. At least it is not super important at
this point.

Jason
  

On Fri, Jul 14, 2023 at 04:02:50PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 12:41 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > The master object owns an optional CD table. If it is exsists it is
> > used by every domain that is attached to that master.
> >
> > In the code flow there are two entry points to attach a domain, attach
> > to a PASID or attach to a RID.
> >
> > For attach to PASID the code should always force the master to have a
> > CD table and then attach the domain to the CD table.
> >
> > For attach to RID the code should do a bunch of checks and decide if
> > it should force the master to have a CD table and attach the domain to
> > that, or directly attach the domain to the STE.
> 
>  Yes. This is the current flow (except that we fail instead of forcing
> when a CD table isn't already attached in the PASID flow).
> owned_s1_cfg is simply a pre-allocated version of your optional CD
> table.

Really? That seems like a terrible name for the CD table.

> > When the master gains a CD table then the CD table object becomes
> > attached to the STE. In all cases we should be able to point to the
> > object the STE points at and don't need a cfg or pointer to cfg since
> > the object itself can provide the cfg.
> 
> Ok, practically speaking, are we just talking about reverting patch 3
> and keeping a handle to the primary domain in arm_smmu_master?

I think the master should have a pointer to the iommu_domain that owns
the STE or if NULL the master should assign its internal CD table to
the STE.

The iommu_domain structs should not have any references to a CD table.

> > I would suggest a 'cd table struct' that as all the stuff related to
> > the CD table, including an API to cacluate the STE this CD table
> > requires. If not in actual code with a real struct, then in a logical
> > sense in that a chunk of the master struct is the "CD table".
> 
> Sure, that's almost exactly what s1_cfg is today (with these patches)....
>   * s1_cfg.arm_smmu_ctx_desc_cfg describes the CD table
>   * s1_cfg.s1fmt and s1_cfg.s1cdmax describes attributes of that CD
> table. These could technically be deduced-back from
> arm_smmu_ctx_desc_cfg's l1_desc and num_l1_ents

Yes, this makes sense, there is some redundancy here for sure.

patch 1 makes sense, arm_smmu_ctx_desc is effectively the Context
Descriptor Entry, and it belongs in the domain

patch 2 should delete arm_smmu_s1_cfg and just put
arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
a weird name for the contex descriptor table, but it is much less
weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.

patch 3 I don't understand, we should not add something called
s1_cfg/s2_cfg to the master. The master should have
'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'

patch 4 should have everything working with the cd table accept a
'struct arm_smmu_ctx_desc_cfg *', eg arm_smmu_get_cd_ptr (get a
pointer to a CD entry in the CD table).

patch 5 makes sense, but something seems odd about the order as we
somehow half moved it in patch 2?

My suggestion for patch structure is to start by cleaning up the CD
table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
and then as the last step just move the arm_smmu_ctx_desc_cfg from the
iommu_domain to the master.

And that is a nice little series on its own - you end up with a shared
CD table in the master, and no CD table in any domains.

Jason
  

On Fri, Jul 14, 2023 at 9:21 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> patch 2 should delete arm_smmu_s1_cfg and just put
> arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
> a weird name for the contex descriptor table, but it is much less
> weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.

s1fmt is fairly trivial to replace but s1cdmax requires inversing
previous computations. I don't really buy that getting rid of it
simplifies anything, even if it's technically redundant.

> patch 3 I don't understand, we should not add something called
> s1_cfg/s2_cfg to the master. The master should have
> 'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'

This was simply meant to be a more convenient way of finding the
currently active cdtable from the
arm_smmu_write_ctx_desc/write_strtab_ent functions without having to
inspect the currently attached domain. But sure, that's easy enough to
revert.

> patch 5 makes sense, but something seems odd about the order as we
> somehow half moved it in patch 2?
Ack; patch 2 can be reordered to come after patch 4, or even squashed
with 5 if you prefer.

> My suggestion for patch structure is to start by cleaning up the CD
> table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
> redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
> 'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
> and then as the last step just move the arm_smmu_ctx_desc_cfg from the
> iommu_domain to the master.
>
> And that is a nice little series on its own - you end up with a shared
> CD table in the master, and no CD table in any domains.

I don't entirely buy that refactoring s1_cfg is worth the extra
effort, nor that it should be tied to this patch series. This series
already makes s1_cfg behave as the CD table; whether we want to
entirely get rid of pre-computed data useful for writing an STE sounds
like a separate cleanup.
  

On Mon, Jul 17, 2023 at 06:06:19PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:21 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > patch 2 should delete arm_smmu_s1_cfg and just put
> > arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
> > a weird name for the contex descriptor table, but it is much less
> > weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.
> 
> s1fmt is fairly trivial to replace but s1cdmax requires inversing
> previous computations. I don't really buy that getting rid of it
> simplifies anything, even if it's technically redundant.

Then store s1cdmax in the arm_smmu_ctx_desc_cfg and still get rid of
arm_smmu_s1_cfg

The point is to get rid of "s1_cfg" entirely as language in the driver
*because it makes zero sense now*. Prior to your restructuring it was
sort of the STE to use for the S1 page table which had an embedded
CD. After all this change it isn't realated to a S1 page table anymore
at all.

> > patch 3 I don't understand, we should not add something called
> > s1_cfg/s2_cfg to the master. The master should have
> > 'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'
> 
> This was simply meant to be a more convenient way of finding the
> currently active cdtable from the

There is only one (meaningful) choice though, the cdtable is either
the master's default CD table or it isn't. You detect that by checking
for NULL ste_domain

> > My suggestion for patch structure is to start by cleaning up the CD
> > table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
> > redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
> > 'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
> > and then as the last step just move the arm_smmu_ctx_desc_cfg from the
> > iommu_domain to the master.
> >
> > And that is a nice little series on its own - you end up with a shared
> > CD table in the master, and no CD table in any domains.
> 
> I don't entirely buy that refactoring s1_cfg is worth the extra
> effort, nor that it should be tied to this patch series. This series
> already makes s1_cfg behave as the CD table; whether we want to
> entirely get rid of pre-computed data useful for writing an STE sounds
> like a separate cleanup.

s1_cfg is a terrible name to describe the cd table. Please do the
tiny bit extra to get rid of it for clarity. 

Jason
  

Haven't had time to address this yet; but Ack on last message. Will
address in a v5 series.
  

Sorry for the delay; I'm trying to refactor these patches now.

> I think the master should have a pointer to the iommu_domain that owns
> the STE or if NULL the master should assign its internal CD table to
> the STE.
Just to clarify, does the nested domain patch series require writing
CDs into the user-space's CD table using arm_smmu_write_ctx_desc()? Or
is there any other requirement for writing a CD into a domain-owned CD
table from arm_smmu_write_ctx_desc?


Writing a CD entry to the CD table involves some dependencies on the
master(s) that the table is attached to:

1. The CD entries STALL bit value in arm_smmu_write_ctx_desc depends
on the master (e.g. if STALL_FORCE is set on the smmu device). This
could potentially be encoded in arm_smmu_ctx_desc_cfg, at which point
that CD table is only attachable to masters with the same
stall_enabled value.
2. arm_smmu_write_ctx_desc must sync the CD for the attached master(s)
in the middle of writing CD entry.

This is all easier to handle in arm_smmu_write_ctx_desc if the table
is always owned by the master.
  

On Thu, Jul 27, 2023 at 07:22:05PM +0800, Michael Shavit wrote:
> Sorry for the delay; I'm trying to refactor these patches now.
> 
> > I think the master should have a pointer to the iommu_domain that owns
> > the STE or if NULL the master should assign its internal CD table to
> > the STE.
> Just to clarify, does the nested domain patch series require writing
> CDs into the user-space's CD table using arm_smmu_write_ctx_desc()?

No, CD entries in nested CD tables are written by userspace only.

> Or is there any other requirement for writing a CD into a
> domain-owned CD table from arm_smmu_write_ctx_desc?

Not that I know of.

The only time the kernel writes a CD entry is to the shared CD table
stored in the master.

> 1. The CD entries STALL bit value in arm_smmu_write_ctx_desc depends
> on the master (e.g. if STALL_FORCE is set on the smmu device). This
> could potentially be encoded in arm_smmu_ctx_desc_cfg, at which point
> that CD table is only attachable to masters with the same
> stall_enabled value.

For cleanness I would orgnize it like this.

> 2. arm_smmu_write_ctx_desc must sync the CD for the attached master(s)
> in the middle of writing CD entry.
>
> This is all easier to handle in arm_smmu_write_ctx_desc if the table
> is always owned by the master.

I think it is fine if you start with a shared CD table being 1:1 with
a single master.

Making the CD table shared between masters (eg for multi-device-group)
is a micro-optimization, and I'm not sure we have workloads where it
is worthwhile. We already block PASID support for multi-device-group.

Though resizable CD table is probably a better place to put efforts.

Jason
  

Awesome that helps a lot. These are the patches I have in mind:

1. Move ctx_desc out of s1_cfg
2. Replace domain->s1_cfg with `struct arm_smmu_ctx_desc_cfg cd_table`
3. Move stall_enabled from domain to arm_smmu_ctx_desc_cfg
4. Accept an arm_smmu_master instead of an arm_smmu_domain as the
parameter for arm_smmu_write_ctx_desc
--- arm_smmu_write_ctx_desc can simply get the cd_table it writes to
from master->domain->cd_table; this get's replaced by master->cd_table
in subsequent commits.
--- SVA is weird if we cut changes here: it iterates over all masters
a domain is attached to in order to call arm_smmu_write_ctx_desc(),
which ends up writing to the shared cd_table since in master->domain.
This becomes more sensible once masters stop sharing the cd _table in
subsequent commits.
--- arm_smmu_write_ctx_desc is used to sync the cd for all masters the
domain was attached to. Now that SVA is calling it in a loop, and to
break the dependency on arm_smmu_domain, we should only sync for the
master passed in as the parameter
5. Introduce a skip_cd_sync parameter to arm_smmu_write_ctx_desc so
that arm_smmu_domain_finalise_s1 can skip the sync_cd before the
cd_table is attached to the master. Before the previous commit,
arm_smmu_domain_finalise_s1 was calling arm_smmu_write_ctx_desc()
before adding the master to the domain->devices list, which was used
by arm_sync_smmu() to issue sync commands to masters. This
optimization was broken by the previous commit since we stopped
depending on domain->devices.
6. Move cd_table from domain to arm_smmu_master->cd_table.
  

On Thu, Jul 27, 2023 at 10:04:04PM +0800, Michael Shavit wrote:
> Awesome that helps a lot. These are the patches I have in mind:
> 
> 1. Move ctx_desc out of s1_cfg
> 2. Replace domain->s1_cfg with `struct arm_smmu_ctx_desc_cfg cd_table`
> 3. Move stall_enabled from domain to arm_smmu_ctx_desc_cfg
> 4. Accept an arm_smmu_master instead of an arm_smmu_domain as the
> parameter for arm_smmu_write_ctx_desc
> --- arm_smmu_write_ctx_desc can simply get the cd_table it writes to
> from master->domain->cd_table; this get's replaced by master->cd_table
> in subsequent commits.

Makes sense

> --- SVA is weird if we cut changes here: it iterates over all masters
> a domain is attached to in order to call arm_smmu_write_ctx_desc(),
> which ends up writing to the shared cd_table since in master->domain.
> This becomes more sensible once masters stop sharing the cd _table in
> subsequent commits.

Seems like it is OK inside the series

> --- arm_smmu_write_ctx_desc is used to sync the cd for all masters the
> domain was attached to. Now that SVA is calling it in a loop, and to
> break the dependency on arm_smmu_domain, we should only sync for the
> master passed in as the parameter

Sounds correct

> 5. Introduce a skip_cd_sync parameter to arm_smmu_write_ctx_desc so
> that arm_smmu_domain_finalise_s1 can skip the sync_cd before the
> cd_table is attached to the master. Before the previous commit,
> arm_smmu_domain_finalise_s1 was calling arm_smmu_write_ctx_desc()
> before adding the master to the domain->devices list, which was used
> by arm_sync_smmu() to issue sync commands to masters. This
> optimization was broken by the previous commit since we stopped
> depending on domain->devices.

Tracking if the cd table is not yet installed in a STE might be a
cleaner approach than a flag?

> 6. Move cd_table from domain to arm_smmu_master->cd_table.

Yep

Jason