Re: [RFC PATCH 0/5] iommu/arm-smmu-v3: Implement Runtime/System Sleep ops

[Pranjal Shrivastava <praan@google.com>]

On Fri, Mar 21, 2025 at 05:35:11PM +0000, Robin Murphy wrote:
> On 21/03/2025 2:18 pm, Pranjal Shrivastava wrote:
> > On Thu, Mar 20, 2025 at 10:25:52PM +0000, Mostafa Saleh wrote:
> > > On Wed, Mar 19, 2025 at 04:07:57PM +0000, Robin Murphy wrote:
> > > > On 19/03/2025 11:57 am, Jason Gunthorpe wrote:
> > > > > On Wed, Mar 19, 2025 at 12:42:49AM +0000, Pranjal Shrivastava wrote:
> > > > > 
> > > > > > 3. Invoking runtime_pm_get/put
> > > > > > Given that most of the configuration done by arm-smmu-v3 is stored in
> > > > > > memory, the initial idea is to focus on areas where the driver accesses
> > > > > > the hw via exposed ops, like iommmu_ops, iommu_flush_ops, sva_ops etc.
> > > > > 
> > > > > This seems weird, if the SMMU is suspended doesn't it also fail DMA
> > > > > transactions? Why would ops like flush even be called if the HW is
> > > > > disabled?
> > > > 
> > > > Because once the device has finished its operation, its driver is free to
> > > > call rpm_put() before calling dma_unmap(), so by the time that gets as far
> > > > as TLB maintenance, the SMMU may already be asleep as well if that device
> > > > was the only thing keeping it awake.
> > > > 
> > > > For direct IOMMU API users, pagetable update may be even more asynchronous
> > > > from device activity, e.g. a GPU buffer might only be unmapped once
> > > > userspace closes the last file handle referencing it, long after the GPU
> > > > itself has moved on to other things.
> > > > 
> > > > > flush is performance path stuff, so it doesn't seem great to be adding
> > > > > extra calls there.
> > > > 
> > > > That much is true - this really wants to be using pm_runtime_get_if_in_use()
> > > > nearly everywhere such that at most it's just juggling refcounts. There's no
> > > > point waking the SMMU up just to issue a CFGI or TLBI, if the act of doing
> > > > so is inherently going to do a full arm_smmu_reset() and thus invalidate
> > > > everything anyway.
> > > 
> > > AFAICT, there is no guarantees that caches are clean on system resume,
> > > but as we do invalidate everything that should be fine, but I am not sure
> 
> That was the point - we're definitely going to do a full software
> invalidation *because* we can't make any assumptions about the hardware
> state, i.e. it may come back full of valid-looking nonsense.
> 
> > I mean we do set GBPA.Abort = 1 right before suspending, I'd want to
> > assume that doing so would ensure that TLB hits don't occur anymore. Let
> > me dig into the spec to see if I can find something regarding TLB
> > behavior when GBPA.Abort = 1
> 
> GBPA doesn't matter here, it's about the CR0.SMMUEN=0 behaviour (see
> 6.3.9.6). That says "Incoming transactions [...] do not undergo
> translation," so although TLB entries are allowed to remain present, they
> must not be *used* - i.e. SMMUEN is not permitted to be cached in a TLB.
> 

Thanks for pointing me to the right section. It also mentions: 
When SMMU_(*_)CR0.SMMUEN == 0:
"Translation and configuration cache entries are not inserted or
modified, except for invalidation by maintenance commands or broadcast
operations."

So, it looks like we don't need to worry much about a disabled
programming interface causing changes to the TLB. 

However, another statement in the same section:

"Note: The ‘other’ Security state might still have SMMUEN == 1 and
therefore be inserting cache entries for that Security state. As these
entries are not visible to or affected by the Non-secure programming
interface, this is only a consideration for the Secure programming
interface which can maintain Non-secure cache entries."

Makes me think of situations where we might elide a TLB invalidate if
the SMMU is SUSPENDED but the secure world gets a hit to the 
invalidated TLB entry. The TLBI command could be a result of a simple
non-driver kernel module unmapping a page based on it's communication
with the secure world. In this case, devlinks may NOT save the day...

I know that the above situation is a burden on the SW designer or
implementer, I just want to discuss is if we have something
like the above case, that would not want us to elide TLBIs while
suspended? (I'm not able to see any case where we share pages with
the secure world at this time).


> > > how that works with distributed SMMUs where the TBU can still be powered
> > > with some TLBs that can be invalid?
> > > 
> > 
> > Hmm.. do you mean some situation like:
> > 
> >   |-----------------------|    |-------------------|
> >   | |-------|   |-------| |    |-------------------|
> >   | | Dev X |   | Dev Y | |    |                   |
> >   | | (TBU) |   |_______| |    |       SMMUv3      |
> >   | |-------|             |    |  (TCU -> TBU_X)   |
> >   |     Power_Domain A    |    |  Power_Domain B   |
> >   |-----------------------|    |-------------------|
> > 
> > Now if Dev Y isn't an SMMU client and Dev X drops the ref_count,
> > Power_Domain A would still remain ON whereas SMMUv3 might assume all
> > it's clients are down and try to suspend (power off Power_Domain B)
> > while the TLBs withing TBU_X are still powered up?
> > 
> > To avoid such a case maybe we should invalidate everything during
> > suspend? I still believe that when CR0.SMMUEN=0, it should broadcast
> > something to all it's TBUs asking them to invalidate all TLBs otherwise
> > this is a miss in the arch (unlikely for Arm) as TLB hits should never
> > occur if the SMMU is disabled. I guess I need to go through the SMMUv3
> > spec (or maybe the MMU-700 TRM) for confirming this..
> 
> I don't think this is allowed to be an issue in practice since TBUs are not
> architecturally visible. Certainly in terms of Arm's implementations, for a
> TBU to be powered off or externally clock gated it would have to do a full
> DTI disconnect (otherwise it would hang CMD_SYNC), and DTI requires that it
> must subsequently come back clean:
> 
> "The TBU must invalidate its caches before entering CONNECTED state."
> 

Interesting! Thanks for clarifying.

> Thanks,
> Robin.

Thanks,
Praan