From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [PATCH 2/6] KVM: PPC: E500: Explicitly mark shadow maps invalid Date: Fri, 18 Jan 2013 13:03:12 -0600 Message-ID: <1358535792.27354.4@snotra> References: <67C46908-05AE-4902-97C2-FB8401FA09D0@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: , To: Alexander Graf Return-path: In-Reply-To: <67C46908-05AE-4902-97C2-FB8401FA09D0@suse.de> (from agraf@suse.de on Fri Jan 18 08:08:01 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 01/18/2013 08:08:01 AM, Alexander Graf wrote: > > On 18.01.2013, at 04:05, Scott Wood wrote: > > > Invalidation paths that call kvmppc_e500_tlbil_all(), such as > MMUCSR0 and tlbivax, need a call to clear_tlb_refs() in order to get > the valid bits cleared. > > Yeah, instead of kvmppc_e500_tlbil_all(). This might need a bit of > rework, since we end up flushing the full host TLB / cache even when > only evicting a single page at times. I assume you're talking about the tlbivax single-page case. That could certainly be improved, but I'd consider it a low priority until we have a guest that actually uses it (Linux does tlbsx+tlbwe to locally invalidate single pages on e500v2). > So one more question I have here is why kvmppc_core_flush_tlb() calls > clear_tlb1_bitmap(). > That function looks like a better candidate for "flush all of the > pending host translation" than clear_tlb_refs() which to me sounds > more like an implementation detail of the host TLB implementation. I don't quite follow. The implementation of kvmppc_core_flush_tlb() is also an implementation detail of the host TLB implementation. > After clear_tlb_refs() there shouldn't be any pending bitmaps lying > around, right? clear_tlb_refs() doesn't clear the bitmap. It does a bulk invalidation rather than calling inval_gtlbe_on_host(). Perhaps we should move the clear_tlb1_bitmap() call into clear_tlb_refs() and rename the latter to kvmppc_core_flush_tlb(). The dirty_tlb ioctl calls clear_tlb_refs() without clearing the bitmap, but that's probably a bug. > > In looking this up, I also saw that tlbilxlpid (T=0) seems to be > broken; it compares PID/TID as if it were T=1. Don't be fooled by > the "lpid" in the name; it's still relevant (and different from T=1) > in the absence of E.HV, and should be treated as "tlbilx all". Once > implemented, that will also presumably use kvmppc_e500_tlbil_all(). > > Yay :). That's a follow-up patch though. Let me put it on the todo > list. Never mind, I wasn't reading the code closely enough. T=0 will use inval_gtlbe_on_host() for all entries, though kvmppc_e500_tlbil_all() would be more efficient. -Scott