[PATCH RFC v1 1/1] lib: sbi: add support for debug triggers

[Anup Patel]

On Tue, Nov 1, 2022 at 4:25 PM Sergey Matyukevich <geomatsi at gmail.com> wrote:
>
> Hi Jessica and all,
>
> > > RISC-V Debug specification includes Sdtrig ISA extension.
> > > This extension describes Trigger Module. Triggers can cause
> > > a breakpoint exception, entry into Debug Mode, or a trace
> > > action without having to execute a special instruction. For
> > > native debugging triggers can be used to implement hardware
> > > breakpoints and watchpoints.
> > >
> > > Software support for RISC-V hardware triggers consists of the
> > > following major components:
> > > - U-mode: gdb support for setting hw breakpoints/watchpoints
> > > - S/VS-mode: hardware breakpoints framework in Linux kernel
> > > - M-mode: SBI firmware code to handle triggers
> > >
> > > SBI Debug Trigger extension proposal (draft v4) has been posted
> > > by Anup Patel to lists.riscv.org tech-debug mailing list:
> > >
> > > https://lists.riscv.org/g/tech-debug/topic/92375492
> > >
> > > This patch provides initial implementation of SBI Debug
> > > Trigger Extension in OpenSBI library based on the
> > > suggested extension proposal.
> > >
> > > Initial implementation has the following limitations:
> > > - only mcontrol6 trigger type is supported
> > > - no support for chained triggers
> > > - trigger update supports only address change
>
> ...
>
> > > +union riscv_dbtr_tdata1 {
> > > +   unsigned long value;
> > > +   struct {
> > > +#if __riscv_xlen == 64
> > > +           unsigned long data:59;
> > > +#elif __riscv_xlen == 32
> > > +           unsigned long data:27;
> > > +#else
> > > +#error "Unexpected __riscv_xlen"
> > > +#endif
> > > +           unsigned long dmode:1;
> > > +           unsigned long type:4;
> > > +   };
> > > +};
> >
> > Bitfield order depends on endianness (type is in the MSBs for
> > little-endian but LSBs for big-endian).
>
> Do we need to support non-standard BE cases as well ? If so, then
> accessing fields in shared memory messages between OpenSBI and kernel
> will have to be wrapped by cpu_to_le/le_to_cpu macros on both sides.

Yes, in the long term we might support BE in both Linux and OpenSBI
so better to avoid bitfield for data structures in shared memory.

>
> > > +/** SBI shared mem messages layout */
> > > +struct sbi_dbtr_data_msg {
> > > +   unsigned long tstate;
> > > +   unsigned long tdata1;
> > > +   unsigned long tdata2;
> > > +   unsigned long tdata3;
> > > +} __packed;
> >
> > Why? This will give awful codegen.
>
> Both packed structures define messages for data exchange via shared
> memory between OpenSBI and kernel. We have to make sure that these
> structures have the same layout on both sides.
>
> Regards,
> Sergey

Regards,
Anup