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

[Himanshu Chauhan]

> On 17-Jan-2024, at 10:29 PM, Daniel Henrique Barboza 
> <dbarboza@ventanamicro.com> wrote:
> 
> 
> 
> On 1/12/24 10:34, Rob Bradford wrote:
>> 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
> 
> If x-sdtrig is 'false' and 'debug' is on, and then we warn if debug=true and 
> x-sdtrig
> is false, we'll warn every time using the defaults.
> 
> Given what you said here:
> 
>> 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/97f234f15c9657c6ec69fa6ed745be8107bf6a>
>>  
> 
> I believe we can deprecate 'debug' and simply ignore its existence. Do 
> everything else with
> x-sdtrig. So if an user sets any 'debug' value we'll:
> 
> - warn that the flag is deprecated
> - set x-sdtrig to whatever value the user set to 'debug'
> 
> 'debug' will become just an alternate way to set x-sdtrig. The logic should 
> just check for
> x-sdtrig.

My v2 patchset which I posted a few ours back, gets rid of ‘debug’. It is 
replaced with x-sdtrig. Keeping them both doesn’t make sense.

Here is v2:
https://mail.gnu.org/archive/html/qemu-riscv/2024-01/msg00570.html


Regards
Himanshu

> 
> 
> Thanks,
> 
> 
> Daniel
>  
>> Hope this is helpful,
>> Rob