Re: [PATCH 2/6] KVM: PPC: E500: Explicitly mark shadow maps invalid

[Scott Wood <scottwood@freescale.com>]

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