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

[Alistair Francis]

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

> to enable the debug triggers via x-sdtrig=true from now on. This will break 
> existing behavior,
> but the way it is now we're always enabling an extension (via the debug flag) 
> that isn't even
> frozen, so we're already in the wrong.

Maybe the debug group can chime in and say what they expect users to
do? It seems like there are conflicting specs

Alistair