Re: [RFC PATCH 5/5] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[Pranjal Shrivastava <praan@google.com>]

On Wed, Mar 19, 2025 at 09:04:04AM -0300, Jason Gunthorpe wrote:
> On Wed, Mar 19, 2025 at 12:42:54AM +0000, Pranjal Shrivastava wrote:
> > -static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
> > +static int arm_smmu_attach_dev_ste(struct iommu_domain *domain,
> >  				    struct device *dev,
> >  				    struct arm_smmu_ste *ste,
> >  				    unsigned int s1dss)
> >  {
> 
> This can't be changed into something that fails! That would mess up
> all the error handling if it ever blows up.
> 
> > @@ -3214,8 +3287,8 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> >  	struct arm_smmu_master master = dev_iommu_priv_get(dev);
> >  
> >  	arm_smmu_make_bypass_ste(master->smmu, &ste);
> > -	arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS);
> > -	return 0;
> > +	return arm_smmu_attach_dev_ste(domain, dev, &ste,
> > +				       STRTAB_STE_1_S1DSS_BYPASS);
> >  }
> >  
> >  static const struct iommu_domain_ops arm_smmu_identity_ops = {
> > @@ -3233,9 +3306,8 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
> >  	struct arm_smmu_ste ste;
> >  
> >  	arm_smmu_make_abort_ste(&ste);
> > -	arm_smmu_attach_dev_ste(domain, dev, &ste,
> > -				STRTAB_STE_1_S1DSS_TERMINATE);
> > -	return 0;
> > +	return arm_smmu_attach_dev_ste(domain, dev, &ste,
> > +				       STRTAB_STE_1_S1DSS_TERMINATE);
> >  }
> 
> Because these functions are required to be non-failing.
> 
> This feels kind of wonky, if we need to update a STE but the PM is off
> then we just need to update the STE memory with some synchornization
> and do nothing else. We don't need to wake up the HW and issue a cache
> flush if the powered off cache could never have held a copy anyhow.

I agree, that we can elide TLBIs if the smmu is asleep (powered-off),
the main reason to wake up the hw is because `arm_smmu_write_ste` issues
a prefetch CMD. Again, it is possible that the smmu sleeps after this op
and the prefetch has no real benefit.

Maybe having something like rpm_get_if_in_use could potentially help
here, wherein we can check if the smmu is powered ON before going ahead
with issuing all the commands. Although, in this case we might lose
slight performance due to wake-ups caused by other clients, for example:

1. SMMU is powered off, client1 attempts to attach to a domain
2. We elide the prefetch CMD since SMMU is powered off
3. Now, another client2 served by the same SMMU, powers on the SMMU

I'm not sure how much of a performance loss this is and also on systems
with IO-coherent SMMUs (where the SMMU can snoop into CPU caches) this
may not be a huge drop, but I'd like to discuss the possibility..

Anyway, having *any* rpm implementation that causes us to lose TLB
content, will cause a hit on the performance, so maybe this
micro-optimization isn't worth at all?

Also, while we're at it, should we have a feature switch for users who'd
like to disable runtime PM for performance reasons? I don't think that
relying on the `power-domain` being present alone is sufficient.

Thanks,
Praan