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

[Alexander Graf <agraf@suse.de>]

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