On 24/04/2024 12:52, mii wrote: > > On 2024/04/24 10:28, Yong Huang wrote: >> >> >> On Tue, Apr 23, 2024 at 9:35 PM Peter Xu <pet...@redhat.com> wrote: >> >> On Tue, Apr 23, 2024 at 09:13:08AM +0000, Masato Imai wrote: >> > When the KVM acceleration parameter is not set, executing >> calc_dirty_rate >> > with the -r or -b option results in a segmentation fault due to >> accessing >> > a null kvm_state pointer in the kvm_dirty_ring_enabled function. >> > This commit adds a check for kvm_enabled to prevent segmentation >> faults. >> > >> > Signed-off-by: Masato Imai <m...@sfc.wide.ad.jp> >> > --- >> > migration/dirtyrate.c | 7 +++++++ >> > 1 file changed, 7 insertions(+) >> > >> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c >> > index 1d2e85746f..2a7df52519 100644 >> > --- a/migration/dirtyrate.c >> > +++ b/migration/dirtyrate.c >> > @@ -799,6 +799,13 @@ void qmp_calc_dirty_rate(int64_t calc_time, >> > * dirty ring mode only works when kvm dirty ring is enabled. >> > * on the contrary, dirty bitmap mode is not. >> > */ >> > + if (!kvm_enabled() && >> > + (mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING || >> > + mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP)) { >> > + error_setg(errp, "mode %s requires kvm to be enabled.", >> > + DirtyRateMeasureMode_str(mode)); >> > + return; >> > + } >> >> Logically dirty bitmap should work with tcg. So the other option is to >> let >> kvm_dirty_ring_enabled() check kvm_state too and return false if >> kvm_state==NULL? >> >> >> Agree, better solution >> >> >> > if (((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) && >> > !kvm_dirty_ring_enabled()) || >> > ((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) && >> > -- >> > 2.34.1 >> > >> >> -- >> Peter Xu >> >> >> Thanks, >> Yong >> >> >> -- >> Best regards > > Thank you for the review. I agree with that solution. > > Update will be like: > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 931f74256e..0f8499365d 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2312,6 +2312,9 @@ bool kvm_vcpu_id_is_valid(int vcpu_id) > > bool kvm_dirty_ring_enabled(void) > {
How about, return kvm_state && vm_state->kvm_dirty_ring_size; Thanks Zhijian > + if (kvm_state == NULL) { > + return false; > + } > return kvm_state->kvm_dirty_ring_size ? true : false; > } >