Re: [PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support

From: Jason Gunthorpe <jgg@nvidia.com>
To: Nicolin Chen <nicolinc@nvidia.com>
Cc: Robin Murphy <robin.murphy@arm.com>,
	will@kernel.org, joro@8bytes.org, mshavit@google.com,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev
Subject: Re: [PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support
Date: Thu, 17 Aug 2023 14:57:58 -0300	[thread overview]
Message-ID: <ZN5fpi1SDp2uqQnn@nvidia.com> (raw)
In-Reply-To: <ZN5RzvGhwRvVKUDy@Asurada-Nvidia>

On Thu, Aug 17, 2023 at 09:58:54AM -0700, Nicolin Chen wrote:
> > > > @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> > > > 
> > > >      if (smmu_domain) {
> > > >              switch (smmu_domain->stage) {
> > > > +            case ARM_SMMU_DOMAIN_BYPASS_S1DSS:
> > > > +                    s1dss = STRTAB_STE_1_S1DSS_BYPASS;
> > > > +                    fallthrough;
> > > >              case ARM_SMMU_DOMAIN_S1:
> > > >                      cd_table = &master->cd_table;
> > > >                      break;
> > > 
> > > Eg, I think the code looks much nicer if the logic here is more like:
> > > 
> > > if (master->cd_table.cdtab)
> > >     arm_smmu_cd_table_get_ste(master->cd_table, &ste)
> 
> So, this means that cd_table is present, indicating either "S1
> translated" or "S1 enabled but S1DSS"...

Yes, if cd table is present then cd table always provides the STE.
So this is S1 page tables on any SSID or SSID=0 IDENTITY

> > > else if (master->domain)
> > >     arm_smmu_domain_get_ste(master->domain, &ste);
> 
> ... and this means that cd_table isn't present, indicating S1
> bypass or S2....

Meaning S2 page table on the RID or IDENTITY.

> > > And you'd check in arm_smmu_cd_table_get_ste() to learn the CD
> > > parameters and also what SSID=0 is. If SSID=0 is IDENTITY then
> > > arm_smmu_cd_table_get_ste would return with S1DSS set.
> > > 
> > > arm_smmu_domain_get_ste() would multiplex based on the domain type.
> 
> ... it then means we need arm_smmu_write_ctx_desc() also when
> attaching an IDENTITY domain.

Yes, if a cd_table is present then it would be good to always write a
consistent CD table entry meaning IDENTITY for SSID. Even if all this
does is zero out the CD table entry.

> > > > @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > > >      list_add(&master->domain_head, &smmu_domain->devices);
> > > >      spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> > > > 
> > > > -    if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> > > > +    if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 ||
> > > > +        smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) {
> > > >              if (!master->cd_table.cdtab) {
> > > >                      ret = arm_smmu_alloc_cd_tables(master);
> > > >                      if (ret) {
> > > 
> > > So more like:
> > > 
> > >   if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev))
> > >       arm_smmu_alloc_cd_tables()
> 
> OK. ARM_SMMU_DOMAIN_S1 with ssid=0 still needs a cd_table though.

Yes, but the atomic ste update is a bit nicer, though it can be
something to do separately as it is just saving a bit of cache.

Jason