From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicholas Piggin Date: Wed, 10 Feb 2021 05:33:53 +0000 Subject: Re: [PATCH 1/2] KVM: PPC: Book3S HV: Remove shared-TLB optimisation from vCPU TLB coherency logic Message-Id: <1612934661.odpjnmvpwe.astroid@bobo.none> List-Id: References: <20200528015331.GD307798@thinks.paulus.ozlabs.org> In-Reply-To: <20200528015331.GD307798@thinks.paulus.ozlabs.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kvm-ppc@vger.kernel.org Excerpts from Paul Mackerras's message of February 10, 2021 12:46 pm: > On Wed, Feb 10, 2021 at 11:44:54AM +1000, Nicholas Piggin wrote: >> Excerpts from Paul Mackerras's message of February 10, 2021 10:39 am: >> > On Tue, Feb 09, 2021 at 06:44:53PM +1000, Nicholas Piggin wrote: >> >> Excerpts from Paul Mackerras's message of February 9, 2021 5:19 pm: >> >> > On Mon, Jan 18, 2021 at 10:26:08PM +1000, Nicholas Piggin wrote: >> >> >> Processors that implement ISA v3.0 or later don't necessarily have >> >> >> threads in a core sharing all translations, and/or TLBIEL does not >> >> >> necessarily invalidate translations on all other threads (the >> >> >> architecture talks only about the effect on translations for "the thread >> >> >> executing the tlbiel instruction". >> >> > >> >> > It seems to me that to have an implementation where TLB entries >> >> > created on one thread (say T0) are visible to and usable by another >> >> > thread (T1), but a tlbiel on thread T0 does not result in the entry >> >> > being removed from visibility/usability on T1, is a pretty insane >> >> > implementation. I'm not sure that the architecture envisaged allowing >> >> > this kind of implementation, though perhaps the language doesn't >> >> > absolutely prohibit it. >> >> > >> >> > This kind of implementation is what you are allowing for in this >> >> > patch, isn't it? >> >> >> >> Not intentionally, and patch 2 removes the possibility. >> >> >> >> The main thing it allows is an implementation where TLB entries created >> >> by T1 which are visble only to T1 do not get removed by TLBIEL on T0. >> > >> > I could understand this patch as trying to accommodate both those >> > implementations where TLB entries are private to each thread, and >> > those implementations where TLB entries are shared, without needing to >> > distinguish between them, at the expense of doing unnecessary >> > invalidations on both kinds of implementation. >> >> That's exactly what it is. Existing code accommodates shared TLBs, this >> patch additionally allows for private. >> >> >> I also have some concern with ordering of in-flight operations (ptesync, >> >> memory ordering, etc) which are mostly avoided with this. >> >> >> >> > The sane implementations would be ones where either (a) TLB entries >> >> > are private to each thread and tlbiel only works on the local thread, >> >> > or (b) TLB entries can be shared and tlbiel works across all threads. >> >> > I think this is the conclusion we collectively came to when working on >> >> > that bug we worked on towards the end of last year. >> >> >> >> I think an implementation could have both types. So it's hard to get >> >> away from flushing all threads. >> > >> > Having both private and shared TLB entries in the same implementation >> > would seem very odd to me. What would determine whether a given entry >> > is shared or private? >> >> Example: an ERAT or L1 TLB per-thread, and a shared L2 TLB behind that. >> The L1 may not be PID/LPID tagged so you don't want to cross-invalidate >> other threads every time, say. > > That's the insane implementation I referred to above, so we're back to > saying we need to allow for that kind of implementation. I misunderstood you then. The really insane implementation we were talking about a couple of months ago is one where a translation can be brought in by T0, then invalidated with TLBIEL on T0, but it is later visible to T1 if it switches to that context. A combination of private and shared TLBs I don't see as being insane, so long as (there is the appearance that) 1. translations are not created on threads that are not running that context, and 2. tlbiel invalidates translations visible to that thread. Thanks, Nick