Re: [PATCH 3/5] gdbstub: reject unsupported flags in handle_set_qemu_sstep

[Philippe Mathieu-Daudé]

On 11/11/21 12:06, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
> 
> handle_query_qemu_sstepbits is reporting NOIRQ and NOTIMER bits
> even if they are not supported (as is the case with record/replay).
> Instead, store the supported singlestep flags and reject
> any unsupported bits in handle_set_qemu_sstep.  This removes
> the need for the get_sstep_flags() wrapper.
> 
> While at it, move the variables in GDBState, instead of using
> global variables.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> [Extracted from Maxim's patch into a separate commit. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  gdbstub.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 47 insertions(+), 26 deletions(-)

>  static void init_gdbserver_state(void)
> @@ -399,6 +382,24 @@ static void init_gdbserver_state(void)
>      gdbserver_state.str_buf = g_string_new(NULL);
>      gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
>      gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 
> 4);

Simpler:

 gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;

> +    /*
> +     * In replay mode all events written into the log should be replayed.
> +     * That is why NOIRQ flag is removed in this mode.
> +     */

 if (replay_mode == REPLAY_MODE_NONE) {
   gdbserver_state.supported_sstep_flags |= SSTEP_NOIRQ | SSTEP_NOTIMER;
 }

> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
> +    } else {
> +        gdbserver_state.supported_sstep_flags =
> +            SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
> +    }
> +
> +    /*
> +     * By default use no IRQs and no timers while single stepping so as to
> +     * make single stepping like an ICE HW step.
> +     */
> +    gdbserver_state.sstep_flags = gdbserver_state.supported_sstep_flags;
> +
>  }

>  static void handle_set_qemu_sstep(GArray *params, void *user_ctx)
>  {
> +    int new_sstep_flags;
> +
>      if (!params->len) {
>          return;
>      }
>  
> -    sstep_flags = get_param(params, 0)->val_ul;
> +    new_sstep_flags = get_param(params, 0)->val_ul;
> +
> +    if (new_sstep_flags  & ~gdbserver_state.supported_sstep_flags) {
> +        put_packet("E22");
> +        return;
> +    }

Please does this change in a separate patch, after moving
to GDBState.

> +    gdbserver_state.sstep_flags = new_sstep_flags;
>      put_packet("OK");
>  }