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

[Jessica Clarke]

On 1 Nov 2022, at 10:55, 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.
> 
>>> +/** 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.

You don’t need packed for that, just like you don’t need to pack every
struct passed to a kernel from userspace. We have a well-defined ABI
that guarantees there will not be any padding in that struct, and I
don’t know of any ABI in the world that would.

Jess