Message-ID: <ZLCNUNRsWy/YuHhQ@nvidia.com>
Date:   Thu, 13 Jul 2023 20:48:32 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Nicolin Chen <nicolinc@...dia.com>
Cc:     Michael Shavit <mshavit@...gle.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Joerg Roedel <joro@...tes.org>, jean-philippe@...aro.org,
        baolu.lu@...ux.intel.com, linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to
 smmu_master

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

Powered by blists - more mailing lists