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

[Chao Du]

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