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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux