Marcelo Tosatti wrote:
On Mon, Jul 07, 2008 at 02:31:55PM -0300, Marcelo Tosatti wrote:
On Sun, Jul 06, 2008 at 12:15:56AM +0300, Avi Kivity wrote:
Marcelo Tosatti wrote:
On Sat, Jul 05, 2008 at 08:25:30PM +0300, Avi Kivity wrote:
@@ -1955,6 +1955,22 @@ void kvm_mmu_slot_remove_write_access(st
}
}
+int kvm_mmu_slot_has_shadowed_page(struct kvm *kvm, int slot)
+{
+ struct kvm_mmu_page *sp;
+ int ret = 0;
+
+ spin_lock(&kvm->mmu_lock);
+ list_for_each_entry(sp, &kvm->arch.active_mmu_pages, link) {
+ if (test_bit(slot, &sp->slot_bitmap)) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+ spin_unlock(&kvm->mmu_lock);
+ return ret;
+}
+
I don't like the guest influencing host actions in this way. It's
just a guest.
But I think it's unneeded. kvm_mmu_zap_page() will mark a root
shadow page invalid and force all vcpus to reload it, so all that's
needed is to keep the mmu spinlock held while removing the slot.
You're still keeping a shadowed page around with sp->gfn pointing to
non-existant memslot. The code generally makes the assumption that
gfn_to_memslot(gfn) on shadowed info will not fail.
kvm_mmu_zap_page -> unaccount_shadowed, for example.
The page has already been zapped, so we might as well
unaccount_shadowed() on the first run. It needs to be moved until after
the reload_remote_mmus() call, though.
Oops, previous patch was unaccounting multiple times for invalid pages.
This should be better:
During RH6.2 graphical installation the following oops is triggered:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffffa00bf172>] :kvm:gfn_to_rmap+0x3e/0x61
Pid: 4559, comm: qemu-system-x86 Not tainted
The problem is that KVM allows shadow pagetable entries that
point to a removed memslot to exist. In this case the cirrus vram
mapping was removed, and the NULL dereference happened during
kvm_set_memory_alias()'s zap_all_pages().
So nuke all shadowed pages before memslot removal.
Signed-off-by: Marcelo Tosatti <[EMAIL PROTECTED]>
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index a4cf4a2..76259da 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1455,6 +1455,10 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
return 0;
}
+int kvm_arch_destroy_memory_region(struct kvm *kvm, int slot)
+{
+ return 0;
+}
This (and its friends) ought to be static inlines.
On the other hand, don't the other arches have to flush their tlbs?
Xiantao/Hollis? So maybe this function needs to be renamed
kvm_flush_shadow() and implemented across the board.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b90da0b..5ef3a5e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -405,6 +405,12 @@ int __kvm_set_memory_region(struct kvm *kvm,
if (mem->slot >= kvm->nmemslots)
kvm->nmemslots = mem->slot + 1;
+ if (!npages) {
+ r = kvm_arch_destroy_memory_region(kvm, mem->slot);
+ if (r)
+ goto out_free;
+ }
+
Destructors should never fail, since there is no possible recovery. And
indeed you have 'return 0' in the actual implementation. So I think the
function better return void.
--
error compiling committee.c: too many arguments to function
--
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
