Patchwork [3/8] KVM: PPC: booke: Added debug handler

login
register
mail settings
Submitter Bharat Bhushan
Date Jan. 16, 2013, 8:24 a.m.
Message ID <1358324685-30225-2-git-send-email-bharat.bhushan@freescale.com>
Download mbox | patch
Permalink /patch/212432/
State New
Headers show

Comments

Bharat Bhushan - Jan. 16, 2013, 8:24 a.m.
From: Bharat Bhushan <Bharat.Bhushan@freescale.com>

Installed debug handler will be used for guest debug support
and debug facility emulation features (patches for these
features will follow this patch).

Signed-off-by: Liu Yu <yu.liu@freescale.com>
[bharat.bhushan@freescale.com: Substantial changes]
Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/kvm_host.h |    1 +
 arch/powerpc/kernel/asm-offsets.c   |    1 +
 arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++-----
 3 files changed, 44 insertions(+), 7 deletions(-)
Alexander Graf - Jan. 25, 2013, 11:42 a.m.
On 16.01.2013, at 09:24, Bharat Bhushan wrote:

> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> 
> Installed debug handler will be used for guest debug support
> and debug facility emulation features (patches for these
> features will follow this patch).
> 
> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> [bharat.bhushan@freescale.com: Substantial changes]
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> arch/powerpc/include/asm/kvm_host.h |    1 +
> arch/powerpc/kernel/asm-offsets.c   |    1 +
> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++-----
> 3 files changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 8a72d59..f4ba881 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
> 	u32 tlbcfg[4];
> 	u32 mmucfg;
> 	u32 epr;
> +	u32 crit_save;
> 	struct kvmppc_booke_debug_reg dbg_reg;
> #endif
> 	gpa_t paddr_accessed;
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 46f6afd..02048f3 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -562,6 +562,7 @@ int main(void)
> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> 	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));
> #endif /* CONFIG_PPC_BOOK3S */
> #endif /* CONFIG_KVM */
> 
> diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
> index eae8483..dd9c5d4 100644
> --- a/arch/powerpc/kvm/booke_interrupts.S
> +++ b/arch/powerpc/kvm/booke_interrupts.S
> @@ -52,12 +52,7 @@
>                        (1<<BOOKE_INTERRUPT_PROGRAM) | \
>                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
> 
> -.macro KVM_HANDLER ivor_nr scratch srr0
> -_GLOBAL(kvmppc_handler_\ivor_nr)
> -	/* Get pointer to vcpu and record exit number. */
> -	mtspr	\scratch , r4
> -	mfspr   r4, SPRN_SPRG_THREAD
> -	lwz     r4, THREAD_KVM_VCPU(r4)
> +.macro __KVM_HANDLER ivor_nr scratch srr0
> 	stw	r3, VCPU_GPR(R3)(r4)
> 	stw	r5, VCPU_GPR(R5)(r4)
> 	stw	r6, VCPU_GPR(R6)(r4)
> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
> 	bctr
> .endm
> 
> +.macro KVM_HANDLER ivor_nr scratch srr0
> +_GLOBAL(kvmppc_handler_\ivor_nr)
> +	/* Get pointer to vcpu and record exit number. */
> +	mtspr	\scratch , r4
> +	mfspr   r4, SPRN_SPRG_THREAD
> +	lwz     r4, THREAD_KVM_VCPU(r4)
> +	__KVM_HANDLER \ivor_nr \scratch \srr0
> +.endm
> +
> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> +_GLOBAL(kvmppc_handler_\ivor_nr)
> +	mtspr   \scratch, r4
> +	mfspr	r4, SPRN_SPRG_THREAD
> +	lwz	r4, THREAD_KVM_VCPU(r4)
> +	stw	r3, VCPU_CRIT_SAVE(r4)
> +	mfcr	r3
> +	mfspr	r4, SPRN_CSRR1
> +	andi.	r4, r4, MSR_PR
> +	bne	1f


> +	/* debug interrupt happened in enter/exit path */
> +	mfspr   r4, SPRN_CSRR1
> +	rlwinm  r4, r4, 0, ~MSR_DE
> +	mtspr   SPRN_CSRR1, r4
> +	lis	r4, 0xffff
> +	ori	r4, r4, 0xffff
> +	mtspr	SPRN_DBSR, r4
> +	mfspr	r4, SPRN_SPRG_THREAD
> +	lwz	r4, THREAD_KVM_VCPU(r4)
> +	mtcr	r3
> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> +	mfspr   r4, \scratch
> +	rfci

What is this part doing? Try to ignore the debug exit? Why would we have MSR_DE enabled in the first place when we can't handle it?

> +1:	/* debug interrupt happened in guest */
> +	mtcr	r3
> +	mfspr	r4, SPRN_SPRG_THREAD
> +	lwz	r4, THREAD_KVM_VCPU(r4)
> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> +	__KVM_HANDLER \ivor_nr \scratch \srr0

I don't think you need the __KVM_HANDLER split. This should be quite easily refactorable into a simple DBG prolog.


Alex

> +.endm
> +
> .macro KVM_HANDLER_ADDR ivor_nr
> 	.long	kvmppc_handler_\ivor_nr
> .endm
> @@ -98,7 +133,7 @@ KVM_HANDLER BOOKE_INTERRUPT_FIT SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
> KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> -KVM_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
> +KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
> KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_ROUND SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> -- 
> 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
Bharat Bhushan - Jan. 30, 2013, 11:30 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, January 25, 2013 5:13 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> 
> 
> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
> 
> > From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >
> > Installed debug handler will be used for guest debug support and debug
> > facility emulation features (patches for these features will follow
> > this patch).
> >
> > Signed-off-by: Liu Yu <yu.liu@freescale.com>
> > [bharat.bhushan@freescale.com: Substantial changes]
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > arch/powerpc/include/asm/kvm_host.h |    1 +
> > arch/powerpc/kernel/asm-offsets.c   |    1 +
> > arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++-----
> > 3 files changed, 44 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_host.h
> > b/arch/powerpc/include/asm/kvm_host.h
> > index 8a72d59..f4ba881 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
> > 	u32 tlbcfg[4];
> > 	u32 mmucfg;
> > 	u32 epr;
> > +	u32 crit_save;
> > 	struct kvmppc_booke_debug_reg dbg_reg; #endif
> > 	gpa_t paddr_accessed;
> > diff --git a/arch/powerpc/kernel/asm-offsets.c
> > b/arch/powerpc/kernel/asm-offsets.c
> > index 46f6afd..02048f3 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -562,6 +562,7 @@ int main(void)
> > 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> > 	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));
> > #endif /* CONFIG_PPC_BOOK3S */
> > #endif /* CONFIG_KVM */
> >
> > diff --git a/arch/powerpc/kvm/booke_interrupts.S
> > b/arch/powerpc/kvm/booke_interrupts.S
> > index eae8483..dd9c5d4 100644
> > --- a/arch/powerpc/kvm/booke_interrupts.S
> > +++ b/arch/powerpc/kvm/booke_interrupts.S
> > @@ -52,12 +52,7 @@
> >                        (1<<BOOKE_INTERRUPT_PROGRAM) | \
> >                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
> >
> > -.macro KVM_HANDLER ivor_nr scratch srr0
> > -_GLOBAL(kvmppc_handler_\ivor_nr)
> > -	/* Get pointer to vcpu and record exit number. */
> > -	mtspr	\scratch , r4
> > -	mfspr   r4, SPRN_SPRG_THREAD
> > -	lwz     r4, THREAD_KVM_VCPU(r4)
> > +.macro __KVM_HANDLER ivor_nr scratch srr0
> > 	stw	r3, VCPU_GPR(R3)(r4)
> > 	stw	r5, VCPU_GPR(R5)(r4)
> > 	stw	r6, VCPU_GPR(R6)(r4)
> > @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
> > 	bctr
> > .endm
> >
> > +.macro KVM_HANDLER ivor_nr scratch srr0
> > +_GLOBAL(kvmppc_handler_\ivor_nr)
> > +	/* Get pointer to vcpu and record exit number. */
> > +	mtspr	\scratch , r4
> > +	mfspr   r4, SPRN_SPRG_THREAD
> > +	lwz     r4, THREAD_KVM_VCPU(r4)
> > +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> > +
> > +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> > +_GLOBAL(kvmppc_handler_\ivor_nr)
> > +	mtspr   \scratch, r4
> > +	mfspr	r4, SPRN_SPRG_THREAD
> > +	lwz	r4, THREAD_KVM_VCPU(r4)
> > +	stw	r3, VCPU_CRIT_SAVE(r4)
> > +	mfcr	r3
> > +	mfspr	r4, SPRN_CSRR1
> > +	andi.	r4, r4, MSR_PR
> > +	bne	1f
> 
> 
> > +	/* debug interrupt happened in enter/exit path */
> > +	mfspr   r4, SPRN_CSRR1
> > +	rlwinm  r4, r4, 0, ~MSR_DE
> > +	mtspr   SPRN_CSRR1, r4
> > +	lis	r4, 0xffff
> > +	ori	r4, r4, 0xffff
> > +	mtspr	SPRN_DBSR, r4
> > +	mfspr	r4, SPRN_SPRG_THREAD
> > +	lwz	r4, THREAD_KVM_VCPU(r4)
> > +	mtcr	r3
> > +	lwz     r3, VCPU_CRIT_SAVE(r4)
> > +	mfspr   r4, \scratch
> > +	rfci
> 
> What is this part doing? Try to ignore the debug exit?

As BOOKE doesn't have hardware support for virtualization, hardware never know current pc is in guest or in host.
So when enable hardware single step for guest, it cannot be disabled at the time guest exit. Thus, we'll see that an single step interrupt happens at the beginning of guest exit path.

With the above code we recognize this kind of single step interrupt disable single step and rfci.

> Why would we have MSR_DE
> enabled in the first place when we can't handle it?

When QEMU is using hardware debug resource then we always set MSR_DE during guest is running.

> 
> > +1:	/* debug interrupt happened in guest */
> > +	mtcr	r3
> > +	mfspr	r4, SPRN_SPRG_THREAD
> > +	lwz	r4, THREAD_KVM_VCPU(r4)
> > +	lwz     r3, VCPU_CRIT_SAVE(r4)
> > +	__KVM_HANDLER \ivor_nr \scratch \srr0
> 
> I don't think you need the __KVM_HANDLER split. This should be quite easily
> refactorable into a simple DBG prolog.

Can you please elaborate how you are envisioning this?

Thanks
-Bharat

> 
> 
> Alex
> 
> > +.endm
> > +
> > .macro KVM_HANDLER_ADDR ivor_nr
> > 	.long	kvmppc_handler_\ivor_nr
> > .endm
> > @@ -98,7 +133,7 @@ KVM_HANDLER BOOKE_INTERRUPT_FIT SPRN_SPRG_RSCRATCH0
> > SPRN_SRR0 KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG SPRN_SPRG_RSCRATCH_CRIT
> > SPRN_CSRR0 KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0
> > SPRN_SRR0 KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0
> > SPRN_SRR0 -KVM_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT
> > SPRN_CSRR0
> > +KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT
> > +SPRN_CSRR0
> > KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_ROUND SPRN_SPRG_RSCRATCH0 SPRN_SRR0
> > --
> > 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
Alexander Graf - Jan. 31, 2013, 12:17 p.m.
On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Friday, January 25, 2013 5:13 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan Bharat-R65777
>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>> 
>> 
>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
>> 
>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> 
>>> Installed debug handler will be used for guest debug support and debug
>>> facility emulation features (patches for these features will follow
>>> this patch).
>>> 
>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>> [bharat.bhushan@freescale.com: Substantial changes]
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>> ---
>>> arch/powerpc/include/asm/kvm_host.h |    1 +
>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
>>> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++-----
>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>> b/arch/powerpc/include/asm/kvm_host.h
>>> index 8a72d59..f4ba881 100644
>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
>>> 	u32 tlbcfg[4];
>>> 	u32 mmucfg;
>>> 	u32 epr;
>>> +	u32 crit_save;
>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
>>> 	gpa_t paddr_accessed;
>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>> b/arch/powerpc/kernel/asm-offsets.c
>>> index 46f6afd..02048f3 100644
>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>> @@ -562,6 +562,7 @@ int main(void)
>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
>>> 	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));
>>> #endif /* CONFIG_PPC_BOOK3S */
>>> #endif /* CONFIG_KVM */
>>> 
>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>> b/arch/powerpc/kvm/booke_interrupts.S
>>> index eae8483..dd9c5d4 100644
>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>> @@ -52,12 +52,7 @@
>>>                       (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>                       (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>> 
>>> -.macro KVM_HANDLER ivor_nr scratch srr0
>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
>>> -	/* Get pointer to vcpu and record exit number. */
>>> -	mtspr	\scratch , r4
>>> -	mfspr   r4, SPRN_SPRG_THREAD
>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>> 	stw	r6, VCPU_GPR(R6)(r4)
>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
>>> 	bctr
>>> .endm
>>> 
>>> +.macro KVM_HANDLER ivor_nr scratch srr0
>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>> +	/* Get pointer to vcpu and record exit number. */
>>> +	mtspr	\scratch , r4
>>> +	mfspr   r4, SPRN_SPRG_THREAD
>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>> +
>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>> +	mtspr   \scratch, r4
>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
>>> +	mfcr	r3
>>> +	mfspr	r4, SPRN_CSRR1
>>> +	andi.	r4, r4, MSR_PR
>>> +	bne	1f
>> 
>> 
>>> +	/* debug interrupt happened in enter/exit path */
>>> +	mfspr   r4, SPRN_CSRR1
>>> +	rlwinm  r4, r4, 0, ~MSR_DE
>>> +	mtspr   SPRN_CSRR1, r4
>>> +	lis	r4, 0xffff
>>> +	ori	r4, r4, 0xffff
>>> +	mtspr	SPRN_DBSR, r4
>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>> +	mtcr	r3
>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>> +	mfspr   r4, \scratch
>>> +	rfci
>> 
>> What is this part doing? Try to ignore the debug exit?
> 
> As BOOKE doesn't have hardware support for virtualization, hardware never know current pc is in guest or in host.
> So when enable hardware single step for guest, it cannot be disabled at the time guest exit. Thus, we'll see that an single step interrupt happens at the beginning of guest exit path.
> 
> With the above code we recognize this kind of single step interrupt disable single step and rfci.
> 
>> Why would we have MSR_DE
>> enabled in the first place when we can't handle it?
> 
> When QEMU is using hardware debug resource then we always set MSR_DE during guest is running.

Right, but why is MSR_DE enabled during the exit path? If MSR_DE wasn't set, you wouldn't get a single step exit. During the exit code path, you could then swap DBSR back to what the host expects (which means no single step). Only after that enable MSR_DE again.

> 
>> 
>>> +1:	/* debug interrupt happened in guest */
>>> +	mtcr	r3
>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
>> 
>> I don't think you need the __KVM_HANDLER split. This should be quite easily
>> refactorable into a simple DBG prolog.
> 
> Can you please elaborate how you are envisioning this?

With this patch, you have

KVM_HANLDER:

  <code>
  __KVM_HANDLER

KVM_DBG_HANDLER:

  <code>
  __KVM_HANDLER

Right?

In KVM_HANDLER, you get:

> .macro KVM_HANDLER ivor_nr scratch srr0
> _GLOBAL(kvmppc_handler_\ivor_nr)
> 	/* Get pointer to vcpu and record exit number. */
> 	mtspr	\scratch , r4
> 	mfspr   r4, SPRN_SPRG_THREAD
> 	lwz     r4, THREAD_KVM_VCPU(r4)
> 	__KVM_HANDLER \ivor_nr \scratch \srr0
> .endm


while KVM_DBG_HANDLER is:

> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> +_GLOBAL(kvmppc_handler_\ivor_nr)
>  <debug specific handling>
> +1:	/* debug interrupt happened in guest */
> +	mtcr	r3
> +	mfspr	r4, SPRN_SPRG_THREAD
> +	lwz	r4, THREAD_KVM_VCPU(r4)
> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> +	__KVM_HANDLER \ivor_nr \scratch \srr0
> +.endm


So if you write this as

KVM_DBG_HANDLER:
	<debug specific handling>
	1:
	mtcr		r3
	mfspr	r4, SPRN_SPRG_THREAD
	lwz		r4, THREAD_KVM_VCPU(r4)
	lwz		r3, VCPU_CRIT_SAVE(r4)
	lwz		r4, \scratch
	<KVM_HANDLER>

then you get code that is slower :) but it should be easier to read, since the interface between the individual pieces is always the same. Debug shouldn't be a fast path anyway, right?


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 - Jan. 31, 2013, 4:58 p.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, January 31, 2013 5:47 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> 
> 
> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Friday, January 25, 2013 5:13 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
> >> Bharat-R65777
> >> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>
> >>
> >> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
> >>
> >>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>
> >>> Installed debug handler will be used for guest debug support and
> >>> debug facility emulation features (patches for these features will
> >>> follow this patch).
> >>>
> >>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> >>> [bharat.bhushan@freescale.com: Substantial changes]
> >>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> >>> ---
> >>> arch/powerpc/include/asm/kvm_host.h |    1 +
> >>> arch/powerpc/kernel/asm-offsets.c   |    1 +
> >>> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++---
> --
> >>> 3 files changed, 44 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/include/asm/kvm_host.h
> >>> b/arch/powerpc/include/asm/kvm_host.h
> >>> index 8a72d59..f4ba881 100644
> >>> --- a/arch/powerpc/include/asm/kvm_host.h
> >>> +++ b/arch/powerpc/include/asm/kvm_host.h
> >>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
> >>> 	u32 tlbcfg[4];
> >>> 	u32 mmucfg;
> >>> 	u32 epr;
> >>> +	u32 crit_save;
> >>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
> >>> 	gpa_t paddr_accessed;
> >>> diff --git a/arch/powerpc/kernel/asm-offsets.c
> >>> b/arch/powerpc/kernel/asm-offsets.c
> >>> index 46f6afd..02048f3 100644
> >>> --- a/arch/powerpc/kernel/asm-offsets.c
> >>> +++ b/arch/powerpc/kernel/asm-offsets.c
> >>> @@ -562,6 +562,7 @@ int main(void)
> >>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> >>> 	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));
> >>> #endif /* CONFIG_PPC_BOOK3S */
> >>> #endif /* CONFIG_KVM */
> >>>
> >>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>> b/arch/powerpc/kvm/booke_interrupts.S
> >>> index eae8483..dd9c5d4 100644
> >>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>> @@ -52,12 +52,7 @@
> >>>                       (1<<BOOKE_INTERRUPT_PROGRAM) | \
> >>>                       (1<<BOOKE_INTERRUPT_DTLB_MISS))
> >>>
> >>> -.macro KVM_HANDLER ivor_nr scratch srr0
> >>> -_GLOBAL(kvmppc_handler_\ivor_nr)
> >>> -	/* Get pointer to vcpu and record exit number. */
> >>> -	mtspr	\scratch , r4
> >>> -	mfspr   r4, SPRN_SPRG_THREAD
> >>> -	lwz     r4, THREAD_KVM_VCPU(r4)
> >>> +.macro __KVM_HANDLER ivor_nr scratch srr0
> >>> 	stw	r3, VCPU_GPR(R3)(r4)
> >>> 	stw	r5, VCPU_GPR(R5)(r4)
> >>> 	stw	r6, VCPU_GPR(R6)(r4)
> >>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
> >>> 	bctr
> >>> .endm
> >>>
> >>> +.macro KVM_HANDLER ivor_nr scratch srr0
> >>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>> +	/* Get pointer to vcpu and record exit number. */
> >>> +	mtspr	\scratch , r4
> >>> +	mfspr   r4, SPRN_SPRG_THREAD
> >>> +	lwz     r4, THREAD_KVM_VCPU(r4)
> >>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>> +
> >>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>> +	mtspr   \scratch, r4
> >>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>> +	stw	r3, VCPU_CRIT_SAVE(r4)
> >>> +	mfcr	r3
> >>> +	mfspr	r4, SPRN_CSRR1
> >>> +	andi.	r4, r4, MSR_PR
> >>> +	bne	1f
> >>
> >>
> >>> +	/* debug interrupt happened in enter/exit path */
> >>> +	mfspr   r4, SPRN_CSRR1
> >>> +	rlwinm  r4, r4, 0, ~MSR_DE
> >>> +	mtspr   SPRN_CSRR1, r4
> >>> +	lis	r4, 0xffff
> >>> +	ori	r4, r4, 0xffff
> >>> +	mtspr	SPRN_DBSR, r4
> >>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>> +	mtcr	r3
> >>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>> +	mfspr   r4, \scratch
> >>> +	rfci
> >>
> >> What is this part doing? Try to ignore the debug exit?
> >
> > As BOOKE doesn't have hardware support for virtualization, hardware never know
> current pc is in guest or in host.
> > So when enable hardware single step for guest, it cannot be disabled at the
> time guest exit. Thus, we'll see that an single step interrupt happens at the
> beginning of guest exit path.
> >
> > With the above code we recognize this kind of single step interrupt disable
> single step and rfci.
> >
> >> Why would we have MSR_DE
> >> enabled in the first place when we can't handle it?
> >
> > When QEMU is using hardware debug resource then we always set MSR_DE during
> guest is running.
> 
> Right, but why is MSR_DE enabled during the exit path? If MSR_DE wasn't set, you
> wouldn't get a single step exit.

We always set MSR_DE in hw MSR when qemu using the debug resource.

> During the exit code path, you could then swap
> DBSR back to what the host expects (which means no single step). Only after that
> enable MSR_DE again.

We do not support deferred debug interrupt, so we do save restore dbsr.

> 
> >
> >>
> >>> +1:	/* debug interrupt happened in guest */
> >>> +	mtcr	r3
> >>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
> >>
> >> I don't think you need the __KVM_HANDLER split. This should be quite
> >> easily refactorable into a simple DBG prolog.
> >
> > Can you please elaborate how you are envisioning this?
> 
> With this patch, you have
> 
> KVM_HANLDER:
> 
>   <code>
>   __KVM_HANDLER
> 
> KVM_DBG_HANDLER:
> 
>   <code>
>   __KVM_HANDLER
> 
> Right?
> 
> In KVM_HANDLER, you get:
> 
> > .macro KVM_HANDLER ivor_nr scratch srr0
> > _GLOBAL(kvmppc_handler_\ivor_nr)
> > 	/* Get pointer to vcpu and record exit number. */
> > 	mtspr	\scratch , r4
> > 	mfspr   r4, SPRN_SPRG_THREAD
> > 	lwz     r4, THREAD_KVM_VCPU(r4)
> > 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> 
> 
> while KVM_DBG_HANDLER is:
> 
> > +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> > +_GLOBAL(kvmppc_handler_\ivor_nr)
> >  <debug specific handling>
> > +1:	/* debug interrupt happened in guest */
> > +	mtcr	r3
> > +	mfspr	r4, SPRN_SPRG_THREAD
> > +	lwz	r4, THREAD_KVM_VCPU(r4)
> > +	lwz     r3, VCPU_CRIT_SAVE(r4)
> > +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> 
> 
> So if you write this as
> 
> KVM_DBG_HANDLER:
> 	<debug specific handling>
> 	1:
> 	mtcr		r3
> 	mfspr	r4, SPRN_SPRG_THREAD
> 	lwz		r4, THREAD_KVM_VCPU(r4)
> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> 	lwz		r4, \scratch
> 	<KVM_HANDLER>
> 
> then you get code that is slower :) but it should be easier to read, since the
> interface between the individual pieces is always the same. Debug shouldn't be a
> fast path anyway, right?

Frankly speaking I do not see much difference :).

If we have to do as you mentioned then I think we can just do

KVM_DBG_HANDLER:
 	<debug specific handling>
 	1:
 	mtcr		r3
 	lwz		r3, VCPU_CRIT_SAVE(r4)
 	lwz		r4, \scratch
 	<KVM_HANDLER>

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
Alexander Graf - Jan. 31, 2013, 5:08 p.m.
On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, January 31, 2013 5:47 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>> 
>> 
>> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Friday, January 25, 2013 5:13 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
>>>> Bharat-R65777
>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>> 
>>>> 
>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
>>>> 
>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>> 
>>>>> Installed debug handler will be used for guest debug support and
>>>>> debug facility emulation features (patches for these features will
>>>>> follow this patch).
>>>>> 
>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>>>> [bharat.bhushan@freescale.com: Substantial changes]
>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>> ---
>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
>>>>> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++---
>> --
>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>>>> 
>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>> index 8a72d59..f4ba881 100644
>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
>>>>> 	u32 tlbcfg[4];
>>>>> 	u32 mmucfg;
>>>>> 	u32 epr;
>>>>> +	u32 crit_save;
>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
>>>>> 	gpa_t paddr_accessed;
>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>> b/arch/powerpc/kernel/asm-offsets.c
>>>>> index 46f6afd..02048f3 100644
>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>> @@ -562,6 +562,7 @@ int main(void)
>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
>>>>> 	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));
>>>>> #endif /* CONFIG_PPC_BOOK3S */
>>>>> #endif /* CONFIG_KVM */
>>>>> 
>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>> index eae8483..dd9c5d4 100644
>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>> @@ -52,12 +52,7 @@
>>>>>                      (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>                      (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>> 
>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>> -	/* Get pointer to vcpu and record exit number. */
>>>>> -	mtspr	\scratch , r4
>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>> 	bctr
>>>>> .endm
>>>>> 
>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>> +	/* Get pointer to vcpu and record exit number. */
>>>>> +	mtspr	\scratch , r4
>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>> +
>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>> +	mtspr   \scratch, r4
>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
>>>>> +	mfcr	r3
>>>>> +	mfspr	r4, SPRN_CSRR1
>>>>> +	andi.	r4, r4, MSR_PR
>>>>> +	bne	1f
>>>> 
>>>> 
>>>>> +	/* debug interrupt happened in enter/exit path */
>>>>> +	mfspr   r4, SPRN_CSRR1
>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
>>>>> +	mtspr   SPRN_CSRR1, r4
>>>>> +	lis	r4, 0xffff
>>>>> +	ori	r4, r4, 0xffff
>>>>> +	mtspr	SPRN_DBSR, r4
>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>> +	mtcr	r3
>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>> +	mfspr   r4, \scratch
>>>>> +	rfci
>>>> 
>>>> What is this part doing? Try to ignore the debug exit?
>>> 
>>> As BOOKE doesn't have hardware support for virtualization, hardware never know
>> current pc is in guest or in host.
>>> So when enable hardware single step for guest, it cannot be disabled at the
>> time guest exit. Thus, we'll see that an single step interrupt happens at the
>> beginning of guest exit path.
>>> 
>>> With the above code we recognize this kind of single step interrupt disable
>> single step and rfci.
>>> 
>>>> Why would we have MSR_DE
>>>> enabled in the first place when we can't handle it?
>>> 
>>> When QEMU is using hardware debug resource then we always set MSR_DE during
>> guest is running.
>> 
>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE wasn't set, you
>> wouldn't get a single step exit.
> 
> We always set MSR_DE in hw MSR when qemu using the debug resource.

In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be set anymore, because we're in an interrupt handler, no? Or is MSR_DE kept alive on interrupts?

> 
>> During the exit code path, you could then swap
>> DBSR back to what the host expects (which means no single step). Only after that
>> enable MSR_DE again.
> 
> We do not support deferred debug interrupt, so we do save restore dbsr.
> 
>> 
>>> 
>>>> 
>>>>> +1:	/* debug interrupt happened in guest */
>>>>> +	mtcr	r3
>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
>>>> 
>>>> I don't think you need the __KVM_HANDLER split. This should be quite
>>>> easily refactorable into a simple DBG prolog.
>>> 
>>> Can you please elaborate how you are envisioning this?
>> 
>> With this patch, you have
>> 
>> KVM_HANLDER:
>> 
>>  <code>
>>  __KVM_HANDLER
>> 
>> KVM_DBG_HANDLER:
>> 
>>  <code>
>>  __KVM_HANDLER
>> 
>> Right?
>> 
>> In KVM_HANDLER, you get:
>> 
>>> .macro KVM_HANDLER ivor_nr scratch srr0
>>> _GLOBAL(kvmppc_handler_\ivor_nr)
>>> 	/* Get pointer to vcpu and record exit number. */
>>> 	mtspr	\scratch , r4
>>> 	mfspr   r4, SPRN_SPRG_THREAD
>>> 	lwz     r4, THREAD_KVM_VCPU(r4)
>>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>> 
>> 
>> while KVM_DBG_HANDLER is:
>> 
>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>> <debug specific handling>
>>> +1:	/* debug interrupt happened in guest */
>>> +	mtcr	r3
>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>> 
>> 
>> So if you write this as
>> 
>> KVM_DBG_HANDLER:
>> 	<debug specific handling>
>> 	1:
>> 	mtcr		r3
>> 	mfspr	r4, SPRN_SPRG_THREAD
>> 	lwz		r4, THREAD_KVM_VCPU(r4)
>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>> 	lwz		r4, \scratch
>> 	<KVM_HANDLER>
>> 
>> then you get code that is slower :) but it should be easier to read, since the
>> interface between the individual pieces is always the same. Debug shouldn't be a
>> fast path anyway, right?
> 
> Frankly speaking I do not see much difference :).
> 
> If we have to do as you mentioned then I think we can just do
> 
> KVM_DBG_HANDLER:
> 	<debug specific handling>
> 	1:
> 	mtcr		r3
> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> 	lwz		r4, \scratch
> 	<KVM_HANDLER>

Whatever it takes to keep the oddball (debug) an oddball and keep the normal case easy :).


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
Alexander Graf - Jan. 31, 2013, 5:11 p.m.
On 31.01.2013, at 18:08, Alexander Graf wrote:

> 
> On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote:
> 
>> 
>> 
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Thursday, January 31, 2013 5:47 PM
>>> To: Bhushan Bharat-R65777
>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>> 
>>> 
>>> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
>>> 
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>> Sent: Friday, January 25, 2013 5:13 PM
>>>>> To: Bhushan Bharat-R65777
>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
>>>>> Bharat-R65777
>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>>> 
>>>>> 
>>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
>>>>> 
>>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>> 
>>>>>> Installed debug handler will be used for guest debug support and
>>>>>> debug facility emulation features (patches for these features will
>>>>>> follow this patch).
>>>>>> 
>>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>>>>> [bharat.bhushan@freescale.com: Substantial changes]
>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>> ---
>>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
>>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
>>>>>> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++---
>>> --
>>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>>>>> 
>>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>>> index 8a72d59..f4ba881 100644
>>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
>>>>>> 	u32 tlbcfg[4];
>>>>>> 	u32 mmucfg;
>>>>>> 	u32 epr;
>>>>>> +	u32 crit_save;
>>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
>>>>>> 	gpa_t paddr_accessed;
>>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>>> b/arch/powerpc/kernel/asm-offsets.c
>>>>>> index 46f6afd..02048f3 100644
>>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>>> @@ -562,6 +562,7 @@ int main(void)
>>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
>>>>>> 	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));
>>>>>> #endif /* CONFIG_PPC_BOOK3S */
>>>>>> #endif /* CONFIG_KVM */
>>>>>> 
>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>> index eae8483..dd9c5d4 100644
>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>> @@ -52,12 +52,7 @@
>>>>>>                     (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>>                     (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>>> 
>>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>> -	/* Get pointer to vcpu and record exit number. */
>>>>>> -	mtspr	\scratch , r4
>>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
>>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
>>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
>>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>> 	bctr
>>>>>> .endm
>>>>>> 
>>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>> +	/* Get pointer to vcpu and record exit number. */
>>>>>> +	mtspr	\scratch , r4
>>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
>>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>> +
>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>> +	mtspr   \scratch, r4
>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
>>>>>> +	mfcr	r3
>>>>>> +	mfspr	r4, SPRN_CSRR1
>>>>>> +	andi.	r4, r4, MSR_PR
>>>>>> +	bne	1f
>>>>> 
>>>>> 
>>>>>> +	/* debug interrupt happened in enter/exit path */
>>>>>> +	mfspr   r4, SPRN_CSRR1
>>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
>>>>>> +	mtspr   SPRN_CSRR1, r4
>>>>>> +	lis	r4, 0xffff
>>>>>> +	ori	r4, r4, 0xffff
>>>>>> +	mtspr	SPRN_DBSR, r4
>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>> +	mtcr	r3
>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>> +	mfspr   r4, \scratch
>>>>>> +	rfci
>>>>> 
>>>>> What is this part doing? Try to ignore the debug exit?
>>>> 
>>>> As BOOKE doesn't have hardware support for virtualization, hardware never know
>>> current pc is in guest or in host.
>>>> So when enable hardware single step for guest, it cannot be disabled at the
>>> time guest exit. Thus, we'll see that an single step interrupt happens at the
>>> beginning of guest exit path.
>>>> 
>>>> With the above code we recognize this kind of single step interrupt disable
>>> single step and rfci.
>>>> 
>>>>> Why would we have MSR_DE
>>>>> enabled in the first place when we can't handle it?
>>>> 
>>>> When QEMU is using hardware debug resource then we always set MSR_DE during
>>> guest is running.
>>> 
>>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE wasn't set, you
>>> wouldn't get a single step exit.
>> 
>> We always set MSR_DE in hw MSR when qemu using the debug resource.
> 
> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be set anymore, because we're in an interrupt handler, no? Or is MSR_DE kept alive on interrupts?

Ah, it's kept for non-debug interrupts. That explains things.


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 - Feb. 1, 2013, 5:04 a.m.
> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
> Behalf Of Alexander Graf
> Sent: Thursday, January 31, 2013 10:38 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> 
> 
> On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, January 31, 2013 5:47 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> >> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>
> >>
> >> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>> Sent: Friday, January 25, 2013 5:13 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
> >>>> Bharat-R65777
> >>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>>>
> >>>>
> >>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
> >>>>
> >>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>
> >>>>> Installed debug handler will be used for guest debug support and
> >>>>> debug facility emulation features (patches for these features will
> >>>>> follow this patch).
> >>>>>
> >>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> >>>>> [bharat.bhushan@freescale.com: Substantial changes]
> >>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> >>>>> ---
> >>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
> >>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
> >>>>> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++-
> --
> >> --
> >>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
> >>>>> b/arch/powerpc/include/asm/kvm_host.h
> >>>>> index 8a72d59..f4ba881 100644
> >>>>> --- a/arch/powerpc/include/asm/kvm_host.h
> >>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
> >>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
> >>>>> 	u32 tlbcfg[4];
> >>>>> 	u32 mmucfg;
> >>>>> 	u32 epr;
> >>>>> +	u32 crit_save;
> >>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
> >>>>> 	gpa_t paddr_accessed;
> >>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
> >>>>> b/arch/powerpc/kernel/asm-offsets.c
> >>>>> index 46f6afd..02048f3 100644
> >>>>> --- a/arch/powerpc/kernel/asm-offsets.c
> >>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
> >>>>> @@ -562,6 +562,7 @@ int main(void)
> >>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> >>>>> 	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));
> >>>>> #endif /* CONFIG_PPC_BOOK3S */
> >>>>> #endif /* CONFIG_KVM */
> >>>>>
> >>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>> index eae8483..dd9c5d4 100644
> >>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>> @@ -52,12 +52,7 @@
> >>>>>                      (1<<BOOKE_INTERRUPT_PROGRAM) | \
> >>>>>                      (1<<BOOKE_INTERRUPT_DTLB_MISS))
> >>>>>
> >>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
> >>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>> -	/* Get pointer to vcpu and record exit number. */
> >>>>> -	mtspr	\scratch , r4
> >>>>> -	mfspr   r4, SPRN_SPRG_THREAD
> >>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
> >>>>> 	stw	r3, VCPU_GPR(R3)(r4)
> >>>>> 	stw	r5, VCPU_GPR(R5)(r4)
> >>>>> 	stw	r6, VCPU_GPR(R6)(r4)
> >>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>> 	bctr
> >>>>> .endm
> >>>>>
> >>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
> >>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>> +	/* Get pointer to vcpu and record exit number. */
> >>>>> +	mtspr	\scratch , r4
> >>>>> +	mfspr   r4, SPRN_SPRG_THREAD
> >>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>> +
> >>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>> +	mtspr   \scratch, r4
> >>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
> >>>>> +	mfcr	r3
> >>>>> +	mfspr	r4, SPRN_CSRR1
> >>>>> +	andi.	r4, r4, MSR_PR
> >>>>> +	bne	1f
> >>>>
> >>>>
> >>>>> +	/* debug interrupt happened in enter/exit path */
> >>>>> +	mfspr   r4, SPRN_CSRR1
> >>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
> >>>>> +	mtspr   SPRN_CSRR1, r4
> >>>>> +	lis	r4, 0xffff
> >>>>> +	ori	r4, r4, 0xffff
> >>>>> +	mtspr	SPRN_DBSR, r4
> >>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>> +	mtcr	r3
> >>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>> +	mfspr   r4, \scratch
> >>>>> +	rfci
> >>>>
> >>>> What is this part doing? Try to ignore the debug exit?
> >>>
> >>> As BOOKE doesn't have hardware support for virtualization, hardware
> >>> never know
> >> current pc is in guest or in host.
> >>> So when enable hardware single step for guest, it cannot be disabled
> >>> at the
> >> time guest exit. Thus, we'll see that an single step interrupt
> >> happens at the beginning of guest exit path.
> >>>
> >>> With the above code we recognize this kind of single step interrupt
> >>> disable
> >> single step and rfci.
> >>>
> >>>> Why would we have MSR_DE
> >>>> enabled in the first place when we can't handle it?
> >>>
> >>> When QEMU is using hardware debug resource then we always set MSR_DE
> >>> during
> >> guest is running.
> >>
> >> Right, but why is MSR_DE enabled during the exit path? If MSR_DE
> >> wasn't set, you wouldn't get a single step exit.
> >
> > We always set MSR_DE in hw MSR when qemu using the debug resource.
> 
> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be set
> anymore, because we're in an interrupt handler, no? Or is MSR_DE kept alive on
> interrupts?
> 
> >
> >> During the exit code path, you could then swap DBSR back to what the
> >> host expects (which means no single step). Only after that enable
> >> MSR_DE again.
> >
> > We do not support deferred debug interrupt, so we do save restore dbsr.
> >
> >>
> >>>
> >>>>
> >>>>> +1:	/* debug interrupt happened in guest */
> >>>>> +	mtcr	r3
> >>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
> >>>>
> >>>> I don't think you need the __KVM_HANDLER split. This should be
> >>>> quite easily refactorable into a simple DBG prolog.
> >>>
> >>> Can you please elaborate how you are envisioning this?
> >>
> >> With this patch, you have
> >>
> >> KVM_HANLDER:
> >>
> >>  <code>
> >>  __KVM_HANDLER
> >>
> >> KVM_DBG_HANDLER:
> >>
> >>  <code>
> >>  __KVM_HANDLER
> >>
> >> Right?
> >>
> >> In KVM_HANDLER, you get:
> >>
> >>> .macro KVM_HANDLER ivor_nr scratch srr0
> >>> _GLOBAL(kvmppc_handler_\ivor_nr)
> >>> 	/* Get pointer to vcpu and record exit number. */
> >>> 	mtspr	\scratch , r4
> >>> 	mfspr   r4, SPRN_SPRG_THREAD
> >>> 	lwz     r4, THREAD_KVM_VCPU(r4)
> >>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>
> >>
> >> while KVM_DBG_HANDLER is:
> >>
> >>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>> <debug specific handling>
> >>> +1:	/* debug interrupt happened in guest */
> >>> +	mtcr	r3
> >>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>
> >>
> >> So if you write this as
> >>
> >> KVM_DBG_HANDLER:
> >> 	<debug specific handling>
> >> 	1:
> >> 	mtcr		r3
> >> 	mfspr	r4, SPRN_SPRG_THREAD
> >> 	lwz		r4, THREAD_KVM_VCPU(r4)
> >> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> >> 	lwz		r4, \scratch
> >> 	<KVM_HANDLER>
> >>
> >> then you get code that is slower :) but it should be easier to read,
> >> since the interface between the individual pieces is always the same.
> >> Debug shouldn't be a fast path anyway, right?
> >
> > Frankly speaking I do not see much difference :).
> >
> > If we have to do as you mentioned then I think we can just do
> >
> > KVM_DBG_HANDLER:
> > 	<debug specific handling>
> > 	1:
> > 	mtcr		r3
> > 	lwz		r3, VCPU_CRIT_SAVE(r4)
> > 	lwz		r4, \scratch
> > 	<KVM_HANDLER>
> 
> Whatever it takes to keep the oddball (debug) an oddball and keep the normal
> case easy :).

I think there will be another problem as  the kvmppc_handler_\ivor_nr will not be the starting address which is required as per our ivor/ivpr usages for booke architecture.

I am thinking of keeping as is :).

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


--
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 - Feb. 1, 2013, 8:06 a.m.
On 01.02.2013, at 06:04, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
>> Behalf Of Alexander Graf
>> Sent: Thursday, January 31, 2013 10:38 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>> 
>> 
>> On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Thursday, January 31, 2013 5:47 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>> 
>>>> 
>>>> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
>>>> 
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Friday, January 25, 2013 5:13 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
>>>>>> Bharat-R65777
>>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>>>> 
>>>>>> 
>>>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
>>>>>> 
>>>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>> 
>>>>>>> Installed debug handler will be used for guest debug support and
>>>>>>> debug facility emulation features (patches for these features will
>>>>>>> follow this patch).
>>>>>>> 
>>>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>>>>>> [bharat.bhushan@freescale.com: Substantial changes]
>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>>> ---
>>>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
>>>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
>>>>>>> arch/powerpc/kvm/booke_interrupts.S |   49 ++++++++++++++++++++++++++++++-
>> --
>>>> --
>>>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>>>> index 8a72d59..f4ba881 100644
>>>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
>>>>>>> 	u32 tlbcfg[4];
>>>>>>> 	u32 mmucfg;
>>>>>>> 	u32 epr;
>>>>>>> +	u32 crit_save;
>>>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
>>>>>>> 	gpa_t paddr_accessed;
>>>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>>>> b/arch/powerpc/kernel/asm-offsets.c
>>>>>>> index 46f6afd..02048f3 100644
>>>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>>>> @@ -562,6 +562,7 @@ int main(void)
>>>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
>>>>>>> 	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));
>>>>>>> #endif /* CONFIG_PPC_BOOK3S */
>>>>>>> #endif /* CONFIG_KVM */
>>>>>>> 
>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>> index eae8483..dd9c5d4 100644
>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>> @@ -52,12 +52,7 @@
>>>>>>>                     (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>>>                     (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>>>> 
>>>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>> -	/* Get pointer to vcpu and record exit number. */
>>>>>>> -	mtspr	\scratch , r4
>>>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
>>>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
>>>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>> 	bctr
>>>>>>> .endm
>>>>>>> 
>>>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>> +	/* Get pointer to vcpu and record exit number. */
>>>>>>> +	mtspr	\scratch , r4
>>>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>>> +
>>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>> +	mtspr   \scratch, r4
>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
>>>>>>> +	mfcr	r3
>>>>>>> +	mfspr	r4, SPRN_CSRR1
>>>>>>> +	andi.	r4, r4, MSR_PR
>>>>>>> +	bne	1f
>>>>>> 
>>>>>> 
>>>>>>> +	/* debug interrupt happened in enter/exit path */
>>>>>>> +	mfspr   r4, SPRN_CSRR1
>>>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
>>>>>>> +	mtspr   SPRN_CSRR1, r4
>>>>>>> +	lis	r4, 0xffff
>>>>>>> +	ori	r4, r4, 0xffff
>>>>>>> +	mtspr	SPRN_DBSR, r4
>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>> +	mtcr	r3
>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>> +	mfspr   r4, \scratch
>>>>>>> +	rfci
>>>>>> 
>>>>>> What is this part doing? Try to ignore the debug exit?
>>>>> 
>>>>> As BOOKE doesn't have hardware support for virtualization, hardware
>>>>> never know
>>>> current pc is in guest or in host.
>>>>> So when enable hardware single step for guest, it cannot be disabled
>>>>> at the
>>>> time guest exit. Thus, we'll see that an single step interrupt
>>>> happens at the beginning of guest exit path.
>>>>> 
>>>>> With the above code we recognize this kind of single step interrupt
>>>>> disable
>>>> single step and rfci.
>>>>> 
>>>>>> Why would we have MSR_DE
>>>>>> enabled in the first place when we can't handle it?
>>>>> 
>>>>> When QEMU is using hardware debug resource then we always set MSR_DE
>>>>> during
>>>> guest is running.
>>>> 
>>>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE
>>>> wasn't set, you wouldn't get a single step exit.
>>> 
>>> We always set MSR_DE in hw MSR when qemu using the debug resource.
>> 
>> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be set
>> anymore, because we're in an interrupt handler, no? Or is MSR_DE kept alive on
>> interrupts?
>> 
>>> 
>>>> During the exit code path, you could then swap DBSR back to what the
>>>> host expects (which means no single step). Only after that enable
>>>> MSR_DE again.
>>> 
>>> We do not support deferred debug interrupt, so we do save restore dbsr.
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>>> +1:	/* debug interrupt happened in guest */
>>>>>>> +	mtcr	r3
>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
>>>>>> 
>>>>>> I don't think you need the __KVM_HANDLER split. This should be
>>>>>> quite easily refactorable into a simple DBG prolog.
>>>>> 
>>>>> Can you please elaborate how you are envisioning this?
>>>> 
>>>> With this patch, you have
>>>> 
>>>> KVM_HANLDER:
>>>> 
>>>> <code>
>>>> __KVM_HANDLER
>>>> 
>>>> KVM_DBG_HANDLER:
>>>> 
>>>> <code>
>>>> __KVM_HANDLER
>>>> 
>>>> Right?
>>>> 
>>>> In KVM_HANDLER, you get:
>>>> 
>>>>> .macro KVM_HANDLER ivor_nr scratch srr0
>>>>> _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>> 	/* Get pointer to vcpu and record exit number. */
>>>>> 	mtspr	\scratch , r4
>>>>> 	mfspr   r4, SPRN_SPRG_THREAD
>>>>> 	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>> 
>>>> 
>>>> while KVM_DBG_HANDLER is:
>>>> 
>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>> <debug specific handling>
>>>>> +1:	/* debug interrupt happened in guest */
>>>>> +	mtcr	r3
>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>> 
>>>> 
>>>> So if you write this as
>>>> 
>>>> KVM_DBG_HANDLER:
>>>> 	<debug specific handling>
>>>> 	1:
>>>> 	mtcr		r3
>>>> 	mfspr	r4, SPRN_SPRG_THREAD
>>>> 	lwz		r4, THREAD_KVM_VCPU(r4)
>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>>>> 	lwz		r4, \scratch
>>>> 	<KVM_HANDLER>
>>>> 
>>>> then you get code that is slower :) but it should be easier to read,
>>>> since the interface between the individual pieces is always the same.
>>>> Debug shouldn't be a fast path anyway, right?
>>> 
>>> Frankly speaking I do not see much difference :).
>>> 
>>> If we have to do as you mentioned then I think we can just do
>>> 
>>> KVM_DBG_HANDLER:
>>> 	<debug specific handling>
>>> 	1:
>>> 	mtcr		r3
>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>>> 	lwz		r4, \scratch
>>> 	<KVM_HANDLER>
>> 
>> Whatever it takes to keep the oddball (debug) an oddball and keep the normal
>> case easy :).
> 
> I think there will be another problem as  the kvmppc_handler_\ivor_nr will not be the starting address which is required as per our ivor/ivpr usages for booke architecture.
> 
> I am thinking of keeping as is :).

How about we take a hybrid approach? You write the code as I described above, but call __KVM_HANDLER at the end. The normal KVM_HANDLER would look like:

KVM_HANDLER:
	kvmppc_handler_\ivor_nr:
	__KVM_HANDLER ...

That way the code should still be more understandable :)


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 - Feb. 1, 2013, 9:07 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, February 01, 2013 1:36 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> 
> 
> On 01.02.2013, at 06:04, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: kvm-ppc-owner@vger.kernel.org
> >> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
> >> Sent: Thursday, January 31, 2013 10:38 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> >> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>
> >>
> >> On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>> Sent: Thursday, January 31, 2013 5:47 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> >>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>>>
> >>>>
> >>>> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>>> Sent: Friday, January 25, 2013 5:13 PM
> >>>>>> To: Bhushan Bharat-R65777
> >>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
> >>>>>> Bharat-R65777
> >>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>>>>>
> >>>>>>
> >>>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
> >>>>>>
> >>>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>>>
> >>>>>>> Installed debug handler will be used for guest debug support and
> >>>>>>> debug facility emulation features (patches for these features
> >>>>>>> will follow this patch).
> >>>>>>>
> >>>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> >>>>>>> [bharat.bhushan@freescale.com: Substantial changes]
> >>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> >>>>>>> ---
> >>>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
> >>>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
> >>>>>>> arch/powerpc/kvm/booke_interrupts.S |   49
> ++++++++++++++++++++++++++++++-
> >> --
> >>>> --
> >>>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
> >>>>>>> b/arch/powerpc/include/asm/kvm_host.h
> >>>>>>> index 8a72d59..f4ba881 100644
> >>>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
> >>>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
> >>>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
> >>>>>>> 	u32 tlbcfg[4];
> >>>>>>> 	u32 mmucfg;
> >>>>>>> 	u32 epr;
> >>>>>>> +	u32 crit_save;
> >>>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
> >>>>>>> 	gpa_t paddr_accessed;
> >>>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
> >>>>>>> b/arch/powerpc/kernel/asm-offsets.c
> >>>>>>> index 46f6afd..02048f3 100644
> >>>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
> >>>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
> >>>>>>> @@ -562,6 +562,7 @@ int main(void)
> >>>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> >>>>>>> 	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));
> >>>>>>> #endif /* CONFIG_PPC_BOOK3S */
> >>>>>>> #endif /* CONFIG_KVM */
> >>>>>>>
> >>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>> index eae8483..dd9c5d4 100644
> >>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>> @@ -52,12 +52,7 @@
> >>>>>>>                     (1<<BOOKE_INTERRUPT_PROGRAM) | \
> >>>>>>>                     (1<<BOOKE_INTERRUPT_DTLB_MISS))
> >>>>>>>
> >>>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
> >>>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>> -	/* Get pointer to vcpu and record exit number. */
> >>>>>>> -	mtspr	\scratch , r4
> >>>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
> >>>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
> >>>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
> >>>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
> >>>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
> >>>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>> 	bctr
> >>>>>>> .endm
> >>>>>>>
> >>>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
> >>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>> +	/* Get pointer to vcpu and record exit number. */
> >>>>>>> +	mtspr	\scratch , r4
> >>>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
> >>>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>>>> +
> >>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>> +	mtspr   \scratch, r4
> >>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
> >>>>>>> +	mfcr	r3
> >>>>>>> +	mfspr	r4, SPRN_CSRR1
> >>>>>>> +	andi.	r4, r4, MSR_PR
> >>>>>>> +	bne	1f
> >>>>>>
> >>>>>>
> >>>>>>> +	/* debug interrupt happened in enter/exit path */
> >>>>>>> +	mfspr   r4, SPRN_CSRR1
> >>>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
> >>>>>>> +	mtspr   SPRN_CSRR1, r4
> >>>>>>> +	lis	r4, 0xffff
> >>>>>>> +	ori	r4, r4, 0xffff
> >>>>>>> +	mtspr	SPRN_DBSR, r4
> >>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>> +	mtcr	r3
> >>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>>>> +	mfspr   r4, \scratch
> >>>>>>> +	rfci
> >>>>>>
> >>>>>> What is this part doing? Try to ignore the debug exit?
> >>>>>
> >>>>> As BOOKE doesn't have hardware support for virtualization,
> >>>>> hardware never know
> >>>> current pc is in guest or in host.
> >>>>> So when enable hardware single step for guest, it cannot be
> >>>>> disabled at the
> >>>> time guest exit. Thus, we'll see that an single step interrupt
> >>>> happens at the beginning of guest exit path.
> >>>>>
> >>>>> With the above code we recognize this kind of single step
> >>>>> interrupt disable
> >>>> single step and rfci.
> >>>>>
> >>>>>> Why would we have MSR_DE
> >>>>>> enabled in the first place when we can't handle it?
> >>>>>
> >>>>> When QEMU is using hardware debug resource then we always set
> >>>>> MSR_DE during
> >>>> guest is running.
> >>>>
> >>>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE
> >>>> wasn't set, you wouldn't get a single step exit.
> >>>
> >>> We always set MSR_DE in hw MSR when qemu using the debug resource.
> >>
> >> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be
> >> set anymore, because we're in an interrupt handler, no? Or is MSR_DE
> >> kept alive on interrupts?
> >>
> >>>
> >>>> During the exit code path, you could then swap DBSR back to what
> >>>> the host expects (which means no single step). Only after that
> >>>> enable MSR_DE again.
> >>>
> >>> We do not support deferred debug interrupt, so we do save restore dbsr.
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +1:	/* debug interrupt happened in guest */
> >>>>>>> +	mtcr	r3
> >>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
> >>>>>>
> >>>>>> I don't think you need the __KVM_HANDLER split. This should be
> >>>>>> quite easily refactorable into a simple DBG prolog.
> >>>>>
> >>>>> Can you please elaborate how you are envisioning this?
> >>>>
> >>>> With this patch, you have
> >>>>
> >>>> KVM_HANLDER:
> >>>>
> >>>> <code>
> >>>> __KVM_HANDLER
> >>>>
> >>>> KVM_DBG_HANDLER:
> >>>>
> >>>> <code>
> >>>> __KVM_HANDLER
> >>>>
> >>>> Right?
> >>>>
> >>>> In KVM_HANDLER, you get:
> >>>>
> >>>>> .macro KVM_HANDLER ivor_nr scratch srr0
> >>>>> _GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>> 	/* Get pointer to vcpu and record exit number. */
> >>>>> 	mtspr	\scratch , r4
> >>>>> 	mfspr   r4, SPRN_SPRG_THREAD
> >>>>> 	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>
> >>>>
> >>>> while KVM_DBG_HANDLER is:
> >>>>
> >>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>> <debug specific handling>
> >>>>> +1:	/* debug interrupt happened in guest */
> >>>>> +	mtcr	r3
> >>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>
> >>>>
> >>>> So if you write this as
> >>>>
> >>>> KVM_DBG_HANDLER:
> >>>> 	<debug specific handling>
> >>>> 	1:
> >>>> 	mtcr		r3
> >>>> 	mfspr	r4, SPRN_SPRG_THREAD
> >>>> 	lwz		r4, THREAD_KVM_VCPU(r4)
> >>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> >>>> 	lwz		r4, \scratch
> >>>> 	<KVM_HANDLER>
> >>>>
> >>>> then you get code that is slower :) but it should be easier to
> >>>> read, since the interface between the individual pieces is always the same.
> >>>> Debug shouldn't be a fast path anyway, right?
> >>>
> >>> Frankly speaking I do not see much difference :).
> >>>
> >>> If we have to do as you mentioned then I think we can just do
> >>>
> >>> KVM_DBG_HANDLER:
> >>> 	<debug specific handling>
> >>> 	1:
> >>> 	mtcr		r3
> >>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> >>> 	lwz		r4, \scratch
> >>> 	<KVM_HANDLER>
> >>
> >> Whatever it takes to keep the oddball (debug) an oddball and keep the
> >> normal case easy :).
> >
> > I think there will be another problem as  the kvmppc_handler_\ivor_nr will not
> be the starting address which is required as per our ivor/ivpr usages for booke
> architecture.
> >
> > I am thinking of keeping as is :).
> 
> How about we take a hybrid approach? You write the code as I described above,
> but call __KVM_HANDLER at the end. The normal KVM_HANDLER would look like:
> 
> KVM_HANDLER:
> 	kvmppc_handler_\ivor_nr:
> 	__KVM_HANDLER ...
> 
> That way the code should still be more understandable :)
> 

With my current Patch it is defined as:

.macro KVM_HANDLER ivor_nr scratch srr0
_GLOBAL(kvmppc_handler_\ivor_nr)
        /* Get pointer to vcpu and record exit number. */
        mtspr   \scratch , r4
        mfspr   r4, SPRN_SPRG_THREAD
        lwz     r4, THREAD_KVM_VCPU(r4)
        __KVM_HANDLER \ivor_nr \scratch \srr0
.endm

.macro KVM_DBG_HANDLER ivor_nr scratch srr0
_GLOBAL(kvmppc_handler_\ivor_nr)

<<<<<<Debug related handling>>>>>

1:      /* debug interrupt happened in guest */
        mtcr    r3
        mfspr   r4, SPRN_SPRG_THREAD
        lwz     r4, THREAD_KVM_VCPU(r4)
        lwz     r3, VCPU_CRIT_SAVE(r4)
        __KVM_HANDLER \ivor_nr \scratch \srr0
.endm

So the kvmppc_handler_\ivor_nr is defined and should always be at the start of exception handling?

So if KVM_DBG_HANDLER need to call KVM_HANDLER then there will be issue of 2 definition for DBG interrupt.

I am sorry but I did not understood how you want this to define. Can you please describe ?

Thanks
-Bharat


--
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 - Feb. 7, 2013, 2:21 p.m.
On 01.02.2013, at 10:07, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Friday, February 01, 2013 1:36 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>> 
>> 
>> On 01.02.2013, at 06:04, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: kvm-ppc-owner@vger.kernel.org
>>>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
>>>> Sent: Thursday, January 31, 2013 10:38 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>> 
>>>> 
>>>> On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote:
>>>> 
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Thursday, January 31, 2013 5:47 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>>>> 
>>>>>> 
>>>>>> On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>>> -----Original Message-----
>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>> Sent: Friday, January 25, 2013 5:13 PM
>>>>>>>> To: Bhushan Bharat-R65777
>>>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
>>>>>>>> Bharat-R65777
>>>>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
>>>>>>>> 
>>>>>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>>> 
>>>>>>>>> Installed debug handler will be used for guest debug support and
>>>>>>>>> debug facility emulation features (patches for these features
>>>>>>>>> will follow this patch).
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>>>>>>>> [bharat.bhushan@freescale.com: Substantial changes]
>>>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>>>>> ---
>>>>>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
>>>>>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
>>>>>>>>> arch/powerpc/kvm/booke_interrupts.S |   49
>> ++++++++++++++++++++++++++++++-
>>>> --
>>>>>> --
>>>>>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>>>>>>>> 
>>>>>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>> index 8a72d59..f4ba881 100644
>>>>>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
>>>>>>>>> 	u32 tlbcfg[4];
>>>>>>>>> 	u32 mmucfg;
>>>>>>>>> 	u32 epr;
>>>>>>>>> +	u32 crit_save;
>>>>>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
>>>>>>>>> 	gpa_t paddr_accessed;
>>>>>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>> b/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>> index 46f6afd..02048f3 100644
>>>>>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>> @@ -562,6 +562,7 @@ int main(void)
>>>>>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
>>>>>>>>> 	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));
>>>>>>>>> #endif /* CONFIG_PPC_BOOK3S */
>>>>>>>>> #endif /* CONFIG_KVM */
>>>>>>>>> 
>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>> index eae8483..dd9c5d4 100644
>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>> @@ -52,12 +52,7 @@
>>>>>>>>>                    (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>>>>>                    (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>>>>>> 
>>>>>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>> -	/* Get pointer to vcpu and record exit number. */
>>>>>>>>> -	mtspr	\scratch , r4
>>>>>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>>>>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>>>>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
>>>>>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>> 	bctr
>>>>>>>>> .endm
>>>>>>>>> 
>>>>>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>> +	/* Get pointer to vcpu and record exit number. */
>>>>>>>>> +	mtspr	\scratch , r4
>>>>>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>>>>> +
>>>>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>> +	mtspr   \scratch, r4
>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>> +	mfcr	r3
>>>>>>>>> +	mfspr	r4, SPRN_CSRR1
>>>>>>>>> +	andi.	r4, r4, MSR_PR
>>>>>>>>> +	bne	1f
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> +	/* debug interrupt happened in enter/exit path */
>>>>>>>>> +	mfspr   r4, SPRN_CSRR1
>>>>>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
>>>>>>>>> +	mtspr   SPRN_CSRR1, r4
>>>>>>>>> +	lis	r4, 0xffff
>>>>>>>>> +	ori	r4, r4, 0xffff
>>>>>>>>> +	mtspr	SPRN_DBSR, r4
>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> +	mtcr	r3
>>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>> +	mfspr   r4, \scratch
>>>>>>>>> +	rfci
>>>>>>>> 
>>>>>>>> What is this part doing? Try to ignore the debug exit?
>>>>>>> 
>>>>>>> As BOOKE doesn't have hardware support for virtualization,
>>>>>>> hardware never know
>>>>>> current pc is in guest or in host.
>>>>>>> So when enable hardware single step for guest, it cannot be
>>>>>>> disabled at the
>>>>>> time guest exit. Thus, we'll see that an single step interrupt
>>>>>> happens at the beginning of guest exit path.
>>>>>>> 
>>>>>>> With the above code we recognize this kind of single step
>>>>>>> interrupt disable
>>>>>> single step and rfci.
>>>>>>> 
>>>>>>>> Why would we have MSR_DE
>>>>>>>> enabled in the first place when we can't handle it?
>>>>>>> 
>>>>>>> When QEMU is using hardware debug resource then we always set
>>>>>>> MSR_DE during
>>>>>> guest is running.
>>>>>> 
>>>>>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE
>>>>>> wasn't set, you wouldn't get a single step exit.
>>>>> 
>>>>> We always set MSR_DE in hw MSR when qemu using the debug resource.
>>>> 
>>>> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be
>>>> set anymore, because we're in an interrupt handler, no? Or is MSR_DE
>>>> kept alive on interrupts?
>>>> 
>>>>> 
>>>>>> During the exit code path, you could then swap DBSR back to what
>>>>>> the host expects (which means no single step). Only after that
>>>>>> enable MSR_DE again.
>>>>> 
>>>>> We do not support deferred debug interrupt, so we do save restore dbsr.
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>>> 
>>>>>>>>> +1:	/* debug interrupt happened in guest */
>>>>>>>>> +	mtcr	r3
>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
>>>>>>>> 
>>>>>>>> I don't think you need the __KVM_HANDLER split. This should be
>>>>>>>> quite easily refactorable into a simple DBG prolog.
>>>>>>> 
>>>>>>> Can you please elaborate how you are envisioning this?
>>>>>> 
>>>>>> With this patch, you have
>>>>>> 
>>>>>> KVM_HANLDER:
>>>>>> 
>>>>>> <code>
>>>>>> __KVM_HANDLER
>>>>>> 
>>>>>> KVM_DBG_HANDLER:
>>>>>> 
>>>>>> <code>
>>>>>> __KVM_HANDLER
>>>>>> 
>>>>>> Right?
>>>>>> 
>>>>>> In KVM_HANDLER, you get:
>>>>>> 
>>>>>>> .macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>> _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>> 	/* Get pointer to vcpu and record exit number. */
>>>>>>> 	mtspr	\scratch , r4
>>>>>>> 	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>> 	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>> 
>>>>>> 
>>>>>> while KVM_DBG_HANDLER is:
>>>>>> 
>>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>> <debug specific handling>
>>>>>>> +1:	/* debug interrupt happened in guest */
>>>>>>> +	mtcr	r3
>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>> 
>>>>>> 
>>>>>> So if you write this as
>>>>>> 
>>>>>> KVM_DBG_HANDLER:
>>>>>> 	<debug specific handling>
>>>>>> 	1:
>>>>>> 	mtcr		r3
>>>>>> 	mfspr	r4, SPRN_SPRG_THREAD
>>>>>> 	lwz		r4, THREAD_KVM_VCPU(r4)
>>>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>>>>>> 	lwz		r4, \scratch
>>>>>> 	<KVM_HANDLER>
>>>>>> 
>>>>>> then you get code that is slower :) but it should be easier to
>>>>>> read, since the interface between the individual pieces is always the same.
>>>>>> Debug shouldn't be a fast path anyway, right?
>>>>> 
>>>>> Frankly speaking I do not see much difference :).
>>>>> 
>>>>> If we have to do as you mentioned then I think we can just do
>>>>> 
>>>>> KVM_DBG_HANDLER:
>>>>> 	<debug specific handling>
>>>>> 	1:
>>>>> 	mtcr		r3
>>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>>>>> 	lwz		r4, \scratch
>>>>> 	<KVM_HANDLER>
>>>> 
>>>> Whatever it takes to keep the oddball (debug) an oddball and keep the
>>>> normal case easy :).
>>> 
>>> I think there will be another problem as  the kvmppc_handler_\ivor_nr will not
>> be the starting address which is required as per our ivor/ivpr usages for booke
>> architecture.
>>> 
>>> I am thinking of keeping as is :).
>> 
>> How about we take a hybrid approach? You write the code as I described above,
>> but call __KVM_HANDLER at the end. The normal KVM_HANDLER would look like:
>> 
>> KVM_HANDLER:
>> 	kvmppc_handler_\ivor_nr:
>> 	__KVM_HANDLER ...
>> 
>> That way the code should still be more understandable :)
>> 
> 
> With my current Patch it is defined as:
> 
> .macro KVM_HANDLER ivor_nr scratch srr0
> _GLOBAL(kvmppc_handler_\ivor_nr)
>        /* Get pointer to vcpu and record exit number. */
>        mtspr   \scratch , r4
>        mfspr   r4, SPRN_SPRG_THREAD
>        lwz     r4, THREAD_KVM_VCPU(r4)

Move these into __KVM_HANDLER (aka: keep the code in there the same as KVM_HANDLER today)

>        __KVM_HANDLER \ivor_nr \scratch \srr0
> .endm
> 
> .macro KVM_DBG_HANDLER ivor_nr scratch srr0
> _GLOBAL(kvmppc_handler_\ivor_nr)
> 
> <<<<<<Debug related handling>>>>>
> 
> 1:      /* debug interrupt happened in guest */
>        mtcr    r3
>        mfspr   r4, SPRN_SPRG_THREAD
>        lwz     r4, THREAD_KVM_VCPU(r4)
>        lwz     r3, VCPU_CRIT_SAVE(r4)

Restore the state here as if a non-debug interrupt occurred. __KVM_HANDLER will fetch r4 itself from SPRG_THREAD.

I'm basically advocating to not optimize the debug case at all. Instead, I would prefer to have the exception ABI be identical to the fallback case ABI. That way we don't have to worry about 4 code paths, but only about 3, keeping the complexity of the code low.


Alex

>        __KVM_HANDLER \ivor_nr \scratch \srr0
> .endm
> 
> So the kvmppc_handler_\ivor_nr is defined and should always be at the start of exception handling?
> 
> So if KVM_DBG_HANDLER need to call KVM_HANDLER then there will be issue of 2 definition for DBG interrupt.
> 
> I am sorry but I did not understood how you want this to define. Can you please describe ?
> 
> Thanks
> -Bharat
> 
> 

--
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 - Feb. 7, 2013, 2:48 p.m.
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>>>>> Sent: Friday, January 25, 2013 5:13 PM
> >>>>>>>> To: Bhushan Bharat-R65777
> >>>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
> >>>>>>>> Bharat-R65777
> >>>>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
> >>>>>>>>
> >>>>>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>>>>>
> >>>>>>>>> Installed debug handler will be used for guest debug support
> >>>>>>>>> and debug facility emulation features (patches for these
> >>>>>>>>> features will follow this patch).
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> >>>>>>>>> [bharat.bhushan@freescale.com: Substantial changes]
> >>>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> >>>>>>>>> ---
> >>>>>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
> >>>>>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
> >>>>>>>>> arch/powerpc/kvm/booke_interrupts.S |   49
> >> ++++++++++++++++++++++++++++++-
> >>>> --
> >>>>>> --
> >>>>>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
> >>>>>>>>> b/arch/powerpc/include/asm/kvm_host.h
> >>>>>>>>> index 8a72d59..f4ba881 100644
> >>>>>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
> >>>>>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
> >>>>>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
> >>>>>>>>> 	u32 tlbcfg[4];
> >>>>>>>>> 	u32 mmucfg;
> >>>>>>>>> 	u32 epr;
> >>>>>>>>> +	u32 crit_save;
> >>>>>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
> >>>>>>>>> 	gpa_t paddr_accessed;
> >>>>>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
> >>>>>>>>> b/arch/powerpc/kernel/asm-offsets.c
> >>>>>>>>> index 46f6afd..02048f3 100644
> >>>>>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
> >>>>>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
> >>>>>>>>> @@ -562,6 +562,7 @@ int main(void)
> >>>>>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> >>>>>>>>> 	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));
> >>>>>>>>> #endif /* CONFIG_PPC_BOOK3S */ #endif /* CONFIG_KVM */
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>> index eae8483..dd9c5d4 100644
> >>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>> @@ -52,12 +52,7 @@
> >>>>>>>>>                    (1<<BOOKE_INTERRUPT_PROGRAM) | \
> >>>>>>>>>                    (1<<BOOKE_INTERRUPT_DTLB_MISS))
> >>>>>>>>>
> >>>>>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
> >>>>>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>>>> -	/* Get pointer to vcpu and record exit number. */
> >>>>>>>>> -	mtspr	\scratch , r4
> >>>>>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
> >>>>>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
> >>>>>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
> >>>>>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
> >>>>>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
> >>>>>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>>>> 	bctr
> >>>>>>>>> .endm
> >>>>>>>>>
> >>>>>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
> >>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>>>> +	/* Get pointer to vcpu and record exit number. */
> >>>>>>>>> +	mtspr	\scratch , r4
> >>>>>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
> >>>>>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>>>>>> +
> >>>>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>>>> +	mtspr   \scratch, r4
> >>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
> >>>>>>>>> +	mfcr	r3
> >>>>>>>>> +	mfspr	r4, SPRN_CSRR1
> >>>>>>>>> +	andi.	r4, r4, MSR_PR
> >>>>>>>>> +	bne	1f
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> +	/* debug interrupt happened in enter/exit path */
> >>>>>>>>> +	mfspr   r4, SPRN_CSRR1
> >>>>>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
> >>>>>>>>> +	mtspr   SPRN_CSRR1, r4
> >>>>>>>>> +	lis	r4, 0xffff
> >>>>>>>>> +	ori	r4, r4, 0xffff
> >>>>>>>>> +	mtspr	SPRN_DBSR, r4
> >>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>>>> +	mtcr	r3
> >>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>>>>>> +	mfspr   r4, \scratch
> >>>>>>>>> +	rfci
> >>>>>>>>
> >>>>>>>> What is this part doing? Try to ignore the debug exit?
> >>>>>>>
> >>>>>>> As BOOKE doesn't have hardware support for virtualization,
> >>>>>>> hardware never know
> >>>>>> current pc is in guest or in host.
> >>>>>>> So when enable hardware single step for guest, it cannot be
> >>>>>>> disabled at the
> >>>>>> time guest exit. Thus, we'll see that an single step interrupt
> >>>>>> happens at the beginning of guest exit path.
> >>>>>>>
> >>>>>>> With the above code we recognize this kind of single step
> >>>>>>> interrupt disable
> >>>>>> single step and rfci.
> >>>>>>>
> >>>>>>>> Why would we have MSR_DE
> >>>>>>>> enabled in the first place when we can't handle it?
> >>>>>>>
> >>>>>>> When QEMU is using hardware debug resource then we always set
> >>>>>>> MSR_DE during
> >>>>>> guest is running.
> >>>>>>
> >>>>>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE
> >>>>>> wasn't set, you wouldn't get a single step exit.
> >>>>>
> >>>>> We always set MSR_DE in hw MSR when qemu using the debug resource.
> >>>>
> >>>> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't
> >>>> be set anymore, because we're in an interrupt handler, no? Or is
> >>>> MSR_DE kept alive on interrupts?
> >>>>
> >>>>>
> >>>>>> During the exit code path, you could then swap DBSR back to what
> >>>>>> the host expects (which means no single step). Only after that
> >>>>>> enable MSR_DE again.
> >>>>>
> >>>>> We do not support deferred debug interrupt, so we do save restore dbsr.
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> +1:	/* debug interrupt happened in guest */
> >>>>>>>>> +	mtcr	r3
> >>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
> >>>>>>>>
> >>>>>>>> I don't think you need the __KVM_HANDLER split. This should be
> >>>>>>>> quite easily refactorable into a simple DBG prolog.
> >>>>>>>
> >>>>>>> Can you please elaborate how you are envisioning this?
> >>>>>>
> >>>>>> With this patch, you have
> >>>>>>
> >>>>>> KVM_HANLDER:
> >>>>>>
> >>>>>> <code>
> >>>>>> __KVM_HANDLER
> >>>>>>
> >>>>>> KVM_DBG_HANDLER:
> >>>>>>
> >>>>>> <code>
> >>>>>> __KVM_HANDLER
> >>>>>>
> >>>>>> Right?
> >>>>>>
> >>>>>> In KVM_HANDLER, you get:
> >>>>>>
> >>>>>>> .macro KVM_HANDLER ivor_nr scratch srr0
> >>>>>>> _GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>> 	/* Get pointer to vcpu and record exit number. */
> >>>>>>> 	mtspr	\scratch , r4
> >>>>>>> 	mfspr   r4, SPRN_SPRG_THREAD
> >>>>>>> 	lwz     r4, THREAD_KVM_VCPU(r4)
> >>>>>>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>>>
> >>>>>>
> >>>>>> while KVM_DBG_HANDLER is:
> >>>>>>
> >>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
> >>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
> >>>>>>> <debug specific handling>
> >>>>>>> +1:	/* debug interrupt happened in guest */
> >>>>>>> +	mtcr	r3
> >>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
> >>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
> >>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >>>>>>
> >>>>>>
> >>>>>> So if you write this as
> >>>>>>
> >>>>>> KVM_DBG_HANDLER:
> >>>>>> 	<debug specific handling>
> >>>>>> 	1:
> >>>>>> 	mtcr		r3
> >>>>>> 	mfspr	r4, SPRN_SPRG_THREAD
> >>>>>> 	lwz		r4, THREAD_KVM_VCPU(r4)
> >>>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> >>>>>> 	lwz		r4, \scratch
> >>>>>> 	<KVM_HANDLER>
> >>>>>>
> >>>>>> then you get code that is slower :) but it should be easier to
> >>>>>> read, since the interface between the individual pieces is always the
> same.
> >>>>>> Debug shouldn't be a fast path anyway, right?
> >>>>>
> >>>>> Frankly speaking I do not see much difference :).
> >>>>>
> >>>>> If we have to do as you mentioned then I think we can just do
> >>>>>
> >>>>> KVM_DBG_HANDLER:
> >>>>> 	<debug specific handling>
> >>>>> 	1:
> >>>>> 	mtcr		r3
> >>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
> >>>>> 	lwz		r4, \scratch
> >>>>> 	<KVM_HANDLER>
> >>>>
> >>>> Whatever it takes to keep the oddball (debug) an oddball and keep
> >>>> the normal case easy :).
> >>>
> >>> I think there will be another problem as  the
> >>> kvmppc_handler_\ivor_nr will not
> >> be the starting address which is required as per our ivor/ivpr usages
> >> for booke architecture.
> >>>
> >>> I am thinking of keeping as is :).
> >>
> >> How about we take a hybrid approach? You write the code as I
> >> described above, but call __KVM_HANDLER at the end. The normal KVM_HANDLER
> would look like:
> >>
> >> KVM_HANDLER:
> >> 	kvmppc_handler_\ivor_nr:
> >> 	__KVM_HANDLER ...
> >>
> >> That way the code should still be more understandable :)
> >>
> >
> > With my current Patch it is defined as:
> >
> > .macro KVM_HANDLER ivor_nr scratch srr0
> > _GLOBAL(kvmppc_handler_\ivor_nr)
> >        /* Get pointer to vcpu and record exit number. */
> >        mtspr   \scratch , r4
> >        mfspr   r4, SPRN_SPRG_THREAD
> >        lwz     r4, THREAD_KVM_VCPU(r4)
> 
> Move these into __KVM_HANDLER (aka: keep the code in there the same as
> KVM_HANDLER today)
> 
> >        __KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >
> > .macro KVM_DBG_HANDLER ivor_nr scratch srr0
> > _GLOBAL(kvmppc_handler_\ivor_nr)
> >
> > <<<<<<Debug related handling>>>>>
> >
> > 1:      /* debug interrupt happened in guest */
> >        mtcr    r3
> >        mfspr   r4, SPRN_SPRG_THREAD
> >        lwz     r4, THREAD_KVM_VCPU(r4)
> >        lwz     r3, VCPU_CRIT_SAVE(r4)
> 
> Restore the state here as if a non-debug interrupt occurred. __KVM_HANDLER will
> fetch r4 itself from SPRG_THREAD.
> 
> I'm basically advocating to not optimize the debug case at all. Instead, I would
> prefer to have the exception ABI be identical to the fallback case ABI. That way
> we don't have to worry about 4 code paths, but only about 3, keeping the
> complexity of the code low.
> 
> 
> Alex
> 
> >        __KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> >

Do you mean something like this ?

.macro __KVM_HANDLER
+        /* Get pointer to vcpu and record exit number. */
+        mtspr   \scratch , r4
+        mfspr   r4, SPRN_SPRG_THREAD
+        lwz     r4, THREAD_KVM_VCPU(r4)
 << Existing code >>


.macro KVM_HANDLER ivor_nr scratch srr0
_GLOBAL(kvmppc_handler_\ivor_nr)
	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm




.macro KVM_DBG_HANDLER ivor_nr scratch srr0
_GLOBAL(kvmppc_handler_\ivor_nr)
<<<<<<Debug related handling>>>>>
1:      /* debug interrupt happened in guest */
        mtcr    r3
        mfspr   r4, SPRN_SPRG_THREAD
        lwz     r4, THREAD_KVM_VCPU(r4)
        lwz     r3, VCPU_CRIT_SAVE(r4)
	 lwz		r4, \scratch 
        __KVM_HANDLER \ivor_nr \scratch \srr0 .endm


Thanks
-Bharat

> > So the kvmppc_handler_\ivor_nr is defined and should always be at the start of
> exception handling?
> >
> > So if KVM_DBG_HANDLER need to call KVM_HANDLER then there will be issue of 2
> definition for DBG interrupt.
> >
> > I am sorry but I did not understood how you want this to define. Can you
> please describe ?
> >
> > Thanks
> > -Bharat
> >
> >
> 
> --
> 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 - Feb. 7, 2013, 3:01 p.m.
On 07.02.2013, at 15:48, Bhushan Bharat-R65777 wrote:

>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>>>> Sent: Friday, January 25, 2013 5:13 PM
>>>>>>>>>> To: Bhushan Bharat-R65777
>>>>>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan
>>>>>>>>>> Bharat-R65777
>>>>>>>>>> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote:
>>>>>>>>>> 
>>>>>>>>>>> From: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>>>>> 
>>>>>>>>>>> Installed debug handler will be used for guest debug support
>>>>>>>>>>> and debug facility emulation features (patches for these
>>>>>>>>>>> features will follow this patch).
>>>>>>>>>>> 
>>>>>>>>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>>>>>>>>>> [bharat.bhushan@freescale.com: Substantial changes]
>>>>>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>>>>>>> ---
>>>>>>>>>>> arch/powerpc/include/asm/kvm_host.h |    1 +
>>>>>>>>>>> arch/powerpc/kernel/asm-offsets.c   |    1 +
>>>>>>>>>>> arch/powerpc/kvm/booke_interrupts.S |   49
>>>> ++++++++++++++++++++++++++++++-
>>>>>> --
>>>>>>>> --
>>>>>>>>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>>>>>>>>>> 
>>>>>>>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>>>> index 8a72d59..f4ba881 100644
>>>>>>>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>>>>>>>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch {
>>>>>>>>>>> 	u32 tlbcfg[4];
>>>>>>>>>>> 	u32 mmucfg;
>>>>>>>>>>> 	u32 epr;
>>>>>>>>>>> +	u32 crit_save;
>>>>>>>>>>> 	struct kvmppc_booke_debug_reg dbg_reg; #endif
>>>>>>>>>>> 	gpa_t paddr_accessed;
>>>>>>>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>>>> b/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>>>> index 46f6afd..02048f3 100644
>>>>>>>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>>>>>>>> @@ -562,6 +562,7 @@ int main(void)
>>>>>>>>>>> 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
>>>>>>>>>>> 	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));
>>>>>>>>>>> #endif /* CONFIG_PPC_BOOK3S */ #endif /* CONFIG_KVM */
>>>>>>>>>>> 
>>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> index eae8483..dd9c5d4 100644
>>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> @@ -52,12 +52,7 @@
>>>>>>>>>>>                   (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>>>>>>>                   (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>>>>>>>> 
>>>>>>>>>>> -.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>>>> -_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>>>> -	/* Get pointer to vcpu and record exit number. */
>>>>>>>>>>> -	mtspr	\scratch , r4
>>>>>>>>>>> -	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>>>>>> -	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>>>>>> +.macro __KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>>>>>>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>>>>>>>>>> 	stw	r6, VCPU_GPR(R6)(r4)
>>>>>>>>>>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>>>> 	bctr
>>>>>>>>>>> .endm
>>>>>>>>>>> 
>>>>>>>>>>> +.macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>>>> +	/* Get pointer to vcpu and record exit number. */
>>>>>>>>>>> +	mtspr	\scratch , r4
>>>>>>>>>>> +	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>>>>>> +	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>>>>>>> +
>>>>>>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>>>> +	mtspr   \scratch, r4
>>>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>>>> +	stw	r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>>>> +	mfcr	r3
>>>>>>>>>>> +	mfspr	r4, SPRN_CSRR1
>>>>>>>>>>> +	andi.	r4, r4, MSR_PR
>>>>>>>>>>> +	bne	1f
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> +	/* debug interrupt happened in enter/exit path */
>>>>>>>>>>> +	mfspr   r4, SPRN_CSRR1
>>>>>>>>>>> +	rlwinm  r4, r4, 0, ~MSR_DE
>>>>>>>>>>> +	mtspr   SPRN_CSRR1, r4
>>>>>>>>>>> +	lis	r4, 0xffff
>>>>>>>>>>> +	ori	r4, r4, 0xffff
>>>>>>>>>>> +	mtspr	SPRN_DBSR, r4
>>>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>>>> +	mtcr	r3
>>>>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>>>> +	mfspr   r4, \scratch
>>>>>>>>>>> +	rfci
>>>>>>>>>> 
>>>>>>>>>> What is this part doing? Try to ignore the debug exit?
>>>>>>>>> 
>>>>>>>>> As BOOKE doesn't have hardware support for virtualization,
>>>>>>>>> hardware never know
>>>>>>>> current pc is in guest or in host.
>>>>>>>>> So when enable hardware single step for guest, it cannot be
>>>>>>>>> disabled at the
>>>>>>>> time guest exit. Thus, we'll see that an single step interrupt
>>>>>>>> happens at the beginning of guest exit path.
>>>>>>>>> 
>>>>>>>>> With the above code we recognize this kind of single step
>>>>>>>>> interrupt disable
>>>>>>>> single step and rfci.
>>>>>>>>> 
>>>>>>>>>> Why would we have MSR_DE
>>>>>>>>>> enabled in the first place when we can't handle it?
>>>>>>>>> 
>>>>>>>>> When QEMU is using hardware debug resource then we always set
>>>>>>>>> MSR_DE during
>>>>>>>> guest is running.
>>>>>>>> 
>>>>>>>> Right, but why is MSR_DE enabled during the exit path? If MSR_DE
>>>>>>>> wasn't set, you wouldn't get a single step exit.
>>>>>>> 
>>>>>>> We always set MSR_DE in hw MSR when qemu using the debug resource.
>>>>>> 
>>>>>> In the _guest_ MSR, yes. But once we exit the guest, it shouldn't
>>>>>> be set anymore, because we're in an interrupt handler, no? Or is
>>>>>> MSR_DE kept alive on interrupts?
>>>>>> 
>>>>>>> 
>>>>>>>> During the exit code path, you could then swap DBSR back to what
>>>>>>>> the host expects (which means no single step). Only after that
>>>>>>>> enable MSR_DE again.
>>>>>>> 
>>>>>>> We do not support deferred debug interrupt, so we do save restore dbsr.
>>>>>>> 
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> +1:	/* debug interrupt happened in guest */
>>>>>>>>>>> +	mtcr	r3
>>>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0
>>>>>>>>>> 
>>>>>>>>>> I don't think you need the __KVM_HANDLER split. This should be
>>>>>>>>>> quite easily refactorable into a simple DBG prolog.
>>>>>>>>> 
>>>>>>>>> Can you please elaborate how you are envisioning this?
>>>>>>>> 
>>>>>>>> With this patch, you have
>>>>>>>> 
>>>>>>>> KVM_HANLDER:
>>>>>>>> 
>>>>>>>> <code>
>>>>>>>> __KVM_HANDLER
>>>>>>>> 
>>>>>>>> KVM_DBG_HANDLER:
>>>>>>>> 
>>>>>>>> <code>
>>>>>>>> __KVM_HANDLER
>>>>>>>> 
>>>>>>>> Right?
>>>>>>>> 
>>>>>>>> In KVM_HANDLER, you get:
>>>>>>>> 
>>>>>>>>> .macro KVM_HANDLER ivor_nr scratch srr0
>>>>>>>>> _GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>> 	/* Get pointer to vcpu and record exit number. */
>>>>>>>>> 	mtspr	\scratch , r4
>>>>>>>>> 	mfspr   r4, SPRN_SPRG_THREAD
>>>>>>>>> 	lwz     r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>>>> 
>>>>>>>> 
>>>>>>>> while KVM_DBG_HANDLER is:
>>>>>>>> 
>>>>>>>>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>>>>>>>> +_GLOBAL(kvmppc_handler_\ivor_nr)
>>>>>>>>> <debug specific handling>
>>>>>>>>> +1:	/* debug interrupt happened in guest */
>>>>>>>>> +	mtcr	r3
>>>>>>>>> +	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>>> +	lwz	r4, THREAD_KVM_VCPU(r4)
>>>>>>>>> +	lwz     r3, VCPU_CRIT_SAVE(r4)
>>>>>>>>> +	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>>>>>>> 
>>>>>>>> 
>>>>>>>> So if you write this as
>>>>>>>> 
>>>>>>>> KVM_DBG_HANDLER:
>>>>>>>> 	<debug specific handling>
>>>>>>>> 	1:
>>>>>>>> 	mtcr		r3
>>>>>>>> 	mfspr	r4, SPRN_SPRG_THREAD
>>>>>>>> 	lwz		r4, THREAD_KVM_VCPU(r4)
>>>>>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>>>>>>>> 	lwz		r4, \scratch
>>>>>>>> 	<KVM_HANDLER>
>>>>>>>> 
>>>>>>>> then you get code that is slower :) but it should be easier to
>>>>>>>> read, since the interface between the individual pieces is always the
>> same.
>>>>>>>> Debug shouldn't be a fast path anyway, right?
>>>>>>> 
>>>>>>> Frankly speaking I do not see much difference :).
>>>>>>> 
>>>>>>> If we have to do as you mentioned then I think we can just do
>>>>>>> 
>>>>>>> KVM_DBG_HANDLER:
>>>>>>> 	<debug specific handling>
>>>>>>> 	1:
>>>>>>> 	mtcr		r3
>>>>>>> 	lwz		r3, VCPU_CRIT_SAVE(r4)
>>>>>>> 	lwz		r4, \scratch
>>>>>>> 	<KVM_HANDLER>
>>>>>> 
>>>>>> Whatever it takes to keep the oddball (debug) an oddball and keep
>>>>>> the normal case easy :).
>>>>> 
>>>>> I think there will be another problem as  the
>>>>> kvmppc_handler_\ivor_nr will not
>>>> be the starting address which is required as per our ivor/ivpr usages
>>>> for booke architecture.
>>>>> 
>>>>> I am thinking of keeping as is :).
>>>> 
>>>> How about we take a hybrid approach? You write the code as I
>>>> described above, but call __KVM_HANDLER at the end. The normal KVM_HANDLER
>> would look like:
>>>> 
>>>> KVM_HANDLER:
>>>> 	kvmppc_handler_\ivor_nr:
>>>> 	__KVM_HANDLER ...
>>>> 
>>>> That way the code should still be more understandable :)
>>>> 
>>> 
>>> With my current Patch it is defined as:
>>> 
>>> .macro KVM_HANDLER ivor_nr scratch srr0
>>> _GLOBAL(kvmppc_handler_\ivor_nr)
>>>       /* Get pointer to vcpu and record exit number. */
>>>       mtspr   \scratch , r4
>>>       mfspr   r4, SPRN_SPRG_THREAD
>>>       lwz     r4, THREAD_KVM_VCPU(r4)
>> 
>> Move these into __KVM_HANDLER (aka: keep the code in there the same as
>> KVM_HANDLER today)
>> 
>>>       __KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>> 
>>> .macro KVM_DBG_HANDLER ivor_nr scratch srr0
>>> _GLOBAL(kvmppc_handler_\ivor_nr)
>>> 
>>> <<<<<<Debug related handling>>>>>
>>> 
>>> 1:      /* debug interrupt happened in guest */
>>>       mtcr    r3
>>>       mfspr   r4, SPRN_SPRG_THREAD
>>>       lwz     r4, THREAD_KVM_VCPU(r4)
>>>       lwz     r3, VCPU_CRIT_SAVE(r4)
>> 
>> Restore the state here as if a non-debug interrupt occurred. __KVM_HANDLER will
>> fetch r4 itself from SPRG_THREAD.
>> 
>> I'm basically advocating to not optimize the debug case at all. Instead, I would
>> prefer to have the exception ABI be identical to the fallback case ABI. That way
>> we don't have to worry about 4 code paths, but only about 3, keeping the
>> complexity of the code low.
>> 
>> 
>> Alex
>> 
>>>       __KVM_HANDLER \ivor_nr \scratch \srr0 .endm
>>> 
> 
> Do you mean something like this ?
> 
> .macro __KVM_HANDLER
> +        /* Get pointer to vcpu and record exit number. */
> +        mtspr   \scratch , r4
> +        mfspr   r4, SPRN_SPRG_THREAD
> +        lwz     r4, THREAD_KVM_VCPU(r4)

This wouldn't be a +, but rather just stay the exact same code as it is, right? :)

> << Existing code >>
> 
> 
> .macro KVM_HANDLER ivor_nr scratch srr0
> _GLOBAL(kvmppc_handler_\ivor_nr)
> 	__KVM_HANDLER \ivor_nr \scratch \srr0 .endm
> 
> 
> 
> 
> .macro KVM_DBG_HANDLER ivor_nr scratch srr0
> _GLOBAL(kvmppc_handler_\ivor_nr)
> <<<<<<Debug related handling>>>>>
> 1:      /* debug interrupt happened in guest */
>        mtcr    r3
>        mfspr   r4, SPRN_SPRG_THREAD
>        lwz     r4, THREAD_KVM_VCPU(r4)
>        lwz     r3, VCPU_CRIT_SAVE(r4)

You need to swap the above 2 operations.

> 	 lwz		r4, \scratch 

s/lwz/mfspr/

>        __KVM_HANDLER \ivor_nr \scratch \srr0 .endm

Otherwise, pretty much, yeah :)


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 8a72d59..f4ba881 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -503,6 +503,7 @@  struct kvm_vcpu_arch {
 	u32 tlbcfg[4];
 	u32 mmucfg;
 	u32 epr;
+	u32 crit_save;
 	struct kvmppc_booke_debug_reg dbg_reg;
 #endif
 	gpa_t paddr_accessed;
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 46f6afd..02048f3 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -562,6 +562,7 @@  int main(void)
 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
 	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));
 #endif /* CONFIG_PPC_BOOK3S */
 #endif /* CONFIG_KVM */
 
diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index eae8483..dd9c5d4 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -52,12 +52,7 @@ 
                        (1<<BOOKE_INTERRUPT_PROGRAM) | \
                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
 
-.macro KVM_HANDLER ivor_nr scratch srr0
-_GLOBAL(kvmppc_handler_\ivor_nr)
-	/* Get pointer to vcpu and record exit number. */
-	mtspr	\scratch , r4
-	mfspr   r4, SPRN_SPRG_THREAD
-	lwz     r4, THREAD_KVM_VCPU(r4)
+.macro __KVM_HANDLER ivor_nr scratch srr0
 	stw	r3, VCPU_GPR(R3)(r4)
 	stw	r5, VCPU_GPR(R5)(r4)
 	stw	r6, VCPU_GPR(R6)(r4)
@@ -74,6 +69,46 @@  _GLOBAL(kvmppc_handler_\ivor_nr)
 	bctr
 .endm
 
+.macro KVM_HANDLER ivor_nr scratch srr0
+_GLOBAL(kvmppc_handler_\ivor_nr)
+	/* Get pointer to vcpu and record exit number. */
+	mtspr	\scratch , r4
+	mfspr   r4, SPRN_SPRG_THREAD
+	lwz     r4, THREAD_KVM_VCPU(r4)
+	__KVM_HANDLER \ivor_nr \scratch \srr0
+.endm
+
+.macro KVM_DBG_HANDLER ivor_nr scratch srr0
+_GLOBAL(kvmppc_handler_\ivor_nr)
+	mtspr   \scratch, r4
+	mfspr	r4, SPRN_SPRG_THREAD
+	lwz	r4, THREAD_KVM_VCPU(r4)
+	stw	r3, VCPU_CRIT_SAVE(r4)
+	mfcr	r3
+	mfspr	r4, SPRN_CSRR1
+	andi.	r4, r4, MSR_PR
+	bne	1f
+	/* debug interrupt happened in enter/exit path */
+	mfspr   r4, SPRN_CSRR1
+	rlwinm  r4, r4, 0, ~MSR_DE
+	mtspr   SPRN_CSRR1, r4
+	lis	r4, 0xffff
+	ori	r4, r4, 0xffff
+	mtspr	SPRN_DBSR, r4
+	mfspr	r4, SPRN_SPRG_THREAD
+	lwz	r4, THREAD_KVM_VCPU(r4)
+	mtcr	r3
+	lwz     r3, VCPU_CRIT_SAVE(r4)
+	mfspr   r4, \scratch
+	rfci
+1:	/* debug interrupt happened in guest */
+	mtcr	r3
+	mfspr	r4, SPRN_SPRG_THREAD
+	lwz	r4, THREAD_KVM_VCPU(r4)
+	lwz     r3, VCPU_CRIT_SAVE(r4)
+	__KVM_HANDLER \ivor_nr \scratch \srr0
+.endm
+
 .macro KVM_HANDLER_ADDR ivor_nr
 	.long	kvmppc_handler_\ivor_nr
 .endm
@@ -98,7 +133,7 @@  KVM_HANDLER BOOKE_INTERRUPT_FIT SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
 KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
-KVM_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
+KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_ROUND SPRN_SPRG_RSCRATCH0 SPRN_SRR0