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@xxxxxxxxxx>
> > > > # 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@xxxxxxxxxx>
> > > >
> > > > 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).
>
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
