| Message ID | 25094a6a-df4e-3dbb-ef69-e478edb46ff3@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018/6/12 20:17, Paolo Bonzini wrote: > 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 <zhaoshenglong@huawei.com> > > 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? > I test below patch. It works for our testcase. Do I need to fold them into one and resend? Thanks,
On 13/06/2018 04:15, Shannon Zhao wrote: > > > On 2018/6/12 20:17, Paolo Bonzini wrote: >> 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 <zhaoshenglong@huawei.com> >> >> 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? >> > I test below patch. It works for our testcase. > Do I need to fold them into one and resend? No need to, 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));