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