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

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

 




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


[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