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