| Message ID | 20230206112010.99871-7-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Support dirty ring | expand |
Gavin Shan <gshan@redhat.com> wrote: > Due to multiple capabilities associated with the dirty ring for different > architectures: KVM_CAP_DIRTY_{LOG_RING, LOG_RING_ACQ_REL} for x86 and > arm64 separately. There will be more to be done in order to support the > dirty ring for arm64. > > Lets add helper kvm_dirty_ring_init() to enable the dirty ring. With this, > the code looks a bit clean. > > No functional change intended. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > accel/kvm/kvm-all.c | 73 ++++++++++++++++++++++++++++----------------- > 1 file changed, 46 insertions(+), 27 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 9ec117c441..399ef0f7e6 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -1476,6 +1476,49 @@ static int kvm_dirty_ring_reaper_init(KVMState *s) > return 0; > } > > +static int kvm_dirty_ring_init(KVMState *s) > +{ > + uint64_t ring_bytes; > + int ret; > + > + /* > + * Read the max supported pages. Fall back to dirty logging mode > + * if the dirty ring isn't supported. > + */ > + ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); > + if (ret <= 0) { > + warn_report("KVM dirty ring not available, using bitmap method"); > + s->kvm_dirty_ring_size = 0; > + return 0; > + } > + > + ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); > + if (ring_bytes > ret) { > + error_report("KVM dirty ring size %" PRIu32 " too big " > + "(maximum is %ld). Please use a smaller value.", > + s->kvm_dirty_ring_size, > + (long)ret / sizeof(struct kvm_dirty_gfn)); > + ret = -EINVAL; > + goto out; > + } > + > + ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); > + if (ret) { > + error_report("Enabling of KVM dirty ring failed: %s. " > + "Suggested minimum value is 1024.", strerror(-ret)); > + ret = -EIO; > + } > + > +out: > + if (ret) { > + s->kvm_dirty_ring_size = 0; > + } else { > + s->kvm_dirty_ring_bytes = ring_bytes; > + } > + > + return ret; > +} If you split it, you don't need the goto. static int kvm_dirty_ring_init(KVMState *s) { s->kvm_dirty_ring_size = 0; /* * Read the max supported pages. Fall back to dirty logging mode * if the dirty ring isn't supported. */ int ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); if (ret <= 0) { warn_report("KVM dirty ring not available, using bitmap method"); return 0; } uint64_t ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); if (ring_bytes > ret) { error_report("KVM dirty ring size %" PRIu32 " too big " "(maximum is %ld). Please use a smaller value.", s->kvm_dirty_ring_size, (long)ret / sizeof(struct kvm_dirty_gfn)); return -EINVAL; } ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); if (ret) { error_report("Enabling of KVM dirty ring failed: %s. " "Suggested minimum value is 1024.", strerror(-ret)); return -EIO; } s->kvm_dirty_ring_bytes = ring_bytes; return ret; } > + > static void kvm_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > @@ -2545,33 +2588,9 @@ static int kvm_init(MachineState *ms) > * dirty logging mode > */ > if (s->kvm_dirty_ring_size > 0) { > - uint64_t ring_bytes; > - > - ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); > - > - /* Read the max supported pages */ > - ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); > - if (ret > 0) { > - if (ring_bytes > ret) { > - error_report("KVM dirty ring size %" PRIu32 " too big " > - "(maximum is %ld). Please use a smaller value.", > - s->kvm_dirty_ring_size, > - (long)ret / sizeof(struct kvm_dirty_gfn)); > - ret = -EINVAL; > - goto err; > - } > - > - ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); > - if (ret) { > - error_report("Enabling of KVM dirty ring failed: %s. " > - "Suggested minimum value is 1024.", strerror(-ret)); > - goto err; > - } > - > - s->kvm_dirty_ring_bytes = ring_bytes; > - } else { > - warn_report("KVM dirty ring not available, using bitmap method"); > - s->kvm_dirty_ring_size = 0; > + ret = kvm_dirty_ring_init(s); > + if (ret < 0) { > + goto err; > } > }
On 2/9/23 9:11 AM, Juan Quintela wrote: > Gavin Shan <gshan@redhat.com> wrote: >> Due to multiple capabilities associated with the dirty ring for different >> architectures: KVM_CAP_DIRTY_{LOG_RING, LOG_RING_ACQ_REL} for x86 and >> arm64 separately. There will be more to be done in order to support the >> dirty ring for arm64. >> >> Lets add helper kvm_dirty_ring_init() to enable the dirty ring. With this, >> the code looks a bit clean. >> >> No functional change intended. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> accel/kvm/kvm-all.c | 73 ++++++++++++++++++++++++++++----------------- >> 1 file changed, 46 insertions(+), 27 deletions(-) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index 9ec117c441..399ef0f7e6 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -1476,6 +1476,49 @@ static int kvm_dirty_ring_reaper_init(KVMState *s) >> return 0; >> } >> >> +static int kvm_dirty_ring_init(KVMState *s) >> +{ >> + uint64_t ring_bytes; >> + int ret; >> + >> + /* >> + * Read the max supported pages. Fall back to dirty logging mode >> + * if the dirty ring isn't supported. >> + */ >> + ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); >> + if (ret <= 0) { >> + warn_report("KVM dirty ring not available, using bitmap method"); >> + s->kvm_dirty_ring_size = 0; >> + return 0; >> + } >> + >> + ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); >> + if (ring_bytes > ret) { >> + error_report("KVM dirty ring size %" PRIu32 " too big " >> + "(maximum is %ld). Please use a smaller value.", >> + s->kvm_dirty_ring_size, >> + (long)ret / sizeof(struct kvm_dirty_gfn)); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); >> + if (ret) { >> + error_report("Enabling of KVM dirty ring failed: %s. " >> + "Suggested minimum value is 1024.", strerror(-ret)); >> + ret = -EIO; >> + } >> + >> +out: >> + if (ret) { >> + s->kvm_dirty_ring_size = 0; >> + } else { >> + s->kvm_dirty_ring_bytes = ring_bytes; >> + } >> + >> + return ret; >> +} > > If you split it, you don't need the goto. > > static int kvm_dirty_ring_init(KVMState *s) > { > s->kvm_dirty_ring_size = 0; > /* > * Read the max supported pages. Fall back to dirty logging mode > * if the dirty ring isn't supported. > */ > int ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); > if (ret <= 0) { > warn_report("KVM dirty ring not available, using bitmap method"); > return 0; > } > > uint64_t ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); > if (ring_bytes > ret) { > error_report("KVM dirty ring size %" PRIu32 " too big " > "(maximum is %ld). Please use a smaller value.", > s->kvm_dirty_ring_size, > (long)ret / sizeof(struct kvm_dirty_gfn)); > return -EINVAL; > } > > ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); > if (ret) { > error_report("Enabling of KVM dirty ring failed: %s. " > "Suggested minimum value is 1024.", strerror(-ret)); > return -EIO; > } > > s->kvm_dirty_ring_bytes = ring_bytes; > return ret; > } > Ok, thanks for your review. The goto will be removed in next revision. > > >> + >> static void kvm_region_add(MemoryListener *listener, >> MemoryRegionSection *section) >> { >> @@ -2545,33 +2588,9 @@ static int kvm_init(MachineState *ms) >> * dirty logging mode >> */ >> if (s->kvm_dirty_ring_size > 0) { >> - uint64_t ring_bytes; >> - >> - ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); >> - >> - /* Read the max supported pages */ >> - ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); >> - if (ret > 0) { >> - if (ring_bytes > ret) { >> - error_report("KVM dirty ring size %" PRIu32 " too big " >> - "(maximum is %ld). Please use a smaller value.", >> - s->kvm_dirty_ring_size, >> - (long)ret / sizeof(struct kvm_dirty_gfn)); >> - ret = -EINVAL; >> - goto err; >> - } >> - >> - ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); >> - if (ret) { >> - error_report("Enabling of KVM dirty ring failed: %s. " >> - "Suggested minimum value is 1024.", strerror(-ret)); >> - goto err; >> - } >> - >> - s->kvm_dirty_ring_bytes = ring_bytes; >> - } else { >> - warn_report("KVM dirty ring not available, using bitmap method"); >> - s->kvm_dirty_ring_size = 0; >> + ret = kvm_dirty_ring_init(s); >> + if (ret < 0) { >> + goto err; >> } >> } > Thanks, Gavin
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 9ec117c441..399ef0f7e6 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1476,6 +1476,49 @@ static int kvm_dirty_ring_reaper_init(KVMState *s) return 0; } +static int kvm_dirty_ring_init(KVMState *s) +{ + uint64_t ring_bytes; + int ret; + + /* + * Read the max supported pages. Fall back to dirty logging mode + * if the dirty ring isn't supported. + */ + ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); + if (ret <= 0) { + warn_report("KVM dirty ring not available, using bitmap method"); + s->kvm_dirty_ring_size = 0; + return 0; + } + + ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); + if (ring_bytes > ret) { + error_report("KVM dirty ring size %" PRIu32 " too big " + "(maximum is %ld). Please use a smaller value.", + s->kvm_dirty_ring_size, + (long)ret / sizeof(struct kvm_dirty_gfn)); + ret = -EINVAL; + goto out; + } + + ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); + if (ret) { + error_report("Enabling of KVM dirty ring failed: %s. " + "Suggested minimum value is 1024.", strerror(-ret)); + ret = -EIO; + } + +out: + if (ret) { + s->kvm_dirty_ring_size = 0; + } else { + s->kvm_dirty_ring_bytes = ring_bytes; + } + + return ret; +} + static void kvm_region_add(MemoryListener *listener, MemoryRegionSection *section) { @@ -2545,33 +2588,9 @@ static int kvm_init(MachineState *ms) * dirty logging mode */ if (s->kvm_dirty_ring_size > 0) { - uint64_t ring_bytes; - - ring_bytes = s->kvm_dirty_ring_size * sizeof(struct kvm_dirty_gfn); - - /* Read the max supported pages */ - ret = kvm_vm_check_extension(s, KVM_CAP_DIRTY_LOG_RING); - if (ret > 0) { - if (ring_bytes > ret) { - error_report("KVM dirty ring size %" PRIu32 " too big " - "(maximum is %ld). Please use a smaller value.", - s->kvm_dirty_ring_size, - (long)ret / sizeof(struct kvm_dirty_gfn)); - ret = -EINVAL; - goto err; - } - - ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes); - if (ret) { - error_report("Enabling of KVM dirty ring failed: %s. " - "Suggested minimum value is 1024.", strerror(-ret)); - goto err; - } - - s->kvm_dirty_ring_bytes = ring_bytes; - } else { - warn_report("KVM dirty ring not available, using bitmap method"); - s->kvm_dirty_ring_size = 0; + ret = kvm_dirty_ring_init(s); + if (ret < 0) { + goto err; } }
Due to multiple capabilities associated with the dirty ring for different architectures: KVM_CAP_DIRTY_{LOG_RING, LOG_RING_ACQ_REL} for x86 and arm64 separately. There will be more to be done in order to support the dirty ring for arm64. Lets add helper kvm_dirty_ring_init() to enable the dirty ring. With this, the code looks a bit clean. No functional change intended. Signed-off-by: Gavin Shan <gshan@redhat.com> --- accel/kvm/kvm-all.c | 73 ++++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 27 deletions(-)