Re: [Qemu-devel] [PATCH RFC] target-ppc: tlbie should have global effect

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2016-09-09 at 10:15 +0530, Nikunj A Dadhania wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > 
> > On Fri, 2016-09-09 at 09:53 +0530, Nikunj A Dadhania wrote:
> > > 
> > > tlbie (and H_REMOVE for pseries) should have a global effect. This is
> > > achieved by iterating and setting tlb_need_flush in all the CPUs.
> > > 
> > > > > > Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > > > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> > > 
> > > --
> > > 
> > > Note: Haven't changed following POWERPC_MMU_32B and POWERPC_MMU_601
> > > yet.
> > > As I am not sure about it.
> > 
> > 604 and 7400 can do SMP.
> 
> Sure, will add similar logic there.
> 
> > 
> > That said, I think the approach in your patch is going to be a bit big
> > of a hammer.
> 
> > 
> > We should have a separate flag indicating that we need to
> > broadcast a flush and only set it on tlbie (non-l) and tlbivax (on
> > BookE) so that we don't end up doing expensive broadcasts on things
> > like context switches.
> 
> I had implemented that initially, and was checking that in
> check_tlb_flush, the logic was getting complicated, so thought about
> this way.
> 
> Moreover, I was thinking about it, that needs to be a global tcg flag
> and not part the CPUPPCState structure, I was then worried about how to
> synchronise a global tcg variable.

No it doesn't.

When a "broadcast TLB" op happens, such as tlbie, you set both flags.
The existing one which just means the current CPU needs flushing, that
logic doesnt' change at all.

The other one means that *this* CPU needs to broadcast a TLB inval,
thus it's also a local flag !

Then you change gen_check_tlb_flush() to take an argument (an immediate
value) which you set to 1 in ptesync on BookS and tlbsync on BookE and
leave 0 on all the others.

Finally, gen_check_tlb_flush() does what it does today, then *additionally*
if that argument was 1 *and* the broadcast_flush flag was set, the helper
can then shoot the invalidations to all the other CPUs (and clear the flag).

	CPU_FOREACH(cs) {
		if (cs != this_cpu) {
			tlb_flush(...)
		}
	}


For MT-TCG, the only change is then in that the above loop does async
procesdure calls to the target CPUs.

You will need to make sure ptesync/tlbsync (the latter only on BookE, make
it a nop on BookS) also exit the TB which is easy to do so that we get
the synchronization with the pending async works as Paolo mentioned separately
(double check with him and/or Alex ot make sure you get that right).

> > We keep the existing logic to flush locally. We additionally replace
> > the one in ptesync (BookS) or tlbsync (BookE) to test for the broadcast
> > flag, and flush the "other" CPUs if set.
> > 
> > That also means you have a nice spot to do the more complex MT-TCG
> > broadcast only when needed in the future.
> > 
> 
> Regards
> Nikunj