On Wed, Apr 24, 2024 at 3:17 PM Zhijian Li (Fujitsu) <lizhij...@fujitsu.com>
wrote:
>
>
> 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;
>
Hi, Masato Imai, would you like to continue this patch with version 3
as suggested?
>
>
> Thanks
> Zhijian
>
>
> > + if (kvm_state == NULL) {
> > + return false;
> > + }
> > return kvm_state->kvm_dirty_ring_size ? true : false;
> > }
> >
Thanks,
Yong
--
Best regards
