[RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[Chao Du]

This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
debug RISC-V KVM guest from the host side, with software breakpoints.

A brief test was done on QEMU RISC-V hypervisor emulator.

A TODO list which will be added later:
1. HW breakpoints support
2. Test cases

This series is based on QEMU 8.2.0-rc4 and is also available at:
https://github.com/Du-Chao/qemu/tree/riscv_gd_sw

This is dependent on KVM side changes:
https://github.com/Du-Chao/linux/tree/riscv_gd_sw

Chao Du (4):
  target/riscv/kvm: add software breakpoints support
  target/riscv/kvm: implement kvm_arch_update_guest_debug()
  target/riscv/kvm: handle the exit with debug reason
  linux-headers: enable KVM GUEST DEBUG for RISC-V

 accel/kvm/kvm-all.c           |   8 +--
 include/sysemu/kvm.h          |   6 +-
 linux-headers/asm-riscv/kvm.h |   1 +
 target/arm/kvm64.c            |   6 +-
 target/i386/kvm/kvm.c         |   6 +-
 target/mips/kvm.c             |   6 +-
 target/ppc/kvm.c              |   6 +-
 target/riscv/kvm/kvm-cpu.c    | 101 ++++++++++++++++++++++++++++++++++
 target/s390x/kvm/kvm.c        |   6 +-
 9 files changed, 130 insertions(+), 16 deletions(-)

--
2.17.1

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

[Chao Du]

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 compressed 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/kvm64.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 e39a810a4e..ccc505d0c2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3231,7 +3231,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;
@@ -3270,7 +3270,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;
         }
@@ -3300,10 +3300,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 d614878164..ab38c23def 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/kvm64.c b/target/arm/kvm64.c
index 3c175c93a7..023e92b577 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1139,7 +1139,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 (have_guest_debug) {
         if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
@@ -1153,7 +1154,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     }
 }
 
-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 4ce80555b4..742b7c8296 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -4935,7 +4935,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;
 
@@ -4946,7 +4947,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 e22e24ed97..2f68938cdf 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -112,13 +112,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 9b1abe2fc4..a99c85b2f3 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1375,7 +1375,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;
@@ -1389,7 +1390,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 45b6cf1cfa..e9110006b0 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1521,3 +1521,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 33ab3551f4..fafacedd6a 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -867,7 +867,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();
 
@@ -879,7 +880,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: [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support

[Daniel Henrique Barboza]

On 12/21/23 06:49, 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 compressed 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/kvm64.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 e39a810a4e..ccc505d0c2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3231,7 +3231,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;
> @@ -3270,7 +3270,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;
>           }
> @@ -3300,10 +3300,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 d614878164..ab38c23def 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/kvm64.c b/target/arm/kvm64.c
> index 3c175c93a7..023e92b577 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -1139,7 +1139,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 (have_guest_debug) {
>           if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> @@ -1153,7 +1154,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>       }
>   }
>   
> -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 4ce80555b4..742b7c8296 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4935,7 +4935,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;
>   
> @@ -4946,7 +4947,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 e22e24ed97..2f68938cdf 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -112,13 +112,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 9b1abe2fc4..a99c85b2f3 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1375,7 +1375,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;
> @@ -1389,7 +1390,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 45b6cf1cfa..e9110006b0 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1521,3 +1521,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;
> +    }

I wonder if this verification should be moved to kvm_insert_breakpoint(). Is
there any known reason why other archs would use 'len' other than 2 or 4? The
parent function can throw the EINVAL in this case. Otherwise all callers from
all archs will need a similar EINVAL check.

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

Same question as above - perhaps the len = 0 | 2 | 4 conditional can be moved to
kvm_remove_breakpoint().


Thanks,


Daniel


> +    } 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 33ab3551f4..fafacedd6a 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -867,7 +867,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();
>   
> @@ -879,7 +880,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];
>   

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

[Chao Du]

On 2024-04-16 17:23, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> 
> On 12/21/23 06:49, 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 compressed 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/kvm64.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 e39a810a4e..ccc505d0c2 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -3231,7 +3231,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;
> > @@ -3270,7 +3270,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;
> >           }
> > @@ -3300,10 +3300,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 d614878164..ab38c23def 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/kvm64.c b/target/arm/kvm64.c
> > index 3c175c93a7..023e92b577 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -1139,7 +1139,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 (have_guest_debug) {
> >           if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> > @@ -1153,7 +1154,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> >       }
> >   }
> >   
> > -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 4ce80555b4..742b7c8296 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -4935,7 +4935,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;
> >   
> > @@ -4946,7 +4947,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 e22e24ed97..2f68938cdf 100644
> > --- a/target/mips/kvm.c
> > +++ b/target/mips/kvm.c
> > @@ -112,13 +112,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 9b1abe2fc4..a99c85b2f3 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -1375,7 +1375,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;
> > @@ -1389,7 +1390,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 45b6cf1cfa..e9110006b0 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -1521,3 +1521,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;
> > +    }
> 
> I wonder if this verification should be moved to kvm_insert_breakpoint(). Is
> there any known reason why other archs would use 'len' other than 2 or 4? The
> parent function can throw the EINVAL in this case. Otherwise all callers from
> all archs will need a similar EINVAL check.
> 

Thanks for the review. And you are right, we can move this check to the parent
function, to avoid the duplicated codes. 
But since I'm not pretty sure whether it will break other archs, so I chose to
do this only for RISC-V. Keep other archs as they are.

As you mentioned, maybe Paolo could comment on this also.

> > +
> > +    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;
> 
> Same question as above - perhaps the len = 0 | 2 | 4 conditional can be moved to
> kvm_remove_breakpoint().
> 

Same as above.

Thanks,
Chao

> 
> Thanks,
> 
> 
> Daniel
> 
> 
> > +    } 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 33ab3551f4..fafacedd6a 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -867,7 +867,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();
> >   
> > @@ -879,7 +880,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];
> >   

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

[Paolo Bonzini]

On Tue, Apr 16, 2024 at 11:23 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > +                                  vaddr len)
> > +{
> > +    if (len != 4 && len != 2) {
> > +        return -EINVAL;
> > +    }
>
> I wonder if this verification should be moved to kvm_insert_breakpoint(). Is
> there any known reason why other archs would use 'len' other than 2 or 4? The
> parent function can throw the EINVAL in this case. Otherwise all callers from
> all archs will need a similar EINVAL check.

I'm not sure how len is defined in the gdb protocol, but x86 has a
breakpoint length of 1 and an instruction length that can be any value
between 1 and 15.

Most architectures could assume that it's always one value, i.e. just
not care about checking len in kvm_arch_insert_sw_breakpoint.

The patches look good, feel free to take them through the RISC-V tree.

One thing that I was wondering is: could RISC-V just use always
c.ebreak if C instructions are supported, and ebreak if they're not?
But if for example that would that mess up the synchronization of the
disassembly in gdb, it's a good reason to add the len argument as you
did here.

Paolo

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

[Chao Du]

On 2024-05-25 00:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On Tue, Apr 16, 2024 at 11:23 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > > +                                  vaddr len)
> > > +{
> > > +    if (len != 4 && len != 2) {
> > > +        return -EINVAL;
> > > +    }
> >
> > I wonder if this verification should be moved to kvm_insert_breakpoint(). Is
> > there any known reason why other archs would use 'len' other than 2 or 4? The
> > parent function can throw the EINVAL in this case. Otherwise all callers from
> > all archs will need a similar EINVAL check.
> 
> I'm not sure how len is defined in the gdb protocol, but x86 has a
> breakpoint length of 1 and an instruction length that can be any value
> between 1 and 15.
> 
> Most architectures could assume that it's always one value, i.e. just
> not care about checking len in kvm_arch_insert_sw_breakpoint.
> 
> The patches look good, feel free to take them through the RISC-V tree.
> 
> One thing that I was wondering is: could RISC-V just use always
> c.ebreak if C instructions are supported, and ebreak if they're not?
> But if for example that would that mess up the synchronization of the
> disassembly in gdb, it's a good reason to add the len argument as you
> did here.

Yes, you are right. If we insert an ebreak instruction whose length is
different from the original one, then the disassembly in gdb may encounter
some problems.

Thanks for the review, I will rebase this series and send it out.

Chao

> 
> Paolo

[RFC PATCH 2/4] target/riscv/kvm: implement kvm_arch_update_guest_debug()

[Chao Du]

Set the control flag when there are active breakpoints. This will
help KVM to know the status in the userspace.

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 target/riscv/kvm/kvm-cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index e9110006b0..94697b09bb 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1598,5 +1598,7 @@ void kvm_arch_remove_all_hw_breakpoints(void)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
-    /* TODO; To be implemented later. */
+    if (kvm_sw_breakpoints_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE;
+    }
 }
-- 
2.17.1

[RFC PATCH 3/4] target/riscv/kvm: handle the exit with debug reason

[Chao Du]

If the breakpoint belongs to the userspace then set the ret value.

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 target/riscv/kvm/kvm-cpu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 94697b09bb..b1aed78780 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1213,6 +1213,21 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
     return ret;
 }
 
+static bool kvm_riscv_handle_debug(CPUState *cs)
+{
+    RISCVCPU *cpu = RISCV_CPU(cs);
+    CPURISCVState *env = &cpu->env;
+
+    /* Ensure PC is synchronised */
+    kvm_cpu_synchronize_state(cs);
+
+    if (kvm_find_sw_breakpoint(cs, env->pc)) {
+        return true;
+    }
+
+    return false;
+}
+
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 {
     int ret = 0;
@@ -1220,6 +1235,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
     case KVM_EXIT_RISCV_SBI:
         ret = kvm_riscv_handle_sbi(cs, run);
         break;
+    case KVM_EXIT_DEBUG:
+        if (kvm_riscv_handle_debug(cs)) {
+            ret = EXCP_DEBUG;
+        }
+        break;
     default:
         qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
                       __func__, run->exit_reason);
-- 
2.17.1

[RFC PATCH 4/4] linux-headers: enable KVM GUEST DEBUG for RISC-V

[Chao Du]

Synchronize the kvm.h file which enables KVM GUEST DEBUG at QEMU side.

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 linux-headers/asm-riscv/kvm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-headers/asm-riscv/kvm.h b/linux-headers/asm-riscv/kvm.h
index 992c5e4071..72942a7aaf 100644
--- a/linux-headers/asm-riscv/kvm.h
+++ b/linux-headers/asm-riscv/kvm.h
@@ -17,6 +17,7 @@
 
 #define __KVM_HAVE_IRQ_LINE
 #define __KVM_HAVE_READONLY_MEM
+#define __KVM_HAVE_GUEST_DEBUG
 
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 
-- 
2.17.1

Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[Daniel Henrique Barboza]

Hi,

It seems that we still need the kernel KVM side to be sorted out first [1],
so I believe we should wait a bit until we can review this RFC. Otherwise we
might risk reviewing something that has to be changed later.


[1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/


Thanks,

Daniel

On 12/21/23 06:49, Chao Du wrote:
> This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> debug RISC-V KVM guest from the host side, with software breakpoints.
> 
> A brief test was done on QEMU RISC-V hypervisor emulator.
> 
> A TODO list which will be added later:
> 1. HW breakpoints support
> 2. Test cases
> 
> This series is based on QEMU 8.2.0-rc4 and is also available at:
> https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> 
> This is dependent on KVM side changes:
> https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> 
> Chao Du (4):
>    target/riscv/kvm: add software breakpoints support
>    target/riscv/kvm: implement kvm_arch_update_guest_debug()
>    target/riscv/kvm: handle the exit with debug reason
>    linux-headers: enable KVM GUEST DEBUG for RISC-V
> 
>   accel/kvm/kvm-all.c           |   8 +--
>   include/sysemu/kvm.h          |   6 +-
>   linux-headers/asm-riscv/kvm.h |   1 +
>   target/arm/kvm64.c            |   6 +-
>   target/i386/kvm/kvm.c         |   6 +-
>   target/mips/kvm.c             |   6 +-
>   target/ppc/kvm.c              |   6 +-
>   target/riscv/kvm/kvm-cpu.c    | 101 ++++++++++++++++++++++++++++++++++
>   target/s390x/kvm/kvm.c        |   6 +-
>   9 files changed, 130 insertions(+), 16 deletions(-)
> 
> --
> 2.17.1
> 

Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[Chao Du]

Hi Daniel and all,

The KVM patches have been reviewd and are in the queue.
https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/

Could you please review in the QEMU side ?
Then I will rebase this series with your comments.

Some Notes:
1. As the first stage, only the software breakpoints is implemented.
2. A 'corner case' in which the debug exception is not inserted by the
debugger, need to be re-injected to the guest. This is not handled yet
in this series.

Thanks,
Chao


On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> 
> Hi,
> 
> It seems that we still need the kernel KVM side to be sorted out first [1],
> so I believe we should wait a bit until we can review this RFC. Otherwise we
> might risk reviewing something that has to be changed later.
> 
> 
> [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
> 
> 
> Thanks,
> 
> Daniel
> 
> On 12/21/23 06:49, Chao Du wrote:
> > This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> > debug RISC-V KVM guest from the host side, with software breakpoints.
> > 
> > A brief test was done on QEMU RISC-V hypervisor emulator.
> > 
> > A TODO list which will be added later:
> > 1. HW breakpoints support
> > 2. Test cases
> > 
> > This series is based on QEMU 8.2.0-rc4 and is also available at:
> > https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> > 
> > This is dependent on KVM side changes:
> > https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> > 
> > Chao Du (4):
> >    target/riscv/kvm: add software breakpoints support
> >    target/riscv/kvm: implement kvm_arch_update_guest_debug()
> >    target/riscv/kvm: handle the exit with debug reason
> >    linux-headers: enable KVM GUEST DEBUG for RISC-V
> > 
> >   accel/kvm/kvm-all.c           |   8 +--
> >   include/sysemu/kvm.h          |   6 +-
> >   linux-headers/asm-riscv/kvm.h |   1 +
> >   target/arm/kvm64.c            |   6 +-
> >   target/i386/kvm/kvm.c         |   6 +-
> >   target/mips/kvm.c             |   6 +-
> >   target/ppc/kvm.c              |   6 +-
> >   target/riscv/kvm/kvm-cpu.c    | 101 ++++++++++++++++++++++++++++++++++
> >   target/s390x/kvm/kvm.c        |   6 +-
> >   9 files changed, 130 insertions(+), 16 deletions(-)
> > 
> > --
> > 2.17.1
> > 

Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[Chao Du]

Gentle ping...

On 2024-04-09 17:43, Chao Du <duchao@eswincomputing.com> wrote:
> 
> Hi Daniel and all,
> 
> The KVM patches have been reviewd and are in the queue.
> https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
> 
> Could you please review in the QEMU side ?
> Then I will rebase this series with your comments.
> 
> Some Notes:
> 1. As the first stage, only the software breakpoints is implemented.
> 2. A 'corner case' in which the debug exception is not inserted by the
> debugger, need to be re-injected to the guest. This is not handled yet
> in this series.
> 
> Thanks,
> Chao
> 
> 
> On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > 
> > Hi,
> > 
> > It seems that we still need the kernel KVM side to be sorted out first [1],
> > so I believe we should wait a bit until we can review this RFC. Otherwise we
> > might risk reviewing something that has to be changed later.
> > 
> > 
> > [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
> > 
> > 
> > Thanks,
> > 
> > Daniel
> > 
> > On 12/21/23 06:49, Chao Du wrote:
> > > This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> > > debug RISC-V KVM guest from the host side, with software breakpoints.
> > > 
> > > A brief test was done on QEMU RISC-V hypervisor emulator.
> > > 
> > > A TODO list which will be added later:
> > > 1. HW breakpoints support
> > > 2. Test cases
> > > 
> > > This series is based on QEMU 8.2.0-rc4 and is also available at:
> > > https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> > > 
> > > This is dependent on KVM side changes:
> > > https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> > > 
> > > Chao Du (4):
> > >    target/riscv/kvm: add software breakpoints support
> > >    target/riscv/kvm: implement kvm_arch_update_guest_debug()
> > >    target/riscv/kvm: handle the exit with debug reason
> > >    linux-headers: enable KVM GUEST DEBUG for RISC-V
> > > 
> > >   accel/kvm/kvm-all.c           |   8 +--
> > >   include/sysemu/kvm.h          |   6 +-
> > >   linux-headers/asm-riscv/kvm.h |   1 +
> > >   target/arm/kvm64.c            |   6 +-
> > >   target/i386/kvm/kvm.c         |   6 +-
> > >   target/mips/kvm.c             |   6 +-
> > >   target/ppc/kvm.c              |   6 +-
> > >   target/riscv/kvm/kvm-cpu.c    | 101 ++++++++++++++++++++++++++++++++++
> > >   target/s390x/kvm/kvm.c        |   6 +-
> > >   9 files changed, 130 insertions(+), 16 deletions(-)
> > > 
> > > --
> > > 2.17.1
> > > 

Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[Daniel Henrique Barboza]

On 4/9/24 06:43, Chao Du wrote:
> Hi Daniel and all,
> 
> The KVM patches have been reviewd and are in the queue.
> https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
> 
> Could you please review in the QEMU side ?
> Then I will rebase this series with your comments.
> 
> Some Notes:
> 1. As the first stage, only the software breakpoints is implemented.
> 2. A 'corner case' in which the debug exception is not inserted by the
> debugger, need to be re-injected to the guest. This is not handled yet
> in this series.

Aside from the comments I made in patch 1 w.r.t checks that (perhaps) can be moved
to kvm-all.c, it looks good to me.

Since you're changing kvm-all.c we'll need Paolo to ack the changes in patch 1, so
feel free to wait for him to take a look before sending v2.


Thanks,


Daniel

> 
> Thanks,
> Chao
> 
> 
> On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>
>> Hi,
>>
>> It seems that we still need the kernel KVM side to be sorted out first [1],
>> so I believe we should wait a bit until we can review this RFC. Otherwise we
>> might risk reviewing something that has to be changed later.
>>
>>
>> [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
>>
>>
>> Thanks,
>>
>> Daniel
>>
>> On 12/21/23 06:49, Chao Du wrote:
>>> This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
>>> debug RISC-V KVM guest from the host side, with software breakpoints.
>>>
>>> A brief test was done on QEMU RISC-V hypervisor emulator.
>>>
>>> A TODO list which will be added later:
>>> 1. HW breakpoints support
>>> 2. Test cases
>>>
>>> This series is based on QEMU 8.2.0-rc4 and is also available at:
>>> https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
>>>
>>> This is dependent on KVM side changes:
>>> https://github.com/Du-Chao/linux/tree/riscv_gd_sw
>>>
>>> Chao Du (4):
>>>     target/riscv/kvm: add software breakpoints support
>>>     target/riscv/kvm: implement kvm_arch_update_guest_debug()
>>>     target/riscv/kvm: handle the exit with debug reason
>>>     linux-headers: enable KVM GUEST DEBUG for RISC-V
>>>
>>>    accel/kvm/kvm-all.c           |   8 +--
>>>    include/sysemu/kvm.h          |   6 +-
>>>    linux-headers/asm-riscv/kvm.h |   1 +
>>>    target/arm/kvm64.c            |   6 +-
>>>    target/i386/kvm/kvm.c         |   6 +-
>>>    target/mips/kvm.c             |   6 +-
>>>    target/ppc/kvm.c              |   6 +-
>>>    target/riscv/kvm/kvm-cpu.c    | 101 ++++++++++++++++++++++++++++++++++
>>>    target/s390x/kvm/kvm.c        |   6 +-
>>>    9 files changed, 130 insertions(+), 16 deletions(-)
>>>
>>> --
>>> 2.17.1
>>>

Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V

[Chao Du]

Hi Paolo,

As Daniel suggested, could you please take a look at this series ?

Thanks,
Chao

On 2024-04-16 17:25, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> 
> On 4/9/24 06:43, Chao Du wrote:
> > Hi Daniel and all,
> > 
> > The KVM patches have been reviewd and are in the queue.
> > https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
> > 
> > Could you please review in the QEMU side ?
> > Then I will rebase this series with your comments.
> > 
> > Some Notes:
> > 1. As the first stage, only the software breakpoints is implemented.
> > 2. A 'corner case' in which the debug exception is not inserted by the
> > debugger, need to be re-injected to the guest. This is not handled yet
> > in this series.
> 
> Aside from the comments I made in patch 1 w.r.t checks that (perhaps) can be moved
> to kvm-all.c, it looks good to me.
> 
> Since you're changing kvm-all.c we'll need Paolo to ack the changes in patch 1, so
> feel free to wait for him to take a look before sending v2.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > Thanks,
> > Chao
> > 
> > 
> > On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> >>
> >> Hi,
> >>
> >> It seems that we still need the kernel KVM side to be sorted out first [1],
> >> so I believe we should wait a bit until we can review this RFC. Otherwise we
> >> might risk reviewing something that has to be changed later.
> >>
> >>
> >> [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
> >>
> >>
> >> Thanks,
> >>
> >> Daniel
> >>
> >> On 12/21/23 06:49, Chao Du wrote:
> >>> This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> >>> debug RISC-V KVM guest from the host side, with software breakpoints.
> >>>
> >>> A brief test was done on QEMU RISC-V hypervisor emulator.
> >>>
> >>> A TODO list which will be added later:
> >>> 1. HW breakpoints support
> >>> 2. Test cases
> >>>
> >>> This series is based on QEMU 8.2.0-rc4 and is also available at:
> >>> https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> >>>
> >>> This is dependent on KVM side changes:
> >>> https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> >>>
> >>> Chao Du (4):
> >>>     target/riscv/kvm: add software breakpoints support
> >>>     target/riscv/kvm: implement kvm_arch_update_guest_debug()
> >>>     target/riscv/kvm: handle the exit with debug reason
> >>>     linux-headers: enable KVM GUEST DEBUG for RISC-V
> >>>
> >>>    accel/kvm/kvm-all.c           |   8 +--
> >>>    include/sysemu/kvm.h          |   6 +-
> >>>    linux-headers/asm-riscv/kvm.h |   1 +
> >>>    target/arm/kvm64.c            |   6 +-
> >>>    target/i386/kvm/kvm.c         |   6 +-
> >>>    target/mips/kvm.c             |   6 +-
> >>>    target/ppc/kvm.c              |   6 +-
> >>>    target/riscv/kvm/kvm-cpu.c    | 101 ++++++++++++++++++++++++++++++++++
> >>>    target/s390x/kvm/kvm.c        |   6 +-
> >>>    9 files changed, 130 insertions(+), 16 deletions(-)
> >>>
> >>> --
> >>> 2.17.1
> >>>