On 23/08/2019 15:26, Rob Herring wrote:
On Fri, Aug 23, 2019 at 9:05 AM Robin Murphy robin.murphy@arm.com wrote:
On 23/08/2019 14:18, Rob Herring wrote:
On Fri, Aug 23, 2019 at 7:56 AM Robin Murphy robin.murphy@arm.com wrote:
On 23/08/2019 03:12, Rob Herring wrote:
tlb_inv_context() hook is only called when freeing the page tables. There's no point in flushing only to free the page tables immediately following.
FWIW, in general the point of flushing is *because* we're about to free the pagetables - if there's any possibility that the hardware could continue to issue translation table walks (speculative or otherwise) after those pages have been reused by someone else, TLB badness may ensue.
For panfrost in particular I suspect we can probably get away without it, at least for the moment, but it might be worth moving the flush to mmu_disable() for complete peace of mind (which kind of preempts the sort of thing that per-process AS switching will want anyway).
There's bigger problem that mmu_disable() is still only called for AS0 and only for driver unload.
Sure, but AS0 is the only one we ever enable, and conceptually we do that once at probe time (AFAICS it stays nominally live through resets and resumes), so while it may be the least-clever AS usage possible it's at least self-consistent. And at the moment the only time we'll call .tlb_inv_context is from panfrost_mmu_fini() when we're unconditionally poking registers anyway, so either way I don't think there's any actual problem today - I'm viewing this patch as getting things straight in preparation for the future.
It was only AS0, but we now have per FD AS.
Ah, silly me, I forgot to look at -next...
I guess we should fix that and then figure out where a flush is needed if at all. I would think changing the TTBR would be enough to quiesce the h/w and TLBs. That seems to be the case in my testing of switching address spaces.
From a quick scan through kbase, kbase_mmu_disable() appears to perform an full FLUSH_MEM before rewriting TRANSTAB, and it looks like that does get called when scheduling out a context. That's in line with what I was imagining, so unless we know better for sure, we may as well play it safe and follow the same pattern.
Agreed.
I guess in that case we probably want it in panfrost_mmu_as_put() when as_count falls to 0, to balance the corresponding enable in as_get(). If the tables are only freed later when the FD is closed and whichever AS is probably in use by some other job, that's even more reason that .tlb_inv_context is now the wrong place to be touching hardware.
Robin.