[PATCH 0/6] Guest debug emulation

[PATCH 0/6] Guest debug emulation

[Bharat Bhushan]

This patchset adds debug register and interrupt emulation support
for guest, which enables running gdb/kgdb etc in guest.
This also have couple of bux fixes

Bharat Bhushan (6):
  KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register
  KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug
  KVM: PPC: BOOKE: allow debug interrupt at "debug level"
  KVM: PPC: BOOKE : Emulate rfdi instruction
  KVM: PPC: BOOKE: Allow guest to change MSR_DE
  KVM: PPC: BOOKE: Emulate debug registers and exception

 arch/powerpc/include/asm/kvm_host.h |   1 +
 arch/powerpc/include/asm/kvm_ppc.h  |   3 +
 arch/powerpc/kvm/booke.c            |  35 ++++++-
 arch/powerpc/kvm/booke_emulate.c    | 180 +++++++++++++++++++++++++++++++++++-
 arch/powerpc/kvm/e500mc.c           |   2 +-
 5 files changed, 216 insertions(+), 5 deletions(-)

-- 
1.9.3

[PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Bharat Bhushan]

This is not used and  even I do not remember why this was added
in first place.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/kvm/booke.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index ab62109..a5ee42c 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 	kvm_guest_protect_msr(vcpu, MSR_DE, true);
 	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;
 
 	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
 		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
-- 
1.9.3

[PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug

[Bharat Bhushan]

When userspace (QEMU) is using the debug resource to debug guest
then we want MSR_DE to be always set. This patch adds missing
MSR_DE setting in "rfci" instruction.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/kvm/booke_emulate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index 27a4b28..80c51a2 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -40,7 +40,11 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
 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);
 }
 
 int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
-- 
1.9.3

[PATCH 3/6] KVM: PPC: BOOKE: allow debug interrupt at "debug level"

[Bharat Bhushan]

Debug interrupt can be either "critical level" or "debug level".
There are separate set of save/restore registers used for different level.
Example: DSRR0/DSRR1 are used for "debug level" and CSRR0/CSRR1
are used for critical level debug interrupt.

Using CPU_FTR_DEBUG_LVL_EXC to decide which interrupt level to be used.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/kvm/booke.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index a5ee42c..fadfe76 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -424,7 +424,11 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
 		allowed = vcpu->arch.shared->msr & MSR_DE;
 		allowed = allowed && !crit;
 		msr_mask = MSR_ME;
-		int_class = INT_CLASS_CRIT;
+		if (cpu_has_feature(CPU_FTR_DEBUG_LVL_EXC))
+			int_class = INT_CLASS_DBG;
+		else
+			int_class = INT_CLASS_CRIT;
+
 		break;
 	}
 
-- 
1.9.3

[PATCH 4/6] KVM: PPC: BOOKE : Emulate rfdi instruction

[Bharat Bhushan]

This patch adds "rfdi" instruction emulation which is required for
guest debug hander on BOOKE-HV

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/include/asm/kvm_host.h |  1 +
 arch/powerpc/kvm/booke_emulate.c    | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 855ba4d..372b977 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -149,6 +149,7 @@ enum kvm_exit_types {
 	EMULATED_TLBWE_EXITS,
 	EMULATED_RFI_EXITS,
 	EMULATED_RFCI_EXITS,
+	EMULATED_RFDI_EXITS,
 	DEC_EXITS,
 	EXT_INTR_EXITS,
 	HALT_WAKEUP,
diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index 80c51a2..2a20194 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -25,6 +25,7 @@
 
 #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
@@ -37,6 +38,16 @@ 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;
@@ -69,6 +80,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;
-- 
1.9.3

[PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE

[Bharat Bhushan]

When userspace is debugging guest then MSR_DE is always set and
MSRP_DEP is set so that guest cannot change MSR_DE.
Guest debug resources are not yet emulated, So there seems no reason
we should stop guest controlling MSR_DE.
Also a followup patch will enable debug emulation and that requires
guest to control MSR_DE.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/kvm/e500mc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index 690499d..bd0a2bd 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -194,7 +194,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.eplc = EPC_EGS | (vcpu->kvm->arch.lpid << EPC_ELPID_SHIFT);
 	vcpu->arch.epsc = vcpu->arch.eplc;
 
-- 
1.9.3

[PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Bharat Bhushan]

This patch emulates debug registers and debug exception
to support guest using debug resource. This enables running
gdb/kgdb etc in guest.

On BOOKE architecture we cannot share debug resources between QEMU and
guest because:
    When QEMU is using debug resources then debug exception must
    be always enabled. To achieve this we set MSR_DE and also set
    MSRP_DEP so guest cannot change MSR_DE.

    When emulating debug resource for guest we want guest
    to control MSR_DE (enable/disable debug interrupt on need).

    So above mentioned two configuration cannot be supported
    at the same time. So the result is that we cannot share
    debug resources between QEMU and Guest on BOOKE architecture.

In the current design QEMU gets priority over guest, this means that if
QEMU is using debug resources then guest cannot use them and if guest is
using debug resource then QEMU can overwrite them.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
Hi Alex,

I thought of having some print in register emulation if QEMU
is using debug resource, Also when QEMU overwrites guest written
values but that looks excessive. If I uses some variable which
get set when guest starts using debug registers and check in
debug set ioctl then that look ugly. Looking for suggestions

 arch/powerpc/include/asm/kvm_ppc.h |   3 +
 arch/powerpc/kvm/booke.c           |  27 +++++++
 arch/powerpc/kvm/booke_emulate.c   | 157 +++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index e2fd5a1..f3f7611 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -173,6 +173,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);
+
 union kvmppc_one_reg {
 	u32	wval;
 	u64	dval;
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index fadfe76..c2471ed 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu)
 	clear_bit(BOOKE_IRQPRIO_WATCHDOG, &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);
+}
+
 static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1)
 {
 #ifdef CONFIG_KVM_BOOKE_HV
@@ -783,6 +793,23 @@ 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;
 
+	if (vcpu->guest_debug == 0) {
+		/* Debug resources belong to Guest */
+		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;
+	}
+
+	/*
+	 * Prepare for userspace exit as debug resources
+	 * are owned by userspace.
+	 */
+	vcpu->arch.dbsr = 0;
 	run->debug.arch.status = 0;
 	run->debug.arch.address = vcpu->arch.pc;
 
diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index 2a20194..3d143fe 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -139,6 +139,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:
@@ -153,14 +154,137 @@ 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_DSRR0:
+		vcpu->arch.dsrr0 = spr_val;
+		break;
+	case SPRN_DSRR1:
+		vcpu->arch.dsrr1 = spr_val;
+		break;
+	case SPRN_IAC1:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.iac1 = spr_val;
+		vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
+		break;
+	case SPRN_IAC2:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.iac2 = spr_val;
+		vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
+		break;
+#if CONFIG_PPC_ADV_DEBUG_IACS > 2
+	case SPRN_IAC3:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.iac3 = spr_val;
+		vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
+		break;
+	case SPRN_IAC4:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.iac4 = spr_val;
+		vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
+		break;
+#endif
+	case SPRN_DAC1:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.dac1 = spr_val;
+		vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
+		break;
+	case SPRN_DAC2:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.dac2 = spr_val;
+		vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
+		break;
 	case SPRN_DBCR0:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		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:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
 		vcpu->arch.dbg_reg.dbcr1 = spr_val;
+		vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
+		break;
+	case SPRN_DBCR2:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
+		debug_inst = true;
+		vcpu->arch.dbg_reg.dbcr2 = spr_val;
+		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
 		break;
 	case SPRN_DBSR:
+		/*
+		 * If userspace is debugging guest then guest
+		 * can not access debug registers.
+		 */
+		if (vcpu->guest_debug)
+			break;
+
 		vcpu->arch.dbsr &= ~spr_val;
+		if (vcpu->arch.dbsr == 0)
+			kvmppc_core_dequeue_debug(vcpu);
 		break;
 	case SPRN_TSR:
 		kvmppc_clr_tsr_bits(vcpu, spr_val);
@@ -273,6 +397,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;
 }
 
@@ -299,12 +427,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;
+#if CONFIG_PPC_ADV_DEBUG_IACS > 2
+	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_DBSR:
 		*spr_val = vcpu->arch.dbsr;
 		break;
-- 
1.9.3

Re: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug

[Alexander Graf]

On 11.07.14 10:38, Bharat Bhushan wrote:
> When userspace (QEMU) is using the debug resource to debug guest
> then we want MSR_DE to be always set. This patch adds missing
> MSR_DE setting in "rfci" instruction.
>
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>

Shouldn't this be in kvmppc_set_msr() instead then to catch all users?


Alex

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Alexander Graf]

On 11.07.14 10:39, Bharat Bhushan wrote:
> This patch emulates debug registers and debug exception
> to support guest using debug resource. This enables running
> gdb/kgdb etc in guest.
>
> On BOOKE architecture we cannot share debug resources between QEMU and
> guest because:
>      When QEMU is using debug resources then debug exception must
>      be always enabled. To achieve this we set MSR_DE and also set
>      MSRP_DEP so guest cannot change MSR_DE.
>
>      When emulating debug resource for guest we want guest
>      to control MSR_DE (enable/disable debug interrupt on need).
>
>      So above mentioned two configuration cannot be supported
>      at the same time. So the result is that we cannot share
>      debug resources between QEMU and Guest on BOOKE architecture.
>
> In the current design QEMU gets priority over guest, this means that if
> QEMU is using debug resources then guest cannot use them and if guest is
> using debug resource then QEMU can overwrite them.
>
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
> Hi Alex,
>
> I thought of having some print in register emulation if QEMU
> is using debug resource, Also when QEMU overwrites guest written
> values but that looks excessive. If I uses some variable which
> get set when guest starts using debug registers and check in
> debug set ioctl then that look ugly. Looking for suggestions

Whatever you do, have QEMU do the print, not the kernel.

>
>   arch/powerpc/include/asm/kvm_ppc.h |   3 +
>   arch/powerpc/kvm/booke.c           |  27 +++++++
>   arch/powerpc/kvm/booke_emulate.c   | 157 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 187 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index e2fd5a1..f3f7611 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -173,6 +173,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);
> +
>   union kvmppc_one_reg {
>   	u32	wval;
>   	u64	dval;
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index fadfe76..c2471ed 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu)
>   	clear_bit(BOOKE_IRQPRIO_WATCHDOG, &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);
> +}
> +
>   static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1)
>   {
>   #ifdef CONFIG_KVM_BOOKE_HV
> @@ -783,6 +793,23 @@ 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;
>   
> +	if (vcpu->guest_debug == 0) {
> +		/* Debug resources belong to Guest */
> +		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);

In that case we would've received a program interrupt and never entered 
this code path, no?


Alex

Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> This is not used and  even I do not remember why this was added
> in first place.
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index ab62109..a5ee42c 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  	kvm_guest_protect_msr(vcpu, MSR_DE, true);
>  	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;
>  
>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
>  		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;

This was intended to let the guest know that the host owns the debug
resources, by analogy to what a JTAG debugger would do.

The Power ISA has this "Virtualized Implementation Note":

        It is the responsibility of the hypervisor to ensure that
        DBCR0[EDM] is consistent with usage of DEP.

-Scott

Re: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug

[Scott Wood]

On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> When userspace (QEMU) is using the debug resource to debug guest
> then we want MSR_DE to be always set. This patch adds missing
> MSR_DE setting in "rfci" instruction.
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>  arch/powerpc/kvm/booke_emulate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
> index 27a4b28..80c51a2 100644
> --- a/arch/powerpc/kvm/booke_emulate.c
> +++ b/arch/powerpc/kvm/booke_emulate.c
> @@ -40,7 +40,11 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
>  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);
>  }
>  
>  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,

It looks like this is already handled by kvmppc_vcpu_sync_debug(), which
is called by kvmppc_set_msr().

Plus, it should only be done for HV mode.

-Scott

Re: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE

[Scott Wood]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> When userspace is debugging guest then MSR_DE is always set and
> MSRP_DEP is set so that guest cannot change MSR_DE.
> Guest debug resources are not yet emulated, So there seems no reason
> we should stop guest controlling MSR_DE.
> Also a followup patch will enable debug emulation and that requires
> guest to control MSR_DE.

Why does it matter whether we emulate debug resources?  We still don't
want the guest to be able to clear MSR[DE] and thus break host debug.

-Scott

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> This patch emulates debug registers and debug exception
> to support guest using debug resource. This enables running
> gdb/kgdb etc in guest.
> 
> On BOOKE architecture we cannot share debug resources between QEMU and
> guest because:
>     When QEMU is using debug resources then debug exception must
>     be always enabled. To achieve this we set MSR_DE and also set
>     MSRP_DEP so guest cannot change MSR_DE.
>
>     When emulating debug resource for guest we want guest
>     to control MSR_DE (enable/disable debug interrupt on need).
> 
>     So above mentioned two configuration cannot be supported
>     at the same time. So the result is that we cannot share
>     debug resources between QEMU and Guest on BOOKE architecture.
> 
> In the current design QEMU gets priority over guest, this means that if
> QEMU is using debug resources then guest cannot use them and if guest is
> using debug resource then QEMU can overwrite them.
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
> Hi Alex,
> 
> I thought of having some print in register emulation if QEMU
> is using debug resource, Also when QEMU overwrites guest written
> values but that looks excessive. If I uses some variable which
> get set when guest starts using debug registers and check in
> debug set ioctl then that look ugly. Looking for suggestions
> 
>  arch/powerpc/include/asm/kvm_ppc.h |   3 +
>  arch/powerpc/kvm/booke.c           |  27 +++++++
>  arch/powerpc/kvm/booke_emulate.c   | 157 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index e2fd5a1..f3f7611 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -173,6 +173,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);
> +
>  union kvmppc_one_reg {
>  	u32	wval;
>  	u64	dval;
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index fadfe76..c2471ed 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu)
>  	clear_bit(BOOKE_IRQPRIO_WATCHDOG, &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);
> +}
> +
>  static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1)
>  {
>  #ifdef CONFIG_KVM_BOOKE_HV
> @@ -783,6 +793,23 @@ 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;
>  
> +	if (vcpu->guest_debug == 0) {
> +		/* Debug resources belong to Guest */
> +		if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> +			kvmppc_core_queue_debug(vcpu);

Should also check for DBCR0[IDM].

> +
> +		/* 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;
> +	}
> +
> +	/*
> +	 * Prepare for userspace exit as debug resources
> +	 * are owned by userspace.
> +	 */
> +	vcpu->arch.dbsr = 0;
>  	run->debug.arch.status = 0;
>  	run->debug.arch.address = vcpu->arch.pc;

Why are you clearing vcpu->arch.dbsr?  Userspace might be interested in
the raw value, plus it's a change from the current API semantics.

Where is this run->debug.arch.<foo> stuff and KVM_EXIT_DEBUG documented?


> diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
> index 2a20194..3d143fe 100644
> --- a/arch/powerpc/kvm/booke_emulate.c
> +++ b/arch/powerpc/kvm/booke_emulate.c
> @@ -139,6 +139,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:
> @@ -153,14 +154,137 @@ 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_DSRR0:
> +		vcpu->arch.dsrr0 = spr_val;
> +		break;
> +	case SPRN_DSRR1:
> +		vcpu->arch.dsrr1 = spr_val;
> +		break;
> +	case SPRN_IAC1:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac1 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
> +		break;
> +	case SPRN_IAC2:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac2 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
> +		break;
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> +	case SPRN_IAC3:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac3 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
> +		break;
> +	case SPRN_IAC4:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac4 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
> +		break;
> +#endif
> +	case SPRN_DAC1:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.dac1 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
> +		break;
> +	case SPRN_DAC2:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.dac2 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
> +		break;
>  	case SPRN_DBCR0:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		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:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
>  		vcpu->arch.dbg_reg.dbcr1 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
> +		break;
> +	case SPRN_DBCR2:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
>  		break;

In what circumstances can the architected and shadow registers differ?

>  	case SPRN_DBSR:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
>  		vcpu->arch.dbsr &= ~spr_val;
> +		if (vcpu->arch.dbsr == 0)
> +			kvmppc_core_dequeue_debug(vcpu);
>  		break;

Not all DBSR bits cause an exception, e.g. IDE and MRR.

>  	case SPRN_TSR:
>  		kvmppc_clr_tsr_bits(vcpu, spr_val);
> @@ -273,6 +397,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;
> +	}

Could you explain what's going on with regard to copying the registers
into current->thread.debug?  Why is it done after loading the registers
into the hardware (is there a race if we get preempted in the middle)?

-Scott
;

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
> On 11.07.14 10:39, Bharat Bhushan wrote:
> > This patch emulates debug registers and debug exception
> > to support guest using debug resource. This enables running
> > gdb/kgdb etc in guest.
> >
> > On BOOKE architecture we cannot share debug resources between QEMU and
> > guest because:
> >      When QEMU is using debug resources then debug exception must
> >      be always enabled. To achieve this we set MSR_DE and also set
> >      MSRP_DEP so guest cannot change MSR_DE.
> >
> >      When emulating debug resource for guest we want guest
> >      to control MSR_DE (enable/disable debug interrupt on need).
> >
> >      So above mentioned two configuration cannot be supported
> >      at the same time. So the result is that we cannot share
> >      debug resources between QEMU and Guest on BOOKE architecture.
> >
> > In the current design QEMU gets priority over guest, this means that if
> > QEMU is using debug resources then guest cannot use them and if guest is
> > using debug resource then QEMU can overwrite them.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> > Hi Alex,
> >
> > I thought of having some print in register emulation if QEMU
> > is using debug resource, Also when QEMU overwrites guest written
> > values but that looks excessive. If I uses some variable which
> > get set when guest starts using debug registers and check in
> > debug set ioctl then that look ugly. Looking for suggestions
> 
> Whatever you do, have QEMU do the print, not the kernel.

How would that be accomplished?  How would the kernel know to exit to
QEMU, and how would the exit reason be conveyed?

-Scott

Re: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE

[Alexander Graf]

On 29.07.14 00:01, Scott Wood wrote:
> On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
>> When userspace is debugging guest then MSR_DE is always set and
>> MSRP_DEP is set so that guest cannot change MSR_DE.
>> Guest debug resources are not yet emulated, So there seems no reason
>> we should stop guest controlling MSR_DE.
>> Also a followup patch will enable debug emulation and that requires
>> guest to control MSR_DE.
> Why does it matter whether we emulate debug resources?  We still don't
> want the guest to be able to clear MSR[DE] and thus break host debug.

The patch description is misleading. This patch changes the default of 
DEP to "guest controlled" when it boots up. Once QEMU wants control over 
the debug registers, it gets switched to "QEMU controlled" (that code is 
already there).


Alex

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Alexander Graf]

On 29.07.14 00:33, Scott Wood wrote:
> On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
>> On 11.07.14 10:39, Bharat Bhushan wrote:
>>> This patch emulates debug registers and debug exception
>>> to support guest using debug resource. This enables running
>>> gdb/kgdb etc in guest.
>>>
>>> On BOOKE architecture we cannot share debug resources between QEMU and
>>> guest because:
>>>       When QEMU is using debug resources then debug exception must
>>>       be always enabled. To achieve this we set MSR_DE and also set
>>>       MSRP_DEP so guest cannot change MSR_DE.
>>>
>>>       When emulating debug resource for guest we want guest
>>>       to control MSR_DE (enable/disable debug interrupt on need).
>>>
>>>       So above mentioned two configuration cannot be supported
>>>       at the same time. So the result is that we cannot share
>>>       debug resources between QEMU and Guest on BOOKE architecture.
>>>
>>> In the current design QEMU gets priority over guest, this means that if
>>> QEMU is using debug resources then guest cannot use them and if guest is
>>> using debug resource then QEMU can overwrite them.
>>>
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> ---
>>> Hi Alex,
>>>
>>> I thought of having some print in register emulation if QEMU
>>> is using debug resource, Also when QEMU overwrites guest written
>>> values but that looks excessive. If I uses some variable which
>>> get set when guest starts using debug registers and check in
>>> debug set ioctl then that look ugly. Looking for suggestions
>> Whatever you do, have QEMU do the print, not the kernel.
> How would that be accomplished?  How would the kernel know to exit to
> QEMU, and how would the exit reason be conveyed?

QEMU is the one forcefully enabling debug and overwriting guest debug 
registers, so it also knows when it did overwrite valid ones.


Alex

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
> On 29.07.14 00:33, Scott Wood wrote:
> > On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
> >> On 11.07.14 10:39, Bharat Bhushan wrote:
> >>> This patch emulates debug registers and debug exception
> >>> to support guest using debug resource. This enables running
> >>> gdb/kgdb etc in guest.
> >>>
> >>> On BOOKE architecture we cannot share debug resources between QEMU and
> >>> guest because:
> >>>       When QEMU is using debug resources then debug exception must
> >>>       be always enabled. To achieve this we set MSR_DE and also set
> >>>       MSRP_DEP so guest cannot change MSR_DE.
> >>>
> >>>       When emulating debug resource for guest we want guest
> >>>       to control MSR_DE (enable/disable debug interrupt on need).
> >>>
> >>>       So above mentioned two configuration cannot be supported
> >>>       at the same time. So the result is that we cannot share
> >>>       debug resources between QEMU and Guest on BOOKE architecture.
> >>>
> >>> In the current design QEMU gets priority over guest, this means that if
> >>> QEMU is using debug resources then guest cannot use them and if guest is
> >>> using debug resource then QEMU can overwrite them.
> >>>
> >>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>> ---
> >>> Hi Alex,
> >>>
> >>> I thought of having some print in register emulation if QEMU
> >>> is using debug resource, Also when QEMU overwrites guest written
> >>> values but that looks excessive. If I uses some variable which
> >>> get set when guest starts using debug registers and check in
> >>> debug set ioctl then that look ugly. Looking for suggestions
> >> Whatever you do, have QEMU do the print, not the kernel.
> > How would that be accomplished?  How would the kernel know to exit to
> > QEMU, and how would the exit reason be conveyed?
> 
> QEMU is the one forcefully enabling debug and overwriting guest debug 
> registers, so it also knows when it did overwrite valid ones.

QEMU knows when it overwrites the guest values, but it doesn't know if,
after enabling host debug, the guest tries to write to the debug
registers and it gets nopped.  If we keep the EDM setting, then we can
at least say the situation is no worse than with a JTAG.

-Scott

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Alexander Graf]

> Am 29.07.2014 um 19:50 schrieb Scott Wood <scottwood@freescale.com>:
> 
>> On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
>>> On 29.07.14 00:33, Scott Wood wrote:
>>>> On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
>>>>> On 11.07.14 10:39, Bharat Bhushan wrote:
>>>>> This patch emulates debug registers and debug exception
>>>>> to support guest using debug resource. This enables running
>>>>> gdb/kgdb etc in guest.
>>>>> 
>>>>> On BOOKE architecture we cannot share debug resources between QEMU and
>>>>> guest because:
>>>>>      When QEMU is using debug resources then debug exception must
>>>>>      be always enabled. To achieve this we set MSR_DE and also set
>>>>>      MSRP_DEP so guest cannot change MSR_DE.
>>>>> 
>>>>>      When emulating debug resource for guest we want guest
>>>>>      to control MSR_DE (enable/disable debug interrupt on need).
>>>>> 
>>>>>      So above mentioned two configuration cannot be supported
>>>>>      at the same time. So the result is that we cannot share
>>>>>      debug resources between QEMU and Guest on BOOKE architecture.
>>>>> 
>>>>> In the current design QEMU gets priority over guest, this means that if
>>>>> QEMU is using debug resources then guest cannot use them and if guest is
>>>>> using debug resource then QEMU can overwrite them.
>>>>> 
>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>> ---
>>>>> Hi Alex,
>>>>> 
>>>>> I thought of having some print in register emulation if QEMU
>>>>> is using debug resource, Also when QEMU overwrites guest written
>>>>> values but that looks excessive. If I uses some variable which
>>>>> get set when guest starts using debug registers and check in
>>>>> debug set ioctl then that look ugly. Looking for suggestions
>>>> Whatever you do, have QEMU do the print, not the kernel.
>>> How would that be accomplished?  How would the kernel know to exit to
>>> QEMU, and how would the exit reason be conveyed?
>> 
>> QEMU is the one forcefully enabling debug and overwriting guest debug 
>> registers, so it also knows when it did overwrite valid ones.
> 
> QEMU knows when it overwrites the guest values, but it doesn't know if,
> after enabling host debug, the guest tries to write to the debug
> registers and it gets nopped.  If we keep the EDM setting, then we can
> at least say the situation is no worse than with a JTAG.

Yeah, I think that's perfectly reasonable. I don't think it'll be likely that a user starts debugging with qemu and then expects guest debugging to work.

The other way around is more likely and would warrant a warning to the user - if we care.

Alex

RE: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 3:22 AM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
> visible register
> 
> On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > This is not used and  even I do not remember why this was added in
> > first place.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> >  arch/powerpc/kvm/booke.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > ab62109..a5ee42c 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
> *vcpu,
> >  	kvm_guest_protect_msr(vcpu, MSR_DE, true);
> >  	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;
> >
> >  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> >  		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
> 
> This was intended to let the guest know that the host owns the debug resources,
> by analogy to what a JTAG debugger would do.
> 
> The Power ISA has this "Virtualized Implementation Note":
> 
>         It is the responsibility of the hypervisor to ensure that
>         DBCR0[EDM] is consistent with usage of DEP.

Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if MSRP_DEP is clear then clear DBCR0_EDM, right?
We need to implement above mentioned this.

Thanks
-Bharat

> 
> -Scott
> 

RE: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 3:25 AM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under
> debug
> 
> On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > When userspace (QEMU) is using the debug resource to debug guest then
> > we want MSR_DE to be always set. This patch adds missing MSR_DE
> > setting in "rfci" instruction.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> >  arch/powerpc/kvm/booke_emulate.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kvm/booke_emulate.c
> > b/arch/powerpc/kvm/booke_emulate.c
> > index 27a4b28..80c51a2 100644
> > --- a/arch/powerpc/kvm/booke_emulate.c
> > +++ b/arch/powerpc/kvm/booke_emulate.c
> > @@ -40,7 +40,11 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
> > 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);
> >  }
> >
> >  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu
> > *vcpu,
> 
> It looks like this is already handled by kvmppc_vcpu_sync_debug(), which is
> called by kvmppc_set_msr().

Yes, you are right. This patch is not needed.

Thanks
-Bharat

> 
> Plus, it should only be done for HV mode.
> 
> -Scott
> 

RE: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, July 29, 2014 7:35 PM
> To: Wood Scott-B07421; Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-B08248
> Subject: Re: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE
> 
> 
> On 29.07.14 00:01, Scott Wood wrote:
> > On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> >> When userspace is debugging guest then MSR_DE is always set and
> >> MSRP_DEP is set so that guest cannot change MSR_DE.
> >> Guest debug resources are not yet emulated, So there seems no reason
> >> we should stop guest controlling MSR_DE.
> >> Also a followup patch will enable debug emulation and that requires
> >> guest to control MSR_DE.
> > Why does it matter whether we emulate debug resources?  We still don't
> > want the guest to be able to clear MSR[DE] and thus break host debug.
> 
> The patch description is misleading. This patch changes the default of DEP to
> "guest controlled" when it boots up. Once QEMU wants control over the debug
> registers, it gets switched to "QEMU controlled" (that code is already there).

Yes, now default MSR_DE is controlled by guest and when QEMU wants to use debug resources then MSR_DEP is set, so guest cannot change MSR_DE.

Thanks
-Bharat 

> 
> 
> Alex

RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 11:20 PM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder
> Stuart-B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> 
> On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
> > On 29.07.14 00:33, Scott Wood wrote:
> > > On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
> > >> On 11.07.14 10:39, Bharat Bhushan wrote:
> > >>> This patch emulates debug registers and debug exception to support
> > >>> guest using debug resource. This enables running gdb/kgdb etc in
> > >>> guest.
> > >>>
> > >>> On BOOKE architecture we cannot share debug resources between QEMU
> > >>> and guest because:
> > >>>       When QEMU is using debug resources then debug exception must
> > >>>       be always enabled. To achieve this we set MSR_DE and also set
> > >>>       MSRP_DEP so guest cannot change MSR_DE.
> > >>>
> > >>>       When emulating debug resource for guest we want guest
> > >>>       to control MSR_DE (enable/disable debug interrupt on need).
> > >>>
> > >>>       So above mentioned two configuration cannot be supported
> > >>>       at the same time. So the result is that we cannot share
> > >>>       debug resources between QEMU and Guest on BOOKE architecture.
> > >>>
> > >>> In the current design QEMU gets priority over guest, this means
> > >>> that if QEMU is using debug resources then guest cannot use them
> > >>> and if guest is using debug resource then QEMU can overwrite them.
> > >>>
> > >>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > >>> ---
> > >>> Hi Alex,
> > >>>
> > >>> I thought of having some print in register emulation if QEMU is
> > >>> using debug resource, Also when QEMU overwrites guest written
> > >>> values but that looks excessive. If I uses some variable which get
> > >>> set when guest starts using debug registers and check in debug set
> > >>> ioctl then that look ugly. Looking for suggestions
> > >> Whatever you do, have QEMU do the print, not the kernel.
> > > How would that be accomplished?  How would the kernel know to exit
> > > to QEMU, and how would the exit reason be conveyed?
> >
> > QEMU is the one forcefully enabling debug and overwriting guest debug
> > registers, so it also knows when it did overwrite valid ones.
> 
> QEMU knows when it overwrites the guest values, but it doesn't know if, after
> enabling host debug, the guest tries to write to the debug registers and it gets
> nopped.

Do we want that QEMU first get DBCR0 to know whether it is overwriting whenever set/clear debug register?

>  If we keep the EDM setting, then we can at least say the situation is
> no worse than with a JTAG.

Yes

Thanks
-Bharat

> 
> -Scott
> 

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Alexander Graf]

> Am 30.07.2014 um 07:43 schrieb "Bharat.Bhushan@freescale.com" <Bharat.Bhushan@freescale.com>:
> 
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Tuesday, July 29, 2014 11:20 PM
>> To: Alexander Graf
>> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder
>> Stuart-B08248
>> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
>> 
>>> On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
>>>> On 29.07.14 00:33, Scott Wood wrote:
>>>>> On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
>>>>>> On 11.07.14 10:39, Bharat Bhushan wrote:
>>>>>> This patch emulates debug registers and debug exception to support
>>>>>> guest using debug resource. This enables running gdb/kgdb etc in
>>>>>> guest.
>>>>>> 
>>>>>> On BOOKE architecture we cannot share debug resources between QEMU
>>>>>> and guest because:
>>>>>>      When QEMU is using debug resources then debug exception must
>>>>>>      be always enabled. To achieve this we set MSR_DE and also set
>>>>>>      MSRP_DEP so guest cannot change MSR_DE.
>>>>>> 
>>>>>>      When emulating debug resource for guest we want guest
>>>>>>      to control MSR_DE (enable/disable debug interrupt on need).
>>>>>> 
>>>>>>      So above mentioned two configuration cannot be supported
>>>>>>      at the same time. So the result is that we cannot share
>>>>>>      debug resources between QEMU and Guest on BOOKE architecture.
>>>>>> 
>>>>>> In the current design QEMU gets priority over guest, this means
>>>>>> that if QEMU is using debug resources then guest cannot use them
>>>>>> and if guest is using debug resource then QEMU can overwrite them.
>>>>>> 
>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>> ---
>>>>>> Hi Alex,
>>>>>> 
>>>>>> I thought of having some print in register emulation if QEMU is
>>>>>> using debug resource, Also when QEMU overwrites guest written
>>>>>> values but that looks excessive. If I uses some variable which get
>>>>>> set when guest starts using debug registers and check in debug set
>>>>>> ioctl then that look ugly. Looking for suggestions
>>>>> Whatever you do, have QEMU do the print, not the kernel.
>>>> How would that be accomplished?  How would the kernel know to exit
>>>> to QEMU, and how would the exit reason be conveyed?
>>> 
>>> QEMU is the one forcefully enabling debug and overwriting guest debug
>>> registers, so it also knows when it did overwrite valid ones.
>> 
>> QEMU knows when it overwrites the guest values, but it doesn't know if, after
>> enabling host debug, the guest tries to write to the debug registers and it gets
>> nopped.
> 
> Do we want that QEMU first get DBCR0 to know whether it is overwriting whenever set/clear debug register?

If you want to implement a warning, yes. But that csn easily be a follow-up. Let's get something properly working upstream first.

Alex

RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 3:58 AM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> 
> On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> > This patch emulates debug registers and debug exception
> > to support guest using debug resource. This enables running
> > gdb/kgdb etc in guest.
> >
> > On BOOKE architecture we cannot share debug resources between QEMU and
> > guest because:
> >     When QEMU is using debug resources then debug exception must
> >     be always enabled. To achieve this we set MSR_DE and also set
> >     MSRP_DEP so guest cannot change MSR_DE.
> >
> >     When emulating debug resource for guest we want guest
> >     to control MSR_DE (enable/disable debug interrupt on need).
> >
> >     So above mentioned two configuration cannot be supported
> >     at the same time. So the result is that we cannot share
> >     debug resources between QEMU and Guest on BOOKE architecture.
> >
> > In the current design QEMU gets priority over guest, this means that if
> > QEMU is using debug resources then guest cannot use them and if guest is
> > using debug resource then QEMU can overwrite them.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> > Hi Alex,
> >
> > I thought of having some print in register emulation if QEMU
> > is using debug resource, Also when QEMU overwrites guest written
> > values but that looks excessive. If I uses some variable which
> > get set when guest starts using debug registers and check in
> > debug set ioctl then that look ugly. Looking for suggestions
> >
> >  arch/powerpc/include/asm/kvm_ppc.h |   3 +
> >  arch/powerpc/kvm/booke.c           |  27 +++++++
> >  arch/powerpc/kvm/booke_emulate.c   | 157
> +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 187 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> b/arch/powerpc/include/asm/kvm_ppc.h
> > index e2fd5a1..f3f7611 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -173,6 +173,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);
> > +
> >  union kvmppc_one_reg {
> >  	u32	wval;
> >  	u64	dval;
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index fadfe76..c2471ed 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu
> *vcpu)
> >  	clear_bit(BOOKE_IRQPRIO_WATCHDOG, &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);
> > +}
> > +
> >  static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32
> srr1)
> >  {
> >  #ifdef CONFIG_KVM_BOOKE_HV
> > @@ -783,6 +793,23 @@ 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;
> >
> > +	if (vcpu->guest_debug == 0) {
> > +		/* Debug resources belong to Guest */
> > +		if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> > +			kvmppc_core_queue_debug(vcpu);
> 
> Should also check for DBCR0[IDM].

Ok

> 
> > +
> > +		/* 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;
> > +	}
> > +
> > +	/*
> > +	 * Prepare for userspace exit as debug resources
> > +	 * are owned by userspace.
> > +	 */
> > +	vcpu->arch.dbsr = 0;
> >  	run->debug.arch.status = 0;
> >  	run->debug.arch.address = vcpu->arch.pc;
> 
> Why are you clearing vcpu->arch.dbsr?

It was discussed in some other email, as soon as we decide that the debug interrupt is handled by QEMU then we clear vcpu->arch->dbsr.
- QEMU cannot inject debug interrupt to guest (as this does not know guest ability to handle debug interrupt; MSR_DE), so will always clear DBSR.
- If QEMU has to always clear DBSR than in handling KVM_EXIT_DEBUG then this avoid doing in SET_SREGS

>  Userspace might be interested in
> the raw value,

With the current design, If userspace is interested then it will not get the DBSR. But why userspace will be interested?

> plus it's a change from the current API semantics.

Can you please let us know how ?

> 
> Where is this run->debug.arch.<foo> stuff and KVM_EXIT_DEBUG documented?

I do not see anything in Documentation/.  While there is some not in arch/powerpc/include/uapi/asm/kvm.h.
(Note: this is not something new exit type added by this patch).

> 
> 
> > diff --git a/arch/powerpc/kvm/booke_emulate.c
> b/arch/powerpc/kvm/booke_emulate.c
> > index 2a20194..3d143fe 100644
> > --- a/arch/powerpc/kvm/booke_emulate.c
> > +++ b/arch/powerpc/kvm/booke_emulate.c
> > @@ -139,6 +139,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:
> > @@ -153,14 +154,137 @@ 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_DSRR0:
> > +		vcpu->arch.dsrr0 = spr_val;
> > +		break;
> > +	case SPRN_DSRR1:
> > +		vcpu->arch.dsrr1 = spr_val;
> > +		break;
> > +	case SPRN_IAC1:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.iac1 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
> > +		break;
> > +	case SPRN_IAC2:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.iac2 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
> > +		break;
> > +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> > +	case SPRN_IAC3:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.iac3 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
> > +		break;
> > +	case SPRN_IAC4:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.iac4 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
> > +		break;
> > +#endif
> > +	case SPRN_DAC1:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.dac1 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
> > +		break;
> > +	case SPRN_DAC2:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.dac2 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
> > +		break;
> >  	case SPRN_DBCR0:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		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:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> >  		vcpu->arch.dbg_reg.dbcr1 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
> > +		break;
> > +	case SPRN_DBCR2:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> > +		debug_inst = true;
> > +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> >  		break;
> 
> In what circumstances can the architected and shadow registers differ?

As of now they are same. But I think that if we want to implement other features like "Freeze Timer (FT)" then they can be different.

> 
> >  	case SPRN_DBSR:
> > +		/*
> > +		 * If userspace is debugging guest then guest
> > +		 * can not access debug registers.
> > +		 */
> > +		if (vcpu->guest_debug)
> > +			break;
> > +
> >  		vcpu->arch.dbsr &= ~spr_val;
> > +		if (vcpu->arch.dbsr == 0)
> > +			kvmppc_core_dequeue_debug(vcpu);
> >  		break;
> 
> Not all DBSR bits cause an exception, e.g. IDE and MRR.

I am not sure what we should in that case ?
As we are currently emulating a subset of debug events (IAC, DAC, IC, BT and TIE --- DBCR0 emulation) then we should expose status of those events in guest dbsr and rest should be cleared ?

> 
> >  	case SPRN_TSR:
> >  		kvmppc_clr_tsr_bits(vcpu, spr_val);
> > @@ -273,6 +397,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;
> > +	}
> 
> Could you explain what's going on with regard to copying the registers
> into current->thread.debug?  Why is it done after loading the registers
> into the hardware (is there a race if we get preempted in the middle)?

Yes, and this was something I was not clear when writing this code. Should we have preempt disable-enable around this.

Thanks
-Bharat

> 
> -Scott
> ;

RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Monday, July 28, 2014 7:35 PM
> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> 
> 
> On 11.07.14 10:39, Bharat Bhushan wrote:
> > This patch emulates debug registers and debug exception to support
> > guest using debug resource. This enables running gdb/kgdb etc in
> > guest.
> >
> > On BOOKE architecture we cannot share debug resources between QEMU and
> > guest because:
> >      When QEMU is using debug resources then debug exception must
> >      be always enabled. To achieve this we set MSR_DE and also set
> >      MSRP_DEP so guest cannot change MSR_DE.
> >
> >      When emulating debug resource for guest we want guest
> >      to control MSR_DE (enable/disable debug interrupt on need).
> >
> >      So above mentioned two configuration cannot be supported
> >      at the same time. So the result is that we cannot share
> >      debug resources between QEMU and Guest on BOOKE architecture.
> >
> > In the current design QEMU gets priority over guest, this means that
> > if QEMU is using debug resources then guest cannot use them and if
> > guest is using debug resource then QEMU can overwrite them.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> > Hi Alex,
> >
> > I thought of having some print in register emulation if QEMU is using
> > debug resource, Also when QEMU overwrites guest written values but
> > that looks excessive. If I uses some variable which get set when guest
> > starts using debug registers and check in debug set ioctl then that
> > look ugly. Looking for suggestions
> 
> Whatever you do, have QEMU do the print, not the kernel.
> 
> >
> >   arch/powerpc/include/asm/kvm_ppc.h |   3 +
> >   arch/powerpc/kvm/booke.c           |  27 +++++++
> >   arch/powerpc/kvm/booke_emulate.c   | 157
> +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 187 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> > b/arch/powerpc/include/asm/kvm_ppc.h
> > index e2fd5a1..f3f7611 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -173,6 +173,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);
> > +
> >   union kvmppc_one_reg {
> >   	u32	wval;
> >   	u64	dval;
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > fadfe76..c2471ed 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu
> *vcpu)
> >   	clear_bit(BOOKE_IRQPRIO_WATCHDOG, &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); }
> > +
> >   static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32
> srr1)
> >   {
> >   #ifdef CONFIG_KVM_BOOKE_HV
> > @@ -783,6 +793,23 @@ 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;
> >
> > +	if (vcpu->guest_debug == 0) {
> > +		/* Debug resources belong to Guest */
> > +		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);
> 
> In that case we would've received a program interrupt and never entered this
> code path, no?

Yes for HV.
But for PR we can be here, MSR_DE is set in h/w msr and guest MSR_DE is not set.
Having a ifdef does not look good but we can have a comment here.

Thanks
-Bharat

> 
> 
> Alex

Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 29, 2014 3:22 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> > B08248
> > Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
> > visible register
> > 
> > On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > > This is not used and  even I do not remember why this was added in
> > > first place.
> > >
> > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > ---
> > >  arch/powerpc/kvm/booke.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > > ab62109..a5ee42c 100644
> > > --- a/arch/powerpc/kvm/booke.c
> > > +++ b/arch/powerpc/kvm/booke.c
> > > @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
> > *vcpu,
> > >  	kvm_guest_protect_msr(vcpu, MSR_DE, true);
> > >  	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;
> > >
> > >  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> > >  		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
> > 
> > This was intended to let the guest know that the host owns the debug resources,
> > by analogy to what a JTAG debugger would do.
> > 
> > The Power ISA has this "Virtualized Implementation Note":
> > 
> >         It is the responsibility of the hypervisor to ensure that
> >         DBCR0[EDM] is consistent with usage of DEP.
> 
> Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if MSRP_DEP is clear then clear DBCR0_EDM, right?
> We need to implement above mentioned this.

We should probably clear EDM only when guest debug emulation is working
and enabled (i.e. not until at least patch 6/6).

-Scott

RE: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, July 30, 2014 11:18 PM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
> visible register
> 
> On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, July 29, 2014 3:22 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > > Yoder Stuart-
> > > B08248
> > > Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM
> > > in guest visible register
> > >
> > > On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > > > This is not used and  even I do not remember why this was added in
> > > > first place.
> > > >
> > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > > ---
> > > >  arch/powerpc/kvm/booke.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > > > index ab62109..a5ee42c 100644
> > > > --- a/arch/powerpc/kvm/booke.c
> > > > +++ b/arch/powerpc/kvm/booke.c
> > > > @@ -1804,8 +1804,6 @@ int
> > > > kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
> > > *vcpu,
> > > >  	kvm_guest_protect_msr(vcpu, MSR_DE, true);
> > > >  	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;
> > > >
> > > >  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> > > >  		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
> > >
> > > This was intended to let the guest know that the host owns the debug
> > > resources, by analogy to what a JTAG debugger would do.
> > >
> > > The Power ISA has this "Virtualized Implementation Note":
> > >
> > >         It is the responsibility of the hypervisor to ensure that
> > >         DBCR0[EDM] is consistent with usage of DEP.
> >
> > Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if MSRP_DEP is
> clear then clear DBCR0_EDM, right?
> > We need to implement above mentioned this.
> 
> We should probably clear EDM only when guest debug emulation is working and
> enabled (i.e. not until at least patch 6/6).

But if EDM is set then guest debug emulation will not start/allowed.


Thanks
-Bharat

> 
> -Scott
> 

Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Wed, 2014-07-30 at 12:57 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, July 30, 2014 11:18 PM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> > B08248
> > Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
> > visible register
> > 
> > On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, July 29, 2014 3:22 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > > > Yoder Stuart-
> > > > B08248
> > > > Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM
> > > > in guest visible register
> > > >
> > > > On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > > > > This is not used and  even I do not remember why this was added in
> > > > > first place.
> > > > >
> > > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > > > > ---
> > > > >  arch/powerpc/kvm/booke.c | 2 --
> > > > >  1 file changed, 2 deletions(-)
> > > > >
> > > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > > > > index ab62109..a5ee42c 100644
> > > > > --- a/arch/powerpc/kvm/booke.c
> > > > > +++ b/arch/powerpc/kvm/booke.c
> > > > > @@ -1804,8 +1804,6 @@ int
> > > > > kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
> > > > *vcpu,
> > > > >  	kvm_guest_protect_msr(vcpu, MSR_DE, true);
> > > > >  	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;
> > > > >
> > > > >  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> > > > >  		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
> > > >
> > > > This was intended to let the guest know that the host owns the debug
> > > > resources, by analogy to what a JTAG debugger would do.
> > > >
> > > > The Power ISA has this "Virtualized Implementation Note":
> > > >
> > > >         It is the responsibility of the hypervisor to ensure that
> > > >         DBCR0[EDM] is consistent with usage of DEP.
> > >
> > > Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if MSRP_DEP is
> > clear then clear DBCR0_EDM, right?
> > > We need to implement above mentioned this.
> > 
> > We should probably clear EDM only when guest debug emulation is working and
> > enabled (i.e. not until at least patch 6/6).
> 
> But if EDM is set then guest debug emulation will not start/allowed.

I don't mean after the guest tries to write to the registers -- I mean
after the code has been added to KVM to allow it to work.

-Scott

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 29, 2014 3:58 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> > B08248
> > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> > 
> >  Userspace might be interested in
> > the raw value,
> 
> With the current design, If userspace is interested then it will not
> get the DBSR.

Oh, because DBSR isn't currently implemented in sregs or one reg?

>  But why userspace will be interested?

Do you expose all of the hardware's debugging features in your
high-level interface?

> > plus it's a change from the current API semantics.
> 
> Can you please let us know how ?

It looked like it was removing dbsr visibility and the requirement for
userspace to clear dbsr.  I guess the old way was that the value in
vcpu->arch.dbsr didn't matter until the next debug exception, when it
would be overwritten by the new SPRN_DBSR?

> > > +	case SPRN_DBCR2:
> > > +		/*
> > > +		 * If userspace is debugging guest then guest
> > > +		 * can not access debug registers.
> > > +		 */
> > > +		if (vcpu->guest_debug)
> > > +			break;
> > > +
> > > +		debug_inst = true;
> > > +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > >  		break;
> > 
> > In what circumstances can the architected and shadow registers differ?
> 
> As of now they are same. But I think that if we want to implement other features like "Freeze Timer (FT)" then they can be different.

I don't think we can possibly implement Freeze Timer.
 
> > >  	case SPRN_DBSR:
> > > +		/*
> > > +		 * If userspace is debugging guest then guest
> > > +		 * can not access debug registers.
> > > +		 */
> > > +		if (vcpu->guest_debug)
> > > +			break;
> > > +
> > >  		vcpu->arch.dbsr &= ~spr_val;
> > > +		if (vcpu->arch.dbsr == 0)
> > > +			kvmppc_core_dequeue_debug(vcpu);
> > >  		break;
> > 
> > Not all DBSR bits cause an exception, e.g. IDE and MRR.
> 
> I am not sure what we should in that case ?
>
> As we are currently emulating a subset of debug events (IAC, DAC, IC,
> BT and TIE --- DBCR0 emulation) then we should expose status of those
> events in guest dbsr and rest should be cleared ?

I'm not saying they need to be exposed to the guest, but I don't see
where you filter out bits like these.

> > > @@ -273,6 +397,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;
> > > +	}
> > 
> > Could you explain what's going on with regard to copying the registers
> > into current->thread.debug?  Why is it done after loading the registers
> > into the hardware (is there a race if we get preempted in the middle)?
> 
> Yes, and this was something I was not clear when writing this code.
> Should we have preempt disable-enable around this.

Can they be reordered instead?

-Scott

RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, July 31, 2014 8:18 AM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> 
> On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, July 29, 2014 3:58 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > > Yoder Stuart-
> > > B08248
> > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
> > > and exception
> > >
> > >  Userspace might be interested in
> > > the raw value,
> >
> > With the current design, If userspace is interested then it will not
> > get the DBSR.
> 
> Oh, because DBSR isn't currently implemented in sregs or one reg?

That is one reason. Another is that if we give dbsr visibility to userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG. And we think there is no gain in doing that because 
" 
- QEMU cannot inject debug interrupt to guest (as this does not know guest ability to handle debug interrupt; MSR_DE), so will always clear DBSR.
- If QEMU has to always clear DBSR in handling KVM_EXIT_DEBUG then this (clearing dbsr in kernel) avoid doing in SET_SREGS/set_one_reg()
" This makes dbsr not visible to userspace.

Also this (clearing of dbsr) should not be part of this patch, this should be a separate patch. I will do that in next version.

> 
> >  But why userspace will be interested?
> 
> Do you expose all of the hardware's debugging features in your high-level
> interface?

We support h/w breakpoint, watchpoint and IC (single stepping) and status in userspace exit provide all required information to userspace.

> 
> > > plus it's a change from the current API semantics.
> >
> > Can you please let us know how ?
> 
> It looked like it was removing dbsr visibility and the requirement for userspace
> to clear dbsr.  I guess the old way was that the value in
> vcpu->arch.dbsr didn't matter until the next debug exception, when it
> would be overwritten by the new SPRN_DBSR?

But that means old dbsr will be visibility to userspace, which is even bad than not visible, no?

Also this can lead to old dbsr visible to guest once userspace releases debug resources, but this can be solved by clearing dbsr in kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control & KVM_GUESTDBG_ENABLE)) { }".

> 
> > > > +	case SPRN_DBCR2:
> > > > +		/*
> > > > +		 * If userspace is debugging guest then guest
> > > > +		 * can not access debug registers.
> > > > +		 */
> > > > +		if (vcpu->guest_debug)
> > > > +			break;
> > > > +
> > > > +		debug_inst = true;
> > > > +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > > +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > > >  		break;
> > >
> > > In what circumstances can the architected and shadow registers differ?
> >
> > As of now they are same. But I think that if we want to implement other
> features like "Freeze Timer (FT)" then they can be different.
> 
> I don't think we can possibly implement Freeze Timer.

May be, but in my opinion we should keep this open.

> 
> > > >  	case SPRN_DBSR:
> > > > +		/*
> > > > +		 * If userspace is debugging guest then guest
> > > > +		 * can not access debug registers.
> > > > +		 */
> > > > +		if (vcpu->guest_debug)
> > > > +			break;
> > > > +
> > > >  		vcpu->arch.dbsr &= ~spr_val;
> > > > +		if (vcpu->arch.dbsr == 0)
> > > > +			kvmppc_core_dequeue_debug(vcpu);
> > > >  		break;
> > >
> > > Not all DBSR bits cause an exception, e.g. IDE and MRR.
> >
> > I am not sure what we should in that case ?
> >
> > As we are currently emulating a subset of debug events (IAC, DAC, IC,
> > BT and TIE --- DBCR0 emulation) then we should expose status of those
> > events in guest dbsr and rest should be cleared ?
> 
> I'm not saying they need to be exposed to the guest, but I don't see where you
> filter out bits like these.

I am trying to get what all bits should be filtered out, all bits except IACx, DACx, IC, BT and TIE (same as event set filtering done when setting DBCR0) ? 

i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?

> 
> > > > @@ -273,6 +397,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;
> > > > +	}
> > >
> > > Could you explain what's going on with regard to copying the
> > > registers into current->thread.debug?  Why is it done after loading
> > > the registers into the hardware (is there a race if we get preempted in the
> middle)?
> >
> > Yes, and this was something I was not clear when writing this code.
> > Should we have preempt disable-enable around this.
> 
> Can they be reordered instead?

Thanks;  Yes, that will work :)


Regards
-Bharat

> 
> -Scott
> 

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, July 31, 2014 8:18 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> > B08248
> > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> > 
> > On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, July 29, 2014 3:58 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > > > Yoder Stuart-
> > > > B08248
> > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
> > > > and exception
> > > >
> > > >  Userspace might be interested in
> > > > the raw value,
> > >
> > > With the current design, If userspace is interested then it will not
> > > get the DBSR.
> > 
> > Oh, because DBSR isn't currently implemented in sregs or one reg?
> 
> That is one reason. Another is that if we give dbsr visibility to
> userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG.

Right -- since I didn't realize DBSR wasn't already exposed, I thought
userspace already had this responsibility.

> > It looked like it was removing dbsr visibility and the requirement for userspace
> > to clear dbsr.  I guess the old way was that the value in
> > vcpu->arch.dbsr didn't matter until the next debug exception, when it
> > would be overwritten by the new SPRN_DBSR?
> 
> But that means old dbsr will be visibility to userspace, which is even bad than not visible, no?
> 
> Also this can lead to old dbsr visible to guest once userspace releases
> debug resources, but this can be solved by clearing dbsr in
> kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control &
> KVM_GUESTDBG_ENABLE)) { }".

I wasn't suggesting that you keep it that way, just clarifying my
understanding of the current code.


> 
> > 
> > > > > +	case SPRN_DBCR2:
> > > > > +		/*
> > > > > +		 * If userspace is debugging guest then guest
> > > > > +		 * can not access debug registers.
> > > > > +		 */
> > > > > +		if (vcpu->guest_debug)
> > > > > +			break;
> > > > > +
> > > > > +		debug_inst = true;
> > > > > +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > > > +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > > > >  		break;
> > > >
> > > > In what circumstances can the architected and shadow registers differ?
> > >
> > > As of now they are same. But I think that if we want to implement other
> > features like "Freeze Timer (FT)" then they can be different.
> > 
> > I don't think we can possibly implement Freeze Timer.
> 
> May be, but in my opinion we should keep this open.

We're not talking about API here -- the implementation should be kept
simple if there's no imminent need for shadow registers.

> > > I am not sure what we should in that case ?
> > >
> > > As we are currently emulating a subset of debug events (IAC, DAC, IC,
> > > BT and TIE --- DBCR0 emulation) then we should expose status of those
> > > events in guest dbsr and rest should be cleared ?
> > 
> > I'm not saying they need to be exposed to the guest, but I don't see where you
> > filter out bits like these.
> 
> I am trying to get what all bits should be filtered out, all bits
> except IACx, DACx, IC, BT and TIE (same as event set filtering done
> when setting DBCR0) ? 
> 
> i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?

Bits like IRPT and RET don't really matter, as you shouldn't see them
happen.  Likewise MRR if you're sure you've cleared it since boot.  But
IDE could be set any time an asynchronous exception happens.  I don't
think you should filter it out, but instead make sure that it doesn't
cause an exception to be delivered.

-Scott

RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, August 01, 2014 2:16 AM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> 
> On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, July 31, 2014 8:18 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder
> Stuart-
> > > B08248
> > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
> > >
> > > On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, July 29, 2014 3:58 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > > > > Yoder Stuart-
> > > > > B08248
> > > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
> > > > > and exception
> > > > >
> > > > >  Userspace might be interested in
> > > > > the raw value,
> > > >
> > > > With the current design, If userspace is interested then it will not
> > > > get the DBSR.
> > >
> > > Oh, because DBSR isn't currently implemented in sregs or one reg?
> >
> > That is one reason. Another is that if we give dbsr visibility to
> > userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG.
> 
> Right -- since I didn't realize DBSR wasn't already exposed, I thought
> userspace already had this responsibility.
> 
> > > It looked like it was removing dbsr visibility and the requirement for
> userspace
> > > to clear dbsr.  I guess the old way was that the value in
> > > vcpu->arch.dbsr didn't matter until the next debug exception, when it
> > > would be overwritten by the new SPRN_DBSR?
> >
> > But that means old dbsr will be visibility to userspace, which is even bad
> than not visible, no?
> >
> > Also this can lead to old dbsr visible to guest once userspace releases
> > debug resources, but this can be solved by clearing dbsr in
> > kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control &
> > KVM_GUESTDBG_ENABLE)) { }".
> 
> I wasn't suggesting that you keep it that way, just clarifying my
> understanding of the current code.
> 
> 
> >
> > >
> > > > > > +	case SPRN_DBCR2:
> > > > > > +		/*
> > > > > > +		 * If userspace is debugging guest then guest
> > > > > > +		 * can not access debug registers.
> > > > > > +		 */
> > > > > > +		if (vcpu->guest_debug)
> > > > > > +			break;
> > > > > > +
> > > > > > +		debug_inst = true;
> > > > > > +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > > > > +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > > > > >  		break;
> > > > >
> > > > > In what circumstances can the architected and shadow registers differ?
> > > >
> > > > As of now they are same. But I think that if we want to implement other
> > > features like "Freeze Timer (FT)" then they can be different.
> > >
> > > I don't think we can possibly implement Freeze Timer.
> >
> > May be, but in my opinion we should keep this open.
> 
> We're not talking about API here -- the implementation should be kept
> simple if there's no imminent need for shadow registers.
> 
> > > > I am not sure what we should in that case ?
> > > >
> > > > As we are currently emulating a subset of debug events (IAC, DAC, IC,
> > > > BT and TIE --- DBCR0 emulation) then we should expose status of those
> > > > events in guest dbsr and rest should be cleared ?
> > >
> > > I'm not saying they need to be exposed to the guest, but I don't see where
> you
> > > filter out bits like these.
> >
> > I am trying to get what all bits should be filtered out, all bits
> > except IACx, DACx, IC, BT and TIE (same as event set filtering done
> > when setting DBCR0) ?
> >
> > i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?
> 
> Bits like IRPT and RET don't really matter, as you shouldn't see them
> happen.  Likewise MRR if you're sure you've cleared it since boot.

We can clear MRR bits when update vcpu->arch->dbsr with SPRM_DBSR in kvm debug handler

>  But
> IDE could be set any time an asynchronous exception happens.  I don't
> think you should filter it out, but instead make sure that it doesn't
> cause an exception to be delivered.

So this means that in kvmpp_handle_debug() if DBSR_IDE is set then do not inject debug interrupt 

 and

on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set.

        case SPRN_DBSR:

                vcpu->arch.dbsr &= ~spr_val;
                if (!(vcpu->arch.dbsr & ~DBSR_IDE))
                        kvmppc_core_dequeue_debug(vcpu);
                break;

or
                vcpu->arch.dbsr &= ~(spr_val | DBSR_IDE);
                if (!vcpu->arch.dbsr)
                        kvmppc_core_dequeue_debug(vcpu);
                break;

Thanks
-Bharat

> 
> -Scott
> 

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Fri, 2014-08-01 at 04:34 -0500, Bhushan Bharat-R65777 wrote:
> on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set.
> 
>         case SPRN_DBSR:
> 
>                 vcpu->arch.dbsr &= ~spr_val;
>                 if (!(vcpu->arch.dbsr & ~DBSR_IDE))
>                         kvmppc_core_dequeue_debug(vcpu);
>                 break;
> 
> or
>                 vcpu->arch.dbsr &= ~(spr_val | DBSR_IDE);
>                 if (!vcpu->arch.dbsr)
>                         kvmppc_core_dequeue_debug(vcpu);
>                 break;

The first option.  I see no reason to have KVM forcibly clear DBSR[IDE].

-Scott