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

[Maxim Levitsky]

On Mon, 2021-04-19 at 17:29 +0100, Alex Bennée wrote:
> 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):

This is something I'll check from now on before sending patches.


> 
>   [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.
I'll take a look!

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

Thanks for the review,
        Best regards,
                Maxim Levitsky
>