From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiang W Date: Sun, 11 Dec 2022 09:40:03 +0800 Subject: [PATCH RFC v2 2/2] lib: sbi: add support for debug triggers In-Reply-To: References: <20221203213929.206429-1-geomatsi@gmail.com> <20221203213929.206429-3-geomatsi@gmail.com> <92833f1aeda3262f4ccb4b7fc796ee4de36c7ef0.camel@126.com> Message-ID: List-Id: To: opensbi@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit ? 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