[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed |
Date: |
Tue, 12 Jun 2018 14:17:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
On 16/05/2018 11:18, Shannon Zhao wrote:
> According to KVM commit 75d61fbc, it needs to delete the slot before
> changing the KVM_MEM_READONLY flag. But QEMU commit 235e8982 only check
> whether KVM_MEM_READONLY flag is set instead of changing. It doesn't
> need to delete the slot if the KVM_MEM_READONLY flag is not changed.
>
> This fixes a issue that migrating a VM at the OVMF startup stage and
> VM is executing the codes in rom. Between the deleting and adding the
> slot in kvm_set_user_memory_region, there is a chance that guest access
> rom and trap to KVM, then KVM can't find the corresponding memslot.
> While KVM (on ARM) injects an abort to guest due to the broken hva, then
> guest will get stuck.
>
> Signed-off-by: Shannon Zhao <address@hidden>
I'm a bit worried about old_flags not being set on all paths to
kvm_set_user_memory_region. This would lead to extra
KVM_SET_USER_MEMORY_REGION calls. It should not be a problem but
it is ugly. Does something like the additional changes below work for you?
Thanks,
Paolo
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b04f193a76..e318bcfb78 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -257,7 +257,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void
*ram,
return 0;
}
-static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot)
+static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot,
bool new)
{
KVMState *s = kvm_state;
struct kvm_userspace_memory_region mem;
@@ -268,7 +268,7 @@ static int kvm_set_user_memory_region(KVMMemoryListener
*kml, KVMSlot *slot)
mem.userspace_addr = (unsigned long)slot->ram;
mem.flags = slot->flags;
- if (slot->memory_size && (mem.flags ^ slot->old_flags) & KVM_MEM_READONLY)
{
+ if (slot->memory_size && !new && (mem.flags ^ slot->old_flags) &
KVM_MEM_READONLY) {
/* Set the slot size to 0 before setting the slot to the desired
* value. This is needed based on KVM commit 75d61fbc. */
mem.memory_size = 0;
@@ -276,6 +276,7 @@ static int kvm_set_user_memory_region(KVMMemoryListener
*kml, KVMSlot *slot)
}
mem.memory_size = slot->memory_size;
ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
+ slot->old_flags = mem.flags;
trace_kvm_set_user_memory(mem.slot, mem.flags, mem.guest_phys_addr,
mem.memory_size, mem.userspace_addr, ret);
return ret;
@@ -394,7 +395,6 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml,
KVMSlot *mem,
{
int old_flags;
- mem->old_flags = mem->flags;
mem->flags = kvm_mem_flags(mr);
/* If nothing changed effectively, no need to issue ioctl */
@@ -402,7 +402,7 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml,
KVMSlot *mem,
return 0;
}
- return kvm_set_user_memory_region(kml, mem);
+ return kvm_set_user_memory_region(kml, mem, false);
}
static int kvm_section_update_flags(KVMMemoryListener *kml,
@@ -756,7 +756,8 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
/* unregister the slot */
mem->memory_size = 0;
- err = kvm_set_user_memory_region(kml, mem);
+ mem->flags = 0;
+ err = kvm_set_user_memory_region(kml, mem, false);
if (err) {
fprintf(stderr, "%s: error unregistering slot: %s\n",
__func__, strerror(-err));
@@ -772,7 +773,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
mem->ram = ram;
mem->flags = kvm_mem_flags(mr);
- err = kvm_set_user_memory_region(kml, mem);
+ err = kvm_set_user_memory_region(kml, mem, true);
if (err) {
fprintf(stderr, "%s: error registering slot: %s\n", __func__,
strerror(-err));