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

[Peter Maydell]

On Wed, 16 Nov 2022 at 17:48, <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.
>
> Signed-off-by: Francesco Cagnin <fcagnin@quarkslab.com>

Hi; sorry it's taken me a while to get to this patchset.

> +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;
> +    bool trap_debug_exceptions = false;
> +
> +    cpu_synchronize_state(cpu);
> +
> +    if (cpu->singlestep_enabled) {
> +        trap_debug_exceptions = true;
> +
> +        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_sw_breakpoints_active(cpu)) {
> +        trap_debug_exceptions = true;
> +    }
> +
> +    if (hvf_arm_hw_debug_active(cpu)) {
> +        trap_debug_exceptions = true;
> +
> +        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);
> +            env->cp15.dbgbcr[i] = bp->bcr;
> +            env->cp15.dbgbvr[i] = bp->bvr;
> +        }
> +        for (i = 0; i < cur_hw_wps; i++) {
> +            HWWatchpoint *bp = get_hw_wp(i);
> +            env->cp15.dbgwcr[i] = bp->wcr;
> +            env->cp15.dbgwvr[i] = bp->wvr;
> +        }

Can you explain how the patches keep the guest's idea of the debug
registers distinct from the QEMU debugstub's values from them?
Looking at this patch it seems like we just directly set the
cp15.* values with what the debug stub wants them to be here;
but in patch 3 any trapped guest writes to the debug registers
also simply write to the same env fields.

With KVM, this is handled by the host kernel: QEMU tells KVM
that it wants to do guest debug, and then uses the KVM_SET_GUEST_DEBUG
ioctl to say what the bp/wp etc register values should be. The
KVM kernel then uses those values when it runs the VM, as well
as ensuring that debug exceptions go to EL2 rather than EL1,
and that EL1 accesses to the debug regs are also trapped to EL2.
Guest attempts to access the debug registers are then handled by
having reads and writes access fields which correspond to the
guest's view of the debug registers, which is separate from the
values that the QEMU debug stub is using. The effect is that while
QEMU is doing debug of the VM, the guest can still read and write
the debug registers, they just don't have any effect. The guest
can't do anything that messes with the state of the debug registers
that QEMU's debug stub is relying on. If/when QEMU says it's
done with doing guest debug, then the host kernel goes back to using
the guest's values of the debug registers. (In the kernel sources
this is done in arch/arm64/kvm/debug.c, with the trapping of debug
registers done in arch/arm64/kvm/sys_regs.c.)

I suspect that with HVF we need to do the equivalent of this ourselves
in QEMU.

thanks
-- PMM