[PATCH v3] powerpc/kvm: support to handle sw breakpoint

This patch adds kernel side support for software breakpoint.
Design is that, by using an illegal instruction, we trap to hypervisor
via Emulation Assistance interrupt, where we check for the illegal instruction
and accordingly we return to Host or Guest. Patch also adds support for
software breakpoint in PR KVM.

Changes v2->v3:
 Changed the debug instructions. Using the all zero opcode in the instruction word
  as illegal instruction as mentioned in Power ISA instead of ABS
 Removed reg updated in emulation assist and added a call to
  kvmppc_emulate_instruction for reg update.

Changes v1->v2:

 Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
 Added code to use KVM get one reg infrastructure to get debug opcode.
 Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
 Made changes to commit message.

Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/include/asm/kvm_book3s.h |  7 +++++++
 arch/powerpc/include/asm/ppc-opcode.h |  5 +++++
 arch/powerpc/kvm/book3s.c             |  3 ++-
 arch/powerpc/kvm/book3s_hv.c          | 12 ++++++++++--
 arch/powerpc/kvm/book3s_pr.c          |  3 +++
 arch/powerpc/kvm/emulate.c            |  9 +++++++++
 6 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index f52f656..f17e3fd 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h

@@ -24,6 +24,13 @@
 #include <linux/kvm_host.h>
 #include <asm/kvm_book3s_asm.h>
 
+/*
+ * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
+ * Based on PowerISA v2.07, Instruction with opcode 0s will be treated as illegal
+ * instruction.
+ */
+#define KVMPPC_INST_BOOK3S_DEBUG	0x00dddd00
+
 struct kvmppc_bat {
 	u64 raw;
 	u32 bepi;

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 3132bb9..56739b3 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h

@@ -111,6 +111,11 @@
 #define OP_31_XOP_LHBRX     790
 #define OP_31_XOP_STHBRX    918
 
+/* KVMPPC_INST_BOOK3S_DEBUG -- Software breakpoint Instruction
+ * 0x00dddd00 -- Primary opcode is 0s
+ */
+#define OP_ZERO                 0x0
+
 #define OP_LWZ  32
 #define OP_LD   58
 #define OP_LWZU 33

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index c254c27..b40fe5d 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c

@@ -789,7 +789,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
-	return -EINVAL;
+	vcpu->guest_debug = dbg->control;
+	return 0;
 }
 
 void kvmppc_decrementer_func(unsigned long data)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 7a12edb..7c16f4f 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c

@@ -725,8 +725,13 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	 * we don't emulate any guest instructions at this stage.
 	 */
 	case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
-		kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
-		r = RESUME_GUEST;
+		if (kvmppc_get_last_inst(vcpu) == KVMPPC_INST_BOOK3S_DEBUG) {
+			kvmppc_emulate_instruction(run, vcpu);
+			r = RESUME_HOST;
+		} else {
+			kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+			r = RESUME_GUEST;
+		}
 		break;
 	/*
 	 * This occurs if the guest (kernel or userspace), does something that

@@ -831,6 +836,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
 	long int i;
 
 	switch (id) {
+	case KVM_REG_PPC_DEBUG_INST:
+		*val = get_reg_val(id, KVMPPC_INST_BOOK3S_DEBUG);
+		break;
 	case KVM_REG_PPC_HIOR:
 		*val = get_reg_val(id, 0);
 		break;

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 8eef1e5..27f5234 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c

@@ -1229,6 +1229,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
 	int r = 0;
 
 	switch (id) {
+	case KVM_REG_PPC_DEBUG_INST:
+		*val = get_reg_val(id, KVMPPC_INST_BOOK3S_DEBUG);
+		break;
 	case KVM_REG_PPC_HIOR:
 		*val = get_reg_val(id, to_book3s(vcpu)->hior);
 		break;

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

@@ -458,6 +458,15 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
 		kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
 		break;
 
+	case OP_ZERO:
+		if((inst & 0x00FFFF00) == KVMPPC_INST_BOOK3S_DEBUG) {
+			run->exit_reason = KVM_EXIT_DEBUG;
+			run->debug.arch.address = kvmppc_get_pc(vcpu);
+			emulated = EMULATE_EXIT_USER;
+			advance = 0;
+		}
+		break;	
+
 	default:
 		emulated = EMULATE_FAIL;
 	}

-- 
1.7.11.4

+/*
+ * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
+ * Based on PowerISA v2.07, Instruction with opcode 0s will be treated as illegal
+ * instruction.
+ */

"primary opcode 0" instead?

+#define OP_ZERO                 0x0

Using 0x0 where you mean 0, making a #define for 0 in the first place...
This all looks rather silly doesn't it.

+	case OP_ZERO:
+		if((inst & 0x00FFFF00) == KVMPPC_INST_BOOK3S_DEBUG) {

You either shouldn't mask at all here, or the mask is wrong (the primary
op is the top six bits, not the top eight).


Segher

On Sunday 03 August 2014 09:21 PM, Segher Boessenkool wrote:

quoted

+/*
+ * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
+ * Based on PowerISA v2.07, Instruction with opcode 0s will be treated as illegal
+ * instruction.
+ */

"primary opcode 0" instead?

ok sure.

quoted

+#define OP_ZERO                 0x0

Using 0x0 where you mean 0, making a #define for 0 in the first place...
This all looks rather silly doesn't it.

I wanted to avoid zero mentioned in the case statement, but can add a
comment explaining it.

quoted

+	case OP_ZERO:
+		if((inst & 0x00FFFF00) == KVMPPC_INST_BOOK3S_DEBUG) {

You either shouldn't mask at all here, or the mask is wrong (the primary
op is the top six bits, not the top eight).

Yes. I guess I dont need to check here. Will resend the patch.

Thanks for review
Regards
Maddy

Segher

On 01.08.14 06:50, Madhavan Srinivasan wrote:

I changed the emulation code flow very recently, so while I advised you 
to write it this way this won't work with recent git versions anymore :(.

Please just create a tiny static function that handles this particular 
inst and duplicate the logic in book3s_emulate.c (for PR) as well as 
here (for HV).

This should be book3s_emulate.c.


Alex

On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:

quoted

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

This should be book3s_emulate.c.

Any reason we can't make that 00dddd00 opcode as breakpoint common to
all powerpc variants ?

Cheers,
Ben.

On 11.08.14 10:51, Benjamin Herrenschmidt wrote:

On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:

quoted

quoted

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

This should be book3s_emulate.c.

Any reason we can't make that 00dddd00 opcode as breakpoint common to
all powerpc variants ?

I can't think of a good reason. We use a hypercall on booke (which traps 
into an illegal instruction for pr) today, but I don't think it has to 
be that way.

Given that the user space API allows us to change it dynamically, there 
should be nothing blocking us from going with 00dddd00 always.


Alex

On Monday 11 August 2014 02:45 PM, Alexander Graf wrote:

On 11.08.14 10:51, Benjamin Herrenschmidt wrote:

quoted

On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:

quoted

quoted

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

This should be book3s_emulate.c.

Any reason we can't make that 00dddd00 opcode as breakpoint common to
all powerpc variants ?

I can't think of a good reason. We use a hypercall on booke (which traps
into an illegal instruction for pr) today, but I don't think it has to
be that way.

Given that the user space API allows us to change it dynamically, there
should be nothing blocking us from going with 00dddd00 always.

Kindly correct me if i am wrong. So we can still have a common code in
emulate.c to set the env for both HV and pr incase of illegal
instruction (i will rebase latest src). But suggestion here to use
00dddd00, in that case current path in embed is kvmppc_handle_exit
(booke.c) -> BOOKE_INTERRUPT_HV_PRIV -> emulation_exit ->
kvmppc_emulate_instruction, will change to kvmppc_handle_exit (booke.c)
-> BOOKE_INTERRUPT_PROGRAM -> if debug instr call emulation_exit else
send to guest?

Thanks for review
regards
Maddy

Alex

On 12.08.14 07:17, Madhavan Srinivasan wrote:

On Monday 11 August 2014 02:45 PM, Alexander Graf wrote:

quoted

On 11.08.14 10:51, Benjamin Herrenschmidt wrote:

quoted

On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:

quoted

quoted

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

This should be book3s_emulate.c.

Any reason we can't make that 00dddd00 opcode as breakpoint common to
all powerpc variants ?

I can't think of a good reason. We use a hypercall on booke (which traps
into an illegal instruction for pr) today, but I don't think it has to
be that way.

Given that the user space API allows us to change it dynamically, there
should be nothing blocking us from going with 00dddd00 always.

Kindly correct me if i am wrong. So we can still have a common code in
emulate.c to set the env for both HV and pr incase of illegal
instruction (i will rebase latest src). But suggestion here to use
00dddd00, in that case current path in embed is kvmppc_handle_exit
(booke.c) -> BOOKE_INTERRUPT_HV_PRIV -> emulation_exit ->
kvmppc_emulate_instruction, will change to kvmppc_handle_exit (booke.c)
-> BOOKE_INTERRUPT_PROGRAM -> if debug instr call emulation_exit else
send to guest?

I can't follow your description above.

With the latest git version HV KVM does not include emulate.c anymore.

Also, it would make a lot of sense of have the same soft breakpoint 
instruction across all ppc targets, so it would make sense to change it 
to 0x00dddd00 for booke as well.

Basically you would have handling code in emulate.c and book3s_hv.c at 
the end of the day.


Alex

On Tuesday 12 August 2014 04:49 PM, Alexander Graf wrote:

On 12.08.14 07:17, Madhavan Srinivasan wrote:

quoted

On Monday 11 August 2014 02:45 PM, Alexander Graf wrote:

quoted

On 11.08.14 10:51, Benjamin Herrenschmidt wrote:

quoted

On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:

quoted

quoted

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

This should be book3s_emulate.c.

Any reason we can't make that 00dddd00 opcode as breakpoint common to
all powerpc variants ?

I can't think of a good reason. We use a hypercall on booke (which traps
into an illegal instruction for pr) today, but I don't think it has to
be that way.

Given that the user space API allows us to change it dynamically, there
should be nothing blocking us from going with 00dddd00 always.

Kindly correct me if i am wrong. So we can still have a common code in
emulate.c to set the env for both HV and pr incase of illegal
instruction (i will rebase latest src). But suggestion here to use
00dddd00, in that case current path in embed is kvmppc_handle_exit
(booke.c) -> BOOKE_INTERRUPT_HV_PRIV -> emulation_exit ->
kvmppc_emulate_instruction, will change to kvmppc_handle_exit (booke.c)
-> BOOKE_INTERRUPT_PROGRAM -> if debug instr call emulation_exit else
send to guest?

I can't follow your description above.

My bad.

With the latest git version HV KVM does not include emulate.c anymore.

Also, it would make a lot of sense of have the same soft breakpoint
instruction across all ppc targets, so it would make sense to change it
to 0x00dddd00 for booke as well.

Got it. Was describing the current control flow with respect to booke
and where changes needed (for same software breakpoint inst). This is
for my understanding and wanted verify.

kvmppc_handle_exit(booke.c)
	-> BOOKE_INTERRUPT_HV_PRIV
		-> emulation_exit
			->kvmppc_emulate_instruction

Incase of using the same software breakpoint instruction (0x00dddd00),
then we need to add code in booke something like this

kvmppc_handle_exit (booke.c)
	-> BOOKE_INTERRUPT_PROGRAM
		->	if debug instr
				->emulation_exit
			else
				->send to guest?
				

Basically you would have handling code in emulate.c and book3s_hv.c at
the end of the day.

Yes. Will resend the patch with updated code.

Alex

On 12.08.14 13:35, Madhavan Srinivasan wrote:

On Tuesday 12 August 2014 04:49 PM, Alexander Graf wrote:

quoted

On 12.08.14 07:17, Madhavan Srinivasan wrote:

quoted

On Monday 11 August 2014 02:45 PM, Alexander Graf wrote:

quoted

On 11.08.14 10:51, Benjamin Herrenschmidt wrote:

quoted

On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:

quoted

quoted

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index da86d9b..d95014e 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c

This should be book3s_emulate.c.

Any reason we can't make that 00dddd00 opcode as breakpoint common to
all powerpc variants ?

I can't think of a good reason. We use a hypercall on booke (which traps
into an illegal instruction for pr) today, but I don't think it has to
be that way.

Given that the user space API allows us to change it dynamically, there
should be nothing blocking us from going with 00dddd00 always.

Kindly correct me if i am wrong. So we can still have a common code in
emulate.c to set the env for both HV and pr incase of illegal
instruction (i will rebase latest src). But suggestion here to use
00dddd00, in that case current path in embed is kvmppc_handle_exit
(booke.c) -> BOOKE_INTERRUPT_HV_PRIV -> emulation_exit ->
kvmppc_emulate_instruction, will change to kvmppc_handle_exit (booke.c)
-> BOOKE_INTERRUPT_PROGRAM -> if debug instr call emulation_exit else
send to guest?

I can't follow your description above.

My bad.

quoted

With the latest git version HV KVM does not include emulate.c anymore.

Also, it would make a lot of sense of have the same soft breakpoint
instruction across all ppc targets, so it would make sense to change it
to 0x00dddd00 for booke as well.

Got it. Was describing the current control flow with respect to booke
and where changes needed (for same software breakpoint inst). This is
for my understanding and wanted verify.

kvmppc_handle_exit(booke.c)
	-> BOOKE_INTERRUPT_HV_PRIV
		-> emulation_exit
			->kvmppc_emulate_instruction

Incase of using the same software breakpoint instruction (0x00dddd00),
then we need to add code in booke something like this

kvmppc_handle_exit (booke.c)
	-> BOOKE_INTERRUPT_PROGRAM
		->	if debug instr
				->emulation_exit
			else
				->send to guest?

Bleks. I see your point. I guess you need something like this for booke:

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 074b7fc..1fdeee0 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c

@@ -876,6 +876,11 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 

kvm_vcpu *vcpu,
      case BOOKE_INTERRUPT_HV_PRIV:
          emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
          break;
+    case BOOKE_INTERRUPT_PROGRAM:
+        /* SW breakpoints arrive as illegal instructions on HV */
+        if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
+            emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
+        break;
      default:
          break;
      }

@@ -953,7 +958,8 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 

kvm_vcpu *vcpu,
          break;

      case BOOKE_INTERRUPT_PROGRAM:
-        if (vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) {
+        if ((vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) &&
+            (last_inst != KVMPPC_INST_SOFT_BREAKPOINT)) {
              /*
               * Program traps generated by user-level software must
               * be handled by the guest kernel.

quoted

Basically you would have handling code in emulate.c and book3s_hv.c at
the end of the day.

Yes. Will resend the patch with updated code.

Thanks,


Alex

On Tuesday 12 August 2014 05:45 PM, Alexander Graf wrote:

Ok make sense.

Regards
Maddy

quoted

quoted

Basically you would have handling code in emulate.c and book3s_hv.c at
the end of the day.

Yes. Will resend the patch with updated code.

Thanks,


Alex