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

[Jason Gunthorpe]

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

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