Re: [Qemu-devel] [RFC PATCH v2 3/3] target/ppc: support single stepping with KVM HV

[David Gibson]

On Wed, Nov 21, 2018 at 04:13:47PM -0200, Fabiano Rosas wrote:
> The hardware singlestep mechanism in POWER works via a Trace Interrupt
> (0xd00) that happens after any instruction executes, whenever MSR_SE =
> 1 (PowerISA Section 6.5.15 - Trace Interrupt).
> 
> However, with kvm_hv, the Trace Interrupt happens inside the guest and
> KVM has no visibility of it. Therefore, when the gdbstub uses the
> KVM_SET_GUEST_DEBUG ioctl to enable singlestep, KVM simply ignores it.
> 
> This patch takes advantage of the Trace Interrupt to perform the step
> inside the guest, but uses a breakpoint at the Trace Interrupt handler
> to return control to KVM. The exit is treated by KVM as a regular
> breakpoint and it returns to the host (and QEMU eventually).
> 
> Before signalling GDB, QEMU sets the Next Instruction Pointer to the
> instruction following the one being stepped, effectively skipping the
> interrupt handler execution and hiding the trace interrupt breakpoint
> from GDB.
> 
> This approach works with both of GDB's 'scheduler-locking' options
> (off, step).
> 
> Signed-off-by: Fabiano Rosas <address@hidden>
> ---
>  target/ppc/kvm.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9d0b4f1f3f..93c20099e6 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -94,6 +94,7 @@ static int cap_ppc_safe_indirect_branch;
>  static int cap_ppc_nested_kvm_hv;
>  
>  static uint32_t debug_inst_opcode;
> +static target_ulong trace_handler_addr;
>  
>  /* XXX We have a race condition where we actually have a level triggered
>   *     interrupt, but the infrastructure can't expose that yet, so the guest
> @@ -509,6 +510,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
>      kvmppc_hw_debug_points_init(cenv);
>  
> +    trace_handler_addr = (cenv->excp_vectors[POWERPC_EXCP_TRACE] |
> +                          AIL_C000_0000_0000_4000_OFFSET);

Effectively caching this as a global variable is pretty horrible.  A
function to make the above calculation when you need it would be
better.

Also, won't the calculation above be wrong if the guest is using the
low AIL value?

> +
>      return ret;
>  }
>  
> @@ -1553,6 +1557,24 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>  
>  void kvm_arch_set_singlestep(CPUState *cs, int enabled)
>  {
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    if (kvmppc_is_pr(kvm_state)) {

Explicitly testing for KVM PR is generally wrong (except as a
fallback).  Instead you should add a capability flag to KVM which you
use to test the feature's presence in qemu.  That capability can then
be set in HV, but not PR.

> +            return;
> +    }
> +
> +    if (enabled) {
> +        /* MSR_SE = 1 will cause a Trace Interrupt in the guest after
> +         * the next instruction executes. */
> +        env->msr |= (1ULL << MSR_SE);
> +
> +        /* We set a breakpoint at the interrupt handler address so
> +         * that the singlestep will be seen by KVM (this is treated by
> +         * KVM like an ordinary breakpoint) and control is returned to
> +         * QEMU. */
> +        kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +    }
>  }
>  
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> @@ -1594,6 +1616,43 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct 
> kvm_guest_debug *dbg)
>      }
>  }
>  
> +static int kvm_handle_singlestep(CPUState *cs,
> +                                 struct kvm_debug_exit_arch *arch_info)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong msr = env->msr;
> +    uint32_t insn;
> +    int ret = 1;

You never set this to anything other than 1...

> +    int reg;
> +
> +    if (kvmppc_is_pr(kvm_state)) {
> +        return ret;
> +    }
> +
> +    if (arch_info->address == trace_handler_addr) {
> +        cpu_synchronize_state(cs);
> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +
> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
> +                            sizeof(insn), 0);
> +
> +        /* If the last instruction was a mfmsr, make sure that the
> +         * MSR_SE bit is not set to avoid the guest kernel knowing
> +         * that it is being single-stepped */
> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
> +            reg = extract32(insn, 21, 5);
> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
> +        }

Hm.  What happens if both qemu and the guest itself attempt to single
step at the same time?  How do you distinguish between the cases?

> +
> +        env->nip = env->spr[SPR_SRR0];
> +        env->msr = msr &= ~(1ULL << MSR_SE);

You clear SE after tracing one instruction; is that intentional?

> +        cpu_synchronize_state(cs);

You're effectively aborting the entry to the single step exception
vector; don't you need to set MSR to SRR1, not to the current MSR
value which will have been modified for the singlestep exception entry?

You only need the first call to sync_state.  It sets a flag causing
the state to be written back before returning to the guest.

> +    }
> +
> +    return ret;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -1604,7 +1663,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct 
> kvm_run *run)
>      int flag = 0;
>  
>      if (cs->singlestep_enabled) {
> -        handle = 1;
> +        handle = kvm_handle_singlestep(cs, arch_info);
>      } else if (arch_info->status) {
>          if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
>              if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {

-- 
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

signature.asc
Description: PGP signature