On 18/10/2023 19:36, Conor Dooley wrote:

On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:

quoted

On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:

quoted

On Tue, Oct 17, 2023 at 6:15 AM Clément Léger [off-list ref] wrote:

quoted

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 <redacted>
---
 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

quoted

quoted

Perhaps some rewording to more explicitly
state the danger would be appropriate. Other than that:

Reviewed-by: Evan Green <redacted>