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

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

[Jerone Young]

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 */
+
+		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))
 
 #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);
+
+		mtmsr(vcpu->arch.host_msr);
+	}
+}
+
+static void guest_load_debug_registers(struct kvm_vcpu *vcpu)
+{
+	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();
+
+		/* 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);
+		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];

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

[Hollis Blanchard]

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

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

[Jerone Young]

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

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

[Hollis Blanchard]

On Fri, 2008-06-06 at 14:11 -0500, Jerone Young wrote:
> 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.

That's my point: why are you using dbg->bp[] instead of DBSR bits? If
anything, I would expect an error message if unexpected DBSR bits were
set, and in any case:
	dbsr = mfspr(SPRN_DBSR);
	mtspr(SPRN_DBSR, 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. 

This isn't about IAC events: you clear all of those.

Yes, we won't "lose" any events. Instead, we will take the same
interrupts whenever MSR[DE] is set, and never clear them. Does that seem
like a good idea to you?

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

WTH is 1<<23 ?

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

And what if the host kernel uses DBCR2? This is exactly what I'm talking
about in the DBSR discussion above. Please fix.

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

After you add a "kvmppc_" prefix, it's getting a little long...

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

That is a totally separate issue. Imagine this in the host:

0xc0001000 load_guest_debug_registers:
	MSR[DE] = 0
	IAC[0] = 0xc0001080
	...
0xc0001080
	...
	enter guest

IAC1 matched. Now what happens?

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

I don't see why we'd need to restore DBSR, so I don't understand this
problem you're thinking about at all.

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

Yes it is (see previous comments).

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Jerone Young]

Didn't get this email till now .. odd .. so didn't put many of the
changes in the latest patch.

On Fri, 2008-06-06 at 14:27 -0500, Hollis Blanchard wrote:
> On Fri, 2008-06-06 at 14:11 -0500, Jerone Young wrote:
> > 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.
> 
> That's my point: why are you using dbg->bp[] instead of DBSR bits? If
> anything, I would expect an error message if unexpected DBSR bits were
> set, and in any case:
> 	dbsr = mfspr(SPRN_DBSR);
> 	mtspr(SPRN_DBSR, dbsr);

Oh the issue is if 2 debug interrupts got off from two different
registers. At least that was my train of the thought. Though the fact if
that does happen that would mean they both had the exact same
breakpoint. So we would only wat to process one of them. 

> > >  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. 
> 
> This isn't about IAC events: you clear all of those.
> 
> Yes, we won't "lose" any events. Instead, we will take the same
> interrupts whenever MSR[DE] is set, and never clear them. Does that seem
> like a good idea to you?
> 
> > > 
> > > (At the very least you should be using DBSR_FOO constants.)
> > ??
> 
> WTH is 1<<23 ?

I changed this to use the definitions.

> 
> > > 
> > > > +		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.
> 
> And what if the host kernel uses DBCR2? This is exactly what I'm talking
> about in the DBSR discussion above. Please fix.
> 
this was fixed in last patch.

> > > 
> > > > +		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.
> 
> After you add a "kvmppc_" prefix, it's getting a little long...
> 
> > > 
> > > > +	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.
> 
> That is a totally separate issue. Imagine this in the host:
> 
> 0xc0001000 load_guest_debug_registers:
> 	MSR[DE] = 0
> 	IAC[0] = 0xc0001080
> 	...
> 0xc0001080
> 	...
> 	enter guest
> 
> IAC1 matched. Now what happens?
> 
> > 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.
> 
> I don't see why we'd need to restore DBSR, so I don't understand this
> problem you're thinking about at all.


> 
> > > 
> > > > +		/* 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.
> 
> Yes it is (see previous comments).
> 

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

[Hollis Blanchard]

On Fri, 2008-06-06 at 15:23 -0500, Jerone Young wrote:
> 
> > > > > +       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.
> > 
> > That's my point: why are you using dbg->bp[] instead of DBSR bits?
> If
> > anything, I would expect an error message if unexpected DBSR bits
> were
> > set, and in any case:
> >       dbsr = mfspr(SPRN_DBSR);
> >       mtspr(SPRN_DBSR, dbsr);
> 
> Oh the issue is if 2 debug interrupts got off from two different
> registers. At least that was my train of the thought. Though the fact
> if
> that does happen that would mean they both had the exact same
> breakpoint. So we would only wat to process one of them. 

That may be your train of thought, but it's not mine. Please reread my
comments and let me know if you still have questions.

-- 
Hollis Blanchard
IBM Linux Technology Center