| Message ID | 1427814488-28467-8-git-send-email-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linaro.org> > > --- > 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. > + > /* 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) } > + /* > + * 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 >
Andrew Jones <drjones@redhat.com> 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@linaro.org> >> >> --- >> 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 >>
Hi Alex, 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. this sentence is hard to read. Do you mean: (a) As userspace can and will manipulate guest register, we must ensure that any tweaking of the registers before restarting the guest happens immediately before... or (b) As userspace manipulates guest registers before restarting the guest, we must ensure that any tweaking of the register happens immediately before... > Furthermore while guest debugging is in effect we need to squash the Furthermore, while guest debugging is in effect, (commas) > 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. I'm not sure I understand this last part of the sentence. Is the point that if we trap on a guest single-step exception we cannot easily inject such an exception back into the guest and therefore we trap the guest if it tries to set itself up for single-stepping? What is our recourse then? To just ignore the single-step setting of the guest and execute it as normal (while single-stepping the guest from the outside)? > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > 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 which guest registers are set/cleared by userspace exactly? s/by the hyper-visor controller/by userspace/ > + * result modification of the guest registers needs to take place As a result, (comma) s/needs to/must/ > + * after they have been restored in the hyp.S trampoline code. trampoline code? The trampoline code we are referring to is in hyp-init.S. Do you mean in EL2? Then just sya in hyp.S or say in EL2 or in hyp mode. > + */ > > 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 */ I couldn't find 'pre' as an independent word in any English dictionaries. I'm also not entirely sure what you mean? Who modifies this when, and why do we need to store this? > + struct { > + u32 pstate_ss_bit; > + u32 mdscr_el1_bits; > + > + } debug_saved_regs; If I understood this state correctly (see below), then guest_debug_state is probably a better name for this struct. > + > /* 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) hmm, not sure this is warranted if the 'saved_regs' is not the current state of the VM, which is sort of what the vcpu_gp_regs() and friends hint at. Maybe I'm just not getting exactly what piece of state it is. > /* > * 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 */ we may mess with? Can you be more specific? > +#define MDSCR_EL1_DEBUG_BITS (DBG_MDSCR_SS | \ > + DBG_MDSCR_KDE | \ > + DBG_MDSCR_MDE) > > /** > * 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; nit: add blank line > + /* > + * 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 not, we must prevent the guest from modifying them. ^^^ ^^^ ^^^ ^^^ Why must we prevent the guest from modifying them if we're not single-stepping the guest? > + */ > + 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 */ > + if (vcpu->guest_debug) { > + /* Restore pstate/mdscr bits we may have messed with */ Can you be more specific than "messed with"? "... bits that may have been modified if we were debugging the guest" ? Drew's suggestion may make this easier to read... > + *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); So the debug_saved_reg is simply the guest's view of the debug registers? In that case I think vcpu_guest_dbg_reg() is more clear. > + } > } > 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 >
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; + /* 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) /** * 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; + /* + * 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 */ + 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, };
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@linaro.org> --- 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