On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:ag...@suse.de]
>> Sent: Friday, January 25, 2013 5:13 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; k...@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.bhus...@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....@freescale.com>
>>> [bharat.bhus...@freescale.com: Substantial changes]
>>> Signed-off-by: Bharat Bhushan <bharat.bhus...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
