* [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-05-16 9:18 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-05-16 9:18 UTC (permalink / raw) To: qemu-devel, pbonzini, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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> --- include/sysemu/kvm_int.h | 1 + kvm-all.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h index 888557a..f838412 100644 --- a/include/sysemu/kvm_int.h +++ b/include/sysemu/kvm_int.h @@ -20,6 +20,7 @@ typedef struct KVMSlot void *ram; int slot; int flags; + int old_flags; } KVMSlot; typedef struct KVMMemoryListener { diff --git a/kvm-all.c b/kvm-all.c index 2515a23..de8250e 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -252,7 +252,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 & KVM_MEM_READONLY) { + if (slot->memory_size && (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; @@ -376,11 +376,11 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, { int old_flags; - old_flags = mem->flags; + mem->old_flags = mem->flags; mem->flags = kvm_mem_flags(mr); /* If nothing changed effectively, no need to issue ioctl */ - if (mem->flags == old_flags) { + if (mem->flags == mem->old_flags) { return 0; } -- 2.0.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-05-16 9:18 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-05-16 9:18 UTC (permalink / raw) To: qemu-devel, pbonzini, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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> --- include/sysemu/kvm_int.h | 1 + kvm-all.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h index 888557a..f838412 100644 --- a/include/sysemu/kvm_int.h +++ b/include/sysemu/kvm_int.h @@ -20,6 +20,7 @@ typedef struct KVMSlot void *ram; int slot; int flags; + int old_flags; } KVMSlot; typedef struct KVMMemoryListener { diff --git a/kvm-all.c b/kvm-all.c index 2515a23..de8250e 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -252,7 +252,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 & KVM_MEM_READONLY) { + if (slot->memory_size && (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; @@ -376,11 +376,11 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, { int old_flags; - old_flags = mem->flags; + mem->old_flags = mem->flags; mem->flags = kvm_mem_flags(mr); /* If nothing changed effectively, no need to issue ioctl */ - if (mem->flags == old_flags) { + if (mem->flags == mem->old_flags) { return 0; } -- 2.0.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-05-16 9:18 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-05-16 9:18 UTC (permalink / raw) To: qemu-devel, pbonzini, guangrong.xiao Cc: kvmarm, shannon.zhaosl, kvm, zhengxiang9 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> --- include/sysemu/kvm_int.h | 1 + kvm-all.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h index 888557a..f838412 100644 --- a/include/sysemu/kvm_int.h +++ b/include/sysemu/kvm_int.h @@ -20,6 +20,7 @@ typedef struct KVMSlot void *ram; int slot; int flags; + int old_flags; } KVMSlot; typedef struct KVMMemoryListener { diff --git a/kvm-all.c b/kvm-all.c index 2515a23..de8250e 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -252,7 +252,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 & KVM_MEM_READONLY) { + if (slot->memory_size && (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; @@ -376,11 +376,11 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, { int old_flags; - old_flags = mem->flags; + mem->old_flags = mem->flags; mem->flags = kvm_mem_flags(mr); /* If nothing changed effectively, no need to issue ioctl */ - if (mem->flags == old_flags) { + if (mem->flags == mem->old_flags) { return 0; } -- 2.0.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed 2018-05-16 9:18 ` [Qemu-devel] " Shannon Zhao (?) @ 2018-06-12 1:36 ` Shannon Zhao -1 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-06-12 1:36 UTC (permalink / raw) To: qemu-devel, pbonzini, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm Ping? On 2018/5/16 17: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> > --- > include/sysemu/kvm_int.h | 1 + > kvm-all.c | 6 +++--- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h > index 888557a..f838412 100644 > --- a/include/sysemu/kvm_int.h > +++ b/include/sysemu/kvm_int.h > @@ -20,6 +20,7 @@ typedef struct KVMSlot > void *ram; > int slot; > int flags; > + int old_flags; > } KVMSlot; > > typedef struct KVMMemoryListener { > diff --git a/kvm-all.c b/kvm-all.c > index 2515a23..de8250e 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -252,7 +252,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 & KVM_MEM_READONLY) { > + if (slot->memory_size && (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; > @@ -376,11 +376,11 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, > { > int old_flags; > > - old_flags = mem->flags; > + mem->old_flags = mem->flags; > mem->flags = kvm_mem_flags(mr); > > /* If nothing changed effectively, no need to issue ioctl */ > - if (mem->flags == old_flags) { > + if (mem->flags == mem->old_flags) { > return 0; > } > > -- Shannon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-06-12 1:36 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-06-12 1:36 UTC (permalink / raw) To: qemu-devel, pbonzini, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm Ping? On 2018/5/16 17: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> > --- > include/sysemu/kvm_int.h | 1 + > kvm-all.c | 6 +++--- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h > index 888557a..f838412 100644 > --- a/include/sysemu/kvm_int.h > +++ b/include/sysemu/kvm_int.h > @@ -20,6 +20,7 @@ typedef struct KVMSlot > void *ram; > int slot; > int flags; > + int old_flags; > } KVMSlot; > > typedef struct KVMMemoryListener { > diff --git a/kvm-all.c b/kvm-all.c > index 2515a23..de8250e 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -252,7 +252,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 & KVM_MEM_READONLY) { > + if (slot->memory_size && (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; > @@ -376,11 +376,11 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, > { > int old_flags; > > - old_flags = mem->flags; > + mem->old_flags = mem->flags; > mem->flags = kvm_mem_flags(mr); > > /* If nothing changed effectively, no need to issue ioctl */ > - if (mem->flags == old_flags) { > + if (mem->flags == mem->old_flags) { > return 0; > } > > -- Shannon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-06-12 1:36 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-06-12 1:36 UTC (permalink / raw) To: qemu-devel, pbonzini, guangrong.xiao Cc: kvmarm, shannon.zhaosl, kvm, zhengxiang9 Ping? On 2018/5/16 17: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> > --- > include/sysemu/kvm_int.h | 1 + > kvm-all.c | 6 +++--- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h > index 888557a..f838412 100644 > --- a/include/sysemu/kvm_int.h > +++ b/include/sysemu/kvm_int.h > @@ -20,6 +20,7 @@ typedef struct KVMSlot > void *ram; > int slot; > int flags; > + int old_flags; > } KVMSlot; > > typedef struct KVMMemoryListener { > diff --git a/kvm-all.c b/kvm-all.c > index 2515a23..de8250e 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -252,7 +252,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 & KVM_MEM_READONLY) { > + if (slot->memory_size && (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; > @@ -376,11 +376,11 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, > { > int old_flags; > > - old_flags = mem->flags; > + mem->old_flags = mem->flags; > mem->flags = kvm_mem_flags(mr); > > /* If nothing changed effectively, no need to issue ioctl */ > - if (mem->flags == old_flags) { > + if (mem->flags == mem->old_flags) { > return 0; > } > > -- Shannon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed 2018-05-16 9:18 ` [Qemu-devel] " Shannon Zhao @ 2018-06-12 12:17 ` Paolo Bonzini -1 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2018-06-12 12:17 UTC (permalink / raw) To: Shannon Zhao, qemu-devel, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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? 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)); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-06-12 12:17 ` Paolo Bonzini 0 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2018-06-12 12:17 UTC (permalink / raw) To: Shannon Zhao, qemu-devel, guangrong.xiao Cc: kvmarm, shannon.zhaosl, kvm, zhengxiang9 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? 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)); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed 2018-06-12 12:17 ` [Qemu-devel] " Paolo Bonzini (?) @ 2018-06-13 2:15 ` Shannon Zhao -1 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-06-13 2:15 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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, -- Shannon > 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)); > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-06-13 2:15 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-06-13 2:15 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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, -- Shannon > 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)); > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-06-13 2:15 ` Shannon Zhao 0 siblings, 0 replies; 13+ messages in thread From: Shannon Zhao @ 2018-06-13 2:15 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, guangrong.xiao Cc: kvmarm, shannon.zhaosl, kvm, zhengxiang9 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, -- Shannon > 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)); > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed 2018-06-13 2:15 ` [Qemu-devel] " Shannon Zhao @ 2018-06-13 15:58 ` Paolo Bonzini -1 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2018-06-13 15:58 UTC (permalink / raw) To: Shannon Zhao, qemu-devel, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed @ 2018-06-13 15:58 ` Paolo Bonzini 0 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2018-06-13 15:58 UTC (permalink / raw) To: Shannon Zhao, qemu-devel, guangrong.xiao; +Cc: shannon.zhaosl, kvmarm, kvm 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-06-13 15:58 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-16 9:18 [PATCH] kvm: Delete the slot only when KVM_MEM_READONLY flag is changed Shannon Zhao 2018-05-16 9:18 ` Shannon Zhao 2018-05-16 9:18 ` [Qemu-devel] " Shannon Zhao 2018-06-12 1:36 ` Shannon Zhao 2018-06-12 1:36 ` Shannon Zhao 2018-06-12 1:36 ` [Qemu-devel] " Shannon Zhao 2018-06-12 12:17 ` Paolo Bonzini 2018-06-12 12:17 ` [Qemu-devel] " Paolo Bonzini 2018-06-13 2:15 ` Shannon Zhao 2018-06-13 2:15 ` Shannon Zhao 2018-06-13 2:15 ` [Qemu-devel] " Shannon Zhao 2018-06-13 15:58 ` Paolo Bonzini 2018-06-13 15:58 ` [Qemu-devel] " Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.