Re: [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep

[Fabiano Rosas]

David Gibson <address@hidden> 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 <address@hidden>
>
> 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 */