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

[Jerone Young <jyoung5@us.ibm.com>]

On Fri, 2008-06-06 at 13:10 -0500, Hollis Blanchard wrote:
> 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?

If it is an IAC register that triggered the event then you have to clear
it. The exact event in the DBSR.

>  What will happen if you leave
> it pending in DBSR?

Basically it will fire an interrupt until it is cleared. So this way we
don't loose any IAC registers that we are not handling at that moment. 

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

Mainly for DBCR1. We don't have to mess with DBCR2 since we are not
using the DAC registers. DBCR2 mainly just controls the modes that the
DAC registers us. But it's DBCR0 that turns them on & off.

> 
> > +		mtmsr(vcpu->arch.host_msr);
> > +	}
> > +}
> > +
> > +static void guest_load_debug_registers(struct kvm_vcpu *vcpu)
> > +{
> 
> Please name these functions consistently.

Can rename load_guest_debug_registers.

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

That is an issue if you are using the IAC registers in the host also at
the same time.

The problem is there is no way to save off the dbsr and restore it. When
you manipulate the dbsr you are actually manipulating a mask, but not
actually writing certain bits (like the IAC debug event bits).

Definitely a potential issue. But of now just don't use IAC registers in
the host while wanting to use them with KVM.

> 
> > +		/* 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?
dbsr2 only is for the DAC registers so we don't have to deal with them.
Since DBCR2 controls the modes of the DAC, and we don't have the on in
DBCR0. This isn't needed.

> 
> > +		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];
>