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..
>
> Heiko
>
> > break;
> >
> > case RISCV_HWPROBE_KEY_IMA_EXT_0:
> > @@ -173,6 +175,13 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> >
> > break;
> >
> > + case RISCV_HWPROBE_KEY_V_EXT_0:
> > + pair->value = 0;
> > + if (has_vector())
> > + pair->value |= RISCV_HWPROBE_V;
> > +
> > + break;
> > +
> > case RISCV_HWPROBE_KEY_CPUPERF_0:
> > pair->value = hwprobe_misaligned(cpus);
> > break;
> >
>
>
>
>
Thanks,
Andy
