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

["Chao Du" <duchao@eswincomputing.com>]

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