On Tue, 09 May 2023 10:32:03 PDT (-0700), Evan Green wrote:
On Tue, May 9, 2023 at 9:41 AM Andy Chiu <andy.chiu@xxxxxxxxxx> wrote:On Tue, May 9, 2023 at 7:05 PM Heiko Stübner <heiko@xxxxxxxxx> wrote: > > Am Dienstag, 9. Mai 2023, 12:30:12 CEST schrieb Andy Chiu: > > Probing kernel support for Vector extension is available now. > > > > Signed-off-by: Andy Chiu <andy.chiu@xxxxxxxxxx> > > --- > > Documentation/riscv/hwprobe.rst | 10 ++++++++++ > > arch/riscv/include/asm/hwprobe.h | 2 +- > > arch/riscv/include/uapi/asm/hwprobe.h | 3 +++ > > arch/riscv/kernel/sys_riscv.c | 9 +++++++++ > > 4 files changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst > > index 9f0dd62dcb5d..b8755e180fbf 100644 > > --- a/Documentation/riscv/hwprobe.rst > > +++ b/Documentation/riscv/hwprobe.rst > > @@ -53,6 +53,9 @@ The following keys are defined: > > programs (it may still be executed in userspace via a > > kernel-controlled mechanism such as the vDSO). > > > > + * :c:macro:`RISCV_HWPROBE_BASE_BEHAVIOR_V`: Support for Vector extension, as > > + defined by verion 1.0 of the RISC-V Vector extension. > > ^^ version [missing the S] > > > + > > * :c:macro:`RISCV_HWPROBE_KEY_IMA_EXT_0`: A bitmask containing the extensions > > that are compatible with the :c:macro:`RISCV_HWPROBE_BASE_BEHAVIOR_IMA`: > > base system behavior. > > @@ -64,6 +67,13 @@ The following keys are defined: > > * :c:macro:`RISCV_HWPROBE_IMA_C`: The C extension is supported, as defined > > by version 2.2 of the RISC-V ISA manual. > > > > +* :c:macro:`RISCV_HWPROBE_KEY_V_EXT_0`: A bitmask containing the extensions > > + that are compatible with the :c:macro:`RISCV_HWPROBE_BASE_BEHAVIOR_V`: base > > + system behavior. > > + > > + * :c:macro:`RISCV_HWPROBE_V`: The V extension is supported, as defined by > > + version 1.0 of the RISC-V Vector extension manual. > > + > > this seems to be doubling the RISCV_HWPROBE_BASE_BEHAVIOR_V state without > adding additional information? Both essentially tell the system that > V extension "defined by verion 1.0 of the RISC-V Vector extension" is supported. I was thinking that RISCV_HWPROBE_BASE_BEHAVIOR_V indicates the kernel has a probe for vector (just like RISCV_HWPROBE_BASE_BEHAVIOR_IMA) and RISCV_HWPROBE_KEY_V_EXT_0 is where the kernel reports what exactly the extension is. This maps to the condition matching of F,D, and C in IMA. If that is not the case then I think there is no need for this entry. > > I don't question that we'll probably need a key for deeper vector- > specifics but I guess I'd the commit message should definitly explain > why there is a duplication here. I suppose something like Zvfh should fall into the category of RISCV_HWPROBE_KEY_V_EXT_0. I will add this example into the commit message if you agree that is a good example. > > > > * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance > > information about the selected set of processors. > > > > diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h > > index 78936f4ff513..39df8604fea1 100644 > > --- a/arch/riscv/include/asm/hwprobe.h > > +++ b/arch/riscv/include/asm/hwprobe.h > > @@ -8,6 +8,6 @@ > > > > #include <uapi/asm/hwprobe.h> > > > > -#define RISCV_HWPROBE_MAX_KEY 5 > > +#define RISCV_HWPROBE_MAX_KEY 6 > > > > #endif > > diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h > > index 8d745a4ad8a2..93a7fd3fd341 100644 > > --- a/arch/riscv/include/uapi/asm/hwprobe.h > > +++ b/arch/riscv/include/uapi/asm/hwprobe.h > > @@ -22,6 +22,7 @@ struct riscv_hwprobe { > > #define RISCV_HWPROBE_KEY_MIMPID 2 > > #define RISCV_HWPROBE_KEY_BASE_BEHAVIOR 3 > > #define RISCV_HWPROBE_BASE_BEHAVIOR_IMA (1 << 0) > > +#define RISCV_HWPROBE_BASE_BEHAVIOR_V (1 << 1) > > #define RISCV_HWPROBE_KEY_IMA_EXT_0 4 > > #define RISCV_HWPROBE_IMA_FD (1 << 0) > > #define RISCV_HWPROBE_IMA_C (1 << 1) > > @@ -32,6 +33,8 @@ struct riscv_hwprobe { > > #define RISCV_HWPROBE_MISALIGNED_FAST (3 << 0) > > #define RISCV_HWPROBE_MISALIGNED_UNSUPPORTED (4 << 0) > > #define RISCV_HWPROBE_MISALIGNED_MASK (7 << 0) > > +#define RISCV_HWPROBE_KEY_V_EXT_0 6 > > +#define RISCV_HWPROBE_V (1 << 0) > > /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */ > > > > #endif > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > index 5db29683ebee..6280a7f778b3 100644 > > --- a/arch/riscv/kernel/sys_riscv.c > > +++ b/arch/riscv/kernel/sys_riscv.c > > @@ -10,6 +10,7 @@ > > #include <asm/cpufeature.h> > > #include <asm/hwprobe.h> > > #include <asm/sbi.h> > > +#include <asm/vector.h> > > #include <asm/switch_to.h> > > #include <asm/uaccess.h> > > #include <asm/unistd.h> > > @@ -161,6 +162,7 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair, > > */ > > case RISCV_HWPROBE_KEY_BASE_BEHAVIOR: > > pair->value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA; > > + pair->value |= RISCV_HWPROBE_BASE_BEHAVIOR_V; > > Doesn't this also need a > if (has_vector()) > If the RISCV_HWPROBE_KEY_BASE_BEHAVIOR part just tells whether hwprobe supports probing of a set of extensions then I think we should not add the if statement here, but maybe I misunderstood something..The intention was to show that the I, M, and A extensions are actually present on this machine, not that the other probe keys exist. Usermode is allowed to query any hwprobe key, they just get back the key set to -1 and value set to 0 on unknown keys. We "cheated" a bit for determining I, M, and A exist since they're already prerequisites of Linux, which is why there's no conditional there.
We should probably add a comment so it doesn't trip someone up again.
-Evan
