| Message ID | 20231017131456.2053396-2-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: report more ISA extensions through hwprobe | expand |
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>
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>
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>
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
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> > >
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
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.
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.
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
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
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 --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. */
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(-)