Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte
On Mon, Sep 22, 2008 at 11:26:36PM +0300, Avi Kivity wrote: Marcelo Tosatti wrote: I think we had cases where the spte.pfn contents changed, for example when a large page was replaced by a normal page, True. And the TLB is not flushed now for large-normal replace, in case the pte thats faulting is read-only. The local (and remote) TLB's must be flushed on large-normal replace. Can you prepare a patch for that, for -stable? First one in the v3 series. -- 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
Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte
On Fri, Sep 19, 2008 at 05:21:09PM -0700, Avi Kivity wrote:
Marcelo Tosatti wrote:
Since the sync page path can collapse flushes.
Also only flush if the spte was writable before.
Signed-off-by: Marcelo Tosatti [EMAIL PROTECTED]
@@ -1241,9 +1239,12 @@ static void mmu_set_spte(struct kvm_vcpu
}
}
if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
- dirty, largepage, gfn, pfn, speculative))
+ dirty, largepage, gfn, pfn, speculative)) {
if (write_fault)
*ptwrite = 1;
+if (was_writeble)
+kvm_x86_ops-tlb_flush(vcpu);
+}
I think we had cases where the spte.pfn contents changed, for example
when a large page was replaced by a normal page,
True. And the TLB is not flushed now for large-normal replace, in case
the pte thats faulting is read-only. The local (and remote) TLB's must
be flushed on large-normal replace.
(BTW the largepage patch is wrong, will reply to that soon).
and also:
} else if (pfn != spte_to_pfn(*shadow_pte)) {
That one is likely to crash the guest anyway, so I don't see the need
for a flush there:
Did you find out what's causing the errors in the first place (if
zap is not used)? It worries me greatly.
Yes, the problem is that the rmap code does not handle the qemu
process
mappings from vanishing while there is a present rmap. If that
happens,
and there is a fault for a gfn whose qemu mapping has been removed, a
different physical zero page will be allocated:
rmap a - gfn 0 - physical host page 0
mapping for gfn 0 gets removed
guest faults in gfn 0 through the same pte chain
rmap a - gfn 0 - physical host page 1
When instantiating the shadow mapping for the second time, the
is_rmap_pte check succeeds, so we release the reference grabbed by
gfn_to_page() at mmu_set_spte(). We now have a shadow mapping
pointing
to a physical page without having an additional reference on that
page.
The following makes the host not crash under such a condition, but
the condition itself is invalid leading to inconsistent state on the
guest.
So IMHO it shouldnt be allowed to happen in the first place.
--
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
Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte
Marcelo Tosatti wrote:
Since the sync page path can collapse flushes.
Also only flush if the spte was writable before.
Signed-off-by: Marcelo Tosatti [EMAIL PROTECTED]
@@ -1241,9 +1239,12 @@ static void mmu_set_spte(struct kvm_vcpu
}
}
if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
- dirty, largepage, gfn, pfn, speculative))
+ dirty, largepage, gfn, pfn, speculative)) {
if (write_fault)
*ptwrite = 1;
+ if (was_writeble)
+ kvm_x86_ops-tlb_flush(vcpu);
+ }
I think we had cases where the spte.pfn contents changed, for example
when a large page was replaced by a normal page, and also:
} else if (pfn != spte_to_pfn(*shadow_pte)) {
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
--
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
[patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte
Since the sync page path can collapse flushes.
Also only flush if the spte was writable before.
Signed-off-by: Marcelo Tosatti [EMAIL PROTECTED]
Index: kvm/arch/x86/kvm/mmu.c
===
--- kvm.orig/arch/x86/kvm/mmu.c
+++ kvm/arch/x86/kvm/mmu.c
@@ -1189,10 +1189,8 @@ static int set_spte(struct kvm_vcpu *vcp
__func__, gfn);
ret = 1;
pte_access = ~ACC_WRITE_MASK;
- if (is_writeble_pte(spte)) {
+ if (is_writeble_pte(spte))
spte = ~PT_WRITABLE_MASK;
- kvm_x86_ops-tlb_flush(vcpu);
- }
}
}
@@ -1241,9 +1239,12 @@ static void mmu_set_spte(struct kvm_vcpu
}
}
if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
- dirty, largepage, gfn, pfn, speculative))
+ dirty, largepage, gfn, pfn, speculative)) {
if (write_fault)
*ptwrite = 1;
+ if (was_writeble)
+ kvm_x86_ops-tlb_flush(vcpu);
+ }
pgprintk(%s: setting spte %llx\n, __func__, *shadow_pte);
pgprintk(instantiating %s PTE (%s) at %ld (%llx) addr %p\n,
--
--
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
