On 01.02.2013, at 06:04, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: [email protected] [mailto:[email protected]] On
>> Behalf Of Alexander Graf
>> Sent: Thursday, January 31, 2013 10:38 PM
>> To: Bhushan Bharat-R65777
>> Cc: [email protected]; [email protected]
>> 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:[email protected]]
>>>> Sent: Thursday, January 31, 2013 5:47 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: [email protected]; [email protected]
>>>> 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:[email protected]]
>>>>>> Sent: Friday, January 25, 2013 5:13 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: [email protected]; [email protected]; 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 <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>> [[email protected]: Substantial changes]
>>>>>>> Signed-off-by: Bharat Bhushan <[email protected]>
>>>>>>> ---
>>>>>>> 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" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
