> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Friday, February 01, 2013 1:36 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-...@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-ow...@vger.kernel.org
> >> [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
> >> Sent: Thursday, January 31, 2013 10:38 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-...@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:ag...@suse.de]
> >>>> Sent: Thursday, January 31, 2013 5:47 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: kvm-...@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:ag...@suse.de]
> >>>>>> Sent: Friday, January 25, 2013 5:13 PM
> >>>>>> To: Bhushan Bharat-R65777
> >>>>>> Cc: kvm-...@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.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.
> >>>
> >>> 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" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to