On Tue, Apr 16, 2024 at 11:23 AM Daniel Henrique Barboza <dbarb...@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