> >>>>>>>> -----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 :)
> >>
> >
> > 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 [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
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
