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

[Mostafa Saleh <smostafa@google.com>]

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
how that works with distributed SMMUs where the TBU can still be powered
with some TLBs that can be invalid?

Thanks,
Mostafa
> 
> > > 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.