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

[Paolo Bonzini]

On 11/11/21 12:38, Philippe Mathieu-Daudé wrote:
> 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;
>   }

This variant may be simpler now, but not with the next patch.  This is
because something like this is nasty (see the "=" vs "|=" difference):

     gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
     if (kvm_enabled()) {
        gdbserver_state.supported_sstep_flags = kvm_get_supported_sstep_flags();
     } else {
         gdbserver_state.supported_sstep_flags |=
             SSTEP_NOIRQ | SSTEP_NOTIMER;
     }

and something like this is technically incorrect, because a hypothetical
non-TCG record/replay would have the same limitation in the sstep_flags:

     gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
     if (kvm_enabled()) {
        gdbserver_state.supported_sstep_flags = kvm_get_supported_sstep_flags();
     } else {
         gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
         if (replay_mode != REPLAY_MODE_NONE) {
             gdbserver_state.supported_sstep_flags |=
                 SSTEP_NOIRQ | SSTEP_NOTIMER;
     }

> > +
> > +    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.

Honestly it seems overkill.  The point of this patch is to add _this_
check, everything else is just a means to this end.  Before, there was
no concept of supported flags, so there was only one variable.  Now that
there are two variables, it makes sense to move them to GDBState.  Also,
with no concept of supported flags it would not be possible to avoid the
get_sstep_flags() function.

If I had to split the patch further, I'd first move sstep_flags to
gdbserver_state.sstep_flags, and then do everything else, but IMO this
patch is already small enough and easy enough to review.

Paolo