Re: [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported.

[Alex Bennée]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  accel/kvm/kvm-all.c  | 25 +++++++++++++++++++
>  gdbstub.c            | 59 ++++++++++++++++++++++++++++++++++++--------
>  include/sysemu/kvm.h | 13 ++++++++++
>  3 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b6d9f92f15..bc7955fb19 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -147,6 +147,8 @@ bool kvm_vm_attributes_allowed;
>  bool kvm_direct_msi_allowed;
>  bool kvm_ioeventfd_any_length_allowed;
>  bool kvm_msi_use_devid;
> +bool kvm_has_guest_debug;
> +int kvm_sstep_flags;
>  static bool kvm_immediate_exit;
>  static hwaddr kvm_max_slot_size = ~0;
>  
> @@ -2186,6 +2188,25 @@ static int kvm_init(MachineState *ms)
>      kvm_ioeventfd_any_length_allowed =
>          (kvm_check_extension(s, KVM_CAP_IOEVENTFD_ANY_LENGTH) > 0);
>  
> +    kvm_has_guest_debug =
> +        (kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG) > 0);
> +
> +    kvm_sstep_flags = 0;
> +
> +    if (kvm_has_guest_debug) {
> +        /* Assume that single stepping is supported */
> +        kvm_sstep_flags = SSTEP_ENABLE;
> +
> +        int guest_debug_flags =
> +            kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG2);
> +
> +        if (guest_debug_flags > 0) {
> +            if (guest_debug_flags & KVM_GUESTDBG_BLOCKIRQ) {
> +                kvm_sstep_flags |= SSTEP_NOIRQ;
> +            }
> +        }
> +    }
> +
>      kvm_state = s;
>  
>      ret = kvm_arch_init(ms, s);
> @@ -2796,6 +2817,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned 
> long reinject_trap)
>  
>      if (cpu->singlestep_enabled) {
>          data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
> +
> +        if (cpu->singlestep_enabled & SSTEP_NOIRQ) {
> +            data.dbg.control |= KVM_GUESTDBG_BLOCKIRQ;
> +        }
>      }
>      kvm_arch_update_guest_debug(cpu, &data.dbg);
>  
> diff --git a/gdbstub.c b/gdbstub.c
> index 054665e93e..f789ded99d 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -369,12 +369,11 @@ 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;
> +static GDBState gdbserver_state;
>  
>  /* Retrieves flags for single step mode. */
>  static int get_sstep_flags(void)
> @@ -386,11 +385,10 @@ static int get_sstep_flags(void)
>      if (replay_mode != REPLAY_MODE_NONE) {
>          return SSTEP_ENABLE;
>      } else {
> -        return sstep_flags;
> +        return gdbserver_state.sstep_flags;
>      }
>  }
>  
> -static GDBState gdbserver_state;
>  
>  static void init_gdbserver_state(void)
>  {
> @@ -400,6 +398,23 @@ 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);
> +
> +
> +    if (kvm_enabled()) {
> +        gdbserver_state.supported_sstep_flags =
>  kvm_get_supported_sstep_flags();

This falls over as soon as you build something without KVM support (like
a TCG only build or an emulation only target):

  [10/1152] Compiling C object libqemu-riscv32-softmmu.fa.p/gdbstub.c.o
  FAILED: libqemu-riscv32-softmmu.fa.p/gdbstub.c.o 
  cc -Ilibqemu-riscv32-softmmu.fa.p -I. -I../.. -Itarget/riscv 
-I../../target/riscv -Idtc/libfdt -I../../dtc/libfdt 
-I../../capstone/include/capstone -Iqapi -Itrace -Iui -Iui/shader 
-I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/spice-server 
-I/usr/include/spice-1 -I/usr/include/glib-2.0 
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -pipe 
-Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g -isystem 
/home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote 
/home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote 
/home/alex/lsrc/qemu.git/disas/libvixl -iquote 
/home/alex/lsrc/qemu.git/tcg/i386 -iquote /home/alex/lsrc/qemu.git/accel/tcg 
-pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -m64 -mcx16 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels 
-Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs 
-Wno-shift-negative-value -Wno-psabi -fstack-protector-strong 
-DLEGACY_RDMA_REG_MR -fPIC -isystem../../linux-headers -isystemlinux-headers 
-DNEED_CPU_H '-DCONFIG_TARGET="riscv32-softmmu-config-target.h"' 
'-DCONFIG_DEVICES="riscv32-softmmu-config-devices.h"' -MD -MQ 
libqemu-riscv32-softmmu.fa.p/gdbstub.c.o -MF 
libqemu-riscv32-softmmu.fa.p/gdbstub.c.o.d -o 
libqemu-riscv32-softmmu.fa.p/gdbstub.c.o -c ../../gdbstub.c
  ../../gdbstub.c: In function ‘init_gdbserver_state’:
  ../../gdbstub.c:404:49: error: implicit declaration of function 
‘kvm_get_supported_sstep_flags’; did you mean ‘hvf_get_supported_cpuid’? 
[-Werror=implicit-function-declaration]
           gdbserver_state.supported_sstep_flags = 
kvm_get_supported_sstep_flags();
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                   hvf_get_supported_cpuid
  ../../gdbstub.c:404:49: error: nested extern declaration of 
‘kvm_get_supported_sstep_flags’ [-Werror=nested-externs]
  ../../gdbstub.c: In function ‘gdbserver_start’:
  ../../gdbstub.c:3536:27: error: implicit declaration of function 
‘kvm_supports_guest_debug’; did you mean ‘kvm_update_guest_debug’? 
[-Werror=implicit-function-declaration]
       if (kvm_enabled() && !kvm_supports_guest_debug()) {
                             ^~~~~~~~~~~~~~~~~~~~~~~~
                             kvm_update_guest_debug
  ../../gdbstub.c:3536:27: error: nested extern declaration of 
‘kvm_supports_guest_debug’ [-Werror=nested-externs]
  cc1: all warnings being treated as errors


> +    } 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 = SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
> +    gdbserver_state.sstep_flags &= gdbserver_state.supported_sstep_flags;
> +
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> @@ -2023,24 +2038,43 @@ static void handle_v_commands(GdbCmdContext *gdb_ctx, 
> void *user_ctx)
>  
>  static void handle_query_qemu_sstepbits(GdbCmdContext *gdb_ctx, 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(GdbCmdContext *gdb_ctx, void *user_ctx)
>  {
> +    int new_sstep_flags;
>      if (!gdb_ctx->num_params) {
>          return;
>      }
>  
> -    sstep_flags = gdb_ctx->params[0].val_ul;
> +    new_sstep_flags = gdb_ctx->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(GdbCmdContext *gdb_ctx, 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();
>  }
>  
> @@ -3499,6 +3533,11 @@ int gdbserver_start(const char *device)
>          return -1;
>      }
>  
> +    if (kvm_enabled() && !kvm_supports_guest_debug()) {
> +        error_report("gdbstub: KVM doesn't support guest debugging");
> +        return -1;
> +    }
> +
<snip>

Otherwise it looks fine as far as it goes, however it would be nice to
have some sort of test in for this. The gdbstub has a hand-rolled gdb
script in tests/guest-debug/test-gdbstub.py but it's not integrated with
the rest of the testing.

As I suspect you need a) KVM enabled, b) a recent enough kernel and c)
some sort of guest kernel that is going to enable timers and IRQs this
might be something worth porting to the acceptance tests.

We have an example in tests/acceptance/reverse_debugging.py which is run
as part of check-acceptance. It's TCG only but perhaps is a template for
how such a test could be implemented.

-- 
Alex Bennée