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

[Alistair Francis]

On Mon, Feb 5, 2024 at 2:50 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Mon, Feb 5, 2024 at 9:36 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Mon, Jan 22, 2024 at 7:16 PM Andrew Jones <ajones@ventanamicro.com> 
> > wrote:
> > >
> > > On Mon, Jan 22, 2024 at 03:42:10PM +1000, Alistair Francis wrote:
> > > > > > 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
> > > ...
> > > > >
> > > > > 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.
> > > >
> > > > Yeah... We used the "stable" from master. That is our mistake there.
> > > >
> > > > I'm pretty sure we targeted the 0.13. The "Sdtrig" was only added in
> > > > the v4 as the changelog says: "mention Sdtrig extension in the commit"
> > > >
> > > > >
> > > > > 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
> > > >
> > > > We can't deprecate a ratified spec. The 0.13 just seems to call it
> > > > "debug" so that's what we are stuck with
> > > >
> > > > > - Add warning if triggers are used and x-sdtrig is not enabled
> > > > > - Update the trigger implementation to match frozen spec
> > > >
> > > > We will need to support two versions, as there are two ratified specs.
> > > >
> > >
> > > We'll likely want to be allowed to deprecate ratified extensions as riscv
> > > evolves. Despite best intentions, extensions may be designed and ratified
> > > which ultimately fail to be of much utility, and new extensions will
> > > supersede old extensions. If QEMU keeps every extension it adds, then
> > > we'll slow progress on new extensions by maintaining old extension code.
> > > The old extensions will also bitrot or waste CI resources getting tested
> > > for no reason.
> >
> > I agree that we might need to deprecate extensions.
> >
> > I'm not sure the debug extension is there though. The debug extension
> > is used in current shipping hardware and has been ratified. The Sdtrig
> > isn't even ratified yet
> > (https://lists.riscv.org/g/tech-announce/message/320)
>
> Is shipping real hardware OR ratification a requirement of
> QEMU patch acceptance ?

We will accept an extension when it is ratified. The question here is,
what if two ratified extensions conflict?

The answer to me seems that we need to support both them. *Maybe* at
some point in the future we can then drop the debug extension. That
would require the Sdtrig extension to be widely used and debug not
used (which is why I brought up shipping hardware).

Alistair

>
> Regards,
> Anup
>
> >
> > Right now I feel that we should at least wait for hardware that
> > supports Sdtrig to start to come out. Then we can look at deprecating
> > debug. Deprecating it now seems a bit premature.
> >
> > Alistair
> >
> > >
> > > I don't know the history of 'debug' and 'sdtrig', other than what I've
> > > read above, but, to me, it looks like 'debug' might be one of the first
> > > extensions which should be deprecated. Assuming we have a long enough
> > > deprecation period, then I think it's always safe to attempt a
> > > deprecation. If somebody shouts, then it can always be taken back off the
> > > chopping block.
> > >
> > > Thanks,
> > > drew
> > >
> >