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

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

 



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. 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" 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