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

[Jason Gunthorpe]

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