diff mbox

[2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest

Message ID 65676863af53426ab5993d1b45b360e3@BLUPR03MB566.namprd03.prod.outlook.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bharat Bhushan July 2, 2014, 11:37 a.m. UTC 
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Tuesday, July 01, 2014 10:11 PM

> To: Alexander Graf

> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org

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

-----------------------------------------------------------------------


------------------

Thanks
-Bharat

> 

> -Scott

>
diff mbox

Patch

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