[v2,2/7] RISC-V: Detect AIA CSRs from ISA string

Commit Message

Anup Patel Jan. 28, 2023, 7:27 a.m. UTC

We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
and Ssaia (S-mode AIA CSRs).

We extend the ISA string parsing to detect Smaia and Ssaia extensions.

Signed-off-by: Anup Patel <[e-mail]>
Reviewed-by: Andrew Jones <[e-mail]>
---
 arch/riscv/include/asm/hwcap.h | 2 ++
 arch/riscv/kernel/cpu.c        | 2 ++
 arch/riscv/kernel/cpufeature.c | 2 ++
 3 files changed, 6 insertions(+)

Comments

Atish Patra Jan. 31, 2023, 9:25 a.m. UTC | #1

On Fri, Jan 27, 2023 at 11:27 PM Anup Patel <[e-mail]> wrote:
>
> We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> and Ssaia (S-mode AIA CSRs).
>
> We extend the ISA string parsing to detect Smaia and Ssaia extensions.
>
> Signed-off-by: Anup Patel <[e-mail]>
> Reviewed-by: Andrew Jones <[e-mail]>
> ---
>  arch/riscv/include/asm/hwcap.h | 2 ++
>  arch/riscv/kernel/cpu.c        | 2 ++
>  arch/riscv/kernel/cpufeature.c | 2 ++
>  3 files changed, 6 insertions(+)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 86328e3acb02..341ef30a3718 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
>         RISCV_ISA_EXT_ZIHINTPAUSE,
>         RISCV_ISA_EXT_SSTC,
>         RISCV_ISA_EXT_SVINVAL,
> +       RISCV_ISA_EXT_SMAIA,
> +       RISCV_ISA_EXT_SSAIA,
>         RISCV_ISA_EXT_ID_MAX
>  };
>  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 1b9a5a66e55a..a215ec929160 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
>   *    extensions by an underscore.
>   */
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
> +       __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> +       __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>         __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>         __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>         __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 93e45560af30..3c5b51f519d5 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
>                                 SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>                                 SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
>                                 SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> +                               SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> +                               SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
>                         }
>  #undef SET_ISA_EXT_MAP
>                 }
> --
> 2.34.1
>


Reviewed-by: Atish Patra <[e-mail]>

Palmer Dabbelt Feb. 3, 2023, 12:24 a.m. UTC | #2

On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> and Ssaia (S-mode AIA CSRs).

This has pretty much the same problem that we had with the other 
AIA-related ISA string patches, where there's that ambiguity with the 
non-ratified chapters.  IIRC when this came up in GCC the rough idea was 
to try and document that we're going to interpret the standard ISA 
strings that way, but now that we're doing custom ISA extensions it 
seems saner to just define on here that removes the ambiguity.

I just sent
<https://lore.kernel.org/r/[e-mail]/> 
which documents that.

> We extend the ISA string parsing to detect Smaia and Ssaia extensions.
>
> Signed-off-by: Anup Patel <[e-mail]>
> Reviewed-by: Andrew Jones <[e-mail]>
> ---
>  arch/riscv/include/asm/hwcap.h | 2 ++
>  arch/riscv/kernel/cpu.c        | 2 ++
>  arch/riscv/kernel/cpufeature.c | 2 ++
>  3 files changed, 6 insertions(+)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 86328e3acb02..341ef30a3718 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
>  	RISCV_ISA_EXT_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_SSTC,
>  	RISCV_ISA_EXT_SVINVAL,
> +	RISCV_ISA_EXT_SMAIA,
> +	RISCV_ISA_EXT_SSAIA,
>  	RISCV_ISA_EXT_ID_MAX
>  };
>  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 1b9a5a66e55a..a215ec929160 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
>   *    extensions by an underscore.
>   */
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
> +	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> +	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),

This will conflict with that ISA string refactoring I just merged.  It 
should be a pretty mechanical merge conflict, but if you want we can do 
a shared tag with the first few patches and I can handle the merge 
conflict locally.

>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>  	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 93e45560af30..3c5b51f519d5 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
>  				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>  				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
>  				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> +				SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> +				SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
>  			}
>  #undef SET_ISA_EXT_MAP
>  		}

Anup Patel Feb. 3, 2023, 12:01 p.m. UTC | #3

On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
>
> On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > and Ssaia (S-mode AIA CSRs).
>
> This has pretty much the same problem that we had with the other
> AIA-related ISA string patches, where there's that ambiguity with the
> non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> to try and document that we're going to interpret the standard ISA
> strings that way, but now that we're doing custom ISA extensions it
> seems saner to just define on here that removes the ambiguity.
>
> I just sent
> <https://lore.kernel.org/r/[e-mail]/>
> which documents that.

I am not sure why you say that these are custom extensions.

Multiple folks have clarified that both Smaia and Ssaia are frozen
ISA extensions as-per RVI process. The individual chapters which
are in the draft state have nothing to do with Smaia and Ssaia CSRs.

Please refer:
https://github.com/riscv/riscv-aia/pull/36
https://lists.riscv.org/g/tech-aia/message/336
https://lists.riscv.org/g/tech-aia/message/337

>
> > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> >
> > Signed-off-by: Anup Patel <[e-mail]>
> > Reviewed-by: Andrew Jones <[e-mail]>
> > ---
> >  arch/riscv/include/asm/hwcap.h | 2 ++
> >  arch/riscv/kernel/cpu.c        | 2 ++
> >  arch/riscv/kernel/cpufeature.c | 2 ++
> >  3 files changed, 6 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 86328e3acb02..341ef30a3718 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> >       RISCV_ISA_EXT_ZIHINTPAUSE,
> >       RISCV_ISA_EXT_SSTC,
> >       RISCV_ISA_EXT_SVINVAL,
> > +     RISCV_ISA_EXT_SMAIA,
> > +     RISCV_ISA_EXT_SSAIA,
> >       RISCV_ISA_EXT_ID_MAX
> >  };
> >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 1b9a5a66e55a..a215ec929160 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> >   *    extensions by an underscore.
> >   */
> >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>
> This will conflict with that ISA string refactoring I just merged.  It
> should be a pretty mechanical merge conflict, but if you want we can do
> a shared tag with the first few patches and I can handle the merge
> conflict locally.

I am planning to send this series as a second PR for Linux-6.3 after your
PR (which includes ISA string refactoring) is merged. Is that okay with you?

With that said, it would request you to ACK this patch as well.

>
> >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 93e45560af30..3c5b51f519d5 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> >                       }
> >  #undef SET_ISA_EXT_MAP
> >               }

Thanks,
Anup

Conor Dooley Feb. 7, 2023, 6:05 p.m. UTC | #4

Hey Anup, Palmer,

On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> >
> > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > and Ssaia (S-mode AIA CSRs).
> >
> > This has pretty much the same problem that we had with the other
> > AIA-related ISA string patches, where there's that ambiguity with the
> > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > to try and document that we're going to interpret the standard ISA
> > strings that way, but now that we're doing custom ISA extensions it
> > seems saner to just define on here that removes the ambiguity.
> >
> > I just sent
> > <https://lore.kernel.org/r/[e-mail]/>
> > which documents that.
> 
> I am not sure why you say that these are custom extensions.
> 
> Multiple folks have clarified that both Smaia and Ssaia are frozen
> ISA extensions as-per RVI process. The individual chapters which
> are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> 
> Please refer:
> https://github.com/riscv/riscv-aia/pull/36
> https://lists.riscv.org/g/tech-aia/message/336
> https://lists.riscv.org/g/tech-aia/message/337

All of these links seem to discuss the draft chapters somehow being
incompatible with the non-draft ones. I would very expect that that,
as pointed out in several places there, that the draft chapters
finalisation would not lead to meaningful (and incompatible!) changes
being made to the non-draft chapters.

Maybe yourself and Palmer are looking at this from different
perspectives? Looking at his patch from Friday:
https://lore.kernel.org/linux-riscv/[e-mail]/
He specifically mentioned this aspect, as opposed to the aspect that
your links refer to.

Surely a duo-plic, if that ever comes to be, could be detected from
compatible strings in DT or w/e - but how do you intend differentiating
between an implementation of S*aia that contains the IOMMU support in
Chapter 9 in a finalised form, versus an implementation that may make
"different decisions" when it comes to that chapter of the spec?
I thought that would be handled by extension versions, but I am told
that those are not a thing any more.
If that's not true, and there'll be a version number that we can pull in
from a DT and parse which will distinguish between the two, then please
correct my misunderstanding here!

Thanks,
Conor.

> > > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> > >
> > > Signed-off-by: Anup Patel <[e-mail]>
> > > Reviewed-by: Andrew Jones <[e-mail]>
> > > ---
> > >  arch/riscv/include/asm/hwcap.h | 2 ++
> > >  arch/riscv/kernel/cpu.c        | 2 ++
> > >  arch/riscv/kernel/cpufeature.c | 2 ++
> > >  3 files changed, 6 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index 86328e3acb02..341ef30a3718 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> > >       RISCV_ISA_EXT_ZIHINTPAUSE,
> > >       RISCV_ISA_EXT_SSTC,
> > >       RISCV_ISA_EXT_SVINVAL,
> > > +     RISCV_ISA_EXT_SMAIA,
> > > +     RISCV_ISA_EXT_SSAIA,
> > >       RISCV_ISA_EXT_ID_MAX
> > >  };
> > >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > index 1b9a5a66e55a..a215ec929160 100644
> > > --- a/arch/riscv/kernel/cpu.c
> > > +++ b/arch/riscv/kernel/cpu.c
> > > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> > >   *    extensions by an underscore.
> > >   */
> > >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> >
> > This will conflict with that ISA string refactoring I just merged.  It
> > should be a pretty mechanical merge conflict, but if you want we can do
> > a shared tag with the first few patches and I can handle the merge
> > conflict locally.
> 
> I am planning to send this series as a second PR for Linux-6.3 after your
> PR (which includes ISA string refactoring) is merged. Is that okay with you?
> 
> With that said, it would request you to ACK this patch as well.
> 
> >
> > >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index 93e45560af30..3c5b51f519d5 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> > >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> > >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> > >                       }
> > >  #undef SET_ISA_EXT_MAP
> > >               }
> 
> Thanks,
> Anup

Conor Dooley Feb. 7, 2023, 6:09 p.m. UTC | #5

On Tue, Feb 07, 2023 at 06:05:11PM +0000, Conor Dooley wrote:
> Hey Anup, Palmer,
> 
> On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > >
> > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > and Ssaia (S-mode AIA CSRs).
> > >
> > > This has pretty much the same problem that we had with the other
> > > AIA-related ISA string patches, where there's that ambiguity with the
> > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > to try and document that we're going to interpret the standard ISA
> > > strings that way, but now that we're doing custom ISA extensions it
> > > seems saner to just define on here that removes the ambiguity.
> > >
> > > I just sent
> > > <https://lore.kernel.org/r/[e-mail]/>
> > > which documents that.
> > 
> > I am not sure why you say that these are custom extensions.
> > 
> > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > ISA extensions as-per RVI process. The individual chapters which
> > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> > 
> > Please refer:
> > https://github.com/riscv/riscv-aia/pull/36
> > https://lists.riscv.org/g/tech-aia/message/336
> > https://lists.riscv.org/g/tech-aia/message/337
> 
> All of these links seem to discuss the draft chapters somehow being
> incompatible with the non-draft ones. I would very expect that that,
> as pointed out in several places there, that the draft chapters
> finalisation would not lead to meaningful (and incompatible!) changes
> being made to the non-draft chapters.
> 
> Maybe yourself and Palmer are looking at this from different
> perspectives? Looking at his patch from Friday:
> https://lore.kernel.org/linux-riscv/[e-mail]/
> He specifically mentioned this aspect, as opposed to the aspect that
> your links refer to.

Meh, bad re-ordering of my paragraphs. By "this aspect" I meant what is
in the below paragraph! Apologies for any confusion there :)

> Surely a duo-plic, if that ever comes to be, could be detected from
> compatible strings in DT or w/e - but how do you intend differentiating
> between an implementation of S*aia that contains the IOMMU support in
> Chapter 9 in a finalised form, versus an implementation that may make
> "different decisions" when it comes to that chapter of the spec?
> I thought that would be handled by extension versions, but I am told
> that those are not a thing any more.
> If that's not true, and there'll be a version number that we can pull in
> from a DT and parse which will distinguish between the two, then please
> correct my misunderstanding here!
> 
> Thanks,
> Conor.
> 
> > > > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> > > >
> > > > Signed-off-by: Anup Patel <[e-mail]>
> > > > Reviewed-by: Andrew Jones <[e-mail]>
> > > > ---
> > > >  arch/riscv/include/asm/hwcap.h | 2 ++
> > > >  arch/riscv/kernel/cpu.c        | 2 ++
> > > >  arch/riscv/kernel/cpufeature.c | 2 ++
> > > >  3 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > > index 86328e3acb02..341ef30a3718 100644
> > > > --- a/arch/riscv/include/asm/hwcap.h
> > > > +++ b/arch/riscv/include/asm/hwcap.h
> > > > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> > > >       RISCV_ISA_EXT_ZIHINTPAUSE,
> > > >       RISCV_ISA_EXT_SSTC,
> > > >       RISCV_ISA_EXT_SVINVAL,
> > > > +     RISCV_ISA_EXT_SMAIA,
> > > > +     RISCV_ISA_EXT_SSAIA,
> > > >       RISCV_ISA_EXT_ID_MAX
> > > >  };
> > > >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > > index 1b9a5a66e55a..a215ec929160 100644
> > > > --- a/arch/riscv/kernel/cpu.c
> > > > +++ b/arch/riscv/kernel/cpu.c
> > > > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> > > >   *    extensions by an underscore.
> > > >   */
> > > >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > > > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > >
> > > This will conflict with that ISA string refactoring I just merged.  It
> > > should be a pretty mechanical merge conflict, but if you want we can do
> > > a shared tag with the first few patches and I can handle the merge
> > > conflict locally.
> > 
> > I am planning to send this series as a second PR for Linux-6.3 after your
> > PR (which includes ISA string refactoring) is merged. Is that okay with you?
> > 
> > With that said, it would request you to ACK this patch as well.
> > 
> > >
> > > >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > > >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > index 93e45560af30..3c5b51f519d5 100644
> > > > --- a/arch/riscv/kernel/cpufeature.c
> > > > +++ b/arch/riscv/kernel/cpufeature.c
> > > > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> > > >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > > >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> > > >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > > > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > > > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> > > >                       }
> > > >  #undef SET_ISA_EXT_MAP
> > > >               }
> > 
> > Thanks,
> > Anup

Atish Patra Feb. 7, 2023, 6:15 p.m. UTC | #6

On Tue, Feb 7, 2023 at 10:05 AM Conor Dooley <[e-mail]> wrote:
>
> Hey Anup, Palmer,
>
> On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > >
> > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > and Ssaia (S-mode AIA CSRs).
> > >
> > > This has pretty much the same problem that we had with the other
> > > AIA-related ISA string patches, where there's that ambiguity with the
> > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > to try and document that we're going to interpret the standard ISA
> > > strings that way, but now that we're doing custom ISA extensions it
> > > seems saner to just define on here that removes the ambiguity.
> > >
> > > I just sent
> > > <https://lore.kernel.org/r/[e-mail]/>
> > > which documents that.
> >
> > I am not sure why you say that these are custom extensions.
> >
> > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > ISA extensions as-per RVI process. The individual chapters which
> > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> >
> > Please refer:
> > https://github.com/riscv/riscv-aia/pull/36
> > https://lists.riscv.org/g/tech-aia/message/336
> > https://lists.riscv.org/g/tech-aia/message/337
>
> All of these links seem to discuss the draft chapters somehow being
> incompatible with the non-draft ones. I would very expect that that,
> as pointed out in several places there, that the draft chapters
> finalisation would not lead to meaningful (and incompatible!) changes
> being made to the non-draft chapters.
>

Here is the status of all RVI specs. It states that the Smaia, Ssaia
extensions are frozen (i.e. public review complete).
https://wiki.riscv.org/display/HOME/Specification+Status

I have added stephano/Jeff to confirm the same.

AFAIK, IOMMU spec is close to the public review phase and should be
frozen in this or next quarter.
IIRC, this chapter in AIA will be frozen along with IOMMU spec.

Anup: Please correct me if that's not correct.

> Maybe yourself and Palmer are looking at this from different
> perspectives? Looking at his patch from Friday:
> https://lore.kernel.org/linux-riscv/[e-mail]/
> He specifically mentioned this aspect, as opposed to the aspect that
> your links refer to.
>
> Surely a duo-plic, if that ever comes to be, could be detected from
> compatible strings in DT or w/e - but how do you intend differentiating
> between an implementation of S*aia that contains the IOMMU support in
> Chapter 9 in a finalised form, versus an implementation that may make
> "different decisions" when it comes to that chapter of the spec?

We will most likely have an extension specific to iommu spec as well.

> I thought that would be handled by extension versions, but I am told
> that those are not a thing any more.
> If that's not true, and there'll be a version number that we can pull in
> from a DT and parse which will distinguish between the two, then please
> correct my misunderstanding here!
>
> Thanks,
> Conor.
>
> > > > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> > > >
> > > > Signed-off-by: Anup Patel <[e-mail]>
> > > > Reviewed-by: Andrew Jones <[e-mail]>
> > > > ---
> > > >  arch/riscv/include/asm/hwcap.h | 2 ++
> > > >  arch/riscv/kernel/cpu.c        | 2 ++
> > > >  arch/riscv/kernel/cpufeature.c | 2 ++
> > > >  3 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > > index 86328e3acb02..341ef30a3718 100644
> > > > --- a/arch/riscv/include/asm/hwcap.h
> > > > +++ b/arch/riscv/include/asm/hwcap.h
> > > > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> > > >       RISCV_ISA_EXT_ZIHINTPAUSE,
> > > >       RISCV_ISA_EXT_SSTC,
> > > >       RISCV_ISA_EXT_SVINVAL,
> > > > +     RISCV_ISA_EXT_SMAIA,
> > > > +     RISCV_ISA_EXT_SSAIA,
> > > >       RISCV_ISA_EXT_ID_MAX
> > > >  };
> > > >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > > index 1b9a5a66e55a..a215ec929160 100644
> > > > --- a/arch/riscv/kernel/cpu.c
> > > > +++ b/arch/riscv/kernel/cpu.c
> > > > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> > > >   *    extensions by an underscore.
> > > >   */
> > > >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > > > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > >
> > > This will conflict with that ISA string refactoring I just merged.  It
> > > should be a pretty mechanical merge conflict, but if you want we can do
> > > a shared tag with the first few patches and I can handle the merge
> > > conflict locally.
> >
> > I am planning to send this series as a second PR for Linux-6.3 after your
> > PR (which includes ISA string refactoring) is merged. Is that okay with you?
> >
> > With that said, it would request you to ACK this patch as well.
> >
> > >
> > > >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > > >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > index 93e45560af30..3c5b51f519d5 100644
> > > > --- a/arch/riscv/kernel/cpufeature.c
> > > > +++ b/arch/riscv/kernel/cpufeature.c
> > > > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> > > >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > > >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> > > >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > > > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > > > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> > > >                       }
> > > >  #undef SET_ISA_EXT_MAP
> > > >               }
> >
> > Thanks,
> > Anup

Conor Dooley Feb. 7, 2023, 8:39 p.m. UTC | #7

On Tue, Feb 07, 2023 at 10:15:22AM -0800, Atish Patra wrote:
> On Tue, Feb 7, 2023 at 10:05 AM Conor Dooley <[e-mail]> wrote:
> > On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > > >
> > > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > > and Ssaia (S-mode AIA CSRs).
> > > >
> > > > This has pretty much the same problem that we had with the other
> > > > AIA-related ISA string patches, where there's that ambiguity with the
> > > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > > to try and document that we're going to interpret the standard ISA
> > > > strings that way, but now that we're doing custom ISA extensions it
> > > > seems saner to just define on here that removes the ambiguity.
> > > >
> > > > I just sent
> > > > <https://lore.kernel.org/r/[e-mail]/>
> > > > which documents that.
> > >
> > > I am not sure why you say that these are custom extensions.
> > >
> > > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > > ISA extensions as-per RVI process. The individual chapters which
> > > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> > >
> > > Please refer:
> > > https://github.com/riscv/riscv-aia/pull/36
> > > https://lists.riscv.org/g/tech-aia/message/336
> > > https://lists.riscv.org/g/tech-aia/message/337
> >
> > All of these links seem to discuss the draft chapters somehow being
> > incompatible with the non-draft ones. I would very expect that that,
> > as pointed out in several places there, that the draft chapters
> > finalisation would not lead to meaningful (and incompatible!) changes
> > being made to the non-draft chapters.
> >
> 
> Here is the status of all RVI specs. It states that the Smaia, Ssaia
> extensions are frozen (i.e. public review complete).
> https://wiki.riscv.org/display/HOME/Specification+Status
> 
> I have added stephano/Jeff to confirm the same.
> 
> AFAIK, IOMMU spec is close to the public review phase and should be
> frozen in this or next quarter.
> IIRC, this chapter in AIA will be frozen along with IOMMU spec.
> 
> Anup: Please correct me if that's not correct.
> 
> > Maybe yourself and Palmer are looking at this from different
> > perspectives? Looking at his patch from Friday:
> > https://lore.kernel.org/linux-riscv/[e-mail]/
> > He specifically mentioned this aspect, as opposed to the aspect that
> > your links refer to.
> >
> > Surely a duo-plic, if that ever comes to be, could be detected from
> > compatible strings in DT or w/e - but how do you intend differentiating
> > between an implementation of S*aia that contains the IOMMU support in
> > Chapter 9 in a finalised form, versus an implementation that may make
> > "different decisions" when it comes to that chapter of the spec?
> 
> We will most likely have an extension specific to iommu spec as well.

Right, but unless I am misunderstanding you, that is an extension for the
IOMMU spec, not for Chapter 9 of the AIA spec?

I would say that it is likely that if you have AIA and IOMMU that you'd
want to be implementing Chapter 9, but that would not appear sufficient to
draw a conclusion from.

Maybe the RVI lads that you've added (or Anup for that matter!) can
clarify if there is a requirement that if you do AIA and IOMMU that you
must do Chapter 9.
If not, my prior question about a differentiation mechanism still applies
I think!

> > I thought that would be handled by extension versions, but I am told
> > that those are not a thing any more.
> > If that's not true, and there'll be a version number that we can pull in
> > from a DT and parse which will distinguish between the two, then please
> > correct my misunderstanding here!

Anup Patel Feb. 8, 2023, 3:06 a.m. UTC | #8

On Tue, Feb 7, 2023 at 11:45 PM Atish Patra <[e-mail]> wrote:
>
> On Tue, Feb 7, 2023 at 10:05 AM Conor Dooley <[e-mail]> wrote:
> >
> > Hey Anup, Palmer,
> >
> > On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > > >
> > > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > > and Ssaia (S-mode AIA CSRs).
> > > >
> > > > This has pretty much the same problem that we had with the other
> > > > AIA-related ISA string patches, where there's that ambiguity with the
> > > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > > to try and document that we're going to interpret the standard ISA
> > > > strings that way, but now that we're doing custom ISA extensions it
> > > > seems saner to just define on here that removes the ambiguity.
> > > >
> > > > I just sent
> > > > <https://lore.kernel.org/r/[e-mail]/>
> > > > which documents that.
> > >
> > > I am not sure why you say that these are custom extensions.
> > >
> > > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > > ISA extensions as-per RVI process. The individual chapters which
> > > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> > >
> > > Please refer:
> > > https://github.com/riscv/riscv-aia/pull/36
> > > https://lists.riscv.org/g/tech-aia/message/336
> > > https://lists.riscv.org/g/tech-aia/message/337
> >
> > All of these links seem to discuss the draft chapters somehow being
> > incompatible with the non-draft ones. I would very expect that that,
> > as pointed out in several places there, that the draft chapters
> > finalisation would not lead to meaningful (and incompatible!) changes
> > being made to the non-draft chapters.
> >
>
> Here is the status of all RVI specs. It states that the Smaia, Ssaia
> extensions are frozen (i.e. public review complete).
> https://wiki.riscv.org/display/HOME/Specification+Status
>
> I have added stephano/Jeff to confirm the same.
>
> AFAIK, IOMMU spec is close to the public review phase and should be
> frozen in this or next quarter.
> IIRC, this chapter in AIA will be frozen along with IOMMU spec.
>
> Anup: Please correct me if that's not correct.

Yes, that's correct.

>
> > Maybe yourself and Palmer are looking at this from different
> > perspectives? Looking at his patch from Friday:
> > https://lore.kernel.org/linux-riscv/[e-mail]/
> > He specifically mentioned this aspect, as opposed to the aspect that
> > your links refer to.
> >
> > Surely a duo-plic, if that ever comes to be, could be detected from
> > compatible strings in DT or w/e - but how do you intend differentiating
> > between an implementation of S*aia that contains the IOMMU support in
> > Chapter 9 in a finalised form, versus an implementation that may make
> > "different decisions" when it comes to that chapter of the spec?
>
> We will most likely have an extension specific to iommu spec as well.
>
> > I thought that would be handled by extension versions, but I am told
> > that those are not a thing any more.
> > If that's not true, and there'll be a version number that we can pull in
> > from a DT and parse which will distinguish between the two, then please
> > correct my misunderstanding here!
> >
> > Thanks,
> > Conor.
> >
> > > > > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> > > > >
> > > > > Signed-off-by: Anup Patel <[e-mail]>
> > > > > Reviewed-by: Andrew Jones <[e-mail]>
> > > > > ---
> > > > >  arch/riscv/include/asm/hwcap.h | 2 ++
> > > > >  arch/riscv/kernel/cpu.c        | 2 ++
> > > > >  arch/riscv/kernel/cpufeature.c | 2 ++
> > > > >  3 files changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > > > index 86328e3acb02..341ef30a3718 100644
> > > > > --- a/arch/riscv/include/asm/hwcap.h
> > > > > +++ b/arch/riscv/include/asm/hwcap.h
> > > > > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> > > > >       RISCV_ISA_EXT_ZIHINTPAUSE,
> > > > >       RISCV_ISA_EXT_SSTC,
> > > > >       RISCV_ISA_EXT_SVINVAL,
> > > > > +     RISCV_ISA_EXT_SMAIA,
> > > > > +     RISCV_ISA_EXT_SSAIA,
> > > > >       RISCV_ISA_EXT_ID_MAX
> > > > >  };
> > > > >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > > > index 1b9a5a66e55a..a215ec929160 100644
> > > > > --- a/arch/riscv/kernel/cpu.c
> > > > > +++ b/arch/riscv/kernel/cpu.c
> > > > > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> > > > >   *    extensions by an underscore.
> > > > >   */
> > > > >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > > > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > > > > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > > >
> > > > This will conflict with that ISA string refactoring I just merged.  It
> > > > should be a pretty mechanical merge conflict, but if you want we can do
> > > > a shared tag with the first few patches and I can handle the merge
> > > > conflict locally.
> > >
> > > I am planning to send this series as a second PR for Linux-6.3 after your
> > > PR (which includes ISA string refactoring) is merged. Is that okay with you?
> > >
> > > With that said, it would request you to ACK this patch as well.
> > >
> > > >
> > > > >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > > >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > > > >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > > index 93e45560af30..3c5b51f519d5 100644
> > > > > --- a/arch/riscv/kernel/cpufeature.c
> > > > > +++ b/arch/riscv/kernel/cpufeature.c
> > > > > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> > > > >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > > > >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> > > > >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > > > > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > > > > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> > > > >                       }
> > > > >  #undef SET_ISA_EXT_MAP
> > > > >               }
> > >
> > > Thanks,
> > > Anup
>
>
>
> --
> Regards,
> Atish

Regards,
Anup

Anup Patel Feb. 8, 2023, 3:54 a.m. UTC | #9

On Wed, Feb 8, 2023 at 2:09 AM Conor Dooley <[e-mail]> wrote:
>
> On Tue, Feb 07, 2023 at 10:15:22AM -0800, Atish Patra wrote:
> > On Tue, Feb 7, 2023 at 10:05 AM Conor Dooley <[e-mail]> wrote:
> > > On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > > > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > > > >
> > > > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > > > and Ssaia (S-mode AIA CSRs).
> > > > >
> > > > > This has pretty much the same problem that we had with the other
> > > > > AIA-related ISA string patches, where there's that ambiguity with the
> > > > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > > > to try and document that we're going to interpret the standard ISA
> > > > > strings that way, but now that we're doing custom ISA extensions it
> > > > > seems saner to just define on here that removes the ambiguity.
> > > > >
> > > > > I just sent
> > > > > <https://lore.kernel.org/r/[e-mail]/>
> > > > > which documents that.
> > > >
> > > > I am not sure why you say that these are custom extensions.
> > > >
> > > > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > > > ISA extensions as-per RVI process. The individual chapters which
> > > > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> > > >
> > > > Please refer:
> > > > https://github.com/riscv/riscv-aia/pull/36
> > > > https://lists.riscv.org/g/tech-aia/message/336
> > > > https://lists.riscv.org/g/tech-aia/message/337
> > >
> > > All of these links seem to discuss the draft chapters somehow being
> > > incompatible with the non-draft ones. I would very expect that that,
> > > as pointed out in several places there, that the draft chapters
> > > finalisation would not lead to meaningful (and incompatible!) changes
> > > being made to the non-draft chapters.
> > >
> >
> > Here is the status of all RVI specs. It states that the Smaia, Ssaia
> > extensions are frozen (i.e. public review complete).
> > https://wiki.riscv.org/display/HOME/Specification+Status
> >
> > I have added stephano/Jeff to confirm the same.
> >
> > AFAIK, IOMMU spec is close to the public review phase and should be
> > frozen in this or next quarter.
> > IIRC, this chapter in AIA will be frozen along with IOMMU spec.
> >
> > Anup: Please correct me if that's not correct.
> >
> > > Maybe yourself and Palmer are looking at this from different
> > > perspectives? Looking at his patch from Friday:
> > > https://lore.kernel.org/linux-riscv/[e-mail]/
> > > He specifically mentioned this aspect, as opposed to the aspect that
> > > your links refer to.
> > >
> > > Surely a duo-plic, if that ever comes to be, could be detected from
> > > compatible strings in DT or w/e - but how do you intend differentiating
> > > between an implementation of S*aia that contains the IOMMU support in
> > > Chapter 9 in a finalised form, versus an implementation that may make
> > > "different decisions" when it comes to that chapter of the spec?
> >
> > We will most likely have an extension specific to iommu spec as well.
>
> Right, but unless I am misunderstanding you, that is an extension for the
> IOMMU spec, not for Chapter 9 of the AIA spec?
>
> I would say that it is likely that if you have AIA and IOMMU that you'd
> want to be implementing Chapter 9, but that would not appear sufficient to
> draw a conclusion from.
>
> Maybe the RVI lads that you've added (or Anup for that matter!) can
> clarify if there is a requirement that if you do AIA and IOMMU that you
> must do Chapter 9.
> If not, my prior question about a differentiation mechanism still applies
> I think!

For the benefit of everyone, the AIA spec mainly defines three
modular components:
1) Extended Local Interrupt CSRs (Smaia and Ssaia extensions)
    (ISA extension covered by: Chapter 2, Chapter 6, and Chapter 7)
2) Incoming MSI Controller (IMSIC)
    (ISA and Non-ISA extension covered by: Chapter 3 and Chapter 8)
3) Advanced PLIC (APLIC)
    (Non-ISA extension covered by: Chapter 4)

Apart from above, we have Chapter 5 ("Duo-PLIC") and Chapter 9
("IOMMU Support for MSIs to Virtual Machines") which are in draft
state.

Currently, there are no RISC-V members who have expressed
interest in implementing Chapter 5 ("Duo-PLIC") so this chapter
will stay in draft state for a foreseeable future.

The Chapter 9 ("IOMMU Support for MSIs to Virtual Machines")
defines an optional feature of IOMMU which can be implemented
by a standard IOMMU (such as RISC-V IOMMU) or a vendor specific
IOMMU. A RISC-V platform can certainly support device pass-through
using IMSIC guest files and an IOMMU which does not implement
Chapter 9. Unfortunately, there is a limit on the maximum number
of per-HART IMSIC guest files which can further limit the number
of pass-through devices. The Chapter 9 allows RISC-V platforms
to support large number of pass-through devices by defining "MRIF
- memory resident interrupt files" for an IOMMU. Further, the MRIFs
defined by Chapter 9 are simply interrupt files located in main memory
and have nothing to do with AIA local interrupt CSRs (Smaia and Ssaia).

The presence of S*aia in ISA string only implies that AIA extended
local interrupt CSRs are implemented by the underlying RISC-V
implementation.

I confirm that it is certainly not mandatory for a RISC-V platform to
implement Chapter 9 of the AIA specification if the RISC-V platform
already implements AIA and IOMMU.

>
> > > I thought that would be handled by extension versions, but I am told
> > > that those are not a thing any more.
> > > If that's not true, and there'll be a version number that we can pull in
> > > from a DT and parse which will distinguish between the two, then please
> > > correct my misunderstanding here!

Regards,
Anup

Conor Dooley Feb. 8, 2023, 12:57 p.m. UTC | #10

Hey Anup!

On Wed, Feb 08, 2023 at 09:24:28AM +0530, Anup Patel wrote:
> On Wed, Feb 8, 2023 at 2:09 AM Conor Dooley <[e-mail]> wrote:
> > On Tue, Feb 07, 2023 at 10:15:22AM -0800, Atish Patra wrote:
> > > On Tue, Feb 7, 2023 at 10:05 AM Conor Dooley <[e-mail]> wrote:
> > > > On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > > > > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > > > > >
> > > > > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > > > > and Ssaia (S-mode AIA CSRs).
> > > > > >
> > > > > > This has pretty much the same problem that we had with the other
> > > > > > AIA-related ISA string patches, where there's that ambiguity with the
> > > > > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > > > > to try and document that we're going to interpret the standard ISA
> > > > > > strings that way, but now that we're doing custom ISA extensions it
> > > > > > seems saner to just define on here that removes the ambiguity.
> > > > > >
> > > > > > I just sent
> > > > > > <https://lore.kernel.org/r/[e-mail]/>
> > > > > > which documents that.
> > > > >
> > > > > I am not sure why you say that these are custom extensions.
> > > > >
> > > > > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > > > > ISA extensions as-per RVI process. The individual chapters which
> > > > > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> > > > >
> > > > > Please refer:
> > > > > https://github.com/riscv/riscv-aia/pull/36
> > > > > https://lists.riscv.org/g/tech-aia/message/336
> > > > > https://lists.riscv.org/g/tech-aia/message/337
> > > >
> > > > All of these links seem to discuss the draft chapters somehow being
> > > > incompatible with the non-draft ones. I would very expect that that,
> > > > as pointed out in several places there, that the draft chapters
> > > > finalisation would not lead to meaningful (and incompatible!) changes
> > > > being made to the non-draft chapters.
> > > >
> > >
> > > Here is the status of all RVI specs. It states that the Smaia, Ssaia
> > > extensions are frozen (i.e. public review complete).
> > > https://wiki.riscv.org/display/HOME/Specification+Status
> > >
> > > I have added stephano/Jeff to confirm the same.
> > >
> > > AFAIK, IOMMU spec is close to the public review phase and should be
> > > frozen in this or next quarter.
> > > IIRC, this chapter in AIA will be frozen along with IOMMU spec.
> > >
> > > Anup: Please correct me if that's not correct.
> > >
> > > > Maybe yourself and Palmer are looking at this from different
> > > > perspectives? Looking at his patch from Friday:
> > > > https://lore.kernel.org/linux-riscv/[e-mail]/
> > > > He specifically mentioned this aspect, as opposed to the aspect that
> > > > your links refer to.
> > > >
> > > > Surely a duo-plic, if that ever comes to be, could be detected from
> > > > compatible strings in DT or w/e - but how do you intend differentiating
> > > > between an implementation of S*aia that contains the IOMMU support in
> > > > Chapter 9 in a finalised form, versus an implementation that may make
> > > > "different decisions" when it comes to that chapter of the spec?
> > >
> > > We will most likely have an extension specific to iommu spec as well.
> >
> > Right, but unless I am misunderstanding you, that is an extension for the
> > IOMMU spec, not for Chapter 9 of the AIA spec?
> >
> > I would say that it is likely that if you have AIA and IOMMU that you'd
> > want to be implementing Chapter 9, but that would not appear sufficient to
> > draw a conclusion from.
> >
> > Maybe the RVI lads that you've added (or Anup for that matter!) can
> > clarify if there is a requirement that if you do AIA and IOMMU that you
> > must do Chapter 9.
> > If not, my prior question about a differentiation mechanism still applies
> > I think!
> 
> For the benefit of everyone, the AIA spec mainly defines three
> modular components:
> 1) Extended Local Interrupt CSRs (Smaia and Ssaia extensions)
>     (ISA extension covered by: Chapter 2, Chapter 6, and Chapter 7)
> 2) Incoming MSI Controller (IMSIC)
>     (ISA and Non-ISA extension covered by: Chapter 3 and Chapter 8)
> 3) Advanced PLIC (APLIC)
>     (Non-ISA extension covered by: Chapter 4)
> 
> Apart from above, we have Chapter 5 ("Duo-PLIC") and Chapter 9
> ("IOMMU Support for MSIs to Virtual Machines") which are in draft
> state.
> 
> Currently, there are no RISC-V members who have expressed
> interest in implementing Chapter 5 ("Duo-PLIC") so this chapter
> will stay in draft state for a foreseeable future.

Thanks for the clarifications :)

> The Chapter 9 ("IOMMU Support for MSIs to Virtual Machines")
> defines an optional feature of IOMMU which can be implemented
> by a standard IOMMU (such as RISC-V IOMMU) or a vendor specific
> IOMMU. A RISC-V platform can certainly support device pass-through
> using IMSIC guest files and an IOMMU which does not implement
> Chapter 9. Unfortunately, there is a limit on the maximum number
> of per-HART IMSIC guest files which can further limit the number
> of pass-through devices. The Chapter 9 allows RISC-V platforms
> to support large number of pass-through devices by defining "MRIF
> - memory resident interrupt files" for an IOMMU. Further, the MRIFs
> defined by Chapter 9 are simply interrupt files located in main memory
> and have nothing to do with AIA local interrupt CSRs (Smaia and Ssaia).
> 
> The presence of S*aia in ISA string only implies that AIA extended
> local interrupt CSRs are implemented by the underlying RISC-V
> implementation.

Would you mind linking to where this is documented & explaining in your
commit message why it is okay operate on the basis of s*aia in the ISA
string only mandates the presence of the CSRs and nothing more.

I think when I was reading it last night, I saw some commentary in this
vein in Section 1.6 of the rc2 spec. Although IIRC it noted changes in
interrupt behaviour there too, so I'm not sure if that section is what you
are referring to here.

Perhaps this is all just a good argument for providing more information
in commit messages ;)

> I confirm that it is certainly not mandatory for a RISC-V platform to
> implement Chapter 9 of the AIA specification if the RISC-V platform
> already implements AIA and IOMMU.

Cool, thanks.
By what mechanism, since you say that "s*aia" in the ISA string only
implies presence of the CSRs [sic], are we meant to discover the
presence of Chapter 9?
A property of the IOMMU node seems the most logical I suppose, or
perhaps inferred from the presence/config of other properties of said
node?

> > > > I thought that would be handled by extension versions, but I am told
> > > > that those are not a thing any more.
> > > > If that's not true, and there'll be a version number that we can pull in
> > > > from a DT and parse which will distinguish between the two, then please
> > > > correct my misunderstanding here!

Thanks again Anup,
Conor.

Anup Patel Feb. 8, 2023, 2:57 p.m. UTC | #11

On Wed, Feb 8, 2023 at 6:27 PM Conor Dooley <[e-mail]> wrote:
>
> Hey Anup!
>
> On Wed, Feb 08, 2023 at 09:24:28AM +0530, Anup Patel wrote:
> > On Wed, Feb 8, 2023 at 2:09 AM Conor Dooley <[e-mail]> wrote:
> > > On Tue, Feb 07, 2023 at 10:15:22AM -0800, Atish Patra wrote:
> > > > On Tue, Feb 7, 2023 at 10:05 AM Conor Dooley <[e-mail]> wrote:
> > > > > On Fri, Feb 03, 2023 at 05:31:01PM +0530, Anup Patel wrote:
> > > > > > On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > > > > > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > > > > > > and Ssaia (S-mode AIA CSRs).
> > > > > > >
> > > > > > > This has pretty much the same problem that we had with the other
> > > > > > > AIA-related ISA string patches, where there's that ambiguity with the
> > > > > > > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > > > > > > to try and document that we're going to interpret the standard ISA
> > > > > > > strings that way, but now that we're doing custom ISA extensions it
> > > > > > > seems saner to just define on here that removes the ambiguity.
> > > > > > >
> > > > > > > I just sent
> > > > > > > <https://lore.kernel.org/r/[e-mail]/>
> > > > > > > which documents that.
> > > > > >
> > > > > > I am not sure why you say that these are custom extensions.
> > > > > >
> > > > > > Multiple folks have clarified that both Smaia and Ssaia are frozen
> > > > > > ISA extensions as-per RVI process. The individual chapters which
> > > > > > are in the draft state have nothing to do with Smaia and Ssaia CSRs.
> > > > > >
> > > > > > Please refer:
> > > > > > https://github.com/riscv/riscv-aia/pull/36
> > > > > > https://lists.riscv.org/g/tech-aia/message/336
> > > > > > https://lists.riscv.org/g/tech-aia/message/337
> > > > >
> > > > > All of these links seem to discuss the draft chapters somehow being
> > > > > incompatible with the non-draft ones. I would very expect that that,
> > > > > as pointed out in several places there, that the draft chapters
> > > > > finalisation would not lead to meaningful (and incompatible!) changes
> > > > > being made to the non-draft chapters.
> > > > >
> > > >
> > > > Here is the status of all RVI specs. It states that the Smaia, Ssaia
> > > > extensions are frozen (i.e. public review complete).
> > > > https://wiki.riscv.org/display/HOME/Specification+Status
> > > >
> > > > I have added stephano/Jeff to confirm the same.
> > > >
> > > > AFAIK, IOMMU spec is close to the public review phase and should be
> > > > frozen in this or next quarter.
> > > > IIRC, this chapter in AIA will be frozen along with IOMMU spec.
> > > >
> > > > Anup: Please correct me if that's not correct.
> > > >
> > > > > Maybe yourself and Palmer are looking at this from different
> > > > > perspectives? Looking at his patch from Friday:
> > > > > https://lore.kernel.org/linux-riscv/[e-mail]/
> > > > > He specifically mentioned this aspect, as opposed to the aspect that
> > > > > your links refer to.
> > > > >
> > > > > Surely a duo-plic, if that ever comes to be, could be detected from
> > > > > compatible strings in DT or w/e - but how do you intend differentiating
> > > > > between an implementation of S*aia that contains the IOMMU support in
> > > > > Chapter 9 in a finalised form, versus an implementation that may make
> > > > > "different decisions" when it comes to that chapter of the spec?
> > > >
> > > > We will most likely have an extension specific to iommu spec as well.
> > >
> > > Right, but unless I am misunderstanding you, that is an extension for the
> > > IOMMU spec, not for Chapter 9 of the AIA spec?
> > >
> > > I would say that it is likely that if you have AIA and IOMMU that you'd
> > > want to be implementing Chapter 9, but that would not appear sufficient to
> > > draw a conclusion from.
> > >
> > > Maybe the RVI lads that you've added (or Anup for that matter!) can
> > > clarify if there is a requirement that if you do AIA and IOMMU that you
> > > must do Chapter 9.
> > > If not, my prior question about a differentiation mechanism still applies
> > > I think!
> >
> > For the benefit of everyone, the AIA spec mainly defines three
> > modular components:
> > 1) Extended Local Interrupt CSRs (Smaia and Ssaia extensions)
> >     (ISA extension covered by: Chapter 2, Chapter 6, and Chapter 7)
> > 2) Incoming MSI Controller (IMSIC)
> >     (ISA and Non-ISA extension covered by: Chapter 3 and Chapter 8)
> > 3) Advanced PLIC (APLIC)
> >     (Non-ISA extension covered by: Chapter 4)
> >
> > Apart from above, we have Chapter 5 ("Duo-PLIC") and Chapter 9
> > ("IOMMU Support for MSIs to Virtual Machines") which are in draft
> > state.
> >
> > Currently, there are no RISC-V members who have expressed
> > interest in implementing Chapter 5 ("Duo-PLIC") so this chapter
> > will stay in draft state for a foreseeable future.
>
> Thanks for the clarifications :)
>
> > The Chapter 9 ("IOMMU Support for MSIs to Virtual Machines")
> > defines an optional feature of IOMMU which can be implemented
> > by a standard IOMMU (such as RISC-V IOMMU) or a vendor specific
> > IOMMU. A RISC-V platform can certainly support device pass-through
> > using IMSIC guest files and an IOMMU which does not implement
> > Chapter 9. Unfortunately, there is a limit on the maximum number
> > of per-HART IMSIC guest files which can further limit the number
> > of pass-through devices. The Chapter 9 allows RISC-V platforms
> > to support large number of pass-through devices by defining "MRIF
> > - memory resident interrupt files" for an IOMMU. Further, the MRIFs
> > defined by Chapter 9 are simply interrupt files located in main memory
> > and have nothing to do with AIA local interrupt CSRs (Smaia and Ssaia).
> >
> > The presence of S*aia in ISA string only implies that AIA extended
> > local interrupt CSRs are implemented by the underlying RISC-V
> > implementation.
>
> Would you mind linking to where this is documented & explaining in your
> commit message why it is okay operate on the basis of s*aia in the ISA
> string only mandates the presence of the CSRs and nothing more.
>
> I think when I was reading it last night, I saw some commentary in this
> vein in Section 1.6 of the rc2 spec. Although IIRC it noted changes in
> interrupt behaviour there too, so I'm not sure if that section is what you
> are referring to here.
>
> Perhaps this is all just a good argument for providing more information
> in commit messages ;)

Sure, I am anyway going to send v3 after rebase so I will cite the
Section 1.6 of AIA spec in the commit description.

>
> > I confirm that it is certainly not mandatory for a RISC-V platform to
> > implement Chapter 9 of the AIA specification if the RISC-V platform
> > already implements AIA and IOMMU.
>
> Cool, thanks.
> By what mechanism, since you say that "s*aia" in the ISA string only
> implies presence of the CSRs [sic], are we meant to discover the
> presence of Chapter 9?
> A property of the IOMMU node seems the most logical I suppose, or
> perhaps inferred from the presence/config of other properties of said
> node?

The discovery of AIA Chapter 9 features is IOMMU specific.

The upcoming RISC-V IOMMU spec defines "capabilities" MMIO register
which has read-only bits MSI_MRIF and MSI_FLAT for discovering features
defined in AIA Chapter 9. Due to this dependency, the AIA Chapter 9 is
going to be ratified along with RISC-V IOMMU ratification.

>
> > > > > I thought that would be handled by extension versions, but I am told
> > > > > that those are not a thing any more.
> > > > > If that's not true, and there'll be a version number that we can pull in
> > > > > from a DT and parse which will distinguish between the two, then please
> > > > > correct my misunderstanding here!
>
> Thanks again Anup,
> Conor.
>

Regards,
Anup

Conor Dooley Feb. 9, 2023, 11:31 p.m. UTC | #12

Hey all,

Just circling back to this one, since the reply from Palmer was to
another thread with a much smaller CC list.

On Wed, Feb 08, 2023 at 08:27:23PM +0530, Anup Patel wrote:
> On Wed, Feb 8, 2023 at 6:27 PM Conor Dooley <[e-mail]> wrote:
> > On Wed, Feb 08, 2023 at 09:24:28AM +0530, Anup Patel wrote:

> > > The presence of S*aia in ISA string only implies that AIA extended
> > > local interrupt CSRs are implemented by the underlying RISC-V
> > > implementation.
> >
> > Would you mind linking to where this is documented & explaining in your
> > commit message why it is okay operate on the basis of s*aia in the ISA
> > string only mandates the presence of the CSRs and nothing more.
> >
> > I think when I was reading it last night, I saw some commentary in this
> > vein in Section 1.6 of the rc2 spec. Although IIRC it noted changes in
> > interrupt behaviour there too, so I'm not sure if that section is what you
> > are referring to here.
> >
> > Perhaps this is all just a good argument for providing more information
> > in commit messages ;)
> 
> Sure, I am anyway going to send v3 after rebase so I will cite the
> Section 1.6 of AIA spec in the commit description.

We had a nice conversation about this on during the weekly patchwork
sync call :)
The end result of that one was "inconclusive" and the outcome appears to
be that we will wait until the entire spec is frozen before doing
anything here.
Palmer left a comment in response to another thread to that effect:
https://lore.kernel.org/linux-riscv/mhng-474f7ecd-65b8-4cfa-8b75-e51b896cc58e@palmer-ri-x1c9/

Cheers,
Conor.

Christoph Müllner Feb. 15, 2023, 3:41 p.m. UTC | #13

On Fri, Feb 3, 2023 at 1:25 AM Palmer Dabbelt <[e-mail]> wrote:
>
> On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > and Ssaia (S-mode AIA CSRs).
>
> This has pretty much the same problem that we had with the other
> AIA-related ISA string patches, where there's that ambiguity with the
> non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> to try and document that we're going to interpret the standard ISA
> strings that way, but now that we're doing custom ISA extensions it
> seems saner to just define on here that removes the ambiguity.


To avoid the impression that I did not work on that, here is the v2
from November,
that attempts to document this:
  https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607326.html

My proposed text was:
+Note, that AIA support (@samp{Smaia} and @samp{Ssaia}) is based on an AIA
+specification, which is frozen but contains draft chapters ("Duo-PLIC" and
+"IOMMU Support").

Btw, I did not get any feedback on that patch.

I also tried to address this on spec level (the PR has been linked) and raised
that to tech-aia (the conversation has been linked).

Another thing that I want to highlight, since it was discussed a lot recently
(e.g. just a few minutes ago in tech-chairs).
There is a chance of a last-minute spec change of AIA:
  https://github.com/riscv/riscv-aia/pull/37

BR
Christoph



>
>
> I just sent
> <https://lore.kernel.org/r/[e-mail]/>
> which documents that.
>
> > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> >
> > Signed-off-by: Anup Patel <[e-mail]>
> > Reviewed-by: Andrew Jones <[e-mail]>
> > ---
> >  arch/riscv/include/asm/hwcap.h | 2 ++
> >  arch/riscv/kernel/cpu.c        | 2 ++
> >  arch/riscv/kernel/cpufeature.c | 2 ++
> >  3 files changed, 6 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 86328e3acb02..341ef30a3718 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> >       RISCV_ISA_EXT_ZIHINTPAUSE,
> >       RISCV_ISA_EXT_SSTC,
> >       RISCV_ISA_EXT_SVINVAL,
> > +     RISCV_ISA_EXT_SMAIA,
> > +     RISCV_ISA_EXT_SSAIA,
> >       RISCV_ISA_EXT_ID_MAX
> >  };
> >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 1b9a5a66e55a..a215ec929160 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> >   *    extensions by an underscore.
> >   */
> >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>
> This will conflict with that ISA string refactoring I just merged.  It
> should be a pretty mechanical merge conflict, but if you want we can do
> a shared tag with the first few patches and I can handle the merge
> conflict locally.
>
> >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 93e45560af30..3c5b51f519d5 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> >                       }
> >  #undef SET_ISA_EXT_MAP
> >               }
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Christoph Müllner Feb. 21, 2023, 7:12 a.m. UTC | #14

Hi all,

The RISC-V Architectural Review Committee has discussed the concerns
regarding the non-ratified chapters in the AIA specification.

Here is the relevant quote from the meeting minutes:
"""
Although the Advanced Interrupt Architecture (AIA) has already passed
Architecture Review (with a minor edit still pending), the committee
has some suggestions about its final steps to ratification, to avoid
the AIA document having a mixture of ratified and non-ratified content:

- The AIA document's remaining draft chapter on the Duo-PLIC, which is
  not currently on a path to ratification, can be removed to a separate
  document.

- Ratification of the full AIA (without Duo-PLIC) can be postponed to
  coincide with ratification of the IOMMU specification, given that
  the latter is now expected in a reasonable time, and the AIA's last
  chapter concerning IOMMUs is already scheduled to go through public
  review and be ratified only together with the IOMMU specification.
"""

The full meeting minutes can be found here:
  https://lists.riscv.org/g/tech-chairs/message/1381

BR
Christoph

On Wed, Feb 15, 2023 at 4:41 PM Christoph Müllner
<[e-mail]> wrote:
>
> On Fri, Feb 3, 2023 at 1:25 AM Palmer Dabbelt <[e-mail]> wrote:
> >
> > On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > > and Ssaia (S-mode AIA CSRs).
> >
> > This has pretty much the same problem that we had with the other
> > AIA-related ISA string patches, where there's that ambiguity with the
> > non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> > to try and document that we're going to interpret the standard ISA
> > strings that way, but now that we're doing custom ISA extensions it
> > seems saner to just define on here that removes the ambiguity.
>
>
> To avoid the impression that I did not work on that, here is the v2
> from November,
> that attempts to document this:
>   https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607326.html
>
> My proposed text was:
> +Note, that AIA support (@samp{Smaia} and @samp{Ssaia}) is based on an AIA
> +specification, which is frozen but contains draft chapters ("Duo-PLIC" and
> +"IOMMU Support").
>
> Btw, I did not get any feedback on that patch.
>
> I also tried to address this on spec level (the PR has been linked) and raised
> that to tech-aia (the conversation has been linked).
>
> Another thing that I want to highlight, since it was discussed a lot recently
> (e.g. just a few minutes ago in tech-chairs).
> There is a chance of a last-minute spec change of AIA:
>   https://github.com/riscv/riscv-aia/pull/37
>
> BR
> Christoph
>
>
>
> >
> >
> > I just sent
> > <https://lore.kernel.org/r/[e-mail]/>
> > which documents that.
> >
> > > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> > >
> > > Signed-off-by: Anup Patel <[e-mail]>
> > > Reviewed-by: Andrew Jones <[e-mail]>
> > > ---
> > >  arch/riscv/include/asm/hwcap.h | 2 ++
> > >  arch/riscv/kernel/cpu.c        | 2 ++
> > >  arch/riscv/kernel/cpufeature.c | 2 ++
> > >  3 files changed, 6 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index 86328e3acb02..341ef30a3718 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> > >       RISCV_ISA_EXT_ZIHINTPAUSE,
> > >       RISCV_ISA_EXT_SSTC,
> > >       RISCV_ISA_EXT_SVINVAL,
> > > +     RISCV_ISA_EXT_SMAIA,
> > > +     RISCV_ISA_EXT_SSAIA,
> > >       RISCV_ISA_EXT_ID_MAX
> > >  };
> > >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > index 1b9a5a66e55a..a215ec929160 100644
> > > --- a/arch/riscv/kernel/cpu.c
> > > +++ b/arch/riscv/kernel/cpu.c
> > > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> > >   *    extensions by an underscore.
> > >   */
> > >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> >
> > This will conflict with that ISA string refactoring I just merged.  It
> > should be a pretty mechanical merge conflict, but if you want we can do
> > a shared tag with the first few patches and I can handle the merge
> > conflict locally.
> >
> > >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index 93e45560af30..3c5b51f519d5 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> > >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> > >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> > >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> > >                       }
> > >  #undef SET_ISA_EXT_MAP
> > >               }
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley Feb. 21, 2023, 10:40 a.m. UTC | #15

Hey Christoph,

On Tue, Feb 21, 2023 at 08:12:58AM +0100, Christoph Müllner wrote:
> Hi all,
> 
> The RISC-V Architectural Review Committee has discussed the concerns
> regarding the non-ratified chapters in the AIA specification.

Thanks for the update!

> Here is the relevant quote from the meeting minutes:
> """
> Although the Advanced Interrupt Architecture (AIA) has already passed
> Architecture Review (with a minor edit still pending), the committee
> has some suggestions about its final steps to ratification, to avoid
> the AIA document having a mixture of ratified and non-ratified content:

> - The AIA document's remaining draft chapter on the Duo-PLIC, which is
>   not currently on a path to ratification, can be removed to a separate
>   document.

That sounds promising...

> - Ratification of the full AIA (without Duo-PLIC) can be postponed to
>   coincide with ratification of the IOMMU specification, given that
>   the latter is now expected in a reasonable time, and the AIA's last
>   chapter concerning IOMMUs is already scheduled to go through public
>   review and be ratified only together with the IOMMU specification.
> """

...and so does this. AIA stuff's acceptability only depending on the
IOMMU spec's freeze (and thus Chapter 9's) seems like a vast improvement
on the status quo to me!

> The full meeting minutes can be found here:
>   https://lists.riscv.org/g/tech-chairs/message/1381

This link is non functional unfortunately :/

Cheers,
Conor.

Jessica Clarke Feb. 21, 2023, 10:51 a.m. UTC | #16

On 21 Feb 2023, at 10:40, Conor Dooley <[e-mail]> wrote:
> 
> Hey Christoph,
> 
> On Tue, Feb 21, 2023 at 08:12:58AM +0100, Christoph Müllner wrote:
>> Hi all,
>> 
>> The RISC-V Architectural Review Committee has discussed the concerns
>> regarding the non-ratified chapters in the AIA specification.
> 
> Thanks for the update!
> 
>> Here is the relevant quote from the meeting minutes:
>> """
>> Although the Advanced Interrupt Architecture (AIA) has already passed
>> Architecture Review (with a minor edit still pending), the committee
>> has some suggestions about its final steps to ratification, to avoid
>> the AIA document having a mixture of ratified and non-ratified content:
> 
>> - The AIA document's remaining draft chapter on the Duo-PLIC, which is
>>  not currently on a path to ratification, can be removed to a separate
>>  document.
> 
> That sounds promising...
> 
>> - Ratification of the full AIA (without Duo-PLIC) can be postponed to
>>  coincide with ratification of the IOMMU specification, given that
>>  the latter is now expected in a reasonable time, and the AIA's last
>>  chapter concerning IOMMUs is already scheduled to go through public
>>  review and be ratified only together with the IOMMU specification.
>> """
> 
> ...and so does this. AIA stuff's acceptability only depending on the
> IOMMU spec's freeze (and thus Chapter 9's) seems like a vast improvement
> on the status quo to me!
> 
>> The full meeting minutes can be found here:
>>  https://lists.riscv.org/g/tech-chairs/message/1381
> 
> This link is non functional unfortunately :/

tech-chairs is private, for (co-)chairs only... not sure why it went
there rather than tech-privileged.

Jess

> Cheers,
> Conor.
> 
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley Feb. 21, 2023, 10:59 a.m. UTC | #17

On Tue, Feb 21, 2023 at 10:51:13AM +0000, Jessica Clarke wrote:
> On 21 Feb 2023, at 10:40, Conor Dooley <[e-mail]> wrote:
> > On Tue, Feb 21, 2023 at 08:12:58AM +0100, Christoph Müllner wrote:
> >> The full meeting minutes can be found here:
> >>  https://lists.riscv.org/g/tech-chairs/message/1381
> > 
> > This link is non functional unfortunately :/
> 
> tech-chairs is private, for (co-)chairs only... not sure why it went
> there rather than tech-privileged.

Yah, that's what I was getting at.. This is a conversation on a public
ML, so it'd be annoying enough for some readers if it was gated around
RVI membership, but gating on membership of the inner circle makes it
kinda useless!

Christoph Müllner Feb. 21, 2023, 11:03 a.m. UTC | #18

On Tue, Feb 21, 2023 at 12:00 PM Conor Dooley
<[e-mail]> wrote:
>
> On Tue, Feb 21, 2023 at 10:51:13AM +0000, Jessica Clarke wrote:
> > On 21 Feb 2023, at 10:40, Conor Dooley <[e-mail]> wrote:
> > > On Tue, Feb 21, 2023 at 08:12:58AM +0100, Christoph Müllner wrote:
> > >> The full meeting minutes can be found here:
> > >>  https://lists.riscv.org/g/tech-chairs/message/1381
> > >
> > > This link is non functional unfortunately :/
> >
> > tech-chairs is private, for (co-)chairs only... not sure why it went
> > there rather than tech-privileged.
>
> Yah, that's what I was getting at.. This is a conversation on a public
> ML, so it'd be annoying enough for some readers if it was gated around
> RVI membership, but gating on membership of the inner circle makes it
> kinda useless!

The mail was forwarded there as well (and to tech-unprivileged):
  https://lists.riscv.org/g/tech-privileged/message/1294
  https://lists.riscv.org/g/tech-unprivileged/message/430

BR
Christoph

Conor Dooley Feb. 21, 2023, 11:22 a.m. UTC | #19

On Tue, Feb 21, 2023 at 12:03:45PM +0100, Christoph Müllner wrote:
> On Tue, Feb 21, 2023 at 12:00 PM Conor Dooley
> <[e-mail]> wrote:
> >
> > On Tue, Feb 21, 2023 at 10:51:13AM +0000, Jessica Clarke wrote:
> > > On 21 Feb 2023, at 10:40, Conor Dooley <[e-mail]> wrote:
> > > > On Tue, Feb 21, 2023 at 08:12:58AM +0100, Christoph Müllner wrote:
> > > >> The full meeting minutes can be found here:
> > > >>  https://lists.riscv.org/g/tech-chairs/message/1381
> > > >
> > > > This link is non functional unfortunately :/
> > >
> > > tech-chairs is private, for (co-)chairs only... not sure why it went
> > > there rather than tech-privileged.
> >
> > Yah, that's what I was getting at.. This is a conversation on a public
> > ML, so it'd be annoying enough for some readers if it was gated around
> > RVI membership, but gating on membership of the inner circle makes it
> > kinda useless!
> 
> The mail was forwarded there as well (and to tech-unprivileged):
>   https://lists.riscv.org/g/tech-privileged/message/1294
>   https://lists.riscv.org/g/tech-unprivileged/message/430

Great, those I can actually read. Thanks for sharing with the unwashed!

Conor.

Anup Patel March 31, 2023, 12:53 p.m. UTC | #20

Hi Palmer

On Fri, Feb 3, 2023 at 5:54 AM Palmer Dabbelt <[e-mail]> wrote:
>
> On Fri, 27 Jan 2023 23:27:32 PST (-0800), [e-mail] wrote:
> > We have two extension names for AIA ISA support: Smaia (M-mode AIA CSRs)
> > and Ssaia (S-mode AIA CSRs).
>
> This has pretty much the same problem that we had with the other
> AIA-related ISA string patches, where there's that ambiguity with the
> non-ratified chapters.  IIRC when this came up in GCC the rough idea was
> to try and document that we're going to interpret the standard ISA
> strings that way, but now that we're doing custom ISA extensions it
> seems saner to just define on here that removes the ambiguity.
>
> I just sent
> <https://lore.kernel.org/r/[e-mail]/>
> which documents that.

The IOMMU and AIA chapter8 are frozen and in public review.
Refer, https://lists.riscv.org/g/tech-aia/message/346

This means the entire AIA specification is now frozen (i.e. no
chapters in draft state).

I will rebase this series and send-out v3. It would be great if you
can ACK the PATCH2 of this series.

Thanks,
Anup

>
> > We extend the ISA string parsing to detect Smaia and Ssaia extensions.
> >
> > Signed-off-by: Anup Patel <[e-mail]>
> > Reviewed-by: Andrew Jones <[e-mail]>
> > ---
> >  arch/riscv/include/asm/hwcap.h | 2 ++
> >  arch/riscv/kernel/cpu.c        | 2 ++
> >  arch/riscv/kernel/cpufeature.c | 2 ++
> >  3 files changed, 6 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 86328e3acb02..341ef30a3718 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -59,6 +59,8 @@ enum riscv_isa_ext_id {
> >       RISCV_ISA_EXT_ZIHINTPAUSE,
> >       RISCV_ISA_EXT_SSTC,
> >       RISCV_ISA_EXT_SVINVAL,
> > +     RISCV_ISA_EXT_SMAIA,
> > +     RISCV_ISA_EXT_SSAIA,
> >       RISCV_ISA_EXT_ID_MAX
> >  };
> >  static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 1b9a5a66e55a..a215ec929160 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -162,6 +162,8 @@ arch_initcall(riscv_cpuinfo_init);
> >   *    extensions by an underscore.
> >   */
> >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> > +     __RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > +     __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>
> This will conflict with that ISA string refactoring I just merged.  It
> should be a pretty mechanical merge conflict, but if you want we can do
> a shared tag with the first few patches and I can handle the merge
> conflict locally.
>
> >       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 93e45560af30..3c5b51f519d5 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -228,6 +228,8 @@ void __init riscv_fill_hwcap(void)
> >                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >                               SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> >                               SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> > +                             SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> > +                             SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> >                       }
> >  #undef SET_ISA_EXT_MAP
> >               }

Patch

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 86328e3acb02..341ef30a3718 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -59,6 +59,8 @@  enum riscv_isa_ext_id {
 	RISCV_ISA_EXT_ZIHINTPAUSE,
 	RISCV_ISA_EXT_SSTC,
 	RISCV_ISA_EXT_SVINVAL,
+	RISCV_ISA_EXT_SMAIA,
+	RISCV_ISA_EXT_SSAIA,
 	RISCV_ISA_EXT_ID_MAX
 };
 static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 1b9a5a66e55a..a215ec929160 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -162,6 +162,8 @@  arch_initcall(riscv_cpuinfo_init);
  *    extensions by an underscore.
  */
 static struct riscv_isa_ext_data isa_ext_arr[] = {
+	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
+	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 93e45560af30..3c5b51f519d5 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -228,6 +228,8 @@  void __init riscv_fill_hwcap(void)
 				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
 				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
 				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
+				SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
+				SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
 			}
 #undef SET_ISA_EXT_MAP
 		}