Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support

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

 



On 04.10.2012, at 16:22, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@xxxxxxx]
>> Sent: Thursday, October 04, 2012 4:56 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support
>> 
>> 
>> On 04.10.2012, at 13:06, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@xxxxxxx]
>>>> Sent: Monday, September 24, 2012 9:50 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Bhushan
>>>> Bharat-R65777
>>>> Subject: Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support
>>>> 
>>>> 
>>>> On 21.08.2012, at 15:52, Bharat Bhushan wrote:
>>>> 
>>>>> This patch adds the debug stub support on booke/bookehv.
>>>>> Now QEMU debug stub can use hw breakpoint, watchpoint and software
>>>>> breakpoint to debug guest.
>>>>> 
>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
>>>>> ---
>>>>> arch/powerpc/include/asm/kvm.h        |   29 ++++++-
>>>>> arch/powerpc/include/asm/kvm_host.h   |    5 +
>>>>> arch/powerpc/kernel/asm-offsets.c     |   26 ++++++
>>>>> arch/powerpc/kvm/booke.c              |  144 +++++++++++++++++++++++++++++--
>> --
>>>>> arch/powerpc/kvm/booke_interrupts.S   |  110 +++++++++++++++++++++++++
>>>>> arch/powerpc/kvm/bookehv_interrupts.S |  141
>> +++++++++++++++++++++++++++++++-
>>>>> arch/powerpc/kvm/e500mc.c             |    3 +-
>>>>> 7 files changed, 435 insertions(+), 23 deletions(-)
>>>>> 
>>>>> diff --git a/arch/powerpc/include/asm/kvm.h
>>>>> b/arch/powerpc/include/asm/kvm.h index 61b197e..53479ea 100644
>>>>> --- a/arch/powerpc/include/asm/kvm.h
>>>>> +++ b/arch/powerpc/include/asm/kvm.h
>>>>> @@ -25,6 +25,7 @@
>>>>> /* Select powerpc specific features in <linux/kvm.h> */ #define
>>>>> __KVM_HAVE_SPAPR_TCE #define __KVM_HAVE_PPC_SMT
>>>>> +#define __KVM_HAVE_GUEST_DEBUG
>>>>> 
>>>>> struct kvm_regs {
>>>>> 	__u64 pc;
>>>>> @@ -264,7 +265,31 @@ struct kvm_fpu {
>>>>> 	__u64 fpr[32];
>>>>> };
>>>>> 
>>>>> +
>>>>> +/*
>>>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
>>>>> + * software breakpoint.
>>>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>>>>> + * for KVM_DEBUG_EXIT.
>>>>> + */
>>>>> +#define KVMPPC_DEBUG_NONE		0x0
>>>>> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>>>> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>>>> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>>>> struct kvm_debug_exit_arch {
>>>>> +	__u64 pc;
>>>>> +	/*
>>>>> +	 * exception -> returns the exception number. If the KVM_DEBUG_EXIT
>>>>> +	 * exit is not handled (say not h/w breakpoint or software breakpoint
>>>>> +	 * set for this address) by qemu then it is supposed to inject this
>>>>> +	 * exception to guest.
>>>>> +	 */
>>>>> +	__u32 exception;
>>>>> +	/*
>>>>> +	 * exiting to userspace because of h/w breakpoint, watchpoint
>>>>> +	 * (read, write or both) and software breakpoint.
>>>>> +	 */
>>>>> +	__u32 status;
>>>>> };
>>>>> 
>>>>> /* for KVM_SET_GUEST_DEBUG */
>>>>> @@ -276,10 +301,6 @@ struct kvm_guest_debug_arch {
>>>>> 		 * Type denotes h/w breakpoint, read watchpoint, write
>>>>> 		 * watchpoint or watchpoint (both read and write).
>>>>> 		 */
>>>>> -#define KVMPPC_DEBUG_NOTYPE		0x0
>>>>> -#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>>>> -#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>>>> -#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>>>> 		__u32 type;
>>>>> 		__u32 pad1;
>>>>> 		__u64 pad2;
>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>> index c7219c1..3ba465a 100644
>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>> @@ -496,7 +496,12 @@ struct kvm_vcpu_arch {
>>>>> 	u32 mmucfg;
>>>>> 	u32 epr;
>>>>> 	u32 crit_save;
>>>>> +	/* guest debug registers*/
>>>>> 	struct kvmppc_booke_debug_reg dbg_reg;
>>>>> +	/* shadow debug registers */
>>>>> +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
>>>>> +	/* host debug registers*/
>>>>> +	struct kvmppc_booke_debug_reg host_dbg_reg;
>>>>> #endif
>>>>> 	gpa_t paddr_accessed;
>>>>> 	gva_t vaddr_accessed;
>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>> b/arch/powerpc/kernel/asm-
>>>> offsets.c
>>>>> index 555448e..6987821 100644
>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>> @@ -564,6 +564,32 @@ int main(void)
>>>>> 	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));
>>>>> +	DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
>>>>> +	DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
>>>>> +	DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
>>>>> +	DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					  dbcr0));
>>>>> +	DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					  dbcr1));
>>>>> +	DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					  dbcr2));
>>>>> +#ifdef CONFIG_KVM_E500MC
>>>>> +	DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					  dbcr4));
>>>>> +#endif
>>>>> +	DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					 iac[0]));
>>>>> +	DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					 iac[1]));
>>>>> +	DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					 iac[2]));
>>>>> +	DEFINE(KVMPPC_DBG_IAC4, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					 iac[3]));
>>>>> +	DEFINE(KVMPPC_DBG_DAC1, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					 dac[0]));
>>>>> +	DEFINE(KVMPPC_DBG_DAC2, offsetof(struct kvmppc_booke_debug_reg,
>>>>> +					 dac[1]));
>>>>> +	DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
>>>>> #endif /* CONFIG_PPC_BOOK3S */
>>>>> #endif /* CONFIG_KVM */
>>>>> 
>>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>>>> index dd0e5b8..4d82e34 100644
>>>>> --- a/arch/powerpc/kvm/booke.c
>>>>> +++ b/arch/powerpc/kvm/booke.c
>>>>> @@ -132,6 +132,9 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32
>>>>> new_msr)
>>>>> 
>>>>> #ifdef CONFIG_KVM_BOOKE_HV
>>>>> 	new_msr |= MSR_GS;
>>>>> +
>>>>> +	if (vcpu->guest_debug)
>>>>> +		new_msr |= MSR_DE;
>>>>> #endif
>>>>> 
>>>>> 	vcpu->arch.shared->msr = new_msr;
>>>>> @@ -667,10 +670,21 @@ out:
>>>>> 	return ret;
>>>>> }
>>>>> 
>>>>> -static int emulation_exit(struct kvm_run *run, struct kvm_vcpu
>>>>> *vcpu)
>>>>> +static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>>>> +			  int exit_nr)
>>>>> {
>>>>> 	enum emulation_result er;
>>>>> 
>>>>> +	if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) &&
>>>>> +		     vcpu->arch.last_inst == KVMPPC_INST_GUEST_GDB) {
>>>> 
>>>> This belongs into the normal emulation code path, behind the same
>>>> switch() that everything else goes through.
>>> 
>>> I am not sure I understood correctly. Below is the reason why I placed this
>> code here.
>>> Instruction where software breakpoint is to be set is replaced by "ehpriv"
>> instruction. On e500v2, this is not a valid instruction can causes program
>> interrupt. On e500mc, "ehpriv" is a valid instruction. Both the exit path calls
>> emulation_exit(), so we have placed the code in this function.
>>> Do you want this code to be moved in program interrupt exit path for e500v2
>> and BOOKE_INTERRUPT_HV_PRIV for e500mc?
>> 
>> Ok, in this patch you do (basically):
>> 
>> int emulation_exit()
>> {
>>    if (inst == DEBUG_INST) {
>>        debug_stuff();
>>        return;
>>    }
>> 
>>    switch (inst) {
>>        case INST_A:
>>            foo();
>>        ....
>>    }
>> }
> 
> Are not we doing something like this:
> int emulation_exit()
> {
>     if (inst == DEBUG_INST) {
>         debug_stuff();
>         return;
>     }
> 
>     status = kvmppc_emulate_instruction()
>     switch (status) {
>         case FAIL:
>             foo();
>         case DONE:
> 		foo1();
>         ....
>     }
> }
> 
> Do you want something like this:
> 
> int emulation_exit()
> {
> 
>     status = kvmppc_emulate_instruction()
>     switch (status) {
>         case FAIL:
>     		if (inst == DEBUG_INST) {
>         		debug_stuff();
>    		      return;
>     		}
>             foo();
> 
>         case DONE:
> 		foo1();
>         ....
>     }
> }

No, I want the DEBUG_INST be handled the same as any other instruction we emulate.

>> 
>> what I want is:
>> 
>> int emulation_exit()
>> {
>>    switch (inst) {
>>        case INST_A:
>>            foo(); break;
>>        case DEBUG_INST:
>>            debug_stuff(); break;
>>        ....
>>    }
>> }
>> 
>>> 
>>> 
>>>> 
>>>>> +		run->exit_reason = KVM_EXIT_DEBUG;
>>>>> +		run->debug.arch.pc = vcpu->arch.pc;
>>>>> +		run->debug.arch.exception = exit_nr;
>>>>> +		run->debug.arch.status = 0;
>>>>> +		kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>>>> +		return RESUME_HOST;
>>>>> +	}
>>>>> +
>>>>> 	er = kvmppc_emulate_instruction(run, vcpu);
>>>>> 	switch (er) {
>>>>> 	case EMULATE_DONE:
>>>>> @@ -697,6 +711,44 @@ static int emulation_exit(struct kvm_run *run,
>>>>> struct
>>>> kvm_vcpu *vcpu)
>>>>> 	default:
>>>>> 		BUG();
>>>>> 	}
>>>>> +
>>>>> +	if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&
>>>>> +	    (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>>> 
>>>> I don't understand how this is supposed to work. When we enable
>>>> singlestep, why would we end up in emulation_exit()?
>>> 
>>> When singlestep is enabled then we set DBCR0[ICMP] and the debug handler
>> should be able to handle this. I think you are right.
>>> 
>>>> 
>>>>> +		run->exit_reason = KVM_EXIT_DEBUG;
>>>>> +		return RESUME_HOST;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu
>>>>> +*vcpu) {
>>>>> +	u32 dbsr;
>>>>> +
>>>>> +#ifndef CONFIG_KVM_BOOKE_HV
>>>>> +	if (cpu_has_feature(CPU_FTR_DEBUG_LVL_EXC))
>>>>> +		vcpu->arch.pc = mfspr(SPRN_DSRR0);
>>>>> +	else
>>>>> +		vcpu->arch.pc = mfspr(SPRN_CSRR0); #endif
>>>> 
>>>> Why doesn't this get handled in the asm code that recovers from the
>>>> respective exceptions?
>>> 
>>> Yes. I will remove this.
>>> 
>>>> 
>>>>> +	dbsr = vcpu->arch.dbsr;
>>>>> +
>>>>> +	run->debug.arch.pc = vcpu->arch.pc;
>>>>> +	run->debug.arch.status = 0;
>>>>> +	vcpu->arch.dbsr = 0;
>>>>> +
>>>>> +	if (dbsr & (DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4)) {
>>>>> +		run->debug.arch.status |= KVMPPC_DEBUG_BREAKPOINT;
>>>>> +	} else {
>>>>> +		if (dbsr & (DBSR_DAC1W | DBSR_DAC2W))
>>>>> +			run->debug.arch.status |= KVMPPC_DEBUG_WATCH_WRITE;
>>>>> +		else if (dbsr & (DBSR_DAC1R | DBSR_DAC2R))
>>>>> +			run->debug.arch.status |= KVMPPC_DEBUG_WATCH_READ;
>>>>> +		if (dbsr & (DBSR_DAC1R | DBSR_DAC1W))
>>>>> +			run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac[0];
>>>>> +		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
>>>>> +			run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac[1];
>>>>> +	}
>>>> 
>>>> Mind to explain the guest register sharing mechanism you are
>>>> envisioning? Which registers are used by the guest? Which are used by
>>>> the host? Who decides what gets used by whom?
>>> 
>>> struct kvm_vcpu_arch {
>>> 	.
>>> 	.
>>> 
>>>      /* guest debug registers*/
>>>       struct kvmppc_booke_debug_reg dbg_reg;
>>>      /* shadow debug registers */
>>>      struct kvmppc_booke_debug_reg shadow_dbg_reg;
>>>      /* host debug registers*/
>>>      struct kvmppc_booke_debug_reg host_dbg_reg;
>>> 
>>> 	.
>>> 	.
>>> }
>>> 
>>> dbg_reg: contains the values written by guest.
>>> shadow_dbg_reg: This contains the actual values to be written on behalf of
>> guest/qemu.
>>> host_dbg_reg: the debug register value of host (needed for store/retore
>> purpose).
>>> 
>>> According to current design both, the qemu debug stub and guest, cannot share
>> the debug resources. QEMU debug stub is given priority.
>>> But host and guest can share all debug registers and host cannot debug guest
>> if it is using the debug resource (which probably does not look like a good
>> case).
>> 
>> Ok. Assume that in guest context, we only ever want guest debugging enabled. If
>> QEMU wants to inject a QEMU breakpoint, it will fiddle with the guest debug
>> registers itself to inject its breakpoint into them.
> 
> I would like to understand the context you are describing.
> QEMU would like to inject debug interrupt to guest only if a debug interrupt happened in guest context and QEMU is not able to handle interrupt (because qemu debug have not set). Right?

Yes

> In that case the only thing that needed to be updated is guest->DBSR?

I think you lost me here :)

> 
>> 
>> With that scheme, would we still need shadow debug registers? Or could we remove
>> that one level of indirection?
>> 
>>> 
>>>> 
>>>>> +
>>>>> +	return RESUME_HOST;
>>>>> }
>>>>> 
>>>>> static void kvmppc_fill_pt_regs(struct pt_regs *regs) @@ -843,7
>>>>> +895,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
>>>> kvm_vcpu *vcpu,
>>>>> 		break;
>>>>> 
>>>>> 	case BOOKE_INTERRUPT_HV_PRIV:
>>>>> -		r = emulation_exit(run, vcpu);
>>>>> +		r = emulation_exit(run, vcpu, exit_nr);
>>>>> 		break;
>>>>> 
>>>>> 	case BOOKE_INTERRUPT_PROGRAM:
>>>>> @@ -862,7 +914,7 @@ int kvmppc_handle_exit(struct kvm_run *run,
>>>>> struct
>>>> kvm_vcpu *vcpu,
>>>>> 			break;
>>>>> 		}
>>>>> 
>>>>> -		r = emulation_exit(run, vcpu);
>>>>> +		r = emulation_exit(run, vcpu, exit_nr);
>>>>> 		break;
>>>>> 
>>>>> 	case BOOKE_INTERRUPT_FP_UNAVAIL:
>>>>> @@ -1052,18 +1104,12 @@ int kvmppc_handle_exit(struct kvm_run *run,
>>>>> struct
>>>> kvm_vcpu *vcpu,
>>>>> 	}
>>>>> 
>>>>> 	case BOOKE_INTERRUPT_DEBUG: {
>>>>> -		u32 dbsr;
>>>>> -
>>>>> -		vcpu->arch.pc = mfspr(SPRN_CSRR0);
>>>>> -
>>>>> -		/* clear IAC events in DBSR register */
>>>>> -		dbsr = mfspr(SPRN_DBSR);
>>>>> -		dbsr &= DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4;
>>>>> -		mtspr(SPRN_DBSR, dbsr);
>>>>> -
>>>>> -		run->exit_reason = KVM_EXIT_DEBUG;
>>>>> +		r = kvmppc_handle_debug(run, vcpu);
>>>>> +		if (r == RESUME_HOST) {
>>>>> +			run->debug.arch.exception = exit_nr;
>>>>> +			run->exit_reason = KVM_EXIT_DEBUG;
>>>>> +		}
>>>>> 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>>>> -		r = RESUME_HOST;
>>>>> 		break;
>>>>> 	}
>>>>> 
>>>>> @@ -1403,10 +1449,78 @@ int kvm_vcpu_ioctl_set_one_reg(struct
>>>>> kvm_vcpu *vcpu,
>>>> struct kvm_one_reg *reg)
>>>>> 	return r;
>>>>> }
>>>>> 
>>>>> +#define BP_NUM	KVMPPC_BOOKE_IAC_NUM
>>>>> +#define WP_NUM	KVMPPC_BOOKE_DAC_NUM
>>>>> +
>>>>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>>>> 					 struct kvm_guest_debug *dbg)
>>>>> {
>>>>> -	return -EINVAL;
>>>>> +
>>>>> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>>>>> +		/* Clear All debug events */
>>>>> +		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>>>> +		vcpu->guest_debug = 0;
>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>> +	vcpu->guest_debug = dbg->control;
>>>>> +	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>>>> +
>>>>> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
>>>>> +		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
>>>>> +
>>>>> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
>>>>> +		struct kvmppc_booke_debug_reg *gdbgr =
>>>>> +				&(vcpu->arch.shadow_dbg_reg);
>>>>> +		int n, b = 0, w = 0;
>>>>> +		const u32 bp_code[] = {
>>>>> +			DBCR0_IAC1 | DBCR0_IDM,
>>>>> +			DBCR0_IAC2 | DBCR0_IDM,
>>>>> +			DBCR0_IAC3 | DBCR0_IDM,
>>>>> +			DBCR0_IAC4 | DBCR0_IDM
>>>>> +		};
>>>>> +		const u32 wp_code[] = {
>>>>> +			DBCR0_DAC1W | DBCR0_IDM,
>>>>> +			DBCR0_DAC2W | DBCR0_IDM,
>>>>> +			DBCR0_DAC1R | DBCR0_IDM,
>>>>> +			DBCR0_DAC2R | DBCR0_IDM
>>>>> +		};
>>>>> +
>>>>> +#ifndef CONFIG_KVM_BOOKE_HV
>>>>> +		gdbgr->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US |
>>>>> +				DBCR1_IAC3US | DBCR1_IAC4US;
>>>>> +		gdbgr->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US; #else
>>>>> +		gdbgr->dbcr1 = 0;
>>>>> +		gdbgr->dbcr2 = 0;
>>>>> +#endif
>>>>> +
>>>>> +		for (n = 0; n < (BP_NUM + WP_NUM); n++) {
>>>>> +			u32 type = dbg->arch.bp[n].type;
>>>>> +
>>>>> +			if (!type)
>>>>> +				break;
>>>>> +
>>>>> +			if (type & (KVMPPC_DEBUG_WATCH_READ |
>>>>> +				    KVMPPC_DEBUG_WATCH_WRITE)) {
>>>>> +				if (w < WP_NUM) {
>>>>> +					if (type & KVMPPC_DEBUG_WATCH_READ)
>>>>> +						gdbgr->dbcr0 |= wp_code[w + 2];
>>>>> +					if (type & KVMPPC_DEBUG_WATCH_WRITE)
>>>>> +						gdbgr->dbcr0 |= wp_code[w];
>>>>> +					gdbgr->dac[w] = dbg->arch.bp[n].addr;
>>>>> +					w++;
>>>>> +				}
>>>>> +			} else if (type & KVMPPC_DEBUG_BREAKPOINT) {
>>>>> +				if (b < BP_NUM) {
>>>>> +					gdbgr->dbcr0 |= bp_code[b];
>>>>> +					gdbgr->iac[b] = dbg->arch.bp[n].addr;
>>>>> +					b++;
>>>>> +				}
>>>>> +			}
>>>>> +		}
>>>>> +	}
>>>>> +	return 0;
>>>>> }
>>>>> 
>>>>> int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct
>>>>> kvm_fpu *fpu) diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>> index dd9c5d4..2202936 100644
>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>> @@ -39,6 +39,8 @@
>>>>> #define HOST_MIN_STACK_SIZE (HOST_NV_GPR(R31) + 4) #define
>>>>> HOST_STACK_SIZE (((HOST_MIN_STACK_SIZE + 15) / 16) * 16) /* Align. */
>>>>> #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
>>>>> +#define DBCR0_AC_BITS	(DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
>>>>> +			 DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
>>>>> 
>>>>> #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>                       (1<<BOOKE_INTERRUPT_DTLB_MISS) | \ @@ -52,6
>>>>> +54,8 @@
>>>>>                      (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>>                      (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>> 
>>>>> +#define NEED_DEBUG_SAVE (1<<BOOKE_INTERRUPT_DEBUG)
>>>>> +
>>>>> .macro __KVM_HANDLER ivor_nr scratch srr0
>>>>> 	stw	r3, VCPU_GPR(R3)(r4)
>>>>> 	stw	r5, VCPU_GPR(R5)(r4)
>>>>> @@ -212,6 +216,61 @@ _GLOBAL(kvmppc_resume_host)
>>>>> 	stw	r9, VCPU_FAULT_ESR(r4)
>>>>> ..skip_esr:
>>>>> 
>>>>> +	addi	r7, r4, VCPU_HOST_DBG
>>>>> +	mfspr	r9, SPRN_DBCR0
>>>>> +	lwz	r8, KVMPPC_DBG_DBCR0(r7)
>>>>> +	andis.	r9, r9, DBCR0_AC_BITS@h
>>>>> +	beq	skip_load_host_debug
>>>>> +	li	r9, 0
>>>>> +	mtspr	SPRN_DBCR0, r9		/* disable all debug event */
>>>>> +	lwz	r9, KVMPPC_DBG_DBCR1(r7)
>>>>> +	mtspr	SPRN_DBCR1, r9
>>>>> +	lwz	r9, KVMPPC_DBG_DBCR2(r7)
>>>>> +	mtspr	SPRN_DBCR2, r9
>>>>> +	lwz	r9, KVMPPC_DBG_IAC1+4(r7)
>>>>> +	mtspr	SPRN_IAC1, r9
>>>>> +	lwz	r9, KVMPPC_DBG_IAC2+4(r7)
>>>>> +	mtspr	SPRN_IAC2, r9
>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>>>> +	lwz	r9, KVMPPC_DBG_IAC3+4(r7)
>>>>> +	mtspr	SPRN_IAC3, r9
>>>>> +	lwz	r9, KVMPPC_DBG_IAC4+4(r7)
>>>>> +	mtspr	SPRN_IAC4, r9
>>>>> +#endif
>>>>> +	lwz	r9, KVMPPC_DBG_DAC1+4(r7)
>>>>> +	mtspr	SPRN_DAC1, r9
>>>>> +	lwz	r9, KVMPPC_DBG_DAC2+4(r7)
>>>> 
>>>> Yikes. Please move this into a struct, similar to how the MSR
>>>> save/restore happens on x86.
>>>> 
>>>> Also, these are 64-bit loads and stores, no?
>>> 
>>> Ok, you mean PPC_LD/STD?
>> 
>> Yeah :).
>> 
>>> 
>>>> 
>>>>> +	mtspr	SPRN_DAC2, r9
>>>>> +skip_load_host_debug:
>>>>> +	/* Clear h/w DBSR and save current(guest) DBSR */
>>>>> +	mfspr	r9, SPRN_DBSR
>>>>> +	mtspr	SPRN_DBSR, r9
>>>>> +	isync
>>>>> +	andi.	r7, r6, NEED_DEBUG_SAVE
>>>>> +	beq	skip_dbsr_save
>>>>> +	/*
>>>>> +	 * If vcpu->guest_debug flag is set then do not check for
>>>>> +	 * shared->msr.DE as this debugging (say by QEMU) does not
>>>>> +	 * depends on shared->msr.de. In these scanerios MSR.DE is
>>>>> +	 * always set using shared_msr and should be handled always.
>>>>> +	 */
>>>>> +	lwz	r7, VCPU_GUEST_DEBUG(r4)
>>>>> +	cmpwi	r7, 0
>>>>> +	bne	skip_save_trap_event
>>>>> +	PPC_LL	r3, VCPU_SHARED(r4)
>>>>> +#ifndef CONFIG_64BIT
>>>>> +	lwz	r3, (VCPU_SHARED_MSR + 4)(r3)
>>>>> +#else
>>>>> +	ld	r3, (VCPU_SHARED_MSR)(r3)
>>>>> +#endif
>>>> 
>>>> Didn't we have 64-bit access helpers for these?
>>> 
>>> I will use PPC_LD.
>>> 
>>>> 
>>>> Also this shouldn't happen in the entry/exit code. We have shadow_msr
>>>> for these purposes.
>>> 
>>> Based on shared_msr.MSR_DE, we set make DBSR (set DBSR_TIE).
>> 
>> Hrm. Yeah, must've misread something here.
>> 
>> It would be awesome if we could make the debug register save/restore a bit more
>> relaxed though. For example during preemption.
> 
> If we delay this save/restore then host cannot do debugging of KVM code. Right?

You mean using kdb? Does anyone actually do that?

But let's leave this for later as an optimization.


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