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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On
> Behalf Of Alexander Graf
> Sent: Thursday, January 31, 2013 10:38 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
> 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:agraf@xxxxxxx]
> >> Sent: Thursday, January 31, 2013 5:47 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
> >> 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@xxxxxxx]
> >>>> Sent: Friday, January 25, 2013 5:13 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxxxxxx>
> >>>>> [bharat.bhushan@xxxxxxxxxxxxx: Substantial changes]
> >>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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 :).

Thanks
-Bharat

> 
> 
> Alex
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body
> of a message to majordomo@xxxxxxxxxxxxxxx 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux