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 <agraf@xxxxxxx>
>> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
