* [PATCH 0/2] gdbstub: implement support for blocking interrupts on single stepping
@ 2021-04-01 14:41 ` Maxim Levitsky
0 siblings, 0 replies; 14+ messages in thread
From: Maxim Levitsky @ 2021-04-01 14:41 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Paolo Bonzini, Alex Bennée, kvm,
Cornelia Huck, Philippe Mathieu-Daudé, Maxim Levitsky
clone of "starship_unstable"
Maxim Levitsky (2):
kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS
gdbstub: implement NOIRQ support for single step on KVM, when kvm's
KVM_GUESTDBG_BLOCKIRQ debug flag is supported.
accel/kvm/kvm-all.c | 25 ++++++++++++++++
gdbstub.c | 59 ++++++++++++++++++++++++++++++-------
include/sysemu/kvm.h | 13 ++++++++
linux-headers/asm-x86/kvm.h | 2 ++
linux-headers/linux/kvm.h | 1 +
5 files changed, 90 insertions(+), 10 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 0/2] gdbstub: implement support for blocking interrupts on single stepping @ 2021-04-01 14:41 ` Maxim Levitsky 0 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-04-01 14:41 UTC (permalink / raw) To: qemu-devel Cc: kvm, Michael S. Tsirkin, Alex Bennée, Cornelia Huck, Maxim Levitsky, Paolo Bonzini, Philippe Mathieu-Daudé clone of "starship_unstable" Maxim Levitsky (2): kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. accel/kvm/kvm-all.c | 25 ++++++++++++++++ gdbstub.c | 59 ++++++++++++++++++++++++++++++------- include/sysemu/kvm.h | 13 ++++++++ linux-headers/asm-x86/kvm.h | 2 ++ linux-headers/linux/kvm.h | 1 + 5 files changed, 90 insertions(+), 10 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS 2021-04-01 14:41 ` Maxim Levitsky @ 2021-04-01 14:41 ` Maxim Levitsky -1 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-04-01 14:41 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Paolo Bonzini, Alex Bennée, kvm, Cornelia Huck, Philippe Mathieu-Daudé, Maxim Levitsky Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- linux-headers/asm-x86/kvm.h | 2 ++ linux-headers/linux/kvm.h | 1 + 2 files changed, 3 insertions(+) diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h index 8e76d3701d..33878cdc34 100644 --- a/linux-headers/asm-x86/kvm.h +++ b/linux-headers/asm-x86/kvm.h @@ -281,6 +281,8 @@ struct kvm_debug_exit_arch { #define KVM_GUESTDBG_USE_HW_BP 0x00020000 #define KVM_GUESTDBG_INJECT_DB 0x00040000 #define KVM_GUESTDBG_INJECT_BP 0x00080000 +#define KVM_GUESTDBG_BLOCKIRQ 0x00100000 + /* for KVM_SET_GUEST_DEBUG */ struct kvm_guest_debug_arch { diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index 020b62a619..2ded7a0630 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190 #define KVM_CAP_SYS_HYPERV_CPUID 191 #define KVM_CAP_DIRTY_LOG_RING 192 +#define KVM_CAP_SET_GUEST_DEBUG2 195 #ifdef KVM_CAP_IRQ_ROUTING -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS @ 2021-04-01 14:41 ` Maxim Levitsky 0 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-04-01 14:41 UTC (permalink / raw) To: qemu-devel Cc: kvm, Michael S. Tsirkin, Alex Bennée, Cornelia Huck, Maxim Levitsky, Paolo Bonzini, Philippe Mathieu-Daudé Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- linux-headers/asm-x86/kvm.h | 2 ++ linux-headers/linux/kvm.h | 1 + 2 files changed, 3 insertions(+) diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h index 8e76d3701d..33878cdc34 100644 --- a/linux-headers/asm-x86/kvm.h +++ b/linux-headers/asm-x86/kvm.h @@ -281,6 +281,8 @@ struct kvm_debug_exit_arch { #define KVM_GUESTDBG_USE_HW_BP 0x00020000 #define KVM_GUESTDBG_INJECT_DB 0x00040000 #define KVM_GUESTDBG_INJECT_BP 0x00080000 +#define KVM_GUESTDBG_BLOCKIRQ 0x00100000 + /* for KVM_SET_GUEST_DEBUG */ struct kvm_guest_debug_arch { diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index 020b62a619..2ded7a0630 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190 #define KVM_CAP_SYS_HYPERV_CPUID 191 #define KVM_CAP_DIRTY_LOG_RING 192 +#define KVM_CAP_SET_GUEST_DEBUG2 195 #ifdef KVM_CAP_IRQ_ROUTING -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS 2021-04-01 14:41 ` Maxim Levitsky @ 2021-04-19 16:22 ` Alex Bennée -1 siblings, 0 replies; 14+ messages in thread From: Alex Bennée @ 2021-04-19 16:22 UTC (permalink / raw) To: Maxim Levitsky Cc: qemu-devel, Michael S. Tsirkin, Paolo Bonzini, kvm, Cornelia Huck, Philippe Mathieu-Daudé Maxim Levitsky <mlevitsk@redhat.com> writes: > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> Generally it's a good idea to reference where these are coming from, is it a current kernel patch in flight or from an release we haven't synced up to yet? Usually linux header updates are done with semi-regular runs on ./scripts/update-linux-headers.sh but obviously it's OK to include standalone patches during the review process. > --- > linux-headers/asm-x86/kvm.h | 2 ++ > linux-headers/linux/kvm.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h > index 8e76d3701d..33878cdc34 100644 > --- a/linux-headers/asm-x86/kvm.h > +++ b/linux-headers/asm-x86/kvm.h > @@ -281,6 +281,8 @@ struct kvm_debug_exit_arch { > #define KVM_GUESTDBG_USE_HW_BP 0x00020000 > #define KVM_GUESTDBG_INJECT_DB 0x00040000 > #define KVM_GUESTDBG_INJECT_BP 0x00080000 > +#define KVM_GUESTDBG_BLOCKIRQ 0x00100000 > + > > /* for KVM_SET_GUEST_DEBUG */ > struct kvm_guest_debug_arch { > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index 020b62a619..2ded7a0630 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190 > #define KVM_CAP_SYS_HYPERV_CPUID 191 > #define KVM_CAP_DIRTY_LOG_RING 192 > +#define KVM_CAP_SET_GUEST_DEBUG2 195 > > #ifdef KVM_CAP_IRQ_ROUTING -- Alex Bennée ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS @ 2021-04-19 16:22 ` Alex Bennée 0 siblings, 0 replies; 14+ messages in thread From: Alex Bennée @ 2021-04-19 16:22 UTC (permalink / raw) To: Maxim Levitsky Cc: kvm, Michael S. Tsirkin, Cornelia Huck, qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé Maxim Levitsky <mlevitsk@redhat.com> writes: > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> Generally it's a good idea to reference where these are coming from, is it a current kernel patch in flight or from an release we haven't synced up to yet? Usually linux header updates are done with semi-regular runs on ./scripts/update-linux-headers.sh but obviously it's OK to include standalone patches during the review process. > --- > linux-headers/asm-x86/kvm.h | 2 ++ > linux-headers/linux/kvm.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h > index 8e76d3701d..33878cdc34 100644 > --- a/linux-headers/asm-x86/kvm.h > +++ b/linux-headers/asm-x86/kvm.h > @@ -281,6 +281,8 @@ struct kvm_debug_exit_arch { > #define KVM_GUESTDBG_USE_HW_BP 0x00020000 > #define KVM_GUESTDBG_INJECT_DB 0x00040000 > #define KVM_GUESTDBG_INJECT_BP 0x00080000 > +#define KVM_GUESTDBG_BLOCKIRQ 0x00100000 > + > > /* for KVM_SET_GUEST_DEBUG */ > struct kvm_guest_debug_arch { > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index 020b62a619..2ded7a0630 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190 > #define KVM_CAP_SYS_HYPERV_CPUID 191 > #define KVM_CAP_DIRTY_LOG_RING 192 > +#define KVM_CAP_SET_GUEST_DEBUG2 195 > > #ifdef KVM_CAP_IRQ_ROUTING -- Alex Bennée ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS 2021-04-19 16:22 ` Alex Bennée @ 2021-05-05 8:35 ` Maxim Levitsky -1 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-05-05 8:35 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, Michael S. Tsirkin, Paolo Bonzini, kvm, Cornelia Huck, Philippe Mathieu-Daudé On Mon, 2021-04-19 at 17:22 +0100, Alex Bennée wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > Generally it's a good idea to reference where these are coming from, is > it a current kernel patch in flight or from an release we haven't synced > up to yet? Hi! As Paolo explained to me, qemu syncs the kernel headers every once in a while thus when I submit a feature to qemu which uses a new KVM feature, while I should submit a patch to add it to the kernel headers, the patch is only for the reference. In this particular case, I first updated the qemu's kernel headers to match the kvm/queue branch, then added my feature to the kernel, and updated the qemu kernel headers again. This patch is the diff between 1st and second update to make it more readable. Best regards, Maxim Levitsky > > Usually linux header updates are done with semi-regular runs on > ./scripts/update-linux-headers.sh but obviously it's OK to include > standalone patches during the review process. > > > --- > > linux-headers/asm-x86/kvm.h | 2 ++ > > linux-headers/linux/kvm.h | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h > > index 8e76d3701d..33878cdc34 100644 > > --- a/linux-headers/asm-x86/kvm.h > > +++ b/linux-headers/asm-x86/kvm.h > > @@ -281,6 +281,8 @@ struct kvm_debug_exit_arch { > > #define KVM_GUESTDBG_USE_HW_BP 0x00020000 > > #define KVM_GUESTDBG_INJECT_DB 0x00040000 > > #define KVM_GUESTDBG_INJECT_BP 0x00080000 > > +#define KVM_GUESTDBG_BLOCKIRQ 0x00100000 > > + > > > > /* for KVM_SET_GUEST_DEBUG */ > > struct kvm_guest_debug_arch { > > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > > index 020b62a619..2ded7a0630 100644 > > --- a/linux-headers/linux/kvm.h > > +++ b/linux-headers/linux/kvm.h > > @@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt { > > #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190 > > #define KVM_CAP_SYS_HYPERV_CPUID 191 > > #define KVM_CAP_DIRTY_LOG_RING 192 > > +#define KVM_CAP_SET_GUEST_DEBUG2 195 > > > > #ifdef KVM_CAP_IRQ_ROUTING > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS @ 2021-05-05 8:35 ` Maxim Levitsky 0 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-05-05 8:35 UTC (permalink / raw) To: Alex Bennée Cc: kvm, Michael S. Tsirkin, Cornelia Huck, qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé On Mon, 2021-04-19 at 17:22 +0100, Alex Bennée wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > Generally it's a good idea to reference where these are coming from, is > it a current kernel patch in flight or from an release we haven't synced > up to yet? Hi! As Paolo explained to me, qemu syncs the kernel headers every once in a while thus when I submit a feature to qemu which uses a new KVM feature, while I should submit a patch to add it to the kernel headers, the patch is only for the reference. In this particular case, I first updated the qemu's kernel headers to match the kvm/queue branch, then added my feature to the kernel, and updated the qemu kernel headers again. This patch is the diff between 1st and second update to make it more readable. Best regards, Maxim Levitsky > > Usually linux header updates are done with semi-regular runs on > ./scripts/update-linux-headers.sh but obviously it's OK to include > standalone patches during the review process. > > > --- > > linux-headers/asm-x86/kvm.h | 2 ++ > > linux-headers/linux/kvm.h | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h > > index 8e76d3701d..33878cdc34 100644 > > --- a/linux-headers/asm-x86/kvm.h > > +++ b/linux-headers/asm-x86/kvm.h > > @@ -281,6 +281,8 @@ struct kvm_debug_exit_arch { > > #define KVM_GUESTDBG_USE_HW_BP 0x00020000 > > #define KVM_GUESTDBG_INJECT_DB 0x00040000 > > #define KVM_GUESTDBG_INJECT_BP 0x00080000 > > +#define KVM_GUESTDBG_BLOCKIRQ 0x00100000 > > + > > > > /* for KVM_SET_GUEST_DEBUG */ > > struct kvm_guest_debug_arch { > > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > > index 020b62a619..2ded7a0630 100644 > > --- a/linux-headers/linux/kvm.h > > +++ b/linux-headers/linux/kvm.h > > @@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt { > > #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190 > > #define KVM_CAP_SYS_HYPERV_CPUID 191 > > #define KVM_CAP_DIRTY_LOG_RING 192 > > +#define KVM_CAP_SET_GUEST_DEBUG2 195 > > > > #ifdef KVM_CAP_IRQ_ROUTING > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. 2021-04-01 14:41 ` Maxim Levitsky @ 2021-04-01 14:41 ` Maxim Levitsky -1 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-04-01 14:41 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Paolo Bonzini, Alex Bennée, kvm, Cornelia Huck, Philippe Mathieu-Daudé, Maxim Levitsky 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(); + } 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; + } + if (!device) return -1; if (strcmp(device, "none") != 0) { diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index a1ab1ee12d..42f9ce6344 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -46,6 +46,8 @@ extern bool kvm_readonly_mem_allowed; extern bool kvm_direct_msi_allowed; extern bool kvm_ioeventfd_any_length_allowed; extern bool kvm_msi_use_devid; +extern bool kvm_has_guest_debug; +extern int kvm_sstep_flags; #define kvm_enabled() (kvm_allowed) /** @@ -167,6 +169,17 @@ extern bool kvm_msi_use_devid; */ #define kvm_msi_devid_required() (kvm_msi_use_devid) +/* + * Does KVM support guest debugging + */ +#define kvm_supports_guest_debug() (kvm_has_guest_debug) + +/* + * kvm_supported_sstep_flags + * Returns: SSTEP_* flags that KVM supports for guest debug + */ +#define kvm_get_supported_sstep_flags() (kvm_sstep_flags) + #else #define kvm_enabled() (0) -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. @ 2021-04-01 14:41 ` Maxim Levitsky 0 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-04-01 14:41 UTC (permalink / raw) To: qemu-devel Cc: kvm, Michael S. Tsirkin, Alex Bennée, Cornelia Huck, Maxim Levitsky, Paolo Bonzini, Philippe Mathieu-Daudé 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(); + } 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; + } + if (!device) return -1; if (strcmp(device, "none") != 0) { diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index a1ab1ee12d..42f9ce6344 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -46,6 +46,8 @@ extern bool kvm_readonly_mem_allowed; extern bool kvm_direct_msi_allowed; extern bool kvm_ioeventfd_any_length_allowed; extern bool kvm_msi_use_devid; +extern bool kvm_has_guest_debug; +extern int kvm_sstep_flags; #define kvm_enabled() (kvm_allowed) /** @@ -167,6 +169,17 @@ extern bool kvm_msi_use_devid; */ #define kvm_msi_devid_required() (kvm_msi_use_devid) +/* + * Does KVM support guest debugging + */ +#define kvm_supports_guest_debug() (kvm_has_guest_debug) + +/* + * kvm_supported_sstep_flags + * Returns: SSTEP_* flags that KVM supports for guest debug + */ +#define kvm_get_supported_sstep_flags() (kvm_sstep_flags) + #else #define kvm_enabled() (0) -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. 2021-04-01 14:41 ` Maxim Levitsky @ 2021-04-19 16:29 ` Alex Bennée -1 siblings, 0 replies; 14+ messages in thread From: Alex Bennée @ 2021-04-19 16:29 UTC (permalink / raw) To: Maxim Levitsky Cc: qemu-devel, Michael S. Tsirkin, Paolo Bonzini, kvm, Cornelia Huck, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. @ 2021-04-19 16:29 ` Alex Bennée 0 siblings, 0 replies; 14+ messages in thread From: Alex Bennée @ 2021-04-19 16:29 UTC (permalink / raw) To: Maxim Levitsky Cc: kvm, Michael S. Tsirkin, Cornelia Huck, qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. 2021-04-19 16:29 ` Alex Bennée @ 2021-05-05 8:37 ` Maxim Levitsky -1 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-05-05 8:37 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, Michael S. Tsirkin, Paolo Bonzini, kvm, Cornelia Huck, Philippe Mathieu-Daudé 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported. @ 2021-05-05 8:37 ` Maxim Levitsky 0 siblings, 0 replies; 14+ messages in thread From: Maxim Levitsky @ 2021-05-05 8:37 UTC (permalink / raw) To: Alex Bennée Cc: kvm, Michael S. Tsirkin, Cornelia Huck, qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-05-05 8:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-04-01 14:41 [PATCH 0/2] gdbstub: implement support for blocking interrupts on single stepping Maxim Levitsky 2021-04-01 14:41 ` Maxim Levitsky 2021-04-01 14:41 ` [PATCH 1/2] kvm: update kernel headers for KVM_GUESTDBG_BLOCKEVENTS Maxim Levitsky 2021-04-01 14:41 ` Maxim Levitsky 2021-04-19 16:22 ` Alex Bennée 2021-04-19 16:22 ` Alex Bennée 2021-05-05 8:35 ` Maxim Levitsky 2021-05-05 8:35 ` Maxim Levitsky 2021-04-01 14:41 ` [PATCH 2/2] gdbstub: implement NOIRQ support for single step on KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported Maxim Levitsky 2021-04-01 14:41 ` Maxim Levitsky 2021-04-19 16:29 ` Alex Bennée 2021-04-19 16:29 ` Alex Bennée 2021-05-05 8:37 ` Maxim Levitsky 2021-05-05 8:37 ` Maxim Levitsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.