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 <peterx@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 <mii@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;
>   }
>