diff mbox series

[v2,01/19] riscv: hwprobe: factorize hwprobe ISA extension reporting

Message ID 20231017131456.2053396-2-cleger@rivosinc.com
State New
Headers
Series riscv: report more ISA extensions through hwprobe |

Commit Message

Clément Léger Oct. 17, 2023, 1:14 p.m. UTC 
  Factorize ISA extension reporting by using a macro rather than
copy/pasting extension names. This will allow adding new extensions more
easily.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Comments

Evan Green Oct. 18, 2023, 5:24 p.m. UTC | #1 
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> Factorize ISA extension reporting by using a macro rather than
> copy/pasting extension names. This will allow adding new extensions more
> easily.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index 473159b5f303..e207874e686e 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>         for_each_cpu(cpu, cpus) {
>                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
>
> -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
> -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
> -               else
> -                       missing |= RISCV_HWPROBE_EXT_ZBA;
> -
> -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
> -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
> -               else
> -                       missing |= RISCV_HWPROBE_EXT_ZBB;
> -
> -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
> -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
> -               else
> -                       missing |= RISCV_HWPROBE_EXT_ZBS;
> +#define CHECK_ISA_EXT(__ext)                                                   \
> +               do {                                                            \
> +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
> +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
> +                       else                                                    \
> +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
> +               } while (false)
> +
> +               /*
> +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
> +                * to userspace, regardless of the kernel's configuration, as no
> +                * other checks, besides presence in the hart_isa bitmap, are
> +                * made.

This comment alludes to a dangerous trap, but I'm having trouble
understanding what it is. Perhaps some rewording to more explicitly
state the danger would be appropriate. Other than that:

Reviewed-by: Evan Green <evan@rivosinc.com>
Conor Dooley Oct. 18, 2023, 5:33 p.m. UTC | #2 
On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
> >
> > Factorize ISA extension reporting by using a macro rather than
> > copy/pasting extension names. This will allow adding new extensions more
> > easily.
> >
> > Signed-off-by: Clément Léger <cleger@rivosinc.com>
> > ---
> >  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
> >  1 file changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> > index 473159b5f303..e207874e686e 100644
> > --- a/arch/riscv/kernel/sys_riscv.c
> > +++ b/arch/riscv/kernel/sys_riscv.c
> > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> >         for_each_cpu(cpu, cpus) {
> >                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
> >
> > -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
> > -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
> > -               else
> > -                       missing |= RISCV_HWPROBE_EXT_ZBA;
> > -
> > -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
> > -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
> > -               else
> > -                       missing |= RISCV_HWPROBE_EXT_ZBB;
> > -
> > -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
> > -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
> > -               else
> > -                       missing |= RISCV_HWPROBE_EXT_ZBS;
> > +#define CHECK_ISA_EXT(__ext)                                                   \
> > +               do {                                                            \
> > +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
> > +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
> > +                       else                                                    \
> > +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
> > +               } while (false)
> > +
> > +               /*
> > +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
> > +                * to userspace, regardless of the kernel's configuration, as no
> > +                * other checks, besides presence in the hart_isa bitmap, are
> > +                * made.
> 
> This comment alludes to a dangerous trap, but I'm having trouble
> understanding what it is.

You cannot, for example, use this for communicating the presence of F or
D, since they require a config option to be set before their use is
safe.

> Perhaps some rewording to more explicitly
> state the danger would be appropriate. Other than that:
> 
> Reviewed-by: Evan Green <evan@rivosinc.com>
Conor Dooley Oct. 18, 2023, 5:36 p.m. UTC | #3 
On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
> > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
> > >
> > > Factorize ISA extension reporting by using a macro rather than
> > > copy/pasting extension names. This will allow adding new extensions more
> > > easily.
> > >
> > > Signed-off-by: Clément Léger <cleger@rivosinc.com>
> > > ---
> > >  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
> > >  1 file changed, 18 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> > > index 473159b5f303..e207874e686e 100644
> > > --- a/arch/riscv/kernel/sys_riscv.c
> > > +++ b/arch/riscv/kernel/sys_riscv.c
> > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > >         for_each_cpu(cpu, cpus) {
> > >                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
> > >
> > > -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
> > > -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
> > > -               else
> > > -                       missing |= RISCV_HWPROBE_EXT_ZBA;
> > > -
> > > -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
> > > -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
> > > -               else
> > > -                       missing |= RISCV_HWPROBE_EXT_ZBB;
> > > -
> > > -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
> > > -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
> > > -               else
> > > -                       missing |= RISCV_HWPROBE_EXT_ZBS;
> > > +#define CHECK_ISA_EXT(__ext)                                                   \
> > > +               do {                                                            \
> > > +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
> > > +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
> > > +                       else                                                    \
> > > +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
> > > +               } while (false)
> > > +
> > > +               /*
> > > +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
> > > +                * to userspace, regardless of the kernel's configuration, as no
> > > +                * other checks, besides presence in the hart_isa bitmap, are
> > > +                * made.
> > 
> > This comment alludes to a dangerous trap, but I'm having trouble
> > understanding what it is.
> 
> You cannot, for example, use this for communicating the presence of F or
> D, since they require a config option to be set before their use is
> safe.

Funnily enough, this comment is immediately contradicted by the vector
subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
in has_vector(). The code looks valid to me, since has_vector() contains
the Kconfig check, but does fly in the face of this comment.

> 
> > Perhaps some rewording to more explicitly
> > state the danger would be appropriate. Other than that:
> > 
> > Reviewed-by: Evan Green <evan@rivosinc.com>
Evan Green Oct. 18, 2023, 5:45 p.m. UTC | #4 
On Wed, Oct 18, 2023 at 10:37 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
> > On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
> > > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
> > > >
> > > > Factorize ISA extension reporting by using a macro rather than
> > > > copy/pasting extension names. This will allow adding new extensions more
> > > > easily.
> > > >
> > > > Signed-off-by: Clément Léger <cleger@rivosinc.com>
> > > > ---
> > > >  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
> > > >  1 file changed, 18 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> > > > index 473159b5f303..e207874e686e 100644
> > > > --- a/arch/riscv/kernel/sys_riscv.c
> > > > +++ b/arch/riscv/kernel/sys_riscv.c
> > > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > >         for_each_cpu(cpu, cpus) {
> > > >                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
> > > >
> > > > -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
> > > > -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
> > > > -               else
> > > > -                       missing |= RISCV_HWPROBE_EXT_ZBA;
> > > > -
> > > > -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
> > > > -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
> > > > -               else
> > > > -                       missing |= RISCV_HWPROBE_EXT_ZBB;
> > > > -
> > > > -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
> > > > -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
> > > > -               else
> > > > -                       missing |= RISCV_HWPROBE_EXT_ZBS;
> > > > +#define CHECK_ISA_EXT(__ext)                                                   \
> > > > +               do {                                                            \
> > > > +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
> > > > +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
> > > > +                       else                                                    \
> > > > +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
> > > > +               } while (false)
> > > > +
> > > > +               /*
> > > > +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
> > > > +                * to userspace, regardless of the kernel's configuration, as no
> > > > +                * other checks, besides presence in the hart_isa bitmap, are
> > > > +                * made.
> > >
> > > This comment alludes to a dangerous trap, but I'm having trouble
> > > understanding what it is.
> >
> > You cannot, for example, use this for communicating the presence of F or
> > D, since they require a config option to be set before their use is
> > safe.
>
> Funnily enough, this comment is immediately contradicted by the vector
> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
> in has_vector(). The code looks valid to me, since has_vector() contains
> the Kconfig check, but does fly in the face of this comment.


Ohh, got it. The word "can" is doing a lot of heavy lifting in that
comment. So maybe something like: "This macro performs little in the
way of extension-specific kernel readiness checks. It's assumed other
gating factors like required Kconfig settings have already been
confirmed to support exposing the given extension to usermode". ...
But, you know, make it sparkle.

-Evan
Clément Léger Oct. 19, 2023, 7:26 a.m. UTC | #5 
On 18/10/2023 19:36, Conor Dooley wrote:
> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
>>>>
>>>> Factorize ISA extension reporting by using a macro rather than
>>>> copy/pasting extension names. This will allow adding new extensions more
>>>> easily.
>>>>
>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>> ---
>>>>  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
>>>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
>>>> index 473159b5f303..e207874e686e 100644
>>>> --- a/arch/riscv/kernel/sys_riscv.c
>>>> +++ b/arch/riscv/kernel/sys_riscv.c
>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>>>>         for_each_cpu(cpu, cpus) {
>>>>                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
>>>>
>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
>>>> -               else
>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBA;
>>>> -
>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
>>>> -               else
>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBB;
>>>> -
>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
>>>> -               else
>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBS;
>>>> +#define CHECK_ISA_EXT(__ext)                                                   \
>>>> +               do {                                                            \
>>>> +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
>>>> +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
>>>> +                       else                                                    \
>>>> +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
>>>> +               } while (false)
>>>> +
>>>> +               /*
>>>> +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
>>>> +                * to userspace, regardless of the kernel's configuration, as no
>>>> +                * other checks, besides presence in the hart_isa bitmap, are
>>>> +                * made.
>>>
>>> This comment alludes to a dangerous trap, but I'm having trouble
>>> understanding what it is.
>>
>> You cannot, for example, use this for communicating the presence of F or
>> D, since they require a config option to be set before their use is
>> safe.
> 
> Funnily enough, this comment is immediately contradicted by the vector
> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
> in has_vector(). The code looks valid to me, since has_vector() contains
> the Kconfig check, but does fly in the face of this comment.

Yes, the KConfig checks are already done by the headers, adding #ifdef
would be redundant even if more coherent with the comment. BTW, wouldn't
it make more sense to get rid out of the unsupported extensions directly
at ISA string parsing ? ie, if kernel is compiled without V support,
then do not set the bits corresponding to these in the riscv_isa_ext[]
array ? But the initial intent was probably to be able to report the
full string through cpuinfo.

Clément

> 
>>
>>> Perhaps some rewording to more explicitly
>>> state the danger would be appropriate. Other than that:
>>>
>>> Reviewed-by: Evan Green <evan@rivosinc.com>
> 
>
Clément Léger Oct. 19, 2023, 9:46 a.m. UTC | #6 
On 18/10/2023 19:45, Evan Green wrote:
> On Wed, Oct 18, 2023 at 10:37 AM Conor Dooley <conor@kernel.org> wrote:
>>
>> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
>>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
>>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
>>>>>
>>>>> Factorize ISA extension reporting by using a macro rather than
>>>>> copy/pasting extension names. This will allow adding new extensions more
>>>>> easily.
>>>>>
>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>>> ---
>>>>>  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
>>>>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
>>>>> index 473159b5f303..e207874e686e 100644
>>>>> --- a/arch/riscv/kernel/sys_riscv.c
>>>>> +++ b/arch/riscv/kernel/sys_riscv.c
>>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>>>>>         for_each_cpu(cpu, cpus) {
>>>>>                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
>>>>>
>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
>>>>> -               else
>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBA;
>>>>> -
>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
>>>>> -               else
>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBB;
>>>>> -
>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
>>>>> -               else
>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBS;
>>>>> +#define CHECK_ISA_EXT(__ext)                                                   \
>>>>> +               do {                                                            \
>>>>> +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
>>>>> +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
>>>>> +                       else                                                    \
>>>>> +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
>>>>> +               } while (false)
>>>>> +
>>>>> +               /*
>>>>> +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
>>>>> +                * to userspace, regardless of the kernel's configuration, as no
>>>>> +                * other checks, besides presence in the hart_isa bitmap, are
>>>>> +                * made.
>>>>
>>>> This comment alludes to a dangerous trap, but I'm having trouble
>>>> understanding what it is.
>>>
>>> You cannot, for example, use this for communicating the presence of F or
>>> D, since they require a config option to be set before their use is
>>> safe.
>>
>> Funnily enough, this comment is immediately contradicted by the vector
>> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
>> in has_vector(). The code looks valid to me, since has_vector() contains
>> the Kconfig check, but does fly in the face of this comment.
> 
> 
> Ohh, got it. The word "can" is doing a lot of heavy lifting in that
> comment. So maybe something like: "This macro performs little in the
> way of extension-specific kernel readiness checks. It's assumed other
> gating factors like required Kconfig settings have already been
> confirmed to support exposing the given extension to usermode". ...
> But, you know, make it sparkle.

Hi Even,

Indeed the comment was a bit misleading, is this more clear ?

/*
 * Only use CHECK_ISA_EXT() for extensions which are usable by
 * userspace with respect to the kernel current configuration.
 * For instance, ISA extensions that uses float operations
 * should not be exposed when CONFIG_FPU is not set.
 */

Clément

> 
> -Evan
Conor Dooley Oct. 19, 2023, 10:22 a.m. UTC | #7 
On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote:
> 
> 
> On 18/10/2023 19:36, Conor Dooley wrote:
> > On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
> >> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
> >>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
> >>>>
> >>>> Factorize ISA extension reporting by using a macro rather than
> >>>> copy/pasting extension names. This will allow adding new extensions more
> >>>> easily.
> >>>>
> >>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >>>> ---
> >>>>  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
> >>>>  1 file changed, 18 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> >>>> index 473159b5f303..e207874e686e 100644
> >>>> --- a/arch/riscv/kernel/sys_riscv.c
> >>>> +++ b/arch/riscv/kernel/sys_riscv.c
> >>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> >>>>         for_each_cpu(cpu, cpus) {
> >>>>                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
> >>>>
> >>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
> >>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
> >>>> -               else
> >>>> -                       missing |= RISCV_HWPROBE_EXT_ZBA;
> >>>> -
> >>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
> >>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
> >>>> -               else
> >>>> -                       missing |= RISCV_HWPROBE_EXT_ZBB;
> >>>> -
> >>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
> >>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
> >>>> -               else
> >>>> -                       missing |= RISCV_HWPROBE_EXT_ZBS;
> >>>> +#define CHECK_ISA_EXT(__ext)                                                   \
> >>>> +               do {                                                            \
> >>>> +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
> >>>> +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
> >>>> +                       else                                                    \
> >>>> +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
> >>>> +               } while (false)
> >>>> +
> >>>> +               /*
> >>>> +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
> >>>> +                * to userspace, regardless of the kernel's configuration, as no
> >>>> +                * other checks, besides presence in the hart_isa bitmap, are
> >>>> +                * made.
> >>>
> >>> This comment alludes to a dangerous trap, but I'm having trouble
> >>> understanding what it is.
> >>
> >> You cannot, for example, use this for communicating the presence of F or
> >> D, since they require a config option to be set before their use is
> >> safe.
> > 
> > Funnily enough, this comment is immediately contradicted by the vector
> > subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
> > in has_vector(). The code looks valid to me, since has_vector() contains
> > the Kconfig check, but does fly in the face of this comment.

> Yes, the KConfig checks are already done by the headers, adding #ifdef
> would be redundant even if more coherent with the comment

I don't really understand what the first part of this means, or why using
avoidable ifdeffery here would be desirable.

> BTW, wouldn't
> it make more sense to get rid out of the unsupported extensions directly
> at ISA string parsing ? ie, if kernel is compiled without V support,
> then do not set the bits corresponding to these in the riscv_isa_ext[]
> array ? But the initial intent was probably to be able to report the
> full string through cpuinfo.

Yeah, hysterical raisins I guess, it's always been that way. I don't
think anyone originally thought about such configurations and that is
how the cpuinfo stuff behaves. I strongly dislike the
riscv_isa_extension_available() interface, but one of Drew's patches
does at least improve things a bit. Kinda waiting for some of the
patches in flight to settle down before deciding if I want to refactor
stuff to be less of a potential for shooting oneself in the foot.
Conor Dooley Oct. 19, 2023, 10:24 a.m. UTC | #8 
On Thu, Oct 19, 2023 at 11:46:51AM +0200, Clément Léger wrote:
> Indeed the comment was a bit misleading, is this more clear ?
> 
> /*
>  * Only use CHECK_ISA_EXT() for extensions which are usable by
>  * userspace with respect to the kernel current configuration.
>  * For instance, ISA extensions that uses float operations

s/that uses/that use/

>  * should not be exposed when CONFIG_FPU is not set.

s/is not set/is not enabled/

But yeah, definitely more clear, thanks.
Clément Léger Oct. 19, 2023, 12:29 p.m. UTC | #9 
On 19/10/2023 12:22, Conor Dooley wrote:
> On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote:
>>
>>
>> On 18/10/2023 19:36, Conor Dooley wrote:
>>> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
>>>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
>>>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
>>>>>>
>>>>>> Factorize ISA extension reporting by using a macro rather than
>>>>>> copy/pasting extension names. This will allow adding new extensions more
>>>>>> easily.
>>>>>>
>>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>>>> ---
>>>>>>  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
>>>>>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
>>>>>> index 473159b5f303..e207874e686e 100644
>>>>>> --- a/arch/riscv/kernel/sys_riscv.c
>>>>>> +++ b/arch/riscv/kernel/sys_riscv.c
>>>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>>>>>>         for_each_cpu(cpu, cpus) {
>>>>>>                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
>>>>>>
>>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
>>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
>>>>>> -               else
>>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBA;
>>>>>> -
>>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
>>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
>>>>>> -               else
>>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBB;
>>>>>> -
>>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
>>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
>>>>>> -               else
>>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBS;
>>>>>> +#define CHECK_ISA_EXT(__ext)                                                   \
>>>>>> +               do {                                                            \
>>>>>> +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
>>>>>> +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
>>>>>> +                       else                                                    \
>>>>>> +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
>>>>>> +               } while (false)
>>>>>> +
>>>>>> +               /*
>>>>>> +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
>>>>>> +                * to userspace, regardless of the kernel's configuration, as no
>>>>>> +                * other checks, besides presence in the hart_isa bitmap, are
>>>>>> +                * made.
>>>>>
>>>>> This comment alludes to a dangerous trap, but I'm having trouble
>>>>> understanding what it is.
>>>>
>>>> You cannot, for example, use this for communicating the presence of F or
>>>> D, since they require a config option to be set before their use is
>>>> safe.
>>>
>>> Funnily enough, this comment is immediately contradicted by the vector
>>> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
>>> in has_vector(). The code looks valid to me, since has_vector() contains
>>> the Kconfig check, but does fly in the face of this comment.
> 
>> Yes, the KConfig checks are already done by the headers, adding #ifdef
>> would be redundant even if more coherent with the comment
> 
> I don't really understand what the first part of this means, or why using
> avoidable ifdeffery here would be desirable.

Sorry, I was not clear enough. What I meant is that the has_fpu() and
has_vector() functions are already ifdef'd in headers based on the
KConfig options for their support (CONFIG_FPU/CONFIG_RISCV_ISA_V) So in
the end, using ifdef here in hwprobe_isa_ext0() would be redundant.

> 
>> BTW, wouldn't
>> it make more sense to get rid out of the unsupported extensions directly
>> at ISA string parsing ? ie, if kernel is compiled without V support,
>> then do not set the bits corresponding to these in the riscv_isa_ext[]
>> array ? But the initial intent was probably to be able to report the
>> full string through cpuinfo.
> 
> Yeah, hysterical raisins I guess, it's always been that way. I don't
> think anyone originally thought about such configurations and that is
> how the cpuinfo stuff behaves. I strongly dislike the
> riscv_isa_extension_available() interface, but one of Drew's patches
> does at least improve things a bit. Kinda waiting for some of the
> patches in flight to settle down before deciding if I want to refactor
> stuff to be less of a potential for shooting oneself in the foot.

Make sense.

Clément
Evan Green Oct. 19, 2023, 3:58 p.m. UTC | #10 
On Thu, Oct 19, 2023 at 3:24 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Oct 19, 2023 at 11:46:51AM +0200, Clément Léger wrote:
> > Indeed the comment was a bit misleading, is this more clear ?
> >
> > /*
> >  * Only use CHECK_ISA_EXT() for extensions which are usable by
> >  * userspace with respect to the kernel current configuration.
> >  * For instance, ISA extensions that uses float operations
>
> s/that uses/that use/
>
> >  * should not be exposed when CONFIG_FPU is not set.
>
> s/is not set/is not enabled/
>
> But yeah, definitely more clear, thanks.

Looks good to me too. Thanks, Clément!
-Evan
Andrew Jones Oct. 20, 2023, 10:27 a.m. UTC | #11 
On Thu, Oct 19, 2023 at 11:22:26AM +0100, Conor Dooley wrote:
> On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote:
...
> > BTW, wouldn't
> > it make more sense to get rid out of the unsupported extensions directly
> > at ISA string parsing ? ie, if kernel is compiled without V support,
> > then do not set the bits corresponding to these in the riscv_isa_ext[]
> > array ? But the initial intent was probably to be able to report the
> > full string through cpuinfo.
> 
> Yeah, hysterical raisins I guess, it's always been that way. I don't
> think anyone originally thought about such configurations and that is
> how the cpuinfo stuff behaves. I strongly dislike the
> riscv_isa_extension_available() interface, but one of Drew's patches
> does at least improve things a bit. Kinda waiting for some of the
> patches in flight to settle down before deciding if I want to refactor
> stuff to be less of a potential for shooting oneself in the foot.

And I recall promising to try and do something with it too, but that
promise got buried under other promises... It's still on the TODO, at
least!

drew
diff mbox series

Patch

diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 473159b5f303..e207874e686e 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -145,20 +145,24 @@  static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 	for_each_cpu(cpu, cpus) {
 		struct riscv_isainfo *isainfo = &hart_isa[cpu];
 
-		if (riscv_isa_extension_available(isainfo->isa, ZBA))
-			pair->value |= RISCV_HWPROBE_EXT_ZBA;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBA;
-
-		if (riscv_isa_extension_available(isainfo->isa, ZBB))
-			pair->value |= RISCV_HWPROBE_EXT_ZBB;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBB;
-
-		if (riscv_isa_extension_available(isainfo->isa, ZBS))
-			pair->value |= RISCV_HWPROBE_EXT_ZBS;
-		else
-			missing |= RISCV_HWPROBE_EXT_ZBS;
+#define CHECK_ISA_EXT(__ext)							\
+		do {								\
+			if (riscv_isa_extension_available(isainfo->isa, __ext))	\
+				pair->value |= RISCV_HWPROBE_EXT_##__ext;	\
+			else							\
+				missing |= RISCV_HWPROBE_EXT_##__ext;		\
+		} while (false)
+
+		/*
+		 * Only use CHECK_ISA_EXT() for extensions which can be exposed
+		 * to userspace, regardless of the kernel's configuration, as no
+		 * other checks, besides presence in the hart_isa bitmap, are
+		 * made.
+		 */
+		CHECK_ISA_EXT(ZBA);
+		CHECK_ISA_EXT(ZBB);
+		CHECK_ISA_EXT(ZBS);
+#undef CHECK_ISA_EXT
 	}
 
 	/* Now turn off reporting features if any CPU is missing it. */