David Gibson <da...@gibson.dropbear.id.au> writes:
> On Wed, Dec 11, 2019 at 04:10:12PM -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 <faro...@linux.ibm.com>
>
> I think this is fine, but it took me a while to figure out what was
> going on. I wonder if better comments and/or function names might
> help.
>
Thanks for the feedback, I will review the patch with this in mind and
add more information.
>> ---
>> accel/kvm/kvm-all.c | 14 ++++++++++++++
>> accel/stubs/kvm-stub.c | 4 ++++
>> exec.c | 2 +-
>> include/sysemu/kvm.h | 4 ++++
>> stubs/Makefile.objs | 1 +
>> stubs/kvm-arch-singlestep.c | 14 ++++++++++++++
>> target/ppc/kvm.c | 14 ++++++++++++++
>> 7 files changed, 52 insertions(+), 1 deletion(-)
>> create mode 100644 stubs/kvm-arch-singlestep.c
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index ca00daa2f5..a61beb0b53 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2599,6 +2599,11 @@ bool kvm_arm_supports_user_irq(void)
>> }
>>
>> #ifdef KVM_CAP_SET_GUEST_DEBUG
>> +bool kvm_has_guest_debug_singlestep(CPUState *cs)
>
> AIUI, this function is saying if setting the singlestep flag in the
> KVM debug regs is sufficient to single step the guest under host
> control. The name doesn't make that terribly obvious to me, though a
> better one isn't really obvious to me.
>
Right, I'll think of something more obvious.
>> +{
>> + return kvm_arch_has_guest_debug_singlestep(cs);
>
> I also don't see a lot of point to this wrapper, rather than just
> calling the arch version directly.
>
Ok. I'll remove the wrapper.
>> +}
>> +
>> struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
>> target_ulong pc)
>> {
>> @@ -2647,6 +2652,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_guest_debug_singlestep(cs)) {
>> + 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 82f118d2df..b4df48b6f1 100644
>> --- a/accel/stubs/kvm-stub.c
>> +++ b/accel/stubs/kvm-stub.c
>> @@ -78,6 +78,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long
>> reinject_trap)
>> return -ENOSYS;
>> }
>>
>> +void kvm_set_singlestep(CPUState *cs, int enabled)
>> +{
>
> AFAICT this should only be called with KVM enabled, so this should
> maybe be a g_assert_not_reached() rather than a no-op.
>
Ok.
>> +}
>> +
>> int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>> target_ulong len, int type)
>> {
>> diff --git a/exec.c b/exec.c
>> index ffdb518535..ff46ea1846 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1202,7 +1202,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 9fe233b9bf..7a42978b11 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -215,6 +215,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);
>> +bool kvm_has_guest_debug_singlestep(CPUState *cs);
>>
>> int kvm_init_vcpu(CPUState *cpu);
>> int kvm_cpu_exec(CPUState *cpu);
>> @@ -247,6 +248,8 @@ bool kvm_memcrypt_enabled(void);
>> */
>> int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>>
>> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs);
>> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
>>
>> #ifdef NEED_CPU_H
>> #include "cpu.h"
>> @@ -259,6 +262,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 4a50e95ec3..361f4088fa 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-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-singlestep.c b/stubs/kvm-arch-singlestep.c
>> new file mode 100644
>> index 0000000000..18bfba61f6
>> --- /dev/null
>> +++ b/stubs/kvm-arch-singlestep.c
>> @@ -0,0 +1,14 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/kvm.h"
>> +
>> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
>> +{
>> + /* for backwards compatibility assume the feature is present */
>> + return true;
>> +}
>> +
>> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled)
>> +{
>> + warn_report("KVM does not support single stepping");
>> +}
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index c77f9848ec..3a2cfe883c 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -85,6 +85,7 @@ static int cap_ppc_safe_indirect_branch;
>> static int cap_ppc_count_cache_flush_assist;
>> static int cap_ppc_nested_kvm_hv;
>> static int cap_large_decr;
>> +static int cap_ppc_singlestep;
>>
>> static uint32_t debug_inst_opcode;
>>
>> @@ -133,6 +134,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>> kvmppc_get_cpu_characteristics(s);
>> cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s,
>> KVM_CAP_PPC_NESTED_HV);
>> cap_large_decr = kvmppc_get_dec_bits();
>> + cap_ppc_singlestep = kvm_vm_check_extension(s,
>> KVM_CAP_PPC_GUEST_DEBUG_SSTEP);
>> /*
>> * Note: setting it to false because there is not such capability
>> * in KVM at this moment.
>> @@ -1380,6 +1382,18 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
>> return 0;
>> }
>>
>> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> +
>> + if (cap_ppc_singlestep) {
>> + return true;
>> + }
>> +
>
> Perhaps note explicitly that this is a fallback guess when the cap is
> not available to give us a good answer.
>
Ok.
>> + return env->excp_model == POWERPC_EXCP_BOOKE || kvmppc_is_pr(kvm_state);
>> +}
>> +
>> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint
>> *bp)
>> {
>> /* Mixed endian case is not handled */
