Message-ID: <ZLChc4A58PkU2Fe5@Asurada-Nvidia>
Date:   Thu, 13 Jul 2023 18:14:27 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Michael Shavit <mshavit@...gle.com>,
        Jason Gunthorpe <jgg@...dia.com>
CC:     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 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

Powered by blists - more mailing lists