> -----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? > > > + 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). > > > + > > + 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? > > > + 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). Thanks -Bharat > > > > 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
