[PATCH -next v19 03/24] riscv: hwprobe: Add support for RISCV_HWPROBE_BASE_BEHAVIOR_V
Palmer Dabbelt
palmer at dabbelt.com
Thu May 11 15:36:56 PDT 2023
On Tue, 09 May 2023 11:29:28 PDT (-0700), Evan Green wrote:
> On Tue, May 9, 2023 at 10:59 AM Palmer Dabbelt <palmer at dabbelt.com> wrote:
>>
>> 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 at sifive.com> wrote:
>> >>
>> >> On Tue, May 9, 2023 at 7:05 PM Heiko Stübner <heiko at sntech.de> 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 at sifive.com>
>> >> > > ---
>> >> > > 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)
V isn't a new base, it's just an addon to IMA like FD and C are. So
this should just be another bit in the RISCV_HWPROBE_KEY_IMA_EXT_0
bitset. That'll also clear up the above about V being indicated twice.
>> >> > > #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.
>
> There is one there, it just got clipped in the context diff. It looks
> like this (after gmail mangles it):
>
> /*
> * The kernel already assumes that the base single-letter ISA
> * extensions are supported on all harts, and only supports the
> * IMA base, so just cheat a bit here and tell that to
> * userspace.
> */
> case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
> pair->value = RISCV_HWPROBE_BASE_BEHAVIOR_IMA;
> break;
OK, not sure how to make that much clearer.
More information about the linux-riscv
mailing list