Andrew Jones <drjones@xxxxxxxxxx> writes:
> On Tue, Mar 31, 2015 at 04:08:05PM +0100, Alex Bennée wrote:
>> This adds support for single-stepping the guest. As userspace can and
>> will manipulate guest registers before restarting any tweaking of the
>> registers has to occur just before control is passed back to the guest.
>> Furthermore while guest debugging is in effect we need to squash the
>> ability of the guest to single-step itself as we have no easy way of
>> re-entering the guest after the exception has been delivered to the
>> hypervisor.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@xxxxxxxxxx>
>>
>> ---
>> v2
>> - Move pstate/mdscr manipulation into C
>> - don't export guest_debug to assembly
>> - add accessor for saved_debug regs
>> - tweak save/restore of mdscr_el1
>>
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index d3bc8dc..c1ed8cb 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -304,7 +304,21 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> kvm_arm_set_running_vcpu(NULL);
>> }
>>
>> -#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE|KVM_GUESTDBG_USE_SW_BP)
>> +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE | \
>> + KVM_GUESTDBG_USE_SW_BP | \
>> + KVM_GUESTDBG_SINGLESTEP)
>> +
>> +/**
>> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
>> + * @kvm: pointer to the KVM struct
>> + * @kvm_guest_debug: the ioctl data buffer
>> + *
>> + * This sets up the VM for guest debugging. Care has to be taken when
>> + * manipulating guest registers as these will be set/cleared by the
>> + * hyper-visor controller, typically before each kvm_run event. As a
>> + * result modification of the guest registers needs to take place
>> + * after they have been restored in the hyp.S trampoline code.
>> + */
>>
>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>> struct kvm_guest_debug *dbg)
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 0631840..6a33647 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -121,6 +121,13 @@ struct kvm_vcpu_arch {
>> * here.
>> */
>>
>> + /* Registers pre any guest debug manipulations */
>> + struct {
>> + u32 pstate_ss_bit;
>> + u32 mdscr_el1_bits;
>> +
>> + } debug_saved_regs;
>
> Hmm, you have a struct called "regs", but then each member is
> suffixed with _bit(s). This looks awkward.
Later on mdscr gets expanded and properly shadowed but your right the
pstate_ss_bit is a bit of a fiddle. I'll see if there is a neater way.
>
>
>> +
>> /* Don't run the guest */
>> bool pause;
>>
>> @@ -143,6 +150,7 @@ struct kvm_vcpu_arch {
>>
>> #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs)
>> #define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
>> +#define vcpu_debug_saved_reg(v, r) ((v)->arch.debug_saved_regs.r)
>> /*
>> * CP14 and CP15 live in the same array, as they are backed by the
>> * same system registers.
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index cff0475..b32362c 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -19,8 +19,16 @@
>>
>> #include <linux/kvm_host.h>
>>
>> +#include <asm/debug-monitors.h>
>> +#include <asm/kvm_asm.h>
>> #include <asm/kvm_arm.h>
>> #include <asm/kvm_host.h>
>> +#include <asm/kvm_emulate.h>
>> +
>> +/* These are the bits of MDSCR_EL1 we may mess with */
>> +#define MDSCR_EL1_DEBUG_BITS (DBG_MDSCR_SS | \
>> + DBG_MDSCR_KDE | \
>> + DBG_MDSCR_MDE)
>
> _MASK instead of _BITS ?
>
>>
>> /**
>> * kvm_arch_setup_debug - set-up debug related stuff
>> @@ -51,15 +59,46 @@ void kvm_arch_setup_debug(struct kvm_vcpu *vcpu)
>> else
>> vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA;
>>
>> - /* Trap breakpoints? */
>> - if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
>> + /* Is Guest debugging in effect? */
>> + if (vcpu->guest_debug) {
>> vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>> - else
>> - vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
>>
>> + /* Save pstate/mdscr */
>> + vcpu_debug_saved_reg(vcpu, pstate_ss_bit) =
>> + *vcpu_cpsr(vcpu) & DBG_SPSR_SS;
>> + vcpu_debug_saved_reg(vcpu, mdscr_el1_bits) =
>> + vcpu_sys_reg(vcpu, MDSCR_EL1) & MDSCR_EL1_DEBUG_BITS;
>
> I think it would be clearer if we embed the masks into helper
> functions, and, assuming we drop the _bits concept too, then
>
> #define SPSR_DEBUG_MASK DBG_SPSR_SS
>
> vcpu_debug_save_regs(vcpu)
> {
> vcpu->arch.debug_saved_regs.pstate = *vcpu_cpsr(vcpu);
> vcpu->arch.debug_saved_regs.mdscr_el1 = vcpu_sys_reg(vcpu, MDSCR_EL1);
> }
>
> vcpu_debug_restore_regs(vcpu)
> {
> *vcpu_cpsr(vcpu) |=
> (vcpu->arch.debug_saved_regs.pstate & SPSR_DEBUG_MASK);
> vcpu_sys_reg(vcpu, MDSCR_EL1) |=
> (vcpu->arch.debug_saved_regs.mdscr_el1 & MDSCR_EL1_DEBUG_MASK)
> }
Makes sense
>
>> + /*
>> + * Single Step (ARM ARM D2.12.3 The software step state
>> + * machine)
>> + *
>> + * If we are doing Single Step we need to manipulate
>> + * MDSCR_EL1.SS and PSTATE.SS. If not we need to
>> + * suppress the guest from messing with it.
>> + */
>> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>> + vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
>> + } else {
>> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
>> + vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
>> + }
>> +
>> + } else {
>> + /* Debug operations can go straight to the guest */
>> + vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
>> + }
>> }
>>
>> void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
>> {
>> - /* Nothing to do yet */
>
> This would now just be
>
> if (vcpu->guest_debug)
> vcpu_debug_restore_regs(vcpu);
>
>> + if (vcpu->guest_debug) {
>> + /* Restore pstate/mdscr bits we may have messed with */
>> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
>> + *vcpu_cpsr(vcpu) |= vcpu_debug_saved_reg(vcpu, pstate_ss_bit);
>> +
>> + vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~MDSCR_EL1_DEBUG_BITS;
>> + vcpu_sys_reg(vcpu, MDSCR_EL1) |=
>> + vcpu_debug_saved_reg(vcpu, mdscr_el1_bits);
>> + }
>> }
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index ed1bbb4..16accae 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -101,6 +101,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> run->debug.arch.hsr = hsr;
>>
>> switch (hsr >> ESR_ELx_EC_SHIFT) {
>> + case ESR_ELx_EC_SOFTSTP_LOW:
>> case ESR_ELx_EC_BKPT32:
>> case ESR_ELx_EC_BRK64:
>> run->debug.arch.pc = *vcpu_pc(vcpu);
>> @@ -127,6 +128,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>> [ESR_ELx_EC_SYS64] = kvm_handle_sys_reg,
>> [ESR_ELx_EC_IABT_LOW] = kvm_handle_guest_abort,
>> [ESR_ELx_EC_DABT_LOW] = kvm_handle_guest_abort,
>> + [ESR_ELx_EC_SOFTSTP_LOW]= kvm_handle_guest_debug,
>> [ESR_ELx_EC_BKPT32] = kvm_handle_guest_debug,
>> [ESR_ELx_EC_BRK64] = kvm_handle_guest_debug,
>> };
>> --
>> 2.3.4
>>
--
Alex Bennée
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
