Re: [PATCH v3 2/3] hvf: implement guest debugging on Apple Silicon hosts

[Peter Maydell]

On Sat, 14 Jan 2023 at 16:13, <francesco.cagnin@gmail.com> wrote:
>
> From: Francesco Cagnin <fcagnin@quarkslab.com>
>
> Support is added for single-stepping, software breakpoints, hardware
> breakpoints and watchpoints. The code has been structured like the KVM
> counterpart (and many parts are basically identical).
>
> Guests can be debugged through the gdbstub.
>
> While guest debugging is enabled, the guest can still read and write the
> DBG*_EL1 registers but they don't have any effect.
>
> Signed-off-by: Francesco Cagnin <fcagnin@quarkslab.com>

This fails to build on x86 macos:
Undefined symbols for architecture x86_64:
  "_hvf_arch_insert_hw_breakpoint", referenced from:
      _hvf_insert_breakpoint in accel_hvf_hvf-accel-ops.c.o
  "_hvf_arch_insert_sw_breakpoint", referenced from:
      _hvf_insert_breakpoint in accel_hvf_hvf-accel-ops.c.o
  "_hvf_arch_remove_all_hw_breakpoints", referenced from:
      _hvf_remove_all_breakpoints in accel_hvf_hvf-accel-ops.c.o
  "_hvf_arch_remove_hw_breakpoint", referenced from:
      _hvf_remove_breakpoint in accel_hvf_hvf-accel-ops.c.o
  "_hvf_arch_remove_sw_breakpoint", referenced from:
      _hvf_remove_breakpoint in accel_hvf_hvf-accel-ops.c.o
      _hvf_remove_all_breakpoints in accel_hvf_hvf-accel-ops.c.o
  "_hvf_arch_update_guest_debug", referenced from:
      _hvf_update_guest_debug in accel_hvf_hvf-all.c.o

I think your abstraction layer between generic hvf code and
architecture-specific hvf code is missing x86 "do nothing"
implementations of functions.



> +static bool hvf_supports_guest_debug(void)
> +{
> +#ifdef TARGET_AARCH64
> +    return true;
> +#else
> +    return false;
> +#endif

This should defer to an architecture-specific function (or just
be an architecture specific function) rather than using an inline
ifdef.


> index bb70082e45..3e99c80416 100644
> --- a/include/sysemu/hvf.h
> +++ b/include/sysemu/hvf.h
> @@ -36,4 +36,33 @@ typedef struct HVFState HVFState;
>  DECLARE_INSTANCE_CHECKER(HVFState, HVF_STATE,
>                           TYPE_HVF_ACCEL)
>
> +#ifdef NEED_CPU_H
> +#include "cpu.h"

Please don't put #include lines in the middle of files -- keep them at the top.

> +
> +int hvf_update_guest_debug(CPUState *cpu);

Ideally new global functions declared in headers should
have documentation comments.

> +            case HV_SYS_REG_DBGWVR15_EL1:
> +            case HV_SYS_REG_DBGWCR15_EL1: {
> +                const ARMCPRegInfo *ri;
> +                ri = get_arm_cp_reginfo(arm_cpu->cp_regs, 
> hvf_sreg_match[i].key);
> +                val = read_raw_cp_reg(env, ri);
> +
> +                arm_cpu->cpreg_values[hvf_sreg_match[i].cp_idx] = val;

This would be much easier to understand if there was some commentary
explaining what it was doing.

> +                continue;
> +            }
> +            }
> +        }
> +
>          ret = hv_vcpu_get_sys_reg(cpu->hvf->fd, hvf_sreg_match[i].reg, &val);
>          assert_hvf_ok(ret);




> +void hvf_arch_update_guest_debug(CPUState *cpu)
> +{
> +    ARMCPU *arm_cpu = ARM_CPU(cpu);
> +    CPUARMState *env = &arm_cpu->env;
> +    hv_return_t r = HV_SUCCESS;
> +
> +    guest_debug_enabled = cpu->singlestep_enabled ||
> +        hvf_sw_breakpoints_active(cpu) ||
> +        hvf_arm_hw_debug_active(cpu);
> +
> +    cpu_synchronize_state(cpu);
> +
> +    if (cpu->singlestep_enabled) {
> +        env->cp15.mdscr_el1 =
> +            deposit64(env->cp15.mdscr_el1, MDSCR_EL1_SS_SHIFT, 1, 1);
> +        pstate_write(env, pstate_read(env) | PSTATE_SS);
> +    } else {
> +        env->cp15.mdscr_el1 =
> +            deposit64(env->cp15.mdscr_el1, MDSCR_EL1_SS_SHIFT, 1, 0);
> +    }
> +
> +    if (hvf_arm_hw_debug_active(cpu)) {
> +        env->cp15.mdscr_el1 =
> +            deposit64(env->cp15.mdscr_el1, MDSCR_EL1_MDE_SHIFT, 1, 1);
> +
> +        int i;
> +        for (i = 0; i < cur_hw_bps; i++) {
> +            HWBreakpoint *bp = get_hw_bp(i);
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgbcr_regs[i], bp->bcr);
> +            assert_hvf_ok(r);
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgbvr_regs[i], bp->bvr);
> +            assert_hvf_ok(r);
> +        }
> +        for (i = 0; i < cur_hw_wps; i++) {
> +            HWWatchpoint *bp = get_hw_wp(i);
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgwcr_regs[i], bp->wcr);
> +            assert_hvf_ok(r);
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgwvr_regs[i], bp->wvr);
> +            assert_hvf_ok(r);
> +        }
> +    } else {
> +        env->cp15.mdscr_el1 =
> +            deposit64(env->cp15.mdscr_el1, MDSCR_EL1_MDE_SHIFT, 1, 0);
> +
> +        int i;
> +        for (i = 0; i < max_hw_bps; i++) {
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgbcr_regs[i], 
> env->cp15.dbgbcr[i]);
> +            assert_hvf_ok(r);
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgbvr_regs[i], 
> env->cp15.dbgbvr[i]);
> +            assert_hvf_ok(r);
> +        }
> +        for (i = 0; i < max_hw_wps; i++) {
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgwcr_regs[i], 
> env->cp15.dbgwcr[i]);
> +            assert_hvf_ok(r);
> +            r = hv_vcpu_set_sys_reg(cpu->hvf->fd, dbgwvr_regs[i], 
> env->cp15.dbgwvr[i]);
> +            assert_hvf_ok(r);
> +        }
> +    }

Can you describe what's going on here? Conceptually there are two lots
of debug register state -- the one the guest vCPU has, and the one we
want to be using when we're doing gdbstub debugging. Where are we
keeping the two copies of the state, how do we decide which one to
feed to hvf as the live version, etc ?

More generally, I would prefer to be able to determine your design
by looking at the commit message and comments, rather than by
reverse-engineering it from the code :-)

thanks
-- PMM