From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755007AbbDINYx (ORCPT ); Thu, 9 Apr 2015 09:24:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55724 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752261AbbDINYv (ORCPT ); Thu, 9 Apr 2015 09:24:51 -0400 Date: Thu, 9 Apr 2015 15:24:43 +0200 From: Andrew Jones To: Alex =?iso-8859-1?Q?Benn=E9e?= Cc: kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org, marc.zyngier@arm.com, peter.maydell@linaro.org, agraf@suse.de, pbonzini@redhat.com, zhichao.huang@linaro.org, jan.kiszka@siemens.com, dahi@linux.vnet.ibm.com, r65777@freescale.com, bp@suse.de, Gleb Natapov , Russell King , Catalin Marinas , Will Deacon , open list Subject: Re: [PATCH v2 07/10] KVM: arm64: guest debug, add support for single-step Message-ID: <20150409132442.GE3212@hawk.usersys.redhat.com> References: <1427814488-28467-1-git-send-email-alex.bennee@linaro.org> <1427814488-28467-8-git-send-email-alex.bennee@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1427814488-28467-8-git-send-email-alex.bennee@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > 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 > > +#include > +#include > #include > #include > +#include > + > +/* 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: drjones@redhat.com (Andrew Jones) Date: Thu, 9 Apr 2015 15:24:43 +0200 Subject: [PATCH v2 07/10] KVM: arm64: guest debug, add support for single-step In-Reply-To: <1427814488-28467-8-git-send-email-alex.bennee@linaro.org> References: <1427814488-28467-1-git-send-email-alex.bennee@linaro.org> <1427814488-28467-8-git-send-email-alex.bennee@linaro.org> Message-ID: <20150409132442.GE3212@hawk.usersys.redhat.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > --- > 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 > > +#include > +#include > #include > #include > +#include > + > +/* 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 >