[PATCH] [v5] Add gdb break point support to PowerPC kvm

[PATCH] [v5] Add gdb break point support to PowerPC kvm

[Jerone Young]

4 files changed, 128 insertions(+), 2 deletions(-)
arch/powerpc/kvm/booke_guest.c      |   16 +++++
arch/powerpc/kvm/booke_interrupts.S |    3 
arch/powerpc/kvm/powerpc.c          |  106 ++++++++++++++++++++++++++++++++++-
include/asm-powerpc/kvm_host.h      |    5 +


 * Boiled down conditional for dbsr to one line, when handling debug interrupts.
   Renamed load_guest_debug_registers() to guest_load_debug_registers()

This patch adds the ability to use breakpoints from a gdb stub in userpace (currently qemu). It does this through the use of hardware debug registers.

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,22 @@ int kvmppc_handle_exit(struct kvm_run *r
 		break;
 	}
 
+	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 = 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,122 @@ 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_DBCR1, vcpu->arch.host_dbcr1);
+		mtspr(SPRN_DBCR2, vcpu->arch.host_dbcr2);
+		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;
+
+	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);
+
+		/* save host dbcr2 */
+		vcpu->arch.host_dbcr2 = mfspr(SPRN_DBCR2);		
+
+		/* set registers up for guest */
+		/* set dbcr0 & set iac */
+
+		if (dbg->bp[0]) {
+			mtspr(SPRN_IAC1, dbg->bp[0]);
+			dbcr0 |= DBCR0_IAC1;
+		}
+		if (dbg->bp[1]) {
+			mtspr(SPRN_IAC2, dbg->bp[1]);
+			dbcr0 |= DBCR0_IAC2;
+		}
+		if (dbg->bp[2]) {
+			mtspr(SPRN_IAC3, dbg->bp[2]);
+			dbcr0 |= DBCR0_IAC3;
+		}
+		if (dbg->bp[3]) {
+			mtspr(SPRN_IAC4, dbg->bp[3]);
+			dbcr0 |= DBCR0_IAC4;
+		}
+
+		mtspr(SPRN_DBCR1, 0);
+		mtspr(SPRN_DBCR2, 0);
+		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,11 @@ struct kvm_vcpu_arch {
 
 	u32 host_stack;
 	u32 host_pid;
+	u32 host_dbcr0;
+	u32 host_dbcr1;
+	u32 host_dbcr2;
+	u32 host_iac[4];
+	u32 host_msr;
 
 	u64 fpr[32];
 	u32 gpr[32];

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

[Hollis Blanchard]

On Mon, 2008-06-09 at 13:19 -0500, Jerone Young wrote:
> 4 files changed, 128 insertions(+), 2 deletions(-)
> arch/powerpc/kvm/booke_guest.c      |   16 +++++
> arch/powerpc/kvm/booke_interrupts.S |    3 
> arch/powerpc/kvm/powerpc.c          |  106 ++++++++++++++++++++++++++++++++++-
> include/asm-powerpc/kvm_host.h      |    5 +
> 
> 
>  * Boiled down conditional for dbsr to one line, when handling debug interrupts.
>    Renamed load_guest_debug_registers() to guest_load_debug_registers()
> 
> This patch adds the ability to use breakpoints from a gdb stub in userpace (currently qemu). It does this through the use of hardware debug registers.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

You still have not renamed restore_host_debug_state() and
guest_load_debug_registers(), which you agreed to previously.

You still have not renamed guest_debug(), which you also agreed to
previously. I don't see why you have a separate function for this
anyways.

You also have not addressed the case I pointed out where an IAC matches
after you've programmed it but before you've entered the guest.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Jerone Young]

On Mon, 2008-06-09 at 15:51 -0500, Hollis Blanchard wrote:
> On Mon, 2008-06-09 at 13:19 -0500, Jerone Young wrote:
> > 4 files changed, 128 insertions(+), 2 deletions(-)
> > arch/powerpc/kvm/booke_guest.c      |   16 +++++
> > arch/powerpc/kvm/booke_interrupts.S |    3 
> > arch/powerpc/kvm/powerpc.c          |  106 ++++++++++++++++++++++++++++++++++-
> > include/asm-powerpc/kvm_host.h      |    5 +
> > 
> > 
> >  * Boiled down conditional for dbsr to one line, when handling debug interrupts.
> >    Renamed load_guest_debug_registers() to guest_load_debug_registers()
> > 
> > This patch adds the ability to use breakpoints from a gdb stub in userpace (currently qemu). It does this through the use of hardware debug registers.
> > 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> You still have not renamed restore_host_debug_state() and
> guest_load_debug_registers(), which you agreed to previously.

My bad.  I was mainly just addressing the issues we went over this
morning. I'll fix the names. 

> 
> You still have not renamed guest_debug(), which you also agreed to
> previously. I don't see why you have a separate function for this
> anyways.
> 
> You also have not addressed the case I pointed out where an IAC matches
> after you've programmed it but before you've entered the guest.

Basically can disable them in the dbcr0 before we do any switching out
of registers.

If an IAC is matched before we enter the guest it will be caught host
that catches it.  

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

[Hollis Blanchard]

On Mon, 2008-06-09 at 16:08 -0500, Jerone Young wrote:
> 
> > You also have not addressed the case I pointed out where an IAC
> matches
> > after you've programmed it but before you've entered the guest.
> 
> Basically can disable them in the dbcr0 before we do any switching out
> of registers.

We'll have to enable DBCR0 some time, right?

> If an IAC is matched before we enter the guest it will be caught host
> that catches it.

But that won't happen because you've disabled MSR[DE].

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Jerone Young]

On Mon, 2008-06-09 at 17:56 -0500, Hollis Blanchard wrote:
> On Mon, 2008-06-09 at 16:08 -0500, Jerone Young wrote:
> > 
> > > You also have not addressed the case I pointed out where an IAC
> > matches
> > > after you've programmed it but before you've entered the guest.
> > 
> > Basically can disable them in the dbcr0 before we do any switching out
> > of registers.
> 
> We'll have to enable DBCR0 some time, right?
> 
Yes it gets enabled when when the guest cuts on the bits it wants & the
we restre the host DBCR0.


> > If an IAC is matched before we enter the guest it will be caught host
> > that catches it.
> 
> But that won't happen because you've disabled MSR[DE].

Ah... you don't rember the whole imprecise interrupts ;-) . When MSR[DE]
=0 and an IAC will go off but not at the exact address. This was fun
figuring out.

I also have a bug in this patch. I placed the wrong definitions when
disabling the debug interrupts. I'll respin again. Look things over more
carefully this time.


> 

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

[Hollis Blanchard]

On Mon, 2008-06-09 at 18:56 -0500, Jerone Young wrote:
> On Mon, 2008-06-09 at 17:56 -0500, Hollis Blanchard wrote:
> > On Mon, 2008-06-09 at 16:08 -0500, Jerone Young wrote:
> > > 
> > > > You also have not addressed the case I pointed out where an IAC
> > > matches
> > > > after you've programmed it but before you've entered the guest.
> > > 
> > > Basically can disable them in the dbcr0 before we do any switching out
> > > of registers.
> > 
> > We'll have to enable DBCR0 some time, right?
> > 
> Yes it gets enabled when when the guest cuts on the bits it wants & the
> we restre the host DBCR0.
> 
> 
> > > If an IAC is matched before we enter the guest it will be caught host
> > > that catches it.
> > 
> > But that won't happen because you've disabled MSR[DE].
> 
> Ah... you don't rember the whole imprecise interrupts ;-) . When MSR[DE]
> =0 and an IAC will go off but not at the exact address. This was fun
> figuring out.

Right. Now, when will the deferred debug event fire, and who will handle
it? What can we do about that?

> I also have a bug in this patch. I placed the wrong definitions when
> disabling the debug interrupts. I'll respin again. Look things over more
> carefully this time.

I think you're respinning these patches too frequently, before
conversation has concluded.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Jerone Young]

On Tue, 2008-06-10 at 10:12 -0500, Hollis Blanchard wrote:
> On Mon, 2008-06-09 at 18:56 -0500, Jerone Young wrote:
> > On Mon, 2008-06-09 at 17:56 -0500, Hollis Blanchard wrote:
> > > On Mon, 2008-06-09 at 16:08 -0500, Jerone Young wrote:
> > > > 
> > > > > You also have not addressed the case I pointed out where an IAC
> > > > matches
> > > > > after you've programmed it but before you've entered the guest.
> > > > 
> > > > Basically can disable them in the dbcr0 before we do any switching out
> > > > of registers.
> > > 
> > > We'll have to enable DBCR0 some time, right?
> > > 
> > Yes it gets enabled when when the guest cuts on the bits it wants & the
> > we restre the host DBCR0.
> > 
> > 
> > > > If an IAC is matched before we enter the guest it will be caught host
> > > > that catches it.
> > > 
> > > But that won't happen because you've disabled MSR[DE].
> > 
> > Ah... you don't rember the whole imprecise interrupts ;-) . When MSR[DE]
> > =0 and an IAC will go off but not at the exact address. This was fun
> > figuring out.
> 
> Right. Now, when will the deferred debug event fire, and who will handle
> it? What can we do about that?

If the debug registers are turned off before we disable MSR[DE], there
will be no debug events at all.

This is what I was talking about in my first couple of emails. If a
debug event does go off and the host does not handle it, the only way to
know is the dbsr register (and there is no way to save that off). So in
the end it could end up going off in the guest.  

This is why I said you should not do host debugging if you want to do
guest debugging at the sametime, as a work around for now.

Really can be addressed in a patch on top of this patch. Though I really
don't want to linger on this problem as it is not critical at the moment
as nobody is using these registers in the host currently.

I can clear the dbsr registers also when disabling the debug interrupts
as a solution?

> 
> > I also have a bug in this patch. I placed the wrong definitions when
> > disabling the debug interrupts. I'll respin again. Look things over more
> > carefully this time.
> 
> I think you're respinning these patches too frequently, before
> conversation has concluded.

I think respining has helped in the conversation to be honest. As I can
better demonstrate what I am thinking (though better without mistakes).
Also I figured this was concluded. I will not spin anymore patches till
we can agree on an answer for everything.

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

[Hollis Blanchard]

On Tue, 2008-06-10 at 10:33 -0500, Jerone Young wrote:
> On Tue, 2008-06-10 at 10:12 -0500, Hollis Blanchard wrote:
> > On Mon, 2008-06-09 at 18:56 -0500, Jerone Young wrote:
> > > > > If an IAC is matched before we enter the guest it will be caught host
> > > > > that catches it.
> > > > 
> > > > But that won't happen because you've disabled MSR[DE].
> > > 
> > > Ah... you don't rember the whole imprecise interrupts ;-) . When MSR[DE]
> > > =0 and an IAC will go off but not at the exact address. This was fun
> > > figuring out.
> > 
> > Right. Now, when will the deferred debug event fire, and who will handle
> > it? What can we do about that?
> 
> If the debug registers are turned off before we disable MSR[DE], there
> will be no debug events at all.
> 
> This is what I was talking about in my first couple of emails. If a
> debug event does go off and the host does not handle it, the only way to
> know is the dbsr register (and there is no way to save that off). So in
> the end it could end up going off in the guest.  
> 
> This is why I said you should not do host debugging if you want to do
> guest debugging at the sametime, as a work around for now.

As I said before, forget host debugging!

In fact, here is the exact text I sent to you on Friday, which you don't
appear to have read, in which I said exactly the same thing:

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

Please think about this scenario.

> I can clear the dbsr registers also when disabling the debug interrupts
> as a solution?

I think you're on the right track, but what you propose here simply
won't affect the above scenario.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Jerone Young]

On Tue, 2008-06-10 at 10:45 -0500, Hollis Blanchard wrote:
> On Tue, 2008-06-10 at 10:33 -0500, Jerone Young wrote:
> > On Tue, 2008-06-10 at 10:12 -0500, Hollis Blanchard wrote:
> > > On Mon, 2008-06-09 at 18:56 -0500, Jerone Young wrote:
> > > > > > If an IAC is matched before we enter the guest it will be caught host
> > > > > > that catches it.
> > > > > 
> > > > > But that won't happen because you've disabled MSR[DE].
> > > > 
> > > > Ah... you don't rember the whole imprecise interrupts ;-) . When MSR[DE]
> > > > =0 and an IAC will go off but not at the exact address. This was fun
> > > > figuring out.
> > > 
> > > Right. Now, when will the deferred debug event fire, and who will handle
> > > it? What can we do about that?
> > 
> > If the debug registers are turned off before we disable MSR[DE], there
> > will be no debug events at all.
> > 
> > This is what I was talking about in my first couple of emails. If a
> > debug event does go off and the host does not handle it, the only way to
> > know is the dbsr register (and there is no way to save that off). So in
> > the end it could end up going off in the guest.  
> > 
> > This is why I said you should not do host debugging if you want to do
> > guest debugging at the sametime, as a work around for now.
> 
> As I said before, forget host debugging!

If it's not about host debugging then I'm confused where you are going
with this. Please be more clear. 

> 
> In fact, here is the exact text I sent to you on Friday, which you don't
> appear to have read, in which I said exactly the same thing:
> 
> On Fri, 2008-06-06 at 14:27 -0500, Hollis Blanchard wrote:
> > On Fri, 2008-06-06 at 14:11 -0500, Jerone Young wrote:
> > > 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?
> 
> Please think about this scenario.

Again I'm lost on where you are going here. We are in the guest it
matches. I'm not sure what you are trying to say here?

> 
> > I can clear the dbsr registers also when disabling the debug interrupts
> > as a solution?
> 
> I think you're on the right track, but what you propose here simply
> won't affect the above scenario.
> 

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

[Jerone Young]

On Tue, 2008-06-10 at 10:45 -0500, Hollis Blanchard wrote:
> On Tue, 2008-06-10 at 10:33 -0500, Jerone Young wrote:
> > On Tue, 2008-06-10 at 10:12 -0500, Hollis Blanchard wrote:
> > > On Mon, 2008-06-09 at 18:56 -0500, Jerone Young wrote:
> > > > > > If an IAC is matched before we enter the guest it will be caught host
> > > > > > that catches it.
> > > > > 
> > > > > But that won't happen because you've disabled MSR[DE].
> > > > 
> > > > Ah... you don't rember the whole imprecise interrupts ;-) . When MSR[DE]
> > > > =0 and an IAC will go off but not at the exact address. This was fun
> > > > figuring out.
> > > 
> > > Right. Now, when will the deferred debug event fire, and who will handle
> > > it? What can we do about that?
> > 
> > If the debug registers are turned off before we disable MSR[DE], there
> > will be no debug events at all.
> > 
> > This is what I was talking about in my first couple of emails. If a
> > debug event does go off and the host does not handle it, the only way to
> > know is the dbsr register (and there is no way to save that off). So in
> > the end it could end up going off in the guest.  
> > 
> > This is why I said you should not do host debugging if you want to do
> > guest debugging at the sametime, as a work around for now.
> 
> As I said before, forget host debugging!
> 
> In fact, here is the exact text I sent to you on Friday, which you don't
> appear to have read, in which I said exactly the same thing:
> 
> On Fri, 2008-06-06 at 14:27 -0500, Hollis Blanchard wrote:
> > On Fri, 2008-06-06 at 14:11 -0500, Jerone Young wrote:
> > > 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?
> 
> Please think about this scenario.

Oh it hit me. So we *could* get an imprecise interrupt once we enable
dbcr0. 

So do you want to enable dbcr0 once we enable MSR_DE in
__kvmppc_vcpu_run() ?

(In the future please just plainly state the issue..it saves
emails..which I guess saves trees .. in someway).

> 
> > I can clear the dbsr registers also when disabling the debug interrupts
> > as a solution?
> 
> I think you're on the right track, but what you propose here simply
> won't affect the above scenario.
>