[PATCH v1 1/4] target/riscv/kvm: add software breakpoints support

Chao Du posted 4 patches 6 months ago
There is a newer version of this series
[PATCH v1 1/4] target/riscv/kvm: add software breakpoints support
Posted by Chao Du 6 months ago
This patch implements insert/remove software breakpoint process:

Add an input parameter for kvm_arch_insert_sw_breakpoint() and
kvm_arch_remove_sw_breakpoint() to pass the length information,
which helps us to know whether it is a RVC instruction.
For some remove cases, we do not have the length info, so we need
to judge by ourselves.

For RISC-V, GDB treats single-step similarly to breakpoint: add a
breakpoint at the next step address, then continue. So this also
works for single-step debugging.

Add some stubs which are necessary for building, and will be
implemented later.

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 accel/kvm/kvm-all.c        |  8 ++--
 include/sysemu/kvm.h       |  6 ++-
 target/arm/kvm.c           |  6 ++-
 target/i386/kvm/kvm.c      |  6 ++-
 target/mips/kvm.c          |  6 ++-
 target/ppc/kvm.c           |  6 ++-
 target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c     |  6 ++-
 8 files changed, 107 insertions(+), 16 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c0be9f5eed..d27e77dbb2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3357,7 +3357,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
         bp = g_new(struct kvm_sw_breakpoint, 1);
         bp->pc = addr;
         bp->use_count = 1;
-        err = kvm_arch_insert_sw_breakpoint(cpu, bp);
+        err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
         if (err) {
             g_free(bp);
             return err;
@@ -3396,7 +3396,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
             return 0;
         }
 
-        err = kvm_arch_remove_sw_breakpoint(cpu, bp);
+        err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
         if (err) {
             return err;
         }
@@ -3426,10 +3426,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
     CPUState *tmpcpu;
 
     QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
-        if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
+        if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
             /* Try harder to find a CPU that currently sees the breakpoint. */
             CPU_FOREACH(tmpcpu) {
-                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
+                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {
                     break;
                 }
             }
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index c31d9c7356..340e094ffb 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
 int kvm_sw_breakpoints_active(CPUState *cpu);
 
 int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
-                                  struct kvm_sw_breakpoint *bp);
+                                  struct kvm_sw_breakpoint *bp,
+                                  vaddr len);
 int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
-                                  struct kvm_sw_breakpoint *bp);
+                                  struct kvm_sw_breakpoint *bp,
+                                  vaddr len);
 int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
 int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
 void kvm_arch_remove_all_hw_breakpoints(void);
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 7cf5cf31de..84593db544 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -2402,7 +2402,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
 /* C6.6.29 BRK instruction */
 static const uint32_t brk_insn = 0xd4200000;
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
         cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
@@ -2411,7 +2412,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     static uint32_t brk;
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index c5943605ee..6449f796d0 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -4992,7 +4992,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
     return 1;
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     static const uint8_t int3 = 0xcc;
 
@@ -5003,7 +5004,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     uint8_t int3;
 
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index a631ab544f..129964cf6d 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -111,13 +111,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
     DPRINTF("%s\n", __func__);
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     DPRINTF("%s\n", __func__);
     return 0;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     DPRINTF("%s\n", __func__);
     return 0;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 46fccff786..9b76bdaa05 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1378,7 +1378,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
     return 0;
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     /* Mixed endian case is not handled */
     uint32_t sc = debug_inst_opcode;
@@ -1392,7 +1393,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     uint32_t sc;
 
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 473416649f..cba55c552d 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1962,3 +1962,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
 };
 
 DEFINE_TYPES(riscv_kvm_cpu_type_infos)
+
+static const uint32_t ebreak_insn = 0x00100073;
+static const uint16_t c_ebreak_insn = 0x9002;
+
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
+{
+    if (len != 4 && len != 2) {
+        return -EINVAL;
+    }
+
+    uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
+                                  (uint8_t *)&c_ebreak_insn;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
+        cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
+{
+    uint8_t length;
+
+    if (len == 4 || len == 2) {
+        length = (uint8_t)len;
+    } else if (len == 0) {
+        /* Need to decide the instruction length in this case. */
+        uint32_t read_4_bytes;
+        uint16_t read_2_bytes;
+
+        if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
+            cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
+            return -EINVAL;
+        }
+
+        if (read_4_bytes == ebreak_insn) {
+            length = 4;
+        } else if (read_2_bytes == c_ebreak_insn) {
+            length = 2;
+        } else {
+            return -EINVAL;
+        }
+    } else {
+        return -EINVAL;
+    }
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
+                            length, 1)) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
+{
+    /* TODO; To be implemented later. */
+    return -EINVAL;
+}
+
+int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
+{
+    /* TODO; To be implemented later. */
+    return -EINVAL;
+}
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+    /* TODO; To be implemented later. */
+}
+
+void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
+{
+    /* TODO; To be implemented later. */
+}
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 1b494ecc20..132952cc84 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -865,7 +865,8 @@ static void determine_sw_breakpoint_instr(void)
         }
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     determine_sw_breakpoint_instr();
 
@@ -877,7 +878,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+                                  vaddr len)
 {
     uint8_t t[MAX_ILEN];
 
-- 
2.17.1
Re: [PATCH v1 1/4] target/riscv/kvm: add software breakpoints support
Posted by Andrew Jones 6 months ago
On Mon, May 27, 2024 at 02:19:13AM GMT, Chao Du wrote:
> This patch implements insert/remove software breakpoint process:
> 
> Add an input parameter for kvm_arch_insert_sw_breakpoint() and
> kvm_arch_remove_sw_breakpoint() to pass the length information,
> which helps us to know whether it is a RVC instruction.
> For some remove cases, we do not have the length info, so we need
> to judge by ourselves.
> 
> For RISC-V, GDB treats single-step similarly to breakpoint: add a
> breakpoint at the next step address, then continue. So this also
> works for single-step debugging.
> 
> Add some stubs which are necessary for building, and will be
> implemented later.
> 
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---
>  accel/kvm/kvm-all.c        |  8 ++--
>  include/sysemu/kvm.h       |  6 ++-
>  target/arm/kvm.c           |  6 ++-
>  target/i386/kvm/kvm.c      |  6 ++-
>  target/mips/kvm.c          |  6 ++-
>  target/ppc/kvm.c           |  6 ++-
>  target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
>  target/s390x/kvm/kvm.c     |  6 ++-
>  8 files changed, 107 insertions(+), 16 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c0be9f5eed..d27e77dbb2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3357,7 +3357,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
>          bp = g_new(struct kvm_sw_breakpoint, 1);
>          bp->pc = addr;
>          bp->use_count = 1;
> -        err = kvm_arch_insert_sw_breakpoint(cpu, bp);
> +        err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
>          if (err) {
>              g_free(bp);
>              return err;
> @@ -3396,7 +3396,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
>              return 0;
>          }
>  
> -        err = kvm_arch_remove_sw_breakpoint(cpu, bp);
> +        err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
>          if (err) {
>              return err;
>          }
> @@ -3426,10 +3426,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
>      CPUState *tmpcpu;
>  
>      QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
> -        if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
> +        if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
>              /* Try harder to find a CPU that currently sees the breakpoint. */
>              CPU_FOREACH(tmpcpu) {
> -                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
> +                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {

It's not nice to need to add 'len' to all arch insert/remove sw breakpoint
implementations, and the fact we have to pass zero sometimes implies it's
not the right approach.

>                      break;
>                  }
>              }
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index c31d9c7356..340e094ffb 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
>  int kvm_sw_breakpoints_active(CPUState *cpu);
>  
>  int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
> -                                  struct kvm_sw_breakpoint *bp);
> +                                  struct kvm_sw_breakpoint *bp,
> +                                  vaddr len);
>  int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
> -                                  struct kvm_sw_breakpoint *bp);
> +                                  struct kvm_sw_breakpoint *bp,
> +                                  vaddr len);
>  int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
>  int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
>  void kvm_arch_remove_all_hw_breakpoints(void);
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 7cf5cf31de..84593db544 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -2402,7 +2402,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>  /* C6.6.29 BRK instruction */
>  static const uint32_t brk_insn = 0xd4200000;
>  
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
>          cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> @@ -2411,7 +2412,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>  
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      static uint32_t brk;
>  
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index c5943605ee..6449f796d0 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4992,7 +4992,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
>      return 1;
>  }
>  
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      static const uint8_t int3 = 0xcc;
>  
> @@ -5003,7 +5004,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>  
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      uint8_t int3;
>  
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index a631ab544f..129964cf6d 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -111,13 +111,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
>      DPRINTF("%s\n", __func__);
>  }
>  
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      DPRINTF("%s\n", __func__);
>      return 0;
>  }
>  
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      DPRINTF("%s\n", __func__);
>      return 0;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 46fccff786..9b76bdaa05 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1378,7 +1378,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
>      return 0;
>  }
>  
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      /* Mixed endian case is not handled */
>      uint32_t sc = debug_inst_opcode;
> @@ -1392,7 +1393,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>  
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      uint32_t sc;
>  
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 473416649f..cba55c552d 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1962,3 +1962,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
>  };
>  
>  DEFINE_TYPES(riscv_kvm_cpu_type_infos)
> +
> +static const uint32_t ebreak_insn = 0x00100073;
> +static const uint16_t c_ebreak_insn = 0x9002;
> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
> +{
> +    if (len != 4 && len != 2) {
> +        return -EINVAL;
> +    }
> +
> +    uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
> +                                  (uint8_t *)&c_ebreak_insn;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
> +        return -EINVAL;
> +    }

We can also read 2 bytes. So how about 

  if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 2, 0)) {
      return -EINVAL;
  }

  if ((bp->saved_insn & 0x3) == 0x3) {
      if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
          cpu_memory_rw_debug(cs, bp->pc, &ebreak_insn, 4, 1)) {
              return -EINVAL;
      }
   } else {
      if (cpu_memory_rw_debug(cs, bp->pc, &c_ebreak_insn, 2, 1)) {
          return -EINVAL;
      }
   }

> +
> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
> +{
> +    uint8_t length;
> +
> +    if (len == 4 || len == 2) {
> +        length = (uint8_t)len;
> +    } else if (len == 0) {
> +        /* Need to decide the instruction length in this case. */
> +        uint32_t read_4_bytes;
> +        uint16_t read_2_bytes;
> +
> +        if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
> +            cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
> +            return -EINVAL;
> +        }
> +
> +        if (read_4_bytes == ebreak_insn) {
> +            length = 4;
> +        } else if (read_2_bytes == c_ebreak_insn) {
> +            length = 2;
> +        } else {
> +            return -EINVAL;
> +        }
> +    } else {
> +        return -EINVAL;
> +    }
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> +                            length, 1)) {
> +        return -EINVAL;
> +    }

Also no need need for 'len'.

   uint16_t ebreak;

   if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&ebreak, 2, 0)) {
       return -EINVAL;
   }

   if ((ebreak & 0x3) == 0x3) {
       if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
          return -EINVAL;
       }
   } else {
       if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 2, 1)) {
          return -EINVAL;
       }
   }

Thanks,
drew

> +
> +    return 0;
> +}
> +
> +int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
> +{
> +    /* TODO; To be implemented later. */
> +    return -EINVAL;
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
> +{
> +    /* TODO; To be implemented later. */
> +    return -EINVAL;
> +}
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> +    /* TODO; To be implemented later. */
> +}
> +
> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> +{
> +    /* TODO; To be implemented later. */
> +}
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 1b494ecc20..132952cc84 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -865,7 +865,8 @@ static void determine_sw_breakpoint_instr(void)
>          }
>  }
>  
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      determine_sw_breakpoint_instr();
>  
> @@ -877,7 +878,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>  
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>  {
>      uint8_t t[MAX_ILEN];
>  
> -- 
> 2.17.1
> 
>
Re: [PATCH v1 1/4] target/riscv/kvm: add software breakpoints support
Posted by Chao Du 6 months ago
On 2024-05-27 23:41, Andrew Jones <ajones@ventanamicro.com> wrote:
> On Mon, May 27, 2024 at 02:19:13AM GMT, Chao Du wrote:
> > This patch implements insert/remove software breakpoint process:
> > 
> > Add an input parameter for kvm_arch_insert_sw_breakpoint() and
> > kvm_arch_remove_sw_breakpoint() to pass the length information,
> > which helps us to know whether it is a RVC instruction.
> > For some remove cases, we do not have the length info, so we need
> > to judge by ourselves.
> > 
> > For RISC-V, GDB treats single-step similarly to breakpoint: add a
> > breakpoint at the next step address, then continue. So this also
> > works for single-step debugging.
> > 
> > Add some stubs which are necessary for building, and will be
> > implemented later.
> > 
> > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > ---
> >  accel/kvm/kvm-all.c        |  8 ++--
> >  include/sysemu/kvm.h       |  6 ++-
> >  target/arm/kvm.c           |  6 ++-
> >  target/i386/kvm/kvm.c      |  6 ++-
> >  target/mips/kvm.c          |  6 ++-
> >  target/ppc/kvm.c           |  6 ++-
> >  target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
> >  target/s390x/kvm/kvm.c     |  6 ++-
> >  8 files changed, 107 insertions(+), 16 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index c0be9f5eed..d27e77dbb2 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -3357,7 +3357,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
> >          bp = g_new(struct kvm_sw_breakpoint, 1);
> >          bp->pc = addr;
> >          bp->use_count = 1;
> > -        err = kvm_arch_insert_sw_breakpoint(cpu, bp);
> > +        err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
> >          if (err) {
> >              g_free(bp);
> >              return err;
> > @@ -3396,7 +3396,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
> >              return 0;
> >          }
> >  
> > -        err = kvm_arch_remove_sw_breakpoint(cpu, bp);
> > +        err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
> >          if (err) {
> >              return err;
> >          }
> > @@ -3426,10 +3426,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
> >      CPUState *tmpcpu;
> >  
> >      QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
> > -        if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
> > +        if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
> >              /* Try harder to find a CPU that currently sees the breakpoint. */
> >              CPU_FOREACH(tmpcpu) {
> > -                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
> > +                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {
> 
> It's not nice to need to add 'len' to all arch insert/remove sw breakpoint
> implementations, and the fact we have to pass zero sometimes implies it's
> not the right approach.

Actually, I've considered checking the instruction length from the tail two
bits, as you suggested. But it may bring one additional memory read.
So I turned to use the length information from gdb. In most cases, we can
get the exact length and read/write accordingly. But the side effect is we
need to add an input parameter and hence all arch implementations need to
be adapted.
I chose the second way after a 'balance'.

Now I think the first one may be a 'cleaner' solution.
Will send the V2 series later.

Thanks.
Chao

> 
> >                      break;
> >                  }
> >              }
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index c31d9c7356..340e094ffb 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
> >  int kvm_sw_breakpoints_active(CPUState *cpu);
> >  
> >  int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
> > -                                  struct kvm_sw_breakpoint *bp);
> > +                                  struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len);
> >  int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
> > -                                  struct kvm_sw_breakpoint *bp);
> > +                                  struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len);
> >  int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
> >  int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
> >  void kvm_arch_remove_all_hw_breakpoints(void);
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 7cf5cf31de..84593db544 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -2402,7 +2402,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> >  /* C6.6.29 BRK instruction */
> >  static const uint32_t brk_insn = 0xd4200000;
> >  
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> >          cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> > @@ -2411,7 +2412,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> >      return 0;
> >  }
> >  
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      static uint32_t brk;
> >  
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index c5943605ee..6449f796d0 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -4992,7 +4992,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
> >      return 1;
> >  }
> >  
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      static const uint8_t int3 = 0xcc;
> >  
> > @@ -5003,7 +5004,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> >      return 0;
> >  }
> >  
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      uint8_t int3;
> >  
> > diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> > index a631ab544f..129964cf6d 100644
> > --- a/target/mips/kvm.c
> > +++ b/target/mips/kvm.c
> > @@ -111,13 +111,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
> >      DPRINTF("%s\n", __func__);
> >  }
> >  
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      DPRINTF("%s\n", __func__);
> >      return 0;
> >  }
> >  
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      DPRINTF("%s\n", __func__);
> >      return 0;
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 46fccff786..9b76bdaa05 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -1378,7 +1378,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> >      return 0;
> >  }
> >  
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      /* Mixed endian case is not handled */
> >      uint32_t sc = debug_inst_opcode;
> > @@ -1392,7 +1393,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> >      return 0;
> >  }
> >  
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      uint32_t sc;
> >  
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index 473416649f..cba55c552d 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -1962,3 +1962,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
> >  };
> >  
> >  DEFINE_TYPES(riscv_kvm_cpu_type_infos)
> > +
> > +static const uint32_t ebreak_insn = 0x00100073;
> > +static const uint16_t c_ebreak_insn = 0x9002;
> > +
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> > +{
> > +    if (len != 4 && len != 2) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
> > +                                  (uint8_t *)&c_ebreak_insn;
> > +
> > +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
> > +        cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
> > +        return -EINVAL;
> > +    }
> 
> We can also read 2 bytes. So how about 
> 
>   if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 2, 0)) {
>       return -EINVAL;
>   }
> 
>   if ((bp->saved_insn & 0x3) == 0x3) {
>       if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
>           cpu_memory_rw_debug(cs, bp->pc, &ebreak_insn, 4, 1)) {
>               return -EINVAL;
>       }
>    } else {
>       if (cpu_memory_rw_debug(cs, bp->pc, &c_ebreak_insn, 2, 1)) {
>           return -EINVAL;
>       }
>    }
> 
> > +
> > +    return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> > +{
> > +    uint8_t length;
> > +
> > +    if (len == 4 || len == 2) {
> > +        length = (uint8_t)len;
> > +    } else if (len == 0) {
> > +        /* Need to decide the instruction length in this case. */
> > +        uint32_t read_4_bytes;
> > +        uint16_t read_2_bytes;
> > +
> > +        if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
> > +            cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
> > +            return -EINVAL;
> > +        }
> > +
> > +        if (read_4_bytes == ebreak_insn) {
> > +            length = 4;
> > +        } else if (read_2_bytes == c_ebreak_insn) {
> > +            length = 2;
> > +        } else {
> > +            return -EINVAL;
> > +        }
> > +    } else {
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> > +                            length, 1)) {
> > +        return -EINVAL;
> > +    }
> 
> Also no need need for 'len'.
> 
>    uint16_t ebreak;
> 
>    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&ebreak, 2, 0)) {
>        return -EINVAL;
>    }
> 
>    if ((ebreak & 0x3) == 0x3) {
>        if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
>           return -EINVAL;
>        }
>    } else {
>        if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 2, 1)) {
>           return -EINVAL;
>        }
>    }
> 
> Thanks,
> drew
> 
> > +
> > +    return 0;
> > +}
> > +
> > +int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
> > +{
> > +    /* TODO; To be implemented later. */
> > +    return -EINVAL;
> > +}
> > +
> > +int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
> > +{
> > +    /* TODO; To be implemented later. */
> > +    return -EINVAL;
> > +}
> > +
> > +void kvm_arch_remove_all_hw_breakpoints(void)
> > +{
> > +    /* TODO; To be implemented later. */
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> > +{
> > +    /* TODO; To be implemented later. */
> > +}
> > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > index 1b494ecc20..132952cc84 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -865,7 +865,8 @@ static void determine_sw_breakpoint_instr(void)
> >          }
> >  }
> >  
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      determine_sw_breakpoint_instr();
> >  
> > @@ -877,7 +878,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> >      return 0;
> >  }
> >  
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> >  {
> >      uint8_t t[MAX_ILEN];
> >  
> > -- 
> > 2.17.1
> > 
> > 
Re: [PATCH v1 1/4] target/riscv/kvm: add software breakpoints support
Posted by Daniel Henrique Barboza 6 months ago

On 5/26/24 23:19, Chao Du wrote:
> This patch implements insert/remove software breakpoint process:
> 
> Add an input parameter for kvm_arch_insert_sw_breakpoint() and
> kvm_arch_remove_sw_breakpoint() to pass the length information,
> which helps us to know whether it is a RVC instruction.
> For some remove cases, we do not have the length info, so we need
> to judge by ourselves.
> 
> For RISC-V, GDB treats single-step similarly to breakpoint: add a
> breakpoint at the next step address, then continue. So this also
> works for single-step debugging.
> 
> Add some stubs which are necessary for building, and will be
> implemented later.
> 
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   accel/kvm/kvm-all.c        |  8 ++--
>   include/sysemu/kvm.h       |  6 ++-
>   target/arm/kvm.c           |  6 ++-
>   target/i386/kvm/kvm.c      |  6 ++-
>   target/mips/kvm.c          |  6 ++-
>   target/ppc/kvm.c           |  6 ++-
>   target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c     |  6 ++-
>   8 files changed, 107 insertions(+), 16 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c0be9f5eed..d27e77dbb2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3357,7 +3357,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
>           bp = g_new(struct kvm_sw_breakpoint, 1);
>           bp->pc = addr;
>           bp->use_count = 1;
> -        err = kvm_arch_insert_sw_breakpoint(cpu, bp);
> +        err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
>           if (err) {
>               g_free(bp);
>               return err;
> @@ -3396,7 +3396,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
>               return 0;
>           }
>   
> -        err = kvm_arch_remove_sw_breakpoint(cpu, bp);
> +        err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
>           if (err) {
>               return err;
>           }
> @@ -3426,10 +3426,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
>       CPUState *tmpcpu;
>   
>       QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
> -        if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
> +        if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
>               /* Try harder to find a CPU that currently sees the breakpoint. */
>               CPU_FOREACH(tmpcpu) {
> -                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
> +                if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {
>                       break;
>                   }
>               }
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index c31d9c7356..340e094ffb 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
>   int kvm_sw_breakpoints_active(CPUState *cpu);
>   
>   int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
> -                                  struct kvm_sw_breakpoint *bp);
> +                                  struct kvm_sw_breakpoint *bp,
> +                                  vaddr len);
>   int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
> -                                  struct kvm_sw_breakpoint *bp);
> +                                  struct kvm_sw_breakpoint *bp,
> +                                  vaddr len);
>   int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
>   int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
>   void kvm_arch_remove_all_hw_breakpoints(void);
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 7cf5cf31de..84593db544 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -2402,7 +2402,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>   /* C6.6.29 BRK instruction */
>   static const uint32_t brk_insn = 0xd4200000;
>   
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
>           cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> @@ -2411,7 +2412,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>       return 0;
>   }
>   
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       static uint32_t brk;
>   
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index c5943605ee..6449f796d0 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4992,7 +4992,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
>       return 1;
>   }
>   
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       static const uint8_t int3 = 0xcc;
>   
> @@ -5003,7 +5004,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>       return 0;
>   }
>   
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       uint8_t int3;
>   
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index a631ab544f..129964cf6d 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -111,13 +111,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
>       DPRINTF("%s\n", __func__);
>   }
>   
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       DPRINTF("%s\n", __func__);
>       return 0;
>   }
>   
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       DPRINTF("%s\n", __func__);
>       return 0;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 46fccff786..9b76bdaa05 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1378,7 +1378,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
>       return 0;
>   }
>   
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       /* Mixed endian case is not handled */
>       uint32_t sc = debug_inst_opcode;
> @@ -1392,7 +1393,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>       return 0;
>   }
>   
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       uint32_t sc;
>   
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 473416649f..cba55c552d 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1962,3 +1962,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
>   };
>   
>   DEFINE_TYPES(riscv_kvm_cpu_type_infos)
> +
> +static const uint32_t ebreak_insn = 0x00100073;
> +static const uint16_t c_ebreak_insn = 0x9002;
> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
> +{
> +    if (len != 4 && len != 2) {
> +        return -EINVAL;
> +    }
> +
> +    uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
> +                                  (uint8_t *)&c_ebreak_insn;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
> +{
> +    uint8_t length;
> +
> +    if (len == 4 || len == 2) {
> +        length = (uint8_t)len;
> +    } else if (len == 0) {
> +        /* Need to decide the instruction length in this case. */
> +        uint32_t read_4_bytes;
> +        uint16_t read_2_bytes;
> +
> +        if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
> +            cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
> +            return -EINVAL;
> +        }
> +
> +        if (read_4_bytes == ebreak_insn) {
> +            length = 4;
> +        } else if (read_2_bytes == c_ebreak_insn) {
> +            length = 2;
> +        } else {
> +            return -EINVAL;
> +        }
> +    } else {
> +        return -EINVAL;
> +    }
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> +                            length, 1)) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
> +{
> +    /* TODO; To be implemented later. */
> +    return -EINVAL;
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
> +{
> +    /* TODO; To be implemented later. */
> +    return -EINVAL;
> +}
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> +    /* TODO; To be implemented later. */
> +}
> +
> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> +{
> +    /* TODO; To be implemented later. */
> +}
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 1b494ecc20..132952cc84 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -865,7 +865,8 @@ static void determine_sw_breakpoint_instr(void)
>           }
>   }
>   
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       determine_sw_breakpoint_instr();
>   
> @@ -877,7 +878,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>       return 0;
>   }
>   
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> +                                  vaddr len)
>   {
>       uint8_t t[MAX_ILEN];
>