On 18.01.2013, at 04:05, Scott Wood wrote: > On 01/17/2013 08:34:53 PM, Alexander Graf wrote: >> When we invalidate shadow TLB maps on the host, we don't mark them >> as not valid. But we should. >> Fix this by removing the E500_TLB_VALID from their flags when >> invalidating. >> Signed-off-by: Alexander Graf <[email protected]> >> --- >> arch/powerpc/kvm/e500_tlb.c | 13 ++++++++++--- >> 1 files changed, 10 insertions(+), 3 deletions(-) >> diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c >> index d38ad63..8efb2ac 100644 >> --- a/arch/powerpc/kvm/e500_tlb.c >> +++ b/arch/powerpc/kvm/e500_tlb.c >> @@ -204,9 +204,13 @@ static void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 >> *vcpu_e500, >> { >> struct kvm_book3e_206_tlb_entry *gtlbe = >> get_entry(vcpu_e500, tlbsel, esel); >> + struct tlbe_ref *ref = &vcpu_e500->gtlb_priv[tlbsel][esel].ref; >> - if (tlbsel == 1 && >> - vcpu_e500->gtlb_priv[1][esel].ref.flags & E500_TLB_BITMAP) { >> + /* Don't bother with unmapped entries */ >> + if (!(ref->flags & E500_TLB_VALID)) >> + return; >> + >> + if (tlbsel == 1 && ref->flags & E500_TLB_BITMAP) { >> u64 tmp = vcpu_e500->g2h_tlb1_map[esel]; >> int hw_tlb_indx; >> unsigned long flags; >> @@ -224,7 +228,7 @@ static void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 >> *vcpu_e500, >> } >> mb(); >> vcpu_e500->g2h_tlb1_map[esel] = 0; >> - vcpu_e500->gtlb_priv[1][esel].ref.flags &= ~E500_TLB_BITMAP; >> + ref->flags &= ~(E500_TLB_BITMAP | E500_TLB_VALID); >> local_irq_restore(flags); >> return; >> @@ -232,6 +236,9 @@ static void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 >> *vcpu_e500, >> /* Guest tlbe is backed by at most one host tlbe per shadow pid. */ >> kvmppc_e500_tlbil_one(vcpu_e500, gtlbe); >> + >> + /* Mark the TLB as not backed by the host anymore */ >> + ref->flags &= ~E500_TLB_VALID; >> } > > 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. 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. After clear_tlb_refs() there shouldn't be any pending bitmaps lying around, right? > 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. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
