Re: [PATCH RFCv1 6/8] kvm: Add helper kvm_dirty_ring_init()
On 2/9/23 9:11 AM, Juan Quintela wrote: Gavin Shan 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 --- 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
Re: [PATCH RFCv1 6/8] kvm: Add helper kvm_dirty_ring_init()
Gavin Shan 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 > --- > 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; > } > }
[PATCH RFCv1 6/8] kvm: Add helper kvm_dirty_ring_init()
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 --- 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; +} + 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; } } -- 2.23.0