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(-)
diff --git a/gdbstub.c b/gdbstub.c
index 23baaef40e..960b9fbcd0 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -368,27 +368,10 @@ typedef struct GDBState {
gdb_syscall_complete_cb current_syscall_cb;
GString *str_buf;
GByteArray *mem_buf;
+ int sstep_flags;
+ int supported_sstep_flags;
} GDBState;
-/* By default use no IRQs and no timers while single stepping so as to
- * make single stepping like an ICE HW step.
- */
-static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
-
-/* Retrieves flags for single step mode. */
-static int get_sstep_flags(void)
-{
- /*
- * 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) {
- return SSTEP_ENABLE;
- } else {
- return sstep_flags;
- }
-}
-
static GDBState gdbserver_state;
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);
+
+ /*
+ * 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_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;
+
}
#ifndef CONFIG_USER_ONLY
@@ -505,7 +506,7 @@ static int gdb_continue_partial(char *newstates)
CPU_FOREACH(cpu) {
if (newstates[cpu->cpu_index] == 's') {
trace_gdbstub_op_stepping(cpu->cpu_index);
- cpu_single_step(cpu, sstep_flags);
+ cpu_single_step(cpu, gdbserver_state.sstep_flags);
}
}
gdbserver_state.running_state = 1;
@@ -524,7 +525,7 @@ static int gdb_continue_partial(char *newstates)
break; /* nothing to do here */
case 's':
trace_gdbstub_op_stepping(cpu->cpu_index);
- cpu_single_step(cpu, get_sstep_flags());
+ cpu_single_step(cpu, gdbserver_state.sstep_flags);
cpu_resume(cpu);
flag = 1;
break;
@@ -1883,7 +1884,7 @@ static void handle_step(GArray *params, void *user_ctx)
gdb_set_cpu_pc((target_ulong)get_param(params, 0)->val_ull);
}
- cpu_single_step(gdbserver_state.c_cpu, get_sstep_flags());
+ cpu_single_step(gdbserver_state.c_cpu, gdbserver_state.sstep_flags);
gdb_continue();
}
@@ -2017,24 +2018,44 @@ static void handle_v_commands(GArray *params, void *user_ctx)
static void handle_query_qemu_sstepbits(GArray *params, void *user_ctx)
{
- g_string_printf(gdbserver_state.str_buf, "ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
- SSTEP_ENABLE, SSTEP_NOIRQ, SSTEP_NOTIMER);
+ g_string_printf(gdbserver_state.str_buf, "ENABLE=%x", SSTEP_ENABLE);
+
+ if (gdbserver_state.supported_sstep_flags & SSTEP_NOIRQ) {
+ g_string_append_printf(gdbserver_state.str_buf, ",NOIRQ=%x",
+ SSTEP_NOIRQ);
+ }
+
+ if (gdbserver_state.supported_sstep_flags & SSTEP_NOTIMER) {
+ g_string_append_printf(gdbserver_state.str_buf, ",NOTIMER=%x",
+ SSTEP_NOTIMER);
+ }
+
put_strbuf();
}
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;
+ }
+
+ gdbserver_state.sstep_flags = new_sstep_flags;
put_packet("OK");
}
static void handle_query_qemu_sstep(GArray *params, void *user_ctx)
{
- g_string_printf(gdbserver_state.str_buf, "0x%x", sstep_flags);
+ g_string_printf(gdbserver_state.str_buf, "0x%x",
+ gdbserver_state.sstep_flags);
put_strbuf();
}
--
2.33.1
Paolo Bonzini <pbonzini@redhat.com> writes:
> 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(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 23baaef40e..960b9fbcd0 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -368,27 +368,10 @@ typedef struct GDBState {
> gdb_syscall_complete_cb current_syscall_cb;
> GString *str_buf;
> GByteArray *mem_buf;
> + int sstep_flags;
> + int supported_sstep_flags;
> } GDBState;
>
> -/* By default use no IRQs and no timers while single stepping so as to
> - * make single stepping like an ICE HW step.
> - */
> -static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
> -
> -/* Retrieves flags for single step mode. */
> -static int get_sstep_flags(void)
> -{
> - /*
> - * 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) {
> - return SSTEP_ENABLE;
> - } else {
> - return sstep_flags;
> - }
> -}
> -
> static GDBState gdbserver_state;
>
> 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);
> +
> + /*
> + * In replay mode all events written into the log should be replayed.
> + * That is why NOIRQ flag is removed in this mode.
I guess if we were being complete we could say something like:
In replay mode all events will come from the log and can't be
suppressed otherwise we would break determinism. However as those
events are tied to the number of executed instructions we won't see
them occurring every time we single step.
> + */
> + 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;
> +
> }
>
> #ifndef CONFIG_USER_ONLY
> @@ -505,7 +506,7 @@ static int gdb_continue_partial(char *newstates)
> CPU_FOREACH(cpu) {
> if (newstates[cpu->cpu_index] == 's') {
> trace_gdbstub_op_stepping(cpu->cpu_index);
> - cpu_single_step(cpu, sstep_flags);
> + cpu_single_step(cpu, gdbserver_state.sstep_flags);
> }
> }
> gdbserver_state.running_state = 1;
> @@ -524,7 +525,7 @@ static int gdb_continue_partial(char *newstates)
> break; /* nothing to do here */
> case 's':
> trace_gdbstub_op_stepping(cpu->cpu_index);
> - cpu_single_step(cpu, get_sstep_flags());
> + cpu_single_step(cpu, gdbserver_state.sstep_flags);
> cpu_resume(cpu);
> flag = 1;
> break;
> @@ -1883,7 +1884,7 @@ static void handle_step(GArray *params, void *user_ctx)
> gdb_set_cpu_pc((target_ulong)get_param(params, 0)->val_ull);
> }
>
> - cpu_single_step(gdbserver_state.c_cpu, get_sstep_flags());
> + cpu_single_step(gdbserver_state.c_cpu, gdbserver_state.sstep_flags);
> gdb_continue();
> }
>
> @@ -2017,24 +2018,44 @@ static void handle_v_commands(GArray *params, void *user_ctx)
>
> static void handle_query_qemu_sstepbits(GArray *params, void *user_ctx)
> {
> - g_string_printf(gdbserver_state.str_buf, "ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
> - SSTEP_ENABLE, SSTEP_NOIRQ, SSTEP_NOTIMER);
> + g_string_printf(gdbserver_state.str_buf, "ENABLE=%x", SSTEP_ENABLE);
> +
> + if (gdbserver_state.supported_sstep_flags & SSTEP_NOIRQ) {
> + g_string_append_printf(gdbserver_state.str_buf, ",NOIRQ=%x",
> + SSTEP_NOIRQ);
> + }
> +
> + if (gdbserver_state.supported_sstep_flags & SSTEP_NOTIMER) {
> + g_string_append_printf(gdbserver_state.str_buf, ",NOTIMER=%x",
> + SSTEP_NOTIMER);
> + }
> +
> put_strbuf();
> }
>
> 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;
> + }
> +
> + gdbserver_state.sstep_flags = new_sstep_flags;
> put_packet("OK");
> }
>
> static void handle_query_qemu_sstep(GArray *params, void *user_ctx)
> {
> - g_string_printf(gdbserver_state.str_buf, "0x%x", sstep_flags);
> + g_string_printf(gdbserver_state.str_buf, "0x%x",
> + gdbserver_state.sstep_flags);
> put_strbuf();
> }
Otherwise LGTM:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
On 11/11/21 18:08, Alex Bennée wrote: >> + >> + /* >> + * In replay mode all events written into the log should be replayed. >> + * That is why NOIRQ flag is removed in this mode. > I guess if we were being complete we could say something like: > > In replay mode all events will come from the log and can't be > suppressed otherwise we would break determinism. However as those > events are tied to the number of executed instructions we won't see > them occurring every time we single step. > Ok, I can integrate this improved comment. Paolo
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");
> }
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
© 2016 - 2026 Red Hat, Inc.