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

[Nicolin Chen]

Hi,

Thank you both for the reviews!

On Thu, Aug 17, 2023 at 05:24:51PM +0100, Robin Murphy wrote:

> > > Changelog
> > > v2:
> > >   * Rebased on top of Michael's series reworking CD table ownership:
> > >     https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@xxxxxxxxxx/
> > >   * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case
> > > v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@xxxxxxxxxx/
> > 
> > After rebasing there really shouldn't be a
> > ARM_SMMU_DOMAIN_BYPASS_S1DSS. I want to get to a model where the
> > identity domain is a global static, so it can't be changing depending
> > on how it is attched.
> > 
> > I continue to think that the right way to think about this is to have
> > the CD table code generate the STE it wants and when doing so it will
> > inspect what SSID0 is. If it is the IDENTITY domain then it fills
> > s1dss / etc
> 
> Indeed, that's what I was getting at with "generalisation of
> ARM_SMMU_DOMAIN_BYPASS based on s1cdmax" - just one type all the way
> down to the bowels of arm_smmu_write_strtab_ent(), which then decides
> whether it means touching S1DSS or Config in the given STE.

Ack. Let me retry with that.

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

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

> > else
> >     ste = not attached
> > 
> > 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.

Will try with that. Thanks for the guidance!

> > > @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > >      } else if (smmu_domain->smmu != smmu)
> > >              ret = -EINVAL;
> > > 
> > > +    /*
> > > +     * When attaching an IDENTITY domain to a master with pasid capability,
> > > +     * the master can still enable SVA feature by allocating a multi-entry
> > > +     * CD table and attaching the IDENTITY domain to its default substream
> > > +     * that alone can be byassed using the S1DSS field of the STE.
> > > +     */
> > > +    if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits &&
> > > +        smmu->features & ARM_SMMU_FEAT_TRANS_S1)
> > > +            smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS;
> > 
> > Then you don't technically need to do this.

Yea, wasn't so confident about it either. Will drop.

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

Thanks!
Nic