| Message ID | 20190228225759.21328-3-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: single step for KVM HV | expand |
On Thu, Feb 28, 2019 at 07:57:56PM -0300, Fabiano Rosas wrote: > For single stepping (via KVM) of a guest vcpu to work, KVM needs not > only to support the SET_GUEST_DEBUG ioctl but to also recognize the > KVM_GUESTDBG_SINGLESTEP bit in the control field of the > kvm_guest_debug struct. > > This patch adds support for querying the single step capability so > that QEMU can decide what to do for the platforms that do not have > such support. > > This will allow architecture-specific implementations of a fallback > mechanism for single stepping in cases where KVM does not support it. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > accel/kvm/kvm-all.c | 16 ++++++++++++++++ > accel/stubs/kvm-stub.c | 4 ++++ > exec.c | 2 +- > include/sysemu/kvm.h | 3 +++ > stubs/Makefile.objs | 1 + > stubs/kvm-arch-set-singlestep.c | 8 ++++++++ > 6 files changed, 33 insertions(+), 1 deletion(-) > create mode 100644 stubs/kvm-arch-set-singlestep.c > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index fd92b6f375..d3ac5a9e5c 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2267,6 +2267,13 @@ bool kvm_arm_supports_user_irq(void) > return kvm_check_extension(kvm_state, KVM_CAP_ARM_USER_IRQ); > } > > +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */ > +int kvm_has_guestdbg_singlestep(void) > +{ > + /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */ I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel trees. Where does that come from? > + return 0; > +} > + > #ifdef KVM_CAP_SET_GUEST_DEBUG > struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu, > target_ulong pc) > @@ -2316,6 +2323,15 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap) > return data.err; > } > > +void kvm_set_singlestep(CPUState *cs, int enabled) > +{ > + if (kvm_has_guestdbg_singlestep()) { > + kvm_update_guest_debug(cs, 0); > + } else { > + kvm_arch_set_singlestep(cs, enabled); > + } > +} > + > int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr, > target_ulong len, int type) > { > diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c > index 02d5170031..69bd07f50e 100644 > --- a/accel/stubs/kvm-stub.c > +++ b/accel/stubs/kvm-stub.c > @@ -79,6 +79,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap) > return -ENOSYS; > } > > +void kvm_set_singlestep(CPUState *cs, int enabled) > +{ > +} > + > int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr, > target_ulong len, int type) > { > diff --git a/exec.c b/exec.c > index 518064530b..8817513e26 100644 > --- a/exec.c > +++ b/exec.c > @@ -1236,7 +1236,7 @@ void cpu_single_step(CPUState *cpu, int enabled) > if (cpu->singlestep_enabled != enabled) { > cpu->singlestep_enabled = enabled; > if (kvm_enabled()) { > - kvm_update_guest_debug(cpu, 0); > + kvm_set_singlestep(cpu, enabled); > } else { > /* must flush all the translated code to avoid inconsistencies */ > /* XXX: only flush what is necessary */ > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index a6d1cd190f..e1ef2f5b99 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -214,6 +214,7 @@ int kvm_has_pit_state2(void); > int kvm_has_many_ioeventfds(void); > int kvm_has_gsi_routing(void); > int kvm_has_intx_set_mask(void); > +int kvm_has_guestdbg_singlestep(void); > > int kvm_init_vcpu(CPUState *cpu); > int kvm_cpu_exec(CPUState *cpu); > @@ -246,6 +247,7 @@ bool kvm_memcrypt_enabled(void); > */ > int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len); > > +void kvm_arch_set_singlestep(CPUState *cpu, int enabled); > > #ifdef NEED_CPU_H > #include "cpu.h" > @@ -258,6 +260,7 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr, > target_ulong len, int type); > void kvm_remove_all_breakpoints(CPUState *cpu); > int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap); > +void kvm_set_singlestep(CPUState *cs, int enabled); > > int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr); > int kvm_on_sigbus(int code, void *addr); > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs > index 269dfa5832..884f9b2268 100644 > --- a/stubs/Makefile.objs > +++ b/stubs/Makefile.objs > @@ -12,6 +12,7 @@ stub-obj-y += get-vm-name.o > stub-obj-y += iothread.o > stub-obj-y += iothread-lock.o > stub-obj-y += is-daemonized.o > +stub-obj-y += kvm-arch-set-singlestep.o > stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o > stub-obj-y += machine-init-done.o > stub-obj-y += migr-blocker.o > diff --git a/stubs/kvm-arch-set-singlestep.c b/stubs/kvm-arch-set-singlestep.c > new file mode 100644 > index 0000000000..ba6e0323d6 > --- /dev/null > +++ b/stubs/kvm-arch-set-singlestep.c > @@ -0,0 +1,8 @@ > +#include "qemu/osdep.h" > +#include "qemu/error-report.h" > +#include "sysemu/kvm.h" > + > +void kvm_arch_set_singlestep(CPUState *cpu, int enabled) > +{ > + warn_report("KVM does not support single stepping"); > +}
David Gibson <david@gibson.dropbear.id.au> writes: >> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */ >> +int kvm_has_guestdbg_singlestep(void) >> +{ >> + /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */ > > I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel > trees. Where does that come from? > I'll submit that to the kernel this week. I was waiting to make sure this wouldn't change too much on QEMU side first.
David Gibson <david@gibson.dropbear.id.au> writes: >> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */ >> +int kvm_has_guestdbg_singlestep(void) >> +{ >> + /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */ > > I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel > trees. Where does that come from? > I realized that this will cause a regression for the other architectures (and even PPC-PR) that already have the feature when we run QEMU on older kernels without this capability. I am becoming inclined to rely on kvmppc_is_pr. Do you see another way around this?
On Fri, Mar 08, 2019 at 04:09:56PM -0300, Fabiano Rosas wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > >> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */ > >> +int kvm_has_guestdbg_singlestep(void) > >> +{ > >> + /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */ > > > > I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel > > trees. Where does that come from? > > > > I realized that this will cause a regression for the other architectures > (and even PPC-PR) that already have the feature when we run QEMU on > older kernels without this capability. > > I am becoming inclined to rely on kvmppc_is_pr. Do you see another way > around this? It's ok to use that for fallback cases. However it's best if you can design things so that modern kernels will provide a clear answer via the CAP, either positive or negative, and only fallback to kvmppc_is_pr() in the case where it's an old kernel. That obviously also won't cover the other arch case.
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index fd92b6f375..d3ac5a9e5c 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2267,6 +2267,13 @@ bool kvm_arm_supports_user_irq(void) return kvm_check_extension(kvm_state, KVM_CAP_ARM_USER_IRQ); } +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */ +int kvm_has_guestdbg_singlestep(void) +{ + /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */ + return 0; +} + #ifdef KVM_CAP_SET_GUEST_DEBUG struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu, target_ulong pc) @@ -2316,6 +2323,15 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap) return data.err; } +void kvm_set_singlestep(CPUState *cs, int enabled) +{ + if (kvm_has_guestdbg_singlestep()) { + kvm_update_guest_debug(cs, 0); + } else { + kvm_arch_set_singlestep(cs, enabled); + } +} + int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr, target_ulong len, int type) { diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c index 02d5170031..69bd07f50e 100644 --- a/accel/stubs/kvm-stub.c +++ b/accel/stubs/kvm-stub.c @@ -79,6 +79,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap) return -ENOSYS; } +void kvm_set_singlestep(CPUState *cs, int enabled) +{ +} + int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr, target_ulong len, int type) { diff --git a/exec.c b/exec.c index 518064530b..8817513e26 100644 --- a/exec.c +++ b/exec.c @@ -1236,7 +1236,7 @@ void cpu_single_step(CPUState *cpu, int enabled) if (cpu->singlestep_enabled != enabled) { cpu->singlestep_enabled = enabled; if (kvm_enabled()) { - kvm_update_guest_debug(cpu, 0); + kvm_set_singlestep(cpu, enabled); } else { /* must flush all the translated code to avoid inconsistencies */ /* XXX: only flush what is necessary */ diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index a6d1cd190f..e1ef2f5b99 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -214,6 +214,7 @@ int kvm_has_pit_state2(void); int kvm_has_many_ioeventfds(void); int kvm_has_gsi_routing(void); int kvm_has_intx_set_mask(void); +int kvm_has_guestdbg_singlestep(void); int kvm_init_vcpu(CPUState *cpu); int kvm_cpu_exec(CPUState *cpu); @@ -246,6 +247,7 @@ bool kvm_memcrypt_enabled(void); */ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len); +void kvm_arch_set_singlestep(CPUState *cpu, int enabled); #ifdef NEED_CPU_H #include "cpu.h" @@ -258,6 +260,7 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr, target_ulong len, int type); void kvm_remove_all_breakpoints(CPUState *cpu); int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap); +void kvm_set_singlestep(CPUState *cs, int enabled); int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr); int kvm_on_sigbus(int code, void *addr); diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 269dfa5832..884f9b2268 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -12,6 +12,7 @@ stub-obj-y += get-vm-name.o stub-obj-y += iothread.o stub-obj-y += iothread-lock.o stub-obj-y += is-daemonized.o +stub-obj-y += kvm-arch-set-singlestep.o stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o stub-obj-y += machine-init-done.o stub-obj-y += migr-blocker.o diff --git a/stubs/kvm-arch-set-singlestep.c b/stubs/kvm-arch-set-singlestep.c new file mode 100644 index 0000000000..ba6e0323d6 --- /dev/null +++ b/stubs/kvm-arch-set-singlestep.c @@ -0,0 +1,8 @@ +#include "qemu/osdep.h" +#include "qemu/error-report.h" +#include "sysemu/kvm.h" + +void kvm_arch_set_singlestep(CPUState *cpu, int enabled) +{ + warn_report("KVM does not support single stepping"); +}
For single stepping (via KVM) of a guest vcpu to work, KVM needs not only to support the SET_GUEST_DEBUG ioctl but to also recognize the KVM_GUESTDBG_SINGLESTEP bit in the control field of the kvm_guest_debug struct. This patch adds support for querying the single step capability so that QEMU can decide what to do for the platforms that do not have such support. This will allow architecture-specific implementations of a fallback mechanism for single stepping in cases where KVM does not support it. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- accel/kvm/kvm-all.c | 16 ++++++++++++++++ accel/stubs/kvm-stub.c | 4 ++++ exec.c | 2 +- include/sysemu/kvm.h | 3 +++ stubs/Makefile.objs | 1 + stubs/kvm-arch-set-singlestep.c | 8 ++++++++ 6 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 stubs/kvm-arch-set-singlestep.c