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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6587 bytes --]

On Fri, Jan 10, 2020 at 12:13:43PM -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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

and ppc parts

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  accel/kvm/kvm-all.c         |  9 +++++++++
>  accel/stubs/kvm-stub.c      |  5 +++++
>  exec.c                      |  2 +-
>  include/sysemu/kvm.h        |  3 +++
>  stubs/Makefile.objs         |  1 +
>  stubs/kvm-arch-singlestep.c | 14 ++++++++++++++
>  target/ppc/kvm.c            | 15 +++++++++++++++
>  7 files changed, 48 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 b2f1a5bcb5..a1ea8f9641 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2668,6 +2668,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_arch_can_singlestep(cs)) {
> +        kvm_update_guest_debug(cs, 0);
> +    } else {
> +        kvm_arch_emulate_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..8773c15713 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -78,6 +78,11 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>      return -ENOSYS;
>  }
>  
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +    g_assert_not_reached();
> +}
> +
>  int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>                            target_ulong len, int type)
>  {
> diff --git a/exec.c b/exec.c
> index d4b769d0d4..10b61fccb8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1204,7 +1204,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 141342de98..43abdb8430 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -247,6 +247,8 @@ bool kvm_memcrypt_enabled(void);
>   */
>  int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>  
> +bool kvm_arch_can_singlestep(CPUState *cs);
> +void kvm_arch_emulate_singlestep(CPUState *cpu, int enabled);
>  
>  #ifdef NEED_CPU_H
>  #include "cpu.h"
> @@ -259,6 +261,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 8b0ff25508..e6310b878c 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..30ee278ba4
> --- /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_can_singlestep(CPUState *cs)
> +{
> +    /* for backwards compatibility assume the feature is present */
> +    return true;
> +}
> +
> +void kvm_arch_emulate_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 0bd4a8d399..6fb8687126 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -88,6 +88,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;
>  
> @@ -136,6 +137,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.
> @@ -1383,6 +1385,19 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
>      return 0;
>  }
>  
> +bool kvm_arch_can_singlestep(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    if (cap_ppc_singlestep) {
> +        return true;
> +    }
> +
> +    /* Fallback for when the capability is not available */
> +    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 */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]