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

[Pranjal Shrivastava <praan@google.com>]

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

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

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

> Thanks,
> Mostafa
> > 

Thanks,
Praan

> > > > Instead of wrapping every exposed op with an rpm_get/put, the idea is to
> > > > follow through till a logical common point (lowest common ancestor in
> > > > the call hierarchies) and wrap those with the rpm_get/put calls.
> > > 
> > > Should the iommu core be doing this instead of hidden in drivers? It
> > > seems like there might be better information there about when we
> > > expect the IOMMU to be powered up and working and when it is OK to
> > > have it shutdown?
> > 
> > On the contrary, I would say it's very much driver-level knowledge as to
> > which operations need the hardware accessible or not, and trying to do it at
> > the core level would inevitably end up very pessimistic and inefficient, or
> > at best just horribly complex.
> > 
> > > And maybe in this direction we can do things like remove the
> > > translation before doing a suspend so that flush ops are not a
> > > problem.
> > 
> > Case in point: that would be a load of extra work to do, make suspend/resume
> > even slower, and even then for no benefit, since map_pages/umnmap_pages can
> > still be called on the unattached domain (especially a default DMA domain),
> > wherein drivers may still end up trying to touch their hardware for various
> > reasons in ways invisible to the core API. Easy example: consider a driver
> > calling dma_alloc_coherent() to prepare a buffer *before* it wakes up its
> > client device (which would then also wake up the SMMU via the device link),
> > and deep within that call, arm_lpae_init_pte() ends up calling back into
> > arm_smmu_tlb_inv_walk() because it's laying down a block mapping where an
> > old empty table PTE happens to be...
> > 
> > Thanks,
> > Robin.