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

[Jason Gunthorpe]

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