diff mbox

bookehv: Handle debug exception on guest exit

Message ID 1363801557-27436-1-git-send-email-Bharat.Bhushan@freescale.com
State New, archived
Headers show

Commit Message

Bharat Bhushan March 20, 2013, 5:45 p.m. UTC 
EPCR.DUVD controls whether the debug events can come in
hypervisor mode or not. When KVM guest is using the debug
resource then we do not want debug events to be captured
in guest entry/exit path. So we set EPCR.DUVD when entering
and clears EPCR.DUVD when exiting from guest.

Debug instruction complete is a post-completion debug
exception but debug event gets posted on the basis of MSR
before the instruction is executed. Now if the instruction
switches the context from guest mode (MSR.GS = 1) to hypervisor
mode (MSR.GS = 0) then the xSRR0 points to first instruction of
KVM handler and xSRR1 points that MSR.GS is clear
(hypervisor context). Now as xSRR1.GS is used to decide whether
KVM handler will be invoked to handle the exception or host
host kernel debug handler will be invoked to handle the exception.
This leads to host kernel debug handler handling the exception
which should either be handled by KVM.

This is tested on e500mc in 32 bit mode

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v0:
 - Do not apply this change for debug_crit as we do not know those chips have issue or not.
 - corrected 64bit case branching

 arch/powerpc/kernel/exceptions-64e.S |   29 ++++++++++++++++++++++++++++-
 arch/powerpc/kernel/head_booke.h     |   26 ++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletions(-)

Comments

Alexander Graf April 4, 2013, 1:25 p.m. UTC | #1 
On 20.03.2013, at 18:45, Bharat Bhushan wrote:

> EPCR.DUVD controls whether the debug events can come in
> hypervisor mode or not. When KVM guest is using the debug
> resource then we do not want debug events to be captured
> in guest entry/exit path. So we set EPCR.DUVD when entering
> and clears EPCR.DUVD when exiting from guest.
> 
> Debug instruction complete is a post-completion debug
> exception but debug event gets posted on the basis of MSR
> before the instruction is executed. Now if the instruction
> switches the context from guest mode (MSR.GS = 1) to hypervisor
> mode (MSR.GS = 0) then the xSRR0 points to first instruction of
> KVM handler and xSRR1 points that MSR.GS is clear
> (hypervisor context). Now as xSRR1.GS is used to decide whether
> KVM handler will be invoked to handle the exception or host
> host kernel debug handler will be invoked to handle the exception.
> This leads to host kernel debug handler handling the exception
> which should either be handled by KVM.
> 
> This is tested on e500mc in 32 bit mode
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v0:
> - Do not apply this change for debug_crit as we do not know those chips have issue or not.
> - corrected 64bit case branching
> 
> arch/powerpc/kernel/exceptions-64e.S |   29 ++++++++++++++++++++++++++++-
> arch/powerpc/kernel/head_booke.h     |   26 ++++++++++++++++++++++++++
> 2 files changed, 54 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 4684e33..8b26294 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -516,6 +516,33 @@ kernel_dbg_exc:
> 	andis.	r15,r14,DBSR_IC@h
> 	beq+	1f
> 
> +#ifdef CONFIG_KVM_BOOKE_HV
> +	/*
> +	 * EPCR.DUVD controls whether the debug events can come in
> +	 * hypervisor mode or not. When KVM guest is using the debug
> +	 * resource then we do not want debug events to be captured
> +	 * in guest entry/exit path. So we set EPCR.DUVD when entering
> +	 * and clears EPCR.DUVD when exiting from guest.
> +	 * Debug instruction complete is a post-completion debug
> +	 * exception but debug event gets posted on the basis of MSR
> +	 * before the instruction is executed. Now if the instruction
> +	 * switches the context from guest mode (MSR.GS = 1) to hypervisor
> +	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of

Can't we just execute that code path with MSR.DE=0?


Alex

> +	 * KVM handler and xSRR1 points that MSR.GS is clear
> +	 * (hypervisor context). Now as xSRR1.GS is used to decide whether
> +	 * KVM handler will be invoked to handle the exception or host
> +	 * host kernel debug handler will be invoked to handle the exception.
> +	 * This leads to host kernel debug handler handling the exception
> +	 * which should either be handled by KVM.
> +	 */
> +	mfspr	r10, SPRN_EPCR
> +	andis.	r10,r10,SPRN_EPCR_DUVD@h
> +	beq+	2f
> +
> +	andis.	r10,r9,MSR_GS@h
> +	beq+	3f
> +2:
> +#endif
> 	LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e)
> 	LOAD_REG_IMMEDIATE(r15,interrupt_end_book3e)
> 	cmpld	cr0,r10,r14
> @@ -523,7 +550,7 @@ kernel_dbg_exc:
> 	blt+	cr0,1f
> 	bge+	cr1,1f
> 
> -	/* here it looks like we got an inappropriate debug exception. */
> +3:	/* here it looks like we got an inappropriate debug exception. */
> 	lis	r14,DBSR_IC@h		/* clear the IC event */
> 	rlwinm	r11,r11,0,~MSR_DE	/* clear DE in the DSRR1 value */
> 	mtspr	SPRN_DBSR,r14
> diff --git a/arch/powerpc/kernel/head_booke.h b/arch/powerpc/kernel/head_booke.h
> index 5f051ee..edc6a3b 100644
> --- a/arch/powerpc/kernel/head_booke.h
> +++ b/arch/powerpc/kernel/head_booke.h
> @@ -285,7 +285,33 @@ label:
> 	mfspr	r10,SPRN_DBSR;		/* check single-step/branch taken */  \
> 	andis.	r10,r10,(DBSR_IC|DBSR_BT)@h;				      \
> 	beq+	2f;							      \
> +#ifdef CONFIG_KVM_BOOKE_HV						      \
> +	/*								      \
> +	 * EPCR.DUVD controls whether the debug events can come in	      \
> +	 * hypervisor mode or not. When KVM guest is using the debug	      \
> +	 * resource then we do not want debug events to be captured 	      \
> +	 * in guest entry/exit path. So we set EPCR.DUVD when entering	      \
> +	 * and clears EPCR.DUVD when exiting from guest.		      \
> +	 * Debug instruction complete is a post-completion debug	      \
> +	 * exception but debug event gets posted on the basis of MSR	      \
> +	 * before the instruction is executed. Now if the instruction	      \
> +	 * switches the context from guest mode (MSR.GS = 1) to hypervisor    \
> +	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of    \
> +	 * KVM handler and xSRR1 points that MSR.GS is clear 		      \
> +	 * (hypervisor context). Now as xSRR1.GS is used to decide whether    \
> +	 * KVM handler will be invoked to handle the exception or host	      \
> +	 * host kernel debug handler will be invoked to handle the exception. \
> +	 * This leads to host kernel debug handler handling the exception     \
> +	 * which should either be handled by KVM.			      \
> +	 */								      \
> +	mfspr	r10, SPRN_EPCR;						      \
> +	andis.	r10,r10,SPRN_EPCR_DUVD@h;				      \
> +	beq+	3f;							      \
> 									      \
> +	andis.	r10,r9,MSR_GS@h;				    	      \
> +	beq+	1f;							      \
> +3:									      \
> +#endif									      \
> 	lis	r10,KERNELBASE@h;	/* check if exception in vectors */   \
> 	ori	r10,r10,KERNELBASE@l;					      \
> 	cmplw	r12,r10;						      \
> -- 
> 1.7.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan April 4, 2013, 2:58 p.m. UTC | #2 
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, April 04, 2013 6:55 PM
> To: Bhushan Bharat-R65777
> Cc: linuxppc-dev@lists.ozlabs.org; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org;
> Wood Scott-B07421; Bhushan Bharat-R65777
> Subject: Re: [PATCH] bookehv: Handle debug exception on guest exit
> 
> 
> On 20.03.2013, at 18:45, Bharat Bhushan wrote:
> 
> > EPCR.DUVD controls whether the debug events can come in hypervisor
> > mode or not. When KVM guest is using the debug resource then we do not
> > want debug events to be captured in guest entry/exit path. So we set
> > EPCR.DUVD when entering and clears EPCR.DUVD when exiting from guest.
> >
> > Debug instruction complete is a post-completion debug exception but
> > debug event gets posted on the basis of MSR before the instruction is
> > executed. Now if the instruction switches the context from guest mode
> > (MSR.GS = 1) to hypervisor mode (MSR.GS = 0) then the xSRR0 points to
> > first instruction of KVM handler and xSRR1 points that MSR.GS is clear
> > (hypervisor context). Now as xSRR1.GS is used to decide whether KVM
> > handler will be invoked to handle the exception or host host kernel
> > debug handler will be invoked to handle the exception.
> > This leads to host kernel debug handler handling the exception which
> > should either be handled by KVM.
> >
> > This is tested on e500mc in 32 bit mode
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v0:
> > - Do not apply this change for debug_crit as we do not know those chips have
> issue or not.
> > - corrected 64bit case branching
> >
> > arch/powerpc/kernel/exceptions-64e.S |   29 ++++++++++++++++++++++++++++-
> > arch/powerpc/kernel/head_booke.h     |   26 ++++++++++++++++++++++++++
> > 2 files changed, 54 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/exceptions-64e.S
> > b/arch/powerpc/kernel/exceptions-64e.S
> > index 4684e33..8b26294 100644
> > --- a/arch/powerpc/kernel/exceptions-64e.S
> > +++ b/arch/powerpc/kernel/exceptions-64e.S
> > @@ -516,6 +516,33 @@ kernel_dbg_exc:
> > 	andis.	r15,r14,DBSR_IC@h
> > 	beq+	1f
> >
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +	/*
> > +	 * EPCR.DUVD controls whether the debug events can come in
> > +	 * hypervisor mode or not. When KVM guest is using the debug
> > +	 * resource then we do not want debug events to be captured
> > +	 * in guest entry/exit path. So we set EPCR.DUVD when entering
> > +	 * and clears EPCR.DUVD when exiting from guest.
> > +	 * Debug instruction complete is a post-completion debug
> > +	 * exception but debug event gets posted on the basis of MSR
> > +	 * before the instruction is executed. Now if the instruction
> > +	 * switches the context from guest mode (MSR.GS = 1) to hypervisor
> > +	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of
> 
> Can't we just execute that code path with MSR.DE=0?

Single stepping uses DBCR0.IC (instruction complete).
Can you describe how MSR.DE = 0 will work?

> 
> 
> Alex
> 
> > +	 * KVM handler and xSRR1 points that MSR.GS is clear
> > +	 * (hypervisor context). Now as xSRR1.GS is used to decide whether
> > +	 * KVM handler will be invoked to handle the exception or host
> > +	 * host kernel debug handler will be invoked to handle the exception.
> > +	 * This leads to host kernel debug handler handling the exception
> > +	 * which should either be handled by KVM.
> > +	 */
> > +	mfspr	r10, SPRN_EPCR
> > +	andis.	r10,r10,SPRN_EPCR_DUVD@h
> > +	beq+	2f
> > +
> > +	andis.	r10,r9,MSR_GS@h
> > +	beq+	3f
> > +2:
> > +#endif
> > 	LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e)
> > 	LOAD_REG_IMMEDIATE(r15,interrupt_end_book3e)
> > 	cmpld	cr0,r10,r14
> > @@ -523,7 +550,7 @@ kernel_dbg_exc:
> > 	blt+	cr0,1f
> > 	bge+	cr1,1f
> >
> > -	/* here it looks like we got an inappropriate debug exception. */
> > +3:	/* here it looks like we got an inappropriate debug exception. */
> > 	lis	r14,DBSR_IC@h		/* clear the IC event */
> > 	rlwinm	r11,r11,0,~MSR_DE	/* clear DE in the DSRR1 value */
> > 	mtspr	SPRN_DBSR,r14
> > diff --git a/arch/powerpc/kernel/head_booke.h
> > b/arch/powerpc/kernel/head_booke.h
> > index 5f051ee..edc6a3b 100644
> > --- a/arch/powerpc/kernel/head_booke.h
> > +++ b/arch/powerpc/kernel/head_booke.h
> > @@ -285,7 +285,33 @@ label:
> > 	mfspr	r10,SPRN_DBSR;		/* check single-step/branch taken */  \
> > 	andis.	r10,r10,(DBSR_IC|DBSR_BT)@h;				      \
> > 	beq+	2f;							      \
> > +#ifdef CONFIG_KVM_BOOKE_HV						      \
> > +	/*								      \
> > +	 * EPCR.DUVD controls whether the debug events can come in	      \
> > +	 * hypervisor mode or not. When KVM guest is using the debug	      \
> > +	 * resource then we do not want debug events to be captured 	      \
> > +	 * in guest entry/exit path. So we set EPCR.DUVD when entering	      \
> > +	 * and clears EPCR.DUVD when exiting from guest.		      \
> > +	 * Debug instruction complete is a post-completion debug	      \
> > +	 * exception but debug event gets posted on the basis of MSR	      \
> > +	 * before the instruction is executed. Now if the instruction	      \
> > +	 * switches the context from guest mode (MSR.GS = 1) to hypervisor    \
> > +	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of    \
> > +	 * KVM handler and xSRR1 points that MSR.GS is clear 		      \
> > +	 * (hypervisor context). Now as xSRR1.GS is used to decide whether    \
> > +	 * KVM handler will be invoked to handle the exception or host	      \
> > +	 * host kernel debug handler will be invoked to handle the exception. \
> > +	 * This leads to host kernel debug handler handling the exception     \
> > +	 * which should either be handled by KVM.			      \
> > +	 */								      \
> > +	mfspr	r10, SPRN_EPCR;						      \
> > +	andis.	r10,r10,SPRN_EPCR_DUVD@h;				      \
> > +	beq+	3f;							      \
> > 									      \
> > +	andis.	r10,r9,MSR_GS@h;				    	      \
> > +	beq+	1f;							      \
> > +3:									      \
> > +#endif									      \
> > 	lis	r10,KERNELBASE@h;	/* check if exception in vectors */   \
> > 	ori	r10,r10,KERNELBASE@l;					      \
> > 	cmplw	r12,r10;						      \
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@vger.kernel.org More majordomo info
> > at  http://vger.kernel.org/majordomo-info.html
> 


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan April 5, 2013, 7:53 a.m. UTC | #3 
Hi Kumar/Benh,

After further looking into the code I think that if we correct the vector range below in DebugDebug handler then we do not need the change I provided in this patch.

Here is the snapshot for 32 bit (head_booke.h, same will be true for 64 bit):

#define DEBUG_DEBUG_EXCEPTION                                                 \
        START_EXCEPTION(DebugDebug);                                          \
        DEBUG_EXCEPTION_PROLOG;                                               \
                                                                              \
        /*                                                                    \
         * If there is a single step or branch-taken exception in an          \
         * exception entry sequence, it was probably meant to apply to        \
         * the code where the exception occurred (since exception entry       \
         * doesn't turn off DE automatically).  We simulate the effect        \
         * of turning off DE on entry to an exception handler by turning      \
         * off DE in the DSRR1 value and clearing the debug status.           \
         */                                                                   \
        mfspr   r10,SPRN_DBSR;          /* check single-step/branch taken */  \
        andis.  r10,r10,(DBSR_IC|DBSR_BT)@h;                                  \
        beq+    2f;                                                           \
                                                                              \
        lis     r10,KERNELBASE@h;       /* check if exception in vectors */   \
        ori     r10,r10,KERNELBASE@l;                                         \
        cmplw   r12,r10;                                                      \
        blt+    2f;                     /* addr below exception vectors */    \
                                                                              \
        lis     r10,DebugDebug@h;                                        \
        ori     r10,r10,DebugDebug@l;                                            \

^^^^
	Here we assume all exception vector ends at DebugDebug, which is not correct.
	We probably should get proper end by using some start_vector and end_vector lebels
	or at least use end at Ehvpriv (which is last defined in head_fsl_booke.S for PowerPC. Is that correct?

	
        cmplw   r12,r10;                                                      \
        bgt+    2f;                     /* addr above exception vectors */    \

Thanks
-Bharat


> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
> Behalf Of Bhushan Bharat-R65777
> Sent: Thursday, April 04, 2013 8:29 PM
> To: Alexander Graf
> Cc: linuxppc-dev@lists.ozlabs.org; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org;
> Wood Scott-B07421
> Subject: RE: [PATCH] bookehv: Handle debug exception on guest exit
> 
> 
> 
> > -----Original Message-----
> > From: Alexander Graf [mailto:agraf@suse.de]
> > Sent: Thursday, April 04, 2013 6:55 PM
> > To: Bhushan Bharat-R65777
> > Cc: linuxppc-dev@lists.ozlabs.org; kvm@vger.kernel.org;
> > kvm-ppc@vger.kernel.org; Wood Scott-B07421; Bhushan Bharat-R65777
> > Subject: Re: [PATCH] bookehv: Handle debug exception on guest exit
> >
> >
> > On 20.03.2013, at 18:45, Bharat Bhushan wrote:
> >
> > > EPCR.DUVD controls whether the debug events can come in hypervisor
> > > mode or not. When KVM guest is using the debug resource then we do
> > > not want debug events to be captured in guest entry/exit path. So we
> > > set EPCR.DUVD when entering and clears EPCR.DUVD when exiting from guest.
> > >
> > > Debug instruction complete is a post-completion debug exception but
> > > debug event gets posted on the basis of MSR before the instruction
> > > is executed. Now if the instruction switches the context from guest
> > > mode (MSR.GS = 1) to hypervisor mode (MSR.GS = 0) then the xSRR0
> > > points to first instruction of KVM handler and xSRR1 points that
> > > MSR.GS is clear (hypervisor context). Now as xSRR1.GS is used to
> > > decide whether KVM handler will be invoked to handle the exception
> > > or host host kernel debug handler will be invoked to handle the exception.
> > > This leads to host kernel debug handler handling the exception which
> > > should either be handled by KVM.
> > >
> > > This is tested on e500mc in 32 bit mode
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > ---
> > > v0:
> > > - Do not apply this change for debug_crit as we do not know those
> > > chips have
> > issue or not.
> > > - corrected 64bit case branching
> > >
> > > arch/powerpc/kernel/exceptions-64e.S |   29 ++++++++++++++++++++++++++++-
> > > arch/powerpc/kernel/head_booke.h     |   26 ++++++++++++++++++++++++++
> > > 2 files changed, 54 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/exceptions-64e.S
> > > b/arch/powerpc/kernel/exceptions-64e.S
> > > index 4684e33..8b26294 100644
> > > --- a/arch/powerpc/kernel/exceptions-64e.S
> > > +++ b/arch/powerpc/kernel/exceptions-64e.S
> > > @@ -516,6 +516,33 @@ kernel_dbg_exc:
> > > 	andis.	r15,r14,DBSR_IC@h
> > > 	beq+	1f
> > >
> > > +#ifdef CONFIG_KVM_BOOKE_HV
> > > +	/*
> > > +	 * EPCR.DUVD controls whether the debug events can come in
> > > +	 * hypervisor mode or not. When KVM guest is using the debug
> > > +	 * resource then we do not want debug events to be captured
> > > +	 * in guest entry/exit path. So we set EPCR.DUVD when entering
> > > +	 * and clears EPCR.DUVD when exiting from guest.
> > > +	 * Debug instruction complete is a post-completion debug
> > > +	 * exception but debug event gets posted on the basis of MSR
> > > +	 * before the instruction is executed. Now if the instruction
> > > +	 * switches the context from guest mode (MSR.GS = 1) to hypervisor
> > > +	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of
> >
> > Can't we just execute that code path with MSR.DE=0?
> 
> Single stepping uses DBCR0.IC (instruction complete).
> Can you describe how MSR.DE = 0 will work?
> 
> >
> >
> > Alex
> >
> > > +	 * KVM handler and xSRR1 points that MSR.GS is clear
> > > +	 * (hypervisor context). Now as xSRR1.GS is used to decide whether
> > > +	 * KVM handler will be invoked to handle the exception or host
> > > +	 * host kernel debug handler will be invoked to handle the exception.
> > > +	 * This leads to host kernel debug handler handling the exception
> > > +	 * which should either be handled by KVM.
> > > +	 */
> > > +	mfspr	r10, SPRN_EPCR
> > > +	andis.	r10,r10,SPRN_EPCR_DUVD@h
> > > +	beq+	2f
> > > +
> > > +	andis.	r10,r9,MSR_GS@h
> > > +	beq+	3f
> > > +2:
> > > +#endif
> > > 	LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e)
> > > 	LOAD_REG_IMMEDIATE(r15,interrupt_end_book3e)
> > > 	cmpld	cr0,r10,r14
> > > @@ -523,7 +550,7 @@ kernel_dbg_exc:
> > > 	blt+	cr0,1f
> > > 	bge+	cr1,1f
> > >
> > > -	/* here it looks like we got an inappropriate debug exception. */
> > > +3:	/* here it looks like we got an inappropriate debug exception. */
> > > 	lis	r14,DBSR_IC@h		/* clear the IC event */
> > > 	rlwinm	r11,r11,0,~MSR_DE	/* clear DE in the DSRR1 value */
> > > 	mtspr	SPRN_DBSR,r14
> > > diff --git a/arch/powerpc/kernel/head_booke.h
> > > b/arch/powerpc/kernel/head_booke.h
> > > index 5f051ee..edc6a3b 100644
> > > --- a/arch/powerpc/kernel/head_booke.h
> > > +++ b/arch/powerpc/kernel/head_booke.h
> > > @@ -285,7 +285,33 @@ label:
> > > 	mfspr	r10,SPRN_DBSR;		/* check single-step/branch taken */  \
> > > 	andis.	r10,r10,(DBSR_IC|DBSR_BT)@h;				      \
> > > 	beq+	2f;							      \
> > > +#ifdef CONFIG_KVM_BOOKE_HV						      \
> > > +	/*								      \
> > > +	 * EPCR.DUVD controls whether the debug events can come in	      \
> > > +	 * hypervisor mode or not. When KVM guest is using the debug	      \
> > > +	 * resource then we do not want debug events to be captured 	      \
> > > +	 * in guest entry/exit path. So we set EPCR.DUVD when entering	      \
> > > +	 * and clears EPCR.DUVD when exiting from guest.		      \
> > > +	 * Debug instruction complete is a post-completion debug	      \
> > > +	 * exception but debug event gets posted on the basis of MSR	      \
> > > +	 * before the instruction is executed. Now if the instruction	      \
> > > +	 * switches the context from guest mode (MSR.GS = 1) to hypervisor    \
> > > +	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of    \
> > > +	 * KVM handler and xSRR1 points that MSR.GS is clear 		      \
> > > +	 * (hypervisor context). Now as xSRR1.GS is used to decide whether    \
> > > +	 * KVM handler will be invoked to handle the exception or host	      \
> > > +	 * host kernel debug handler will be invoked to handle the exception. \
> > > +	 * This leads to host kernel debug handler handling the exception     \
> > > +	 * which should either be handled by KVM.			      \
> > > +	 */								      \
> > > +	mfspr	r10, SPRN_EPCR;						      \
> > > +	andis.	r10,r10,SPRN_EPCR_DUVD@h;				      \
> > > +	beq+	3f;							      \
> > > 									      \
> > > +	andis.	r10,r9,MSR_GS@h;				    	      \
> > > +	beq+	1f;							      \
> > > +3:									      \
> > > +#endif									      \
> > > 	lis	r10,KERNELBASE@h;	/* check if exception in vectors */   \
> > > 	ori	r10,r10,KERNELBASE@l;					      \
> > > 	cmplw	r12,r10;						      \
> > > --
> > > 1.7.0.4
> > >
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm-ppc"
> > > in the body of a message to majordomo@vger.kernel.org More majordomo
> > > info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kumar Gala April 11, 2013, 6:33 p.m. UTC | #4 
On Apr 5, 2013, at 2:53 AM, Bhushan Bharat-R65777 wrote:

> Hi Kumar/Benh,
> 
> After further looking into the code I think that if we correct the vector range below in DebugDebug handler then we do not need the change I provided in this patch.
> 
> Here is the snapshot for 32 bit (head_booke.h, same will be true for 64 bit):
> 
> #define DEBUG_DEBUG_EXCEPTION                                                 \
>        START_EXCEPTION(DebugDebug);                                          \
>        DEBUG_EXCEPTION_PROLOG;                                               \
>                                                                              \
>        /*                                                                    \
>         * If there is a single step or branch-taken exception in an          \
>         * exception entry sequence, it was probably meant to apply to        \
>         * the code where the exception occurred (since exception entry       \
>         * doesn't turn off DE automatically).  We simulate the effect        \
>         * of turning off DE on entry to an exception handler by turning      \
>         * off DE in the DSRR1 value and clearing the debug status.           \
>         */                                                                   \
>        mfspr   r10,SPRN_DBSR;          /* check single-step/branch taken */  \
>        andis.  r10,r10,(DBSR_IC|DBSR_BT)@h;                                  \
>        beq+    2f;                                                           \
>                                                                              \
>        lis     r10,KERNELBASE@h;       /* check if exception in vectors */   \
>        ori     r10,r10,KERNELBASE@l;                                         \
>        cmplw   r12,r10;                                                      \
>        blt+    2f;                     /* addr below exception vectors */    \
>                                                                              \
>        lis     r10,DebugDebug@h;                                        \
>        ori     r10,r10,DebugDebug@l;                                            \
> 
> ^^^^
> 	Here we assume all exception vector ends at DebugDebug, which is not correct.
> 	We probably should get proper end by using some start_vector and end_vector lebels
> 	or at least use end at Ehvpriv (which is last defined in head_fsl_booke.S for PowerPC. Is that correct?
> 
> 	
>        cmplw   r12,r10;                                                      \
>        bgt+    2f;                     /* addr above exception vectors */    \
> 
> Thanks
> -Bharat

I talked to Stuart and this general approach is good.  Just make sure to update both head_44x.S and head_fsl_booke.S.  Plus do this for both DEBUG_CRIT_EXCEPTION & DEBUG_DEBUG_EXCEPTION

- k--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stuart Yoder April 11, 2013, 6:37 p.m. UTC | #5 
On Thu, Apr 11, 2013 at 1:33 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Apr 5, 2013, at 2:53 AM, Bhushan Bharat-R65777 wrote:
>
>> Hi Kumar/Benh,
>>
>> After further looking into the code I think that if we correct the vector range below in DebugDebug handler then we do not need the change I provided in this patch.
>>
>> Here is the snapshot for 32 bit (head_booke.h, same will be true for 64 bit):
>>
>> #define DEBUG_DEBUG_EXCEPTION                                                 \
>>        START_EXCEPTION(DebugDebug);                                          \
>>        DEBUG_EXCEPTION_PROLOG;                                               \
>>                                                                              \
>>        /*                                                                    \
>>         * If there is a single step or branch-taken exception in an          \
>>         * exception entry sequence, it was probably meant to apply to        \
>>         * the code where the exception occurred (since exception entry       \
>>         * doesn't turn off DE automatically).  We simulate the effect        \
>>         * of turning off DE on entry to an exception handler by turning      \
>>         * off DE in the DSRR1 value and clearing the debug status.           \
>>         */                                                                   \
>>        mfspr   r10,SPRN_DBSR;          /* check single-step/branch taken */  \
>>        andis.  r10,r10,(DBSR_IC|DBSR_BT)@h;                                  \
>>        beq+    2f;                                                           \
>>                                                                              \
>>        lis     r10,KERNELBASE@h;       /* check if exception in vectors */   \
>>        ori     r10,r10,KERNELBASE@l;                                         \
>>        cmplw   r12,r10;                                                      \
>>        blt+    2f;                     /* addr below exception vectors */    \
>>                                                                              \
>>        lis     r10,DebugDebug@h;                                        \
>>        ori     r10,r10,DebugDebug@l;                                            \
>>
>> ^^^^
>>       Here we assume all exception vector ends at DebugDebug, which is not correct.
>>       We probably should get proper end by using some start_vector and end_vector lebels
>>       or at least use end at Ehvpriv (which is last defined in head_fsl_booke.S for PowerPC. Is that correct?
>>
>>
>>        cmplw   r12,r10;                                                      \
>>        bgt+    2f;                     /* addr above exception vectors */    \
>>
>> Thanks
>> -Bharat
>
> I talked to Stuart and this general approach is good.  Just make sure to update both head_44x.S and head_fsl_booke.S.  Plus do this for both DEBUG_CRIT_EXCEPTION & DEBUG_DEBUG_EXCEPTION

Also, it looks like 64-bit already handles this properly with symbols
identifying the
start/end of the vectors (exceptions-64e.S).

Stuart
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 4684e33..8b26294 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -516,6 +516,33 @@  kernel_dbg_exc:
 	andis.	r15,r14,DBSR_IC@h
 	beq+	1f
 
+#ifdef CONFIG_KVM_BOOKE_HV
+	/*
+	 * EPCR.DUVD controls whether the debug events can come in
+	 * hypervisor mode or not. When KVM guest is using the debug
+	 * resource then we do not want debug events to be captured
+	 * in guest entry/exit path. So we set EPCR.DUVD when entering
+	 * and clears EPCR.DUVD when exiting from guest.
+	 * Debug instruction complete is a post-completion debug
+	 * exception but debug event gets posted on the basis of MSR
+	 * before the instruction is executed. Now if the instruction
+	 * switches the context from guest mode (MSR.GS = 1) to hypervisor
+	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of
+	 * KVM handler and xSRR1 points that MSR.GS is clear
+	 * (hypervisor context). Now as xSRR1.GS is used to decide whether
+	 * KVM handler will be invoked to handle the exception or host
+	 * host kernel debug handler will be invoked to handle the exception.
+	 * This leads to host kernel debug handler handling the exception
+	 * which should either be handled by KVM.
+	 */
+	mfspr	r10, SPRN_EPCR
+	andis.	r10,r10,SPRN_EPCR_DUVD@h
+	beq+	2f
+
+	andis.	r10,r9,MSR_GS@h
+	beq+	3f
+2:
+#endif
 	LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e)
 	LOAD_REG_IMMEDIATE(r15,interrupt_end_book3e)
 	cmpld	cr0,r10,r14
@@ -523,7 +550,7 @@  kernel_dbg_exc:
 	blt+	cr0,1f
 	bge+	cr1,1f
 
-	/* here it looks like we got an inappropriate debug exception. */
+3:	/* here it looks like we got an inappropriate debug exception. */
 	lis	r14,DBSR_IC@h		/* clear the IC event */
 	rlwinm	r11,r11,0,~MSR_DE	/* clear DE in the DSRR1 value */
 	mtspr	SPRN_DBSR,r14
diff --git a/arch/powerpc/kernel/head_booke.h b/arch/powerpc/kernel/head_booke.h
index 5f051ee..edc6a3b 100644
--- a/arch/powerpc/kernel/head_booke.h
+++ b/arch/powerpc/kernel/head_booke.h
@@ -285,7 +285,33 @@  label:
 	mfspr	r10,SPRN_DBSR;		/* check single-step/branch taken */  \
 	andis.	r10,r10,(DBSR_IC|DBSR_BT)@h;				      \
 	beq+	2f;							      \
+#ifdef CONFIG_KVM_BOOKE_HV						      \
+	/*								      \
+	 * EPCR.DUVD controls whether the debug events can come in	      \
+	 * hypervisor mode or not. When KVM guest is using the debug	      \
+	 * resource then we do not want debug events to be captured 	      \
+	 * in guest entry/exit path. So we set EPCR.DUVD when entering	      \
+	 * and clears EPCR.DUVD when exiting from guest.		      \
+	 * Debug instruction complete is a post-completion debug	      \
+	 * exception but debug event gets posted on the basis of MSR	      \
+	 * before the instruction is executed. Now if the instruction	      \
+	 * switches the context from guest mode (MSR.GS = 1) to hypervisor    \
+	 * mode (MSR.GS = 0) then the xSRR0 points to first instruction of    \
+	 * KVM handler and xSRR1 points that MSR.GS is clear 		      \
+	 * (hypervisor context). Now as xSRR1.GS is used to decide whether    \
+	 * KVM handler will be invoked to handle the exception or host	      \
+	 * host kernel debug handler will be invoked to handle the exception. \
+	 * This leads to host kernel debug handler handling the exception     \
+	 * which should either be handled by KVM.			      \
+	 */								      \
+	mfspr	r10, SPRN_EPCR;						      \
+	andis.	r10,r10,SPRN_EPCR_DUVD@h;				      \
+	beq+	3f;							      \
 									      \
+	andis.	r10,r9,MSR_GS@h;				    	      \
+	beq+	1f;							      \
+3:									      \
+#endif									      \
 	lis	r10,KERNELBASE@h;	/* check if exception in vectors */   \
 	ori	r10,r10,KERNELBASE@l;					      \
 	cmplw	r12,r10;						      \