Re: [PATCH 0/2] Export debug triggers as an extension

[Rob Bradford]

On Fri, 2024-01-12 at 13:52 +1000, Alistair Francis wrote:
> On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > 
> > Himanshu,
> > 
> > We spoke offline but let's make everyone aware:
> > 
> > - 'sdtrig' should be marked with 'x-' and be an experimental
> > extension since
> > the spec isn't yet frozen;
> > 
> > - Alvin sent a patch to the ML adding the 'mcontext' CSR for
> > 'sdtrig' some time
> > ago:
> > 
> > "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug
> > Sdtrig extension"
> > 
> > It would be good to put his patch on top of this series to ease the
> > review for everyone.
> > The changes done in patch 2 would also be applicable to the
> > mcontext CSR;
> > 
> > 
> > - last but probably the most important: the existing 'debug' flag
> > seems to be acting as
> > the actual 'sdtrig' extension due to how the flag is gating trigger
> > code, e.g.:
> > 
> >    if (cpu->cfg.debug) {
> >          riscv_trigger_realize(&cpu->env);
> >      }
> > 
> > and
> > 
> >      if (cpu->cfg.debug) {
> >          riscv_trigger_reset_hold(env);
> >      }
> > 
> > 
> > If that's really the case, all the checks with cpu->cfg.debug will
> > need to also include
> > cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make
> > an option: do we leave
> > the debug triggers (i.e. the 'debug' flag) as always enabled?
> 
> From memory the "debug" property is for the original debug spec:
> https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> 
> That was ratified and is an official extension. AFAIK this is what is
> in physical hardware as well.
> 
> The actual PDF says draft though, I'm not sure what's going on there.
> 
> The debug spec doesn't have a Z* name, so it's just "debug", at least
> AFAIK.
> 
> "sdtrig" seems to be a new backwards-incompatible extension doing
> basically the same thing. What a mess
> 
> > 
> > If it's up to me I would make 'debug' as default 'false' and
> > deprecate it. Users will need
> 
> I don't think that's the right approach. It's a ratified extension
> that we are supporting and is in hardware. I think we are stuck
> supporting it
> 
> 

I've done a bit of digging and I agree things are quite messy. Here are
my discoveries:

The debug option and the code for triggers was added in these commits:

c9711bd778 target/riscv: cpu: Enable native debug feature
38b4e781a4 target/riscv: machine: Add debug state description
b6092544fc target/riscv: csr: Hook debug CSR read/write
1acdb3b013 target/riscv: cpu: Add a config option for native debug
95799e36c1 target/riscv: Add initial support for the Sdtrig extension

In March 2022 - since the commit refers to the Sdtrig extension name
and from the date this was an implementation not of the ratified 0.13
debug spec (which did not have Sdtrig as a separate extension) but
rather a version of the in development 1.0 debug spec.

It's not trivial to tell if it's closer to the ratified 0.13 version or
the (hopefully soon to be frozen) 1.0 version.

As the only part of the debug specification to be implemented is the
triggers then effectively the debug option is x-sdtrig.

I don't think there is any way for code running on the machine to
identify what version of the debug is implemented - the appropriate
register is only available for external debug. Once 1.0 is frozen then
the presence of Sdtrig isa string would indicate 1.0 trigger support is
available.

According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
specification should freeze this month.

How about considering this as a solution:

- Add a new x-sdtrig option that defaults to false
- Deprecate debug option - but retain it with default on
- Add warning if triggers are used and x-sdtrig is not enabled
- Update the trigger implementation to match frozen spec

There is potentially a chance that some use cases will be broken but I
don't think triggers are being widely use - the SBI support only just
got merged:
https://github.com/riscv-software-src/opensbi/commit/97f234f15c9657c6ec69fa6ed745be8107bf6ae2

Hope this is helpful,

Rob