Re: [PATCH 2 of 2] Add gdb break point support to PowerPC kvm

[Hollis Blanchard <hollisb@us.ibm.com>]

On Fri, 2008-06-06 at 11:54 -0500, Jerone Young wrote:
> 4 files changed, 132 insertions(+), 2 deletions(-)
> arch/powerpc/kvm/booke_guest.c      |   25 ++++++++
> arch/powerpc/kvm/booke_interrupts.S |    3 -
> arch/powerpc/kvm/powerpc.c          |  102 ++++++++++++++++++++++++++++++++++-
> include/asm-powerpc/kvm_host.h      |    4 +
> 
> 
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1212770972 18000
> # Node ID d917195fbef25227967df7a28b99c32bd71c6187
> # Parent  5149614e9c977f352e441c3a4ceca0b4664d2cda
> Add gdb break point support to PowerPC kvm
> 
> This patch adds the ability to use break points from a gdb stub in userpace (currently) qemu.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kvm/booke_guest.c b/arch/powerpc/kvm/booke_guest.c
> --- a/arch/powerpc/kvm/booke_guest.c
> +++ b/arch/powerpc/kvm/booke_guest.c
> @@ -410,6 +410,31 @@ int kvmppc_handle_exit(struct kvm_run *r
>  		break;
>  	}
> 
> +	case BOOKE_INTERRUPT_DEBUG: {
> +		u32 dbsr;
> +		struct kvm_guest_debug *dbg = &vcpu->guest_debug;
> +
> +		vcpu->arch.pc = mfspr(SPRN_CSRR0);
> +
> +		/* clear events in DBSR register */
> +		dbsr = mfspr(SPRN_DBSR);
> +
> +		if (vcpu->arch.pc = dbg->bp[0])
> +			dbsr |= 1<<23; /* clear IAC1 event */
> +		if (vcpu->arch.pc = dbg->bp[1])
> +			dbsr |= 1<<22; /* clear IAC2 event */
> +		if (vcpu->arch.pc = dbg->bp[2])
> +			dbsr |= 1<<21; /* clear IAC3 event */
> +		if (vcpu->arch.pc = dbg->bp[3])
> +			dbsr |= 1<<20; /* clear IAC4 event */

This doesn't seem like the right thing to do here. What if the
breakpoint isn't due to any of the IACs? What will happen if you leave
it pending in DBSR?

(At the very least you should be using DBSR_FOO constants.)

> +		mtspr (SPRN_DBSR, dbsr);
> +
> +		run->exit_reason = KVM_EXIT_DEBUG;
> +		r = RESUME_HOST;
> +		break;
> +	}
> +
>  	default:
>  		printk(KERN_EMERG "exit_nr %d\n", exit_nr);
>  		BUG();
> diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
> --- a/arch/powerpc/kvm/booke_interrupts.S
> +++ b/arch/powerpc/kvm/booke_interrupts.S
> @@ -42,7 +42,8 @@
>  #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
> 
>  #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
> -                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
> +                        (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
> +                        (1<<BOOKE_INTERRUPT_DEBUG))

I need to split the instruction copy from the nonvolatile GPR saving,
but that's my issue.

>  #define NEED_DEAR_MASK ((1<<BOOKE_INTERRUPT_DATA_STORAGE) | \
>                          (1<<BOOKE_INTERRUPT_DTLB_MISS))
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -232,18 +232,118 @@ void kvm_arch_vcpu_uninit(struct kvm_vcp
>  {
>  }
> 
> +static void disable_msr_debug_interrupts(void)
> +{
> +	u32 msr;
> +
> +	msr = mfmsr();
> +	msr &= ~MSR_DE;
> +	mtmsr(msr);
> +}
> +
> +static void restore_host_debug_state(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_guest_debug *dbg = &vcpu->guest_debug;
> +
> +	if (dbg->enabled) {
> +		disable_msr_debug_interrupts();
> +		
> +		mtspr(SPRN_IAC1, vcpu->arch.host_iac[0]);
> +		mtspr(SPRN_IAC2, vcpu->arch.host_iac[1]);
> +		mtspr(SPRN_IAC3, vcpu->arch.host_iac[2]);
> +		mtspr(SPRN_IAC4, vcpu->arch.host_iac[3]);
> +
> +		mtspr(SPRN_DBCR0, vcpu->arch.host_dbcr0);

You've omitted DBCR1 (and DBCR2) here.

> +		mtmsr(vcpu->arch.host_msr);
> +	}
> +}
> +
> +static void guest_load_debug_registers(struct kvm_vcpu *vcpu)
> +{

Please name these functions consistently.

> +	struct kvm_guest_debug *dbg = &vcpu->guest_debug;
> +	u32 dbcr0 = 0;
> +	u32 dbcr1 = 0;
> +
> +	if (dbg->enabled) {
> +		/* save hosts regs */
> +		/* save host msr */
> +		vcpu->arch.host_msr = mfmsr();
> +
> +		disable_msr_debug_interrupts();

What happens if an IAC actually fires between here and the guest?

> +		/* save host debug registers */
> +		vcpu->arch.host_iac[0] = mfspr(SPRN_IAC1);
> +		vcpu->arch.host_iac[1] = mfspr(SPRN_IAC2);
> +		vcpu->arch.host_iac[2] = mfspr(SPRN_IAC3);
> +		vcpu->arch.host_iac[3] = mfspr(SPRN_IAC4);
> +
> +		/* save host dbcr0 */
> +		vcpu->arch.host_dbcr0 = mfspr(SPRN_DBCR0);
> +
> +		/* save host dbcr1 */
> +		vcpu->arch.host_dbcr1 = mfspr(SPRN_DBCR1);
> +
> +		/* set registers up for guest */
> +		/* set dbcr0 & set iac */
> +
> +		if (dbg->bp[0]) {
> +			mtspr(SPRN_IAC1, dbg->bp[0]);
> +			dbcr0 |= DBCR0_IA1;
> +		}
> +		if (dbg->bp[1]) {
> +			mtspr(SPRN_IAC2, dbg->bp[1]);
> +			dbcr0 |= DBCR0_IA2;
> +		}
> +		if (dbg->bp[2]) {
> +			mtspr(SPRN_IAC3, dbg->bp[2]);
> +			dbcr0 |= DBCR0_IA3;
> +		}
> +		if (dbg->bp[3]) {
> +			mtspr(SPRN_IAC4, dbg->bp[3]);
> +			dbcr0 |= DBCR0_IA4;
> +		}
> +
> +		mtspr(SPRN_DBCR1, dbcr1);

Why not just
	mtspr(SPRN_DBCR1, 0);

What about DBCR2?

> +		mtspr(SPRN_DBCR0, dbcr0| DBCR0_IDM);
> +	}
> +}
> +
>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
> +	guest_load_debug_registers(vcpu);
>  }
> 
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  {
> +	restore_host_debug_state(vcpu);
>  }
> +
> +int guest_debug(struct kvm_vcpu *vcpu,
> +		struct kvm_debug_guest *dbg)
> +{
> +	int ret=0;
> +	int i;
> +
> +	vcpu->guest_debug.enabled = dbg->enabled;
> +	if (vcpu->guest_debug.enabled) {
> +		for (i=0; i < ARRAY_SIZE(vcpu->guest_debug.bp); i++) {
> +			if (dbg->breakpoints[i].enabled)
> +				vcpu->guest_debug.bp[i] = \ 
> +					dbg->breakpoints[i].address;
> +			else
> +				vcpu->guest_debug.bp[i] = 0;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> 
>  int kvm_arch_vcpu_ioctl_debug_guest(struct kvm_vcpu *vcpu,
>                                      struct kvm_debug_guest *dbg)
>  {
> -	return -ENOTSUPP;
> +	return guest_debug(vcpu, dbg);
>  }
> 
>  static void kvmppc_complete_dcr_load(struct kvm_vcpu *vcpu,
> diff --git a/include/asm-powerpc/kvm_host.h b/include/asm-powerpc/kvm_host.h
> --- a/include/asm-powerpc/kvm_host.h
> +++ b/include/asm-powerpc/kvm_host.h
> @@ -84,6 +84,10 @@ struct kvm_vcpu_arch {
> 
>  	u32 host_stack;
>  	u32 host_pid;
> +	u32 host_dbcr0;
> +	u32 host_dbcr1;
> +	u32 host_iac[4];
> +	u32 host_msr;
> 
>  	u64 fpr[32];
>  	u32 gpr[32];

-- 
Hollis Blanchard
IBM Linux Technology Center