| Message ID | 20231221094923.7349-2-duchao@eswincomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V | expand |
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]; >
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]; > >
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
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
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];
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(-)