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