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

[Michael Shavit]

On Fri, Jul 14, 2023 at 12:41 AM Jason Gunthorpe <jgg@xxxxxxxxxx> 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).