> -----Original Message----- > From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On > Behalf Of Alexander Graf > Sent: Thursday, March 14, 2013 5:20 PM > To: Bhushan Bharat-R65777 > Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Wood Scott-B07421 > Subject: Re: [PATCH 7/7] KVM: PPC: Add userspace debug stub support > > > On 14.03.2013, at 06:18, Bhushan Bharat-R65777 wrote: > > > > > > >> -----Original Message----- > >> From: Alexander Graf [mailto:agraf@xxxxxxx] > >> Sent: Thursday, March 07, 2013 7:09 PM > >> To: Bhushan Bharat-R65777 > >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Wood Scott-B07421; > >> Bhushan > >> Bharat-R65777 > >> Subject: Re: [PATCH 7/7] KVM: PPC: Add userspace debug stub support > >> > >> > >> On 28.02.2013, at 05:13, 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/uapi/asm/kvm.h | 22 +++++- > >>> arch/powerpc/kvm/booke.c | 143 +++++++++++++++++++++++++++++++-- > - > >>> arch/powerpc/kvm/e500_emulate.c | 6 ++ > >>> arch/powerpc/kvm/e500mc.c | 3 +- > >>> 4 files changed, 155 insertions(+), 19 deletions(-) > >>> > >>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h > >>> b/arch/powerpc/include/uapi/asm/kvm.h > >>> index 15f9a00..d7ce449 100644 > >>> --- a/arch/powerpc/include/uapi/asm/kvm.h > >>> +++ b/arch/powerpc/include/uapi/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; > >>> @@ -267,7 +268,24 @@ 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 address; > >>> + /* > >>> + * exiting to userspace because of h/w breakpoint, watchpoint > >>> + * (read, write or both) and software breakpoint. > >>> + */ > >>> + __u32 status; > >>> + __u32 reserved; > >>> }; > >>> > >>> /* for KVM_SET_GUEST_DEBUG */ > >>> @@ -279,10 +297,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 reserved; > >>> } bp[16]; > >>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > >>> index > >>> 1de93a8..21b0313 100644 > >>> --- a/arch/powerpc/kvm/booke.c > >>> +++ b/arch/powerpc/kvm/booke.c > >>> @@ -133,6 +133,30 @@ static void kvmppc_vcpu_sync_fpu(struct > >>> kvm_vcpu > >>> *vcpu) #endif } > >>> > >>> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { > >>> + /* Synchronize guest's desire to get debug interrupts into shadow > >>> +MSR */ #ifndef CONFIG_KVM_BOOKE_HV > >>> + vcpu->arch.shadow_msr &= ~MSR_DE; > >>> + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif > >>> + > >>> + /* Force enable debug interrupts when user space wants to debug */ > >>> + if (vcpu->guest_debug) { > >>> +#ifdef CONFIG_KVM_BOOKE_HV > >>> + /* > >>> + * Since there is no shadow MSR, sync MSR_DE into the guest > >>> + * visible MSR. Do not allow guest to change MSR[DE]. > >>> + */ > >>> + vcpu->arch.shared->msr |= MSR_DE; > >>> + mtspr(SPRN_MSRP, mfspr(SPRN_MSRP) | MSRP_DEP); #else > >>> + vcpu->arch.shadow_msr |= MSR_DE; > >>> + vcpu->arch.shared->msr &= ~MSR_DE; #endif > >>> + } > >>> +} > >>> + > >>> /* > >>> * Helper function for "full" MSR writes. No need to call this if > >>> only > >>> * EE/CE/ME/DE/RI are changing. > >>> @@ -150,6 +174,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr) > >>> kvmppc_mmu_msr_notify(vcpu, old_msr); > >>> kvmppc_vcpu_sync_spe(vcpu); > >>> kvmppc_vcpu_sync_fpu(vcpu); > >>> + kvmppc_vcpu_sync_debug(vcpu); > >>> } > >>> > >>> static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, @@ > >>> -736,6 +761,13 @@ static int emulation_exit(struct kvm_run *run, > >>> struct > >> kvm_vcpu *vcpu) > >>> run->exit_reason = KVM_EXIT_DCR; > >>> return RESUME_HOST; > >>> > >>> + case EMULATE_EXIT_USER: > >>> + run->exit_reason = KVM_EXIT_DEBUG; > >>> + run->debug.arch.address = vcpu->arch.pc; > >>> + run->debug.arch.status = 0; > >>> + kvmppc_account_exit(vcpu, DEBUG_EXITS); > >> > >> As mentioned previously, this is wrong and needs to go into the > >> instruction emulation code for that opcode. > > > > ok > > > >> > >>> + return RESUME_HOST; > >>> + > >>> case EMULATE_FAIL: > >>> printk(KERN_CRIT "%s: emulation at %lx failed (%08x)\n", > >>> __func__, vcpu->arch.pc, vcpu->arch.last_inst); @@ -751,6 > >>> +783,28 @@ static int emulation_exit(struct kvm_run *run, struct > >>> +kvm_vcpu > >> *vcpu) > >>> } > >>> } > >>> > >>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu > >>> +*vcpu) { > >>> + u32 dbsr = vcpu->arch.dbsr; > >>> + run->debug.arch.status = 0; > >>> + run->debug.arch.address = vcpu->arch.pc; > >> > >> This should go into the if(breakpoint) branch. > > > > Can there be the case when do breakpoint and debug interrupt happen? > > At least not according to the code below :). If that's a valid case, then quite > a bit of code would need to be remodeled. I'd say ignore the possibility for > now. ok > > > > >> > >>> + > >>> + 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.address = vcpu->arch.shadow_dbg_reg.dac[0]; > >>> + else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W)) > >>> + run->debug.arch.address = vcpu->arch.shadow_dbg_reg.dac[1]; > >>> + } > >>> + > >>> + return RESUME_HOST; > >>> +} > >>> + > >>> static void kvmppc_fill_pt_regs(struct pt_regs *regs) { > >>> ulong r1, ip, msr, lr; > >>> @@ -1110,18 +1164,11 @@ 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->exit_reason = KVM_EXIT_DEBUG; > >>> + } > >>> kvmppc_account_exit(vcpu, DEBUG_EXITS); > >>> - r = RESUME_HOST; > >>> break; > >>> } > >>> > >>> @@ -1172,7 +1219,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) > >>> kvmppc_set_msr(vcpu, 0); > >>> > >>> #ifndef CONFIG_KVM_BOOKE_HV > >>> - vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS; > >>> + vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS; > >>> vcpu->arch.shadow_pid = 1; > >>> vcpu->arch.shared->msr = 0; > >>> #endif > >>> @@ -1527,10 +1574,80 @@ 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; > >>> + /* Set DBCR0_EDM in guest visible DBCR0 register. */ > >>> + vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM; > >>> + > >>> + 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) { > >> > >> if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) { > >> /* Code below handles only HW breakpoints */ > >> return 0; > >> } > > > > ok > > > >> > >>> + 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 > >> > >> Please no double negation. > > You mean we should use > > #ifdef CONFIG_KVM_BOOKE_HV > > gdbgr->dbcr1 = 0; > > gdbgr->dbcr2 = 0; > > #else > > gdbgr->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | > > DBCR1_IAC3US | DBCR1_IAC4US; > > gdbgr->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US; #endif > > > >> Also, what is this about? > > > > This These bits says that IAC1-4 and DAC1-2 can happen when MSR.PR is set of > not. > > On BOOKE (e500v2); MSR.PR = 1 when guest is running. So we need to set > > these bits On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we do > not need these bits to be set. > > Ah, please add a comment explaining this here. ok > > > > >> > >>> + 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/e500_emulate.c > >>> b/arch/powerpc/kvm/e500_emulate.c index e78f353..83ac877 100644 > >>> --- a/arch/powerpc/kvm/e500_emulate.c > >>> +++ b/arch/powerpc/kvm/e500_emulate.c > >>> @@ -26,6 +26,7 @@ > >>> #define XOP_TLBRE 946 > >>> #define XOP_TLBWE 978 > >>> #define XOP_TLBILX 18 > >>> +#define XOP_EHPRIV 270 > >>> > >>> #ifdef CONFIG_KVM_E500MC > >>> static int dbell2prio(ulong param) > >>> @@ -130,6 +131,11 @@ int kvmppc_core_emulate_op(struct kvm_run *run, > >>> struct > >> kvm_vcpu *vcpu, > >>> emulated = kvmppc_e500_emul_tlbivax(vcpu, ea); > >>> break; > >>> > >>> + case XOP_EHPRIV: > >>> + emulated = EMULATE_EXIT_USER; > >>> + *advance = 0; > >>> + break; > >>> + > >>> default: > >>> emulated = EMULATE_FAIL; > >>> } > >>> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c > >>> index 1f89d26..f5fc6f5 100644 > >>> --- a/arch/powerpc/kvm/e500mc.c > >>> +++ b/arch/powerpc/kvm/e500mc.c > >>> @@ -182,8 +182,7 @@ int kvmppc_core_vcpu_setup(struct kvm_vcpu > >>> *vcpu) { > >>> struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu); > >>> > >>> - vcpu->arch.shadow_epcr = SPRN_EPCR_DSIGS | SPRN_EPCR_DGTMI | \ > >>> - SPRN_EPCR_DUVD; > >>> + vcpu->arch.shadow_epcr = SPRN_EPCR_DSIGS | SPRN_EPCR_DGTMI; > >> > >> Doesn't this route all debug events through the host? > > > > No; This means that debug events can occur in hypervisor state or not. > > > > EPCR.DUVD = 0 ; Debug events can occur in the hypervisor state. > > > > EPCR.DUVD = 1 ; Debug events cannot occur in the hypervisor state. > > > > So we allow debug events to occur in hypervisor state. > > Why do we care about debug events in our entry/exit code and didn't care about > them before? We care for single stepping in guest to not step in KVM code. > If anything, this is a completely separate patch, orthogonal to > this patch series, and requires a good bit of explanation. Not sure why you think separate patch; this patch add support for single stepping and also takes care that debug event does not comes in host when doing single stepping. -Bharat > > > Alex > > > On lightweight exit we set ECPU.DUVD (if guest using debug facility) so debug > events will not come during guest entry/exit code. On guest exit we clear this > bit (after restoring host state) so hypervisor can use debug features. > > > > Thanks > > -Bharat > >> > >> > >> Alex > >> > >>> #ifdef CONFIG_64BIT > >>> vcpu->arch.shadow_epcr |= SPRN_EPCR_ICM; #endif > >>> -- > >>> 1.7.0.4 > >>> > >>> > >>> -- > >>> 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-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
