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

[Daniel Henrique Barboza]

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?

If it's up to me I would make 'debug' as default 'false' and deprecate it. 
Users will need
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.


Alistair, any thoughts?


Thanks,


Daniel


On 1/10/24 01:02, Himanshu Chauhan wrote:
> All the CPUs may or may not implement the debug trigger (sdtrig)
> extension. The presence of it should be dynamically detectable.
> This patch exports the debug triggers as an extension which
> can be turned on or off by sdtrig=<true/false> option. It is
> turned on by default.
>
> "sdtrig" is concatenated to ISA string when it is enabled.
> Like so:
> rv64imafdch_zicbom_*_sdtrig_*_sstc_svadu
>
>
> Himanshu Chauhan (2):
>    target/riscv: Export sdtrig as an extension and ISA string
>    target/riscv: Raise an exception when sdtrig is turned off
>
>   target/riscv/cpu.c     |  2 ++
>   target/riscv/cpu_cfg.h |  1 +
>   target/riscv/csr.c     | 20 ++++++++++++++++++++
>   3 files changed, 23 insertions(+)