Patchwork [4/7] booke: Save and restore debug registers on guest entry and exit

login
register
mail settings
Submitter Bharat Bhushan
Date Feb. 28, 2013, 4:13 a.m.
Message ID <1362024796-4237-5-git-send-email-bharat.bhushan@freescale.com>
Download mbox | patch
Permalink /patch/223768/
State New
Headers show

Comments

Bharat Bhushan - Feb. 28, 2013, 4:13 a.m.
On Guest entry: if guest is wants to use the debug register then
save h/w debug register in host_dbg_reg and load the debug registers
with shadow_dbg_reg. Otherwise leave h/w debug registers as is.

On guest exit: If guest/user-space is using the debug resource then
restore the h/w debug register with host_dbg_reg. No need to save guest
debug register as shadow_dbg_reg is having required values. If guest is not
using the debug resources then no need to restore h/w registers.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/kvm_host.h |    5 ++
 arch/powerpc/kernel/asm-offsets.c   |   26 ++++++++
 arch/powerpc/kvm/booke_interrupts.S |  114 +++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 0 deletions(-)
Alexander Graf - March 7, 2013, 1:25 p.m.
On 28.02.2013, at 05:13, Bharat Bhushan wrote:

> On Guest entry: if guest is wants to use the debug register then
> save h/w debug register in host_dbg_reg and load the debug registers
> with shadow_dbg_reg. Otherwise leave h/w debug registers as is.

Why can't we switch the majority of registers on vcpu_put/get and only enable or disable debugging on guest entry/exit?


Alex

> 
> On guest exit: If guest/user-space is using the debug resource then
> restore the h/w debug register with host_dbg_reg. No need to save guest
> debug register as shadow_dbg_reg is having required values. If guest is not
> using the debug resources then no need to restore h/w registers.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> arch/powerpc/include/asm/kvm_host.h |    5 ++
> arch/powerpc/kernel/asm-offsets.c   |   26 ++++++++
> arch/powerpc/kvm/booke_interrupts.S |  114 +++++++++++++++++++++++++++++++++++
> 3 files changed, 145 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index f4ba881..a9feeb0 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -504,7 +504,12 @@ struct kvm_vcpu_arch {
> 	u32 mmucfg;
> 	u32 epr;
> 	u32 crit_save;
> +	/* guest debug registers*/
> 	struct kvmppc_booke_debug_reg dbg_reg;
> +	/* shadow debug registers */
> +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
> +	/* host debug registers*/
> +	struct kvmppc_booke_debug_reg host_dbg_reg;
> #endif
> 	gpa_t paddr_accessed;
> 	gva_t vaddr_accessed;
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 02048f3..22deda7 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -563,6 +563,32 @@ int main(void)
> 	DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
> 	DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
> 	DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save));
> +	DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
> +	DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
> +	DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
> +	DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
> +					  dbcr0));
> +	DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
> +					  dbcr1));
> +	DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
> +					  dbcr2));
> +#ifdef CONFIG_KVM_E500MC
> +	DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
> +					  dbcr4));
> +#endif
> +	DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
> +					 iac[0]));
> +	DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
> +					 iac[1]));
> +	DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
> +					 iac[2]));
> +	DEFINE(KVMPPC_DBG_IAC4, offsetof(struct kvmppc_booke_debug_reg,
> +					 iac[3]));
> +	DEFINE(KVMPPC_DBG_DAC1, offsetof(struct kvmppc_booke_debug_reg,
> +					 dac[0]));
> +	DEFINE(KVMPPC_DBG_DAC2, offsetof(struct kvmppc_booke_debug_reg,
> +					 dac[1]));
> +	DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
> #endif /* CONFIG_PPC_BOOK3S */
> #endif /* CONFIG_KVM */
> 
> diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
> index 2c6deb5..6d78e01 100644
> --- a/arch/powerpc/kvm/booke_interrupts.S
> +++ b/arch/powerpc/kvm/booke_interrupts.S
> @@ -39,6 +39,8 @@
> #define HOST_MIN_STACK_SIZE (HOST_NV_GPR(R31) + 4)
> #define HOST_STACK_SIZE (((HOST_MIN_STACK_SIZE + 15) / 16) * 16) /* Align. */
> #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
> +#define DBCR0_AC_BITS	(DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
> +			 DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
> 
> #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
>                         (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
> @@ -54,6 +56,8 @@
>                        (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
>                        (1<<BOOKE_INTERRUPT_ALIGNMENT))
> 
> +#define NEED_DEBUG_SAVE (1<<BOOKE_INTERRUPT_DEBUG)
> +
> .macro __KVM_HANDLER ivor_nr scratch srr0
> 	/* Get pointer to vcpu and record exit number. */
> 	mtspr	\scratch , r4
> @@ -215,6 +219,59 @@ _GLOBAL(kvmppc_resume_host)
> 	stw	r9, VCPU_FAULT_ESR(r4)
> ..skip_esr:
> 
> +	lwz	r9, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR0(r4)
> +	rlwinm.	r8, r9, 0, ~DBCR0_IDM
> +	beq	skip_load_host_debug
> +	lwz	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR0(r4)
> +	andis.	r9, r9, DBCR0_AC_BITS@h
> +	li	r9, 0
> +	mtspr	SPRN_DBCR0, r9		/* disable all debug event */
> +	beq	skip_load_hw_bkpts
> +	lwz	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
> +	lwz	r9, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
> +	mtspr	SPRN_DBCR1, r7
> +	mtspr	SPRN_DBCR2, r9
> +	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
> +	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
> +	mtspr	SPRN_IAC1, r7
> +	mtspr	SPRN_IAC2, r9
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> +	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
> +	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
> +	mtspr	SPRN_IAC3, r3
> +	mtspr	SPRN_IAC4, r4
> +#endif
> +	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
> +	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
> +	mtspr	SPRN_DAC1, r7
> +	mtspr	SPRN_DAC2, r9
> +skip_load_hw_bkpts:
> +	/* Clear h/w DBSR and save current(guest) DBSR */
> +	mfspr	r9, SPRN_DBSR
> +	mtspr	SPRN_DBSR, r9
> +	isync
> +	andi.	r7, r6, NEED_DEBUG_SAVE
> +	beq	skip_dbsr_save
> +	/*
> +	 * If vcpu->guest_debug flag is set then do not check for
> +	 * shared->msr.DE as this debugging (say by QEMU) does not
> +	 * depends on shared->msr.de. In these scanerios MSR.DE is
> +	 * always set using shared_msr and should be handled always.
> +	 */
> +	lwz	r7, VCPU_GUEST_DEBUG(r4)
> +	cmpwi	r7, 0
> +	bne	skip_save_trap_event
> +	PPC_LL	r3, VCPU_SHARED(r4)
> +	PPC_LD(r3, VCPU_SHARED_MSR, r3)
> +	andi.	r3, r3, MSR_DE
> +	bne	skip_save_trap_event
> +	andis.	r9, r9, DBSR_TIE@h
> +skip_save_trap_event:
> +	stw	r9, VCPU_DBSR(r4)
> +skip_dbsr_save:
> +	mtspr	SPRN_DBCR0, r8
> +skip_load_host_debug:
> +
> 	/* Save remaining volatile guest register state to vcpu. */
> 	stw	r0, VCPU_GPR(R0)(r4)
> 	stw	r1, VCPU_GPR(R1)(r4)
> @@ -468,6 +525,63 @@ lightweight_exit:
> 	PPC_LD(r3, VCPU_SHARED_SPRG7, r5)
> 	mtspr	SPRN_SPRG7W, r3
> 
> +	mfmsr	r7
> +	rlwinm	r7, r7, 0, ~MSR_DE
> +	mtmsr	r7
> +	lwz	r6, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR0(r4)
> +	rlwinm.	r7, r6, 0, ~DBCR0_IDM
> +	beq	skip_load_guest_debug
> +	mfspr	r8, SPRN_DBCR0
> +	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR0(r4)
> +	andis.	r3, r6, DBCR0_AC_BITS@h
> +	beq	skip_hw_bkpts
> +	mfspr	r7, SPRN_DBCR1
> +	stw	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
> +	mfspr	r8, SPRN_DBCR2
> +	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
> +	mfspr	r7, SPRN_IAC1
> +	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
> +	mfspr	r8, SPRN_IAC2
> +	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> +	mfspr	r7, SPRN_IAC3
> +	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
> +	mfspr	r8, SPRN_IAC4
> +	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
> +#endif
> +	mfspr	r7, SPRN_DAC1
> +	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
> +	mfspr	r8, SPRN_DAC2
> +	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
> +	li	r8, 0
> +	mtspr	SPRN_DBCR0, r8		/* disable all debug event */
> +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR1, r4)
> +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR2, r4)
> +	mtspr	SPRN_DBCR1, r7
> +	mtspr	SPRN_DBCR2, r8
> +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC1, r4)
> +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC2, r4)
> +	mtspr	SPRN_IAC1, r7
> +	mtspr	SPRN_IAC2, r8
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC3, r4)
> +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC4, r4)
> +	mtspr	SPRN_IAC3, r7
> +	mtspr	SPRN_IAC4, r8
> +#endif
> +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC1, r4)
> +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC2, r4)
> +	mtspr	SPRN_DAC1, r7
> +	mtspr	SPRN_DAC2, r8
> +skip_hw_bkpts:
> +	/* Clear if any deferred debug event */
> +	mfspr	r8, SPRN_DBSR
> +	mtspr	SPRN_DBSR, r8
> +	isync
> +	/* Restore guest DBCR */
> +	mtspr	SPRN_DBCR0, r6
> +skip_load_guest_debug:
> +
> #ifdef CONFIG_KVM_EXIT_TIMING
> 	/* save enter time */
> 1:
> -- 
> 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 - March 14, 2013, 4:50 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, March 07, 2013 6:56 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 4/7] booke: Save and restore debug registers on guest entry
> and exit
> 
> 
> On 28.02.2013, at 05:13, Bharat Bhushan wrote:
> 
> > On Guest entry: if guest is wants to use the debug register then save
> > h/w debug register in host_dbg_reg and load the debug registers with
> > shadow_dbg_reg. Otherwise leave h/w debug registers as is.
> 
> Why can't we switch the majority of registers on vcpu_put/get and only enable or
> disable debugging on guest entry/exit?


One of the reason for not doing this is that the KVM is a host kernel module and let this be debugged by host (I do not this how much useful this is :)) 
So I am not able to recall the specific reason, maybe we have just coded this like this and tried to keep overhead as low as possible by switching registers only when they are used.

As we discussed before, we can keep this option open for future.

-Bharat

> 
> 
> Alex
> 
> >
> > On guest exit: If guest/user-space is using the debug resource then
> > restore the h/w debug register with host_dbg_reg. No need to save
> > guest debug register as shadow_dbg_reg is having required values. If
> > guest is not using the debug resources then no need to restore h/w registers.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > arch/powerpc/include/asm/kvm_host.h |    5 ++
> > arch/powerpc/kernel/asm-offsets.c   |   26 ++++++++
> > arch/powerpc/kvm/booke_interrupts.S |  114
> > +++++++++++++++++++++++++++++++++++
> > 3 files changed, 145 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_host.h
> > b/arch/powerpc/include/asm/kvm_host.h
> > index f4ba881..a9feeb0 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -504,7 +504,12 @@ struct kvm_vcpu_arch {
> > 	u32 mmucfg;
> > 	u32 epr;
> > 	u32 crit_save;
> > +	/* guest debug registers*/
> > 	struct kvmppc_booke_debug_reg dbg_reg;
> > +	/* shadow debug registers */
> > +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
> > +	/* host debug registers*/
> > +	struct kvmppc_booke_debug_reg host_dbg_reg;
> > #endif
> > 	gpa_t paddr_accessed;
> > 	gva_t vaddr_accessed;
> > diff --git a/arch/powerpc/kernel/asm-offsets.c
> > b/arch/powerpc/kernel/asm-offsets.c
> > index 02048f3..22deda7 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -563,6 +563,32 @@ int main(void)
> > 	DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
> > 	DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
> > 	DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save));
> > +	DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
> > +	DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
> > +	DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
> > +	DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
> > +					  dbcr0));
> > +	DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
> > +					  dbcr1));
> > +	DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
> > +					  dbcr2));
> > +#ifdef CONFIG_KVM_E500MC
> > +	DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
> > +					  dbcr4));
> > +#endif
> > +	DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
> > +					 iac[0]));
> > +	DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
> > +					 iac[1]));
> > +	DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
> > +					 iac[2]));
> > +	DEFINE(KVMPPC_DBG_IAC4, offsetof(struct kvmppc_booke_debug_reg,
> > +					 iac[3]));
> > +	DEFINE(KVMPPC_DBG_DAC1, offsetof(struct kvmppc_booke_debug_reg,
> > +					 dac[0]));
> > +	DEFINE(KVMPPC_DBG_DAC2, offsetof(struct kvmppc_booke_debug_reg,
> > +					 dac[1]));
> > +	DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
> > #endif /* CONFIG_PPC_BOOK3S */
> > #endif /* CONFIG_KVM */
> >
> > diff --git a/arch/powerpc/kvm/booke_interrupts.S
> > b/arch/powerpc/kvm/booke_interrupts.S
> > index 2c6deb5..6d78e01 100644
> > --- a/arch/powerpc/kvm/booke_interrupts.S
> > +++ b/arch/powerpc/kvm/booke_interrupts.S
> > @@ -39,6 +39,8 @@
> > #define HOST_MIN_STACK_SIZE (HOST_NV_GPR(R31) + 4) #define
> > HOST_STACK_SIZE (((HOST_MIN_STACK_SIZE + 15) / 16) * 16) /* Align. */
> > #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
> > +#define DBCR0_AC_BITS	(DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
> > +			 DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
> >
> > #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
> >                         (1<<BOOKE_INTERRUPT_DTLB_MISS) | \ @@ -54,6
> > +56,8 @@
> >                        (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
> >                        (1<<BOOKE_INTERRUPT_ALIGNMENT))
> >
> > +#define NEED_DEBUG_SAVE (1<<BOOKE_INTERRUPT_DEBUG)
> > +
> > .macro __KVM_HANDLER ivor_nr scratch srr0
> > 	/* Get pointer to vcpu and record exit number. */
> > 	mtspr	\scratch , r4
> > @@ -215,6 +219,59 @@ _GLOBAL(kvmppc_resume_host)
> > 	stw	r9, VCPU_FAULT_ESR(r4)
> > ..skip_esr:
> >
> > +	lwz	r9, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR0(r4)
> > +	rlwinm.	r8, r9, 0, ~DBCR0_IDM
> > +	beq	skip_load_host_debug
> > +	lwz	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR0(r4)
> > +	andis.	r9, r9, DBCR0_AC_BITS@h
> > +	li	r9, 0
> > +	mtspr	SPRN_DBCR0, r9		/* disable all debug event */
> > +	beq	skip_load_hw_bkpts
> > +	lwz	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
> > +	lwz	r9, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
> > +	mtspr	SPRN_DBCR1, r7
> > +	mtspr	SPRN_DBCR2, r9
> > +	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
> > +	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
> > +	mtspr	SPRN_IAC1, r7
> > +	mtspr	SPRN_IAC2, r9
> > +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> > +	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
> > +	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
> > +	mtspr	SPRN_IAC3, r3
> > +	mtspr	SPRN_IAC4, r4
> > +#endif
> > +	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
> > +	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
> > +	mtspr	SPRN_DAC1, r7
> > +	mtspr	SPRN_DAC2, r9
> > +skip_load_hw_bkpts:
> > +	/* Clear h/w DBSR and save current(guest) DBSR */
> > +	mfspr	r9, SPRN_DBSR
> > +	mtspr	SPRN_DBSR, r9
> > +	isync
> > +	andi.	r7, r6, NEED_DEBUG_SAVE
> > +	beq	skip_dbsr_save
> > +	/*
> > +	 * If vcpu->guest_debug flag is set then do not check for
> > +	 * shared->msr.DE as this debugging (say by QEMU) does not
> > +	 * depends on shared->msr.de. In these scanerios MSR.DE is
> > +	 * always set using shared_msr and should be handled always.
> > +	 */
> > +	lwz	r7, VCPU_GUEST_DEBUG(r4)
> > +	cmpwi	r7, 0
> > +	bne	skip_save_trap_event
> > +	PPC_LL	r3, VCPU_SHARED(r4)
> > +	PPC_LD(r3, VCPU_SHARED_MSR, r3)
> > +	andi.	r3, r3, MSR_DE
> > +	bne	skip_save_trap_event
> > +	andis.	r9, r9, DBSR_TIE@h
> > +skip_save_trap_event:
> > +	stw	r9, VCPU_DBSR(r4)
> > +skip_dbsr_save:
> > +	mtspr	SPRN_DBCR0, r8
> > +skip_load_host_debug:
> > +
> > 	/* Save remaining volatile guest register state to vcpu. */
> > 	stw	r0, VCPU_GPR(R0)(r4)
> > 	stw	r1, VCPU_GPR(R1)(r4)
> > @@ -468,6 +525,63 @@ lightweight_exit:
> > 	PPC_LD(r3, VCPU_SHARED_SPRG7, r5)
> > 	mtspr	SPRN_SPRG7W, r3
> >
> > +	mfmsr	r7
> > +	rlwinm	r7, r7, 0, ~MSR_DE
> > +	mtmsr	r7
> > +	lwz	r6, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR0(r4)
> > +	rlwinm.	r7, r6, 0, ~DBCR0_IDM
> > +	beq	skip_load_guest_debug
> > +	mfspr	r8, SPRN_DBCR0
> > +	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR0(r4)
> > +	andis.	r3, r6, DBCR0_AC_BITS@h
> > +	beq	skip_hw_bkpts
> > +	mfspr	r7, SPRN_DBCR1
> > +	stw	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
> > +	mfspr	r8, SPRN_DBCR2
> > +	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
> > +	mfspr	r7, SPRN_IAC1
> > +	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
> > +	mfspr	r8, SPRN_IAC2
> > +	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4) #if
> > +CONFIG_PPC_ADV_DEBUG_IACS > 2
> > +	mfspr	r7, SPRN_IAC3
> > +	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
> > +	mfspr	r8, SPRN_IAC4
> > +	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4) #endif
> > +	mfspr	r7, SPRN_DAC1
> > +	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
> > +	mfspr	r8, SPRN_DAC2
> > +	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
> > +	li	r8, 0
> > +	mtspr	SPRN_DBCR0, r8		/* disable all debug event */
> > +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR1, r4)
> > +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR2, r4)
> > +	mtspr	SPRN_DBCR1, r7
> > +	mtspr	SPRN_DBCR2, r8
> > +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC1, r4)
> > +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC2, r4)
> > +	mtspr	SPRN_IAC1, r7
> > +	mtspr	SPRN_IAC2, r8
> > +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> > +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC3, r4)
> > +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC4, r4)
> > +	mtspr	SPRN_IAC3, r7
> > +	mtspr	SPRN_IAC4, r8
> > +#endif
> > +	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC1, r4)
> > +	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC2, r4)
> > +	mtspr	SPRN_DAC1, r7
> > +	mtspr	SPRN_DAC2, r8
> > +skip_hw_bkpts:
> > +	/* Clear if any deferred debug event */
> > +	mfspr	r8, SPRN_DBSR
> > +	mtspr	SPRN_DBSR, r8
> > +	isync
> > +	/* Restore guest DBCR */
> > +	mtspr	SPRN_DBCR0, r6
> > +skip_load_guest_debug:
> > +
> > #ifdef CONFIG_KVM_EXIT_TIMING
> > 	/* save enter time */
> > 1:
> > --
> > 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
Alexander Graf - March 14, 2013, 11:52 a.m.
On 14.03.2013, at 05:50, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, March 07, 2013 6:56 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 4/7] booke: Save and restore debug registers on guest entry
>> and exit
>> 
>> 
>> On 28.02.2013, at 05:13, Bharat Bhushan wrote:
>> 
>>> On Guest entry: if guest is wants to use the debug register then save
>>> h/w debug register in host_dbg_reg and load the debug registers with
>>> shadow_dbg_reg. Otherwise leave h/w debug registers as is.
>> 
>> Why can't we switch the majority of registers on vcpu_put/get and only enable or
>> disable debugging on guest entry/exit?
> 
> 
> One of the reason for not doing this is that the KVM is a host kernel module and let this be debugged by host (I do not this how much useful this is :)) 
> So I am not able to recall the specific reason, maybe we have just coded this like this and tried to keep overhead as low as possible by switching registers only when they are used.

My point is that the overhead is _higher_ this way, because we need to do checks and switches on every guest entry/exit, which happens a _lot_ more often than a host context switch.

> As we discussed before, we can keep this option open for future.

What future? Just ignore debug events in the entry/exit code path and suddenly a lot of the code becomes a lot easier.


Alex

--
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 - March 14, 2013, 2:44 p.m.
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, March 07, 2013 6:56 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421;
> >> Bhushan
> >> Bharat-R65777
> >> Subject: Re: [PATCH 4/7] booke: Save and restore debug registers on
> >> guest entry and exit
> >>
> >>
> >> On 28.02.2013, at 05:13, Bharat Bhushan wrote:
> >>
> >>> On Guest entry: if guest is wants to use the debug register then
> >>> save h/w debug register in host_dbg_reg and load the debug registers
> >>> with shadow_dbg_reg. Otherwise leave h/w debug registers as is.
> >>
> >> Why can't we switch the majority of registers on vcpu_put/get and
> >> only enable or disable debugging on guest entry/exit?
> >
> >
> > One of the reason for not doing this is that the KVM is a host kernel
> > module and let this be debugged by host (I do not this how much useful this is
> :)) So I am not able to recall the specific reason, maybe we have just coded
> this like this and tried to keep overhead as low as possible by switching
> registers only when they are used.
> 
> My point is that the overhead is _higher_ this way, because we need to do checks
> and switches on every guest entry/exit, which happens a _lot_ more often than a
> host context switch.
> 
> > As we discussed before, we can keep this option open for future.
> 
> What future? Just ignore debug events in the entry/exit code path and suddenly a
> lot of the code becomes a lot easier.

Just to summarize what we agreed upon:

- Save/restore will happen on vcpu_get()/vcpu_put(). This will happen only if guest is using debug registers. Probably using a flag to indicate guest is using debug APU. 
- On debug register access from QEMU, always set value in h/w debug register.
- On guest access of debug register, also save xxx h/w register in vcpu->host_debug_reg.xxx and load guest provided value in h/w debug register, ensure this happen on first access only, probably all debug registers once debug events enabled in dbcr0. Direct access from guest was not part of this patchset and support for this will be done separately.

Thanks
-Bharat

> 
> 
> Alex
> 


--
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

Patch

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index f4ba881..a9feeb0 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -504,7 +504,12 @@  struct kvm_vcpu_arch {
 	u32 mmucfg;
 	u32 epr;
 	u32 crit_save;
+	/* guest debug registers*/
 	struct kvmppc_booke_debug_reg dbg_reg;
+	/* shadow debug registers */
+	struct kvmppc_booke_debug_reg shadow_dbg_reg;
+	/* host debug registers*/
+	struct kvmppc_booke_debug_reg host_dbg_reg;
 #endif
 	gpa_t paddr_accessed;
 	gva_t vaddr_accessed;
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 02048f3..22deda7 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -563,6 +563,32 @@  int main(void)
 	DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
 	DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
 	DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save));
+	DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
+	DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
+	DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
+	DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr0));
+	DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr1));
+	DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr2));
+#ifdef CONFIG_KVM_E500MC
+	DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr4));
+#endif
+	DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[0]));
+	DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[1]));
+	DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[2]));
+	DEFINE(KVMPPC_DBG_IAC4, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[3]));
+	DEFINE(KVMPPC_DBG_DAC1, offsetof(struct kvmppc_booke_debug_reg,
+					 dac[0]));
+	DEFINE(KVMPPC_DBG_DAC2, offsetof(struct kvmppc_booke_debug_reg,
+					 dac[1]));
+	DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
 #endif /* CONFIG_PPC_BOOK3S */
 #endif /* CONFIG_KVM */
 
diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index 2c6deb5..6d78e01 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -39,6 +39,8 @@ 
 #define HOST_MIN_STACK_SIZE (HOST_NV_GPR(R31) + 4)
 #define HOST_STACK_SIZE (((HOST_MIN_STACK_SIZE + 15) / 16) * 16) /* Align. */
 #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
+#define DBCR0_AC_BITS	(DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
+			 DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
 
 #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
                         (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
@@ -54,6 +56,8 @@ 
                        (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
                        (1<<BOOKE_INTERRUPT_ALIGNMENT))
 
+#define NEED_DEBUG_SAVE (1<<BOOKE_INTERRUPT_DEBUG)
+
 .macro __KVM_HANDLER ivor_nr scratch srr0
 	/* Get pointer to vcpu and record exit number. */
 	mtspr	\scratch , r4
@@ -215,6 +219,59 @@  _GLOBAL(kvmppc_resume_host)
 	stw	r9, VCPU_FAULT_ESR(r4)
 ..skip_esr:
 
+	lwz	r9, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR0(r4)
+	rlwinm.	r8, r9, 0, ~DBCR0_IDM
+	beq	skip_load_host_debug
+	lwz	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR0(r4)
+	andis.	r9, r9, DBCR0_AC_BITS@h
+	li	r9, 0
+	mtspr	SPRN_DBCR0, r9		/* disable all debug event */
+	beq	skip_load_hw_bkpts
+	lwz	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
+	lwz	r9, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
+	mtspr	SPRN_DBCR1, r7
+	mtspr	SPRN_DBCR2, r9
+	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
+	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
+	mtspr	SPRN_IAC1, r7
+	mtspr	SPRN_IAC2, r9
+#if CONFIG_PPC_ADV_DEBUG_IACS > 2
+	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
+	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
+	mtspr	SPRN_IAC3, r3
+	mtspr	SPRN_IAC4, r4
+#endif
+	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
+	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
+	mtspr	SPRN_DAC1, r7
+	mtspr	SPRN_DAC2, r9
+skip_load_hw_bkpts:
+	/* Clear h/w DBSR and save current(guest) DBSR */
+	mfspr	r9, SPRN_DBSR
+	mtspr	SPRN_DBSR, r9
+	isync
+	andi.	r7, r6, NEED_DEBUG_SAVE
+	beq	skip_dbsr_save
+	/*
+	 * If vcpu->guest_debug flag is set then do not check for
+	 * shared->msr.DE as this debugging (say by QEMU) does not
+	 * depends on shared->msr.de. In these scanerios MSR.DE is
+	 * always set using shared_msr and should be handled always.
+	 */
+	lwz	r7, VCPU_GUEST_DEBUG(r4)
+	cmpwi	r7, 0
+	bne	skip_save_trap_event
+	PPC_LL	r3, VCPU_SHARED(r4)
+	PPC_LD(r3, VCPU_SHARED_MSR, r3)
+	andi.	r3, r3, MSR_DE
+	bne	skip_save_trap_event
+	andis.	r9, r9, DBSR_TIE@h
+skip_save_trap_event:
+	stw	r9, VCPU_DBSR(r4)
+skip_dbsr_save:
+	mtspr	SPRN_DBCR0, r8
+skip_load_host_debug:
+
 	/* Save remaining volatile guest register state to vcpu. */
 	stw	r0, VCPU_GPR(R0)(r4)
 	stw	r1, VCPU_GPR(R1)(r4)
@@ -468,6 +525,63 @@  lightweight_exit:
 	PPC_LD(r3, VCPU_SHARED_SPRG7, r5)
 	mtspr	SPRN_SPRG7W, r3
 
+	mfmsr	r7
+	rlwinm	r7, r7, 0, ~MSR_DE
+	mtmsr	r7
+	lwz	r6, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR0(r4)
+	rlwinm.	r7, r6, 0, ~DBCR0_IDM
+	beq	skip_load_guest_debug
+	mfspr	r8, SPRN_DBCR0
+	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR0(r4)
+	andis.	r3, r6, DBCR0_AC_BITS@h
+	beq	skip_hw_bkpts
+	mfspr	r7, SPRN_DBCR1
+	stw	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
+	mfspr	r8, SPRN_DBCR2
+	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
+	mfspr	r7, SPRN_IAC1
+	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
+	mfspr	r8, SPRN_IAC2
+	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
+#if CONFIG_PPC_ADV_DEBUG_IACS > 2
+	mfspr	r7, SPRN_IAC3
+	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
+	mfspr	r8, SPRN_IAC4
+	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
+#endif
+	mfspr	r7, SPRN_DAC1
+	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
+	mfspr	r8, SPRN_DAC2
+	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
+	li	r8, 0
+	mtspr	SPRN_DBCR0, r8		/* disable all debug event */
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR1, r4)
+	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR2, r4)
+	mtspr	SPRN_DBCR1, r7
+	mtspr	SPRN_DBCR2, r8
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC1, r4)
+	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC2, r4)
+	mtspr	SPRN_IAC1, r7
+	mtspr	SPRN_IAC2, r8
+#if CONFIG_PPC_ADV_DEBUG_IACS > 2
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC3, r4)
+	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC4, r4)
+	mtspr	SPRN_IAC3, r7
+	mtspr	SPRN_IAC4, r8
+#endif
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC1, r4)
+	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC2, r4)
+	mtspr	SPRN_DAC1, r7
+	mtspr	SPRN_DAC2, r8
+skip_hw_bkpts:
+	/* Clear if any deferred debug event */
+	mfspr	r8, SPRN_DBSR
+	mtspr	SPRN_DBSR, r8
+	isync
+	/* Restore guest DBCR */
+	mtspr	SPRN_DBCR0, r6
+skip_load_guest_debug:
+
 #ifdef CONFIG_KVM_EXIT_TIMING
 	/* save enter time */
 1: