> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 01, 2014 10:11 PM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to
> guest
>
> On Tue, 2014-07-01 at 18:22 +0200, Alexander Graf wrote:
> > On 01.07.14 17:35, Scott Wood wrote:
> > > On Tue, 2014-07-01 at 17:04 +0200, Alexander Graf wrote:
> > >> On 01.07.14 16:58, Scott Wood wrote:
> > >>> On Tue, 2014-07-01 at 08:23 +0200, Alexander Graf wrote:
> > >>>> I don't think QEMU should be aware of these limitations.
> > >>> OK, but we should at least have some idea of how the whole thing
> > >>> is supposed to work, in order to determine if this is the correct
> > >>> behavior for QEMU. I thought the model was that debug resources
> > >>> are either owned by QEMU or by the guest, and in the latter case,
> > >>> QEMU would never see the debug exception to begin with.
> > >> That's bad for a number of reasons. For starters it's different
> > >> from how
> > >> x86 handles debug registers - and I hate to be different just for
> > >> the sake of being different.
> > > How does it work on x86?
> >
> > It overwrites more-or-less random breakpoints with its own ones, but
> > leaves the others intact ;).
>
> Are you talking about software breakpoints or management of hardware debug
> registers?
>
> > >> So if we do want to declare that debug registers are owned by
> > >> either QEMU or the guest, we should change the semantics for all
> > >> architectures.
> > > If we want to say that ownership of the registers is shared, we need
> > > a plan for how that would actually work.
> >
> > I think you're overengineering here :). When do people actually use
> > gdbstub? Usually when they want to debug a broken guest. We can either
> >
> > * overengineer heavily and reduce the number of registers available
> > to the guest to always have spares
> > * overengineer a bit and turn off guest debugging completely when
> > we use gdbstub
> > * just leave as much alive as we can, hoping that it helps with the
> > debugging
> >
> > Option 3 is what x86 does - and I think it's a reasonable approach.
> > This is not an interface that needs to be 100% consistent and bullet
> > proof, it's a best effort to enable you to debug as much as possible.
>
> I'm not insisting on 100% -- just hoping for some explanation/discussion about
> how it's intended to work for the cases where it can.
>
> How will MSR[DE] and MSRP[DEP] be handled?
>
> How would I go about telling QEMU/KVM that I don't want this shared mode,
> because I don't want guest to interfere with the debugging I'm trying to do from
> QEMU?
>
> Will guest accesses to debug registers cause a userspace exit when guest_debug
> is enabled?
>
> > >> I think we're in a path that is slow enough already to not worry
> > >> about performance.
> > > It's not just about performance, but simplicity of use, and
> > > consistency of API.
> > >
> > > Oh, and it looks like there already exist one reg definitions and
> > > implementations for most of the debug registers.
> >
> > For BookE? Where?
>
> arch/powerpc/kvm/booke.c: KVM_REG_PPC_IACn, KVM_REG_PPC_DACn
I tried to quickly prototype what I think we want to do (this is not tested)
-----------------------------------------------------------------------
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index e8b3982..746b5c6 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -179,6 +179,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, u32 *server,
extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
+void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu);
+void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
+
/*
* Cuts out inst bits with ordering according to spec.
* That means the leftmost bit is zero. All given bits are included.
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 9f13056..0b7e4e4 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -235,6 +235,16 @@ void kvmppc_core_dequeue_dec(struct kvm_vcpu *vcpu)
clear_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions);
}
+void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
+{
+ kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
+}
+
+void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
+{
+ clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions);
+}
+
void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
struct kvm_interrupt *irq)
{
@@ -841,6 +851,20 @@ static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
u32 dbsr = vcpu->arch.dbsr;
+ /* Userspace (QEMU) is not using debug resource, so inject debug interrupt
+ * directly to guest debug.
+ */
+ if (vcpu->guest_debug == 0) {
+ if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
+ kvmppc_core_queue_debug(vcpu);
+
+ /* Inject a program interrupt if trap debug is not allowed */
+ if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE))
+ kvmppc_core_queue_program(vcpu, ESR_PTR);
+
+ return RESUME_GUEST;
+ }
+
run->debug.arch.status = 0;
run->debug.arch.address = vcpu->arch.pc;
@@ -1920,7 +1944,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
vcpu->guest_debug = dbg->control;
vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
/* Set DBCR0_EDM in guest visible DBCR0 register. */
- vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
+// vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index aaff1b7..ef8de7f 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -26,6 +26,7 @@
#define OP_19_XOP_RFMCI 38
#define OP_19_XOP_RFI 50
#define OP_19_XOP_RFCI 51
+#define OP_19_XOP_RFDI 39
#define OP_31_XOP_MFMSR 83
#define OP_31_XOP_WRTEE 131
@@ -38,10 +39,24 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
kvmppc_set_msr(vcpu, vcpu->arch.shared->srr1);
}
+static void kvmppc_emul_rfdi(struct kvm_vcpu *vcpu)
+{
+ vcpu->arch.pc = vcpu->arch.dsrr0;
+ /* Force MSR_DE when guest does not own debug facilities */
+ if (vcpu->guest_debug)
+ kvmppc_set_msr(vcpu, vcpu->arch.dsrr1 | MSR_DE);
+ else
+ kvmppc_set_msr(vcpu, vcpu->arch.dsrr1);
+}
+
static void kvmppc_emul_rfci(struct kvm_vcpu *vcpu)
{
vcpu->arch.pc = vcpu->arch.csrr0;
- kvmppc_set_msr(vcpu, vcpu->arch.csrr1);
+ /* Force MSR_DE when guest does not own debug facilities */
+ if (vcpu->guest_debug)
+ kvmppc_set_msr(vcpu, vcpu->arch.csrr1 | MSR_DE);
+ else
+ kvmppc_set_msr(vcpu, vcpu->arch.csrr1);
}
static void kvmppc_emul_rfmci(struct kvm_vcpu *vcpu)
@@ -78,6 +93,12 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
*advance = 0;
break;
+ case OP_19_XOP_RFDI:
+ kvmppc_emul_rfdi(vcpu);
+// kvmppc_set_exit_type(vcpu, EMULATED_RFDI_EXITS);
+ *advance = 0;
+ break;
+
default:
emulated = EMULATE_FAIL;
break;
@@ -131,6 +152,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
{
int emulated = EMULATE_DONE;
+ bool debug_inst = false;
switch (sprn) {
case SPRN_DEAR:
@@ -145,21 +167,74 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
case SPRN_CSRR1:
vcpu->arch.csrr1 = spr_val;
break;
- case SPRN_DBCR0:
- vcpu->arch.dbg_reg.dbcr0 = spr_val;
+ case SPRN_DSRR0:
+ vcpu->arch.dsrr0 = spr_val;
break;
- case SPRN_DBCR1:
- vcpu->arch.dbg_reg.dbcr1 = spr_val;
+ case SPRN_DSRR1:
+ vcpu->arch.dsrr1 = spr_val;
+ break;
+ case SPRN_IAC1:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.iac1 = spr_val;
+ vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
+ break;
+ case SPRN_IAC2:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.iac2 = spr_val;
+ vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
+ break;
+#ifndef CONFIG_PPC_FSL_BOOK3E
+ case SPRN_IAC3:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.iac3 = spr_val;
+ vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
break;
+ case SPRN_IAC4:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.iac4 = spr_val;
+ vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
+ break;
+#endif
+ case SPRN_DAC1:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.dac1 = spr_val;
+ vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
+ break;
+ case SPRN_DAC2:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.dac2 = spr_val;
+ vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
+ break;
+ case SPRN_DBCR0:
+ debug_inst = true;
+ spr_val &= (DBCR0_IDM | DBCR0_IC | DBCR0_BT | DBCR0_TIE |
+ DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 |
+ DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W);
+
+ vcpu->arch.dbg_reg.dbcr0 = spr_val;
+ vcpu->arch.shadow_dbg_reg.dbcr0 = spr_val;
+ break;
+ case SPRN_DBCR1:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.dbcr1 = spr_val;
+ vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
+ break;
+ case SPRN_DBCR2:
+ debug_inst = true;
+ vcpu->arch.dbg_reg.dbcr2 = spr_val;
+ vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
+ break;
+ case SPRN_DBSR:
+ vcpu->arch.dbsr &= ~spr_val;
+ if (vcpu->arch.dbsr == 0)
+ kvmppc_core_dequeue_debug(vcpu);
+ break;
case SPRN_MCSRR0:
vcpu->arch.mcsrr0 = spr_val;
break;
case SPRN_MCSRR1:
vcpu->arch.mcsrr1 = spr_val;
break;
- case SPRN_DBSR:
- vcpu->arch.dbsr &= ~spr_val;
- break;
case SPRN_TSR:
kvmppc_clr_tsr_bits(vcpu, spr_val);
break;
@@ -271,6 +346,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
emulated = EMULATE_FAIL;
}
+ if (debug_inst) {
+ switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg);
+ current->thread.debug = vcpu->arch.shadow_dbg_reg;
+ }
return emulated;
}
@@ -297,12 +376,41 @@ int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val)
case SPRN_CSRR1:
*spr_val = vcpu->arch.csrr1;
break;
+ case SPRN_DSRR0:
+ *spr_val = vcpu->arch.dsrr0;
+ break;
+ case SPRN_DSRR1:
+ *spr_val = vcpu->arch.dsrr1;
+ break;
+ case SPRN_IAC1:
+ *spr_val = vcpu->arch.dbg_reg.iac1;
+ break;
+ case SPRN_IAC2:
+ *spr_val = vcpu->arch.dbg_reg.iac2;
+ break;
+#ifndef CONFIG_PPC_FSL_BOOK3E
+ case SPRN_IAC3:
+ *spr_val = vcpu->arch.dbg_reg.iac3;
+ break;
+ case SPRN_IAC4:
+ *spr_val = vcpu->arch.dbg_reg.iac4;
+ break;
+#endif
+ case SPRN_DAC1:
+ *spr_val = vcpu->arch.dbg_reg.dac1;
+ break;
+ case SPRN_DAC2:
+ *spr_val = vcpu->arch.dbg_reg.dac2;
+ break;
case SPRN_DBCR0:
*spr_val = vcpu->arch.dbg_reg.dbcr0;
break;
case SPRN_DBCR1:
*spr_val = vcpu->arch.dbg_reg.dbcr1;
break;
+ case SPRN_DBCR2:
+ *spr_val = vcpu->arch.dbg_reg.dbcr2;
+ break;
case SPRN_MCSRR0:
*spr_val = vcpu->arch.mcsrr0;
break;
diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index bbf7cdd..560b576 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -249,7 +249,7 @@ int kvmppc_core_vcpu_setup(struct kvm_vcpu *vcpu)
#ifdef CONFIG_64BIT
vcpu->arch.shadow_epcr |= SPRN_EPCR_ICM;
#endif
- vcpu->arch.shadow_msrp = MSRP_UCLEP | MSRP_DEP | MSRP_PMMP;
+ vcpu->arch.shadow_msrp = MSRP_UCLEP | MSRP_PMMP;
vcpu->arch.pvr = mfspr(SPRN_PVR);
vcpu_e500->svr = mfspr(SPRN_SVR);
------------------
Thanks
-Bharat
>
> -Scott
>
��.n��������+%������w��{.n�����o��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
