RE: [PATCH 7/7] KVM: PPC: Add userspace debug stub support

[Bhushan Bharat-R65777 <R65777@freescale.com>]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, March 07, 2013 7:09 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; 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@freescale.com>
> > ---
> > 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?

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

> 
> > +		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. 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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>