[PATCH RFC v2 2/2] lib: sbi: add support for debug triggers

[Xiang W]

在 2022-12-10星期六的 22:15 +0300，Sergey Matyukevich写道：
> > > 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 triggers consists of the following
> > > major components:
> > > - U-mode: gdb
> > > - S-mode: hardware breakpoints framework in Linux kernel
> > > - M-mode: SBI firmware code to handle triggers
> > > 
> > > SBI Debug Trigger extension proposal 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:
> > > - supported triggers: mcontrol, mcontrol6
> > > - no support for chained triggers
> > > - only build test for RV32
> 
> ... [snip]
> 
> > > +struct sbi_dbtr_trig_info {
> > > +       unsigned long type_mask;
> > > +       union sbi_dbtr_trig_state state;
> > > +       unsigned long tdata1;
> > > +       unsigned long tdata2;
> > > +       unsigned long tdata3;
> > tdata1~tdata3 can be removed and written directly to scr during install
> 
> We need to write those values not only when we install triggers. We have
> to rewrite them when we re-enable triggers after disabling or updating.
> So I think it is more convenient to keep the whole state at hand.

when install, write directly to tdata1~data3
when read, read from tdata1~tdata3, and OR the vs/vu/s/u bits in trig_status with tdata1  
when disable, clear vs/vu/s/u in tdata1 
when enable, write vs/vu/s/u in trig_state to tdata1
when updat, Only need write recv to tdata2

Does not require more memory to save tdata1~tdata3

Regards,
Xiang W
> 
> > > +};
> > > +
> > > +/** SBI shared mem messages layout */
> > > +struct sbi_dbtr_data_msg {
> > > +       unsigned long tstate;
> > > +       unsigned long tdata1;
> > > +       unsigned long tdata2;
> > > +       unsigned long tdata3;
> > > +};
> > > +
> > > +struct sbi_dbtr_id_msg {
> > > +       unsigned long idx;
> > > +};
> 
> ... [snip]
> 
> > > +int sbi_dbtr_init(struct sbi_scratch *scratch)
> > > +{
> > > +       struct sbi_trap_info trap = {0};
> > > +       u32 hartid = current_hartid();
> > > +       union riscv_dbtr_tdata1 tdata1;
> > > +       unsigned long val;
> > > +       int i;
> > > +
> > > +       total_trigs = 0;
> > > +
> > > +       for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> > > +               csr_write_allowed(CSR_TSELECT, (ulong)&trap, i);
> > > +               if (trap.cause)
> > > +                       break;
> > > +
> > > +               val = csr_read_allowed(CSR_TSELECT, (ulong)&trap);
> > > +               if (trap.cause)
> > > +                       break;
> > > +
> > > +               /* Read back tselect and check that it contains the written value */
> > > +               if (val != i)
> > > +                       break;
> > > +
> > > +               val = csr_read_allowed(CSR_TINFO, (ulong)&trap);
> > > +               if (trap.cause) {
> > > +                       /**
> > > +                        * If reading tinfo caused an exception, the debugger
> > > +                        * must read tdata1 to discover the type.
> > > +                        */
> > > +                       tdata1.value = csr_read_allowed(CSR_TDATA1, (ulong)&trap);
> > > +                       if (trap.cause)
> > > +                               break;
> > > +
> > > +                       if (tdata1.type == 0)
> > > +                               break;
> > > +
> > > +
> > > +                       sbi_triggers_table_init(hartid, i, BIT(tdata1.type));
> > > +                       total_trigs++;
> > > +               } else {
> > > +                       if (val == 1)
> > > +                               break;
> > tinfo = 0 have to break, this is a invalid value.
> 
> Good catch! But it looks like a grey area in the spec.  This function
> follows enumeration procedure described in the section 5.1 of Debug
> spec v1.0.0. That procedure has the following steps for tinfo:
> 
> : 3. Read tinfo.
> : 4. If that caused an exception, the debugger must read tdata1 to discover the type.
>      (If type is 0, this trigger doesn’t exist. Exit the loop.)
> : 5. If info is 1, this trigger doesn’t exist. Exit the loop.
> : 6. Otherwise, the selected trigger supports the types discovered in info.
> : 7. Repeat, incrementing the value in tselect.
> 
> So it looks like tinfo = 0 can be interpreted as valid. For instance,
> tinfo = 0 can be returned for a trigger disabled on a hardware level.
> Note that type 0 (tinfo = 1) can not be used to do the same since that
> value prevents futher enumeration.
> 
> I will write to riscv tech-debug mailing list to clarify this point so
> that the next revision of patches will include either fix (as per your
> comment) or appropriate comment.
> 
> Regards,
> Sergey