From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: Chao Du <duchao@eswincomputing.com>,
qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
pbonzini@redhat.com, alistair23@gmail.com,
bin.meng@windriver.com, liweiwei@iscas.ac.cn,
zhiwei_liu@linux.alibaba.com, palmer@dabbelt.com,
anup@brainfault.org, atishp@atishpatra.org
Subject: Re: [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support
Date: Tue, 16 Apr 2024 06:23:43 -0300 [thread overview]
Message-ID: <ada42503-dab4-474f-a61a-a9fe3fa63afa@ventanamicro.com> (raw)
In-Reply-To: <20231221094923.7349-2-duchao@eswincomputing.com>
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];
>
next prev parent reply other threads:[~2024-04-16 9:24 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
2023-12-21 9:49 ` [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support Chao Du
2024-04-16 9:23 ` Daniel Henrique Barboza [this message]
2024-04-18 9:46 ` Chao Du
2024-05-24 16:11 ` Paolo Bonzini
2024-05-27 2:12 ` Chao Du
2023-12-21 9:49 ` [RFC PATCH 2/4] target/riscv/kvm: implement kvm_arch_update_guest_debug() Chao Du
2023-12-21 9:49 ` [RFC PATCH 3/4] target/riscv/kvm: handle the exit with debug reason Chao Du
2023-12-21 9:49 ` [RFC PATCH 4/4] linux-headers: enable KVM GUEST DEBUG for RISC-V Chao Du
2023-12-22 14:16 ` [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Daniel Henrique Barboza
2024-04-09 9:43 ` Chao Du
2024-04-15 1:47 ` Chao Du
2024-04-16 9:25 ` Daniel Henrique Barboza
2024-05-20 10:22 ` Chao Du
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ada42503-dab4-474f-a61a-a9fe3fa63afa@ventanamicro.com \
--to=dbarboza@ventanamicro.com \
--cc=alistair23@gmail.com \
--cc=anup@brainfault.org \
--cc=atishp@atishpatra.org \
--cc=bin.meng@windriver.com \
--cc=duchao@eswincomputing.com \
--cc=liweiwei@iscas.ac.cn \
--cc=palmer@dabbelt.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).