Re: [PATCH -next v19 03/24] riscv: hwprobe: Add support for RISCV_HWPROBE_BASE_BEHAVIOR_V

From: Evan Green <evan@rivosinc.com>
To: Andy Chiu <andy.chiu@sifive.com>
Cc: "Heiko Stübner" <heiko@sntech.de>,
	linux-riscv@lists.infradead.org, palmer@dabbelt.com,
	anup@brainfault.org, atishp@atishpatra.org,
	kvm-riscv@lists.infradead.org, kvm@vger.kernel.org,
	vineetg@rivosinc.com, greentime.hu@sifive.com,
	guoren@linux.alibaba.com, "Jonathan Corbet" <corbet@lwn.net>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Andrew Jones" <ajones@ventanamicro.com>,
	"Celeste Liu" <coelacanthus@outlook.com>,
	"Andrew Bresticker" <abrestic@rivosinc.com>
Subject: Re: [PATCH -next v19 03/24] riscv: hwprobe: Add support for RISCV_HWPROBE_BASE_BEHAVIOR_V
Date: Tue, 9 May 2023 10:32:03 -0700	[thread overview]
Message-ID: <CALs-Hss8L478Pg7zdcVZkL-jGGqdXtQd+uy+JdNEey90+eBYiA@mail.gmail.com> (raw)
In-Reply-To: <CABgGipW0U6=4AhaM9zWPDKZYdKEYDi04Gm-uRmf_WoOioTaf6w@mail.gmail.com>

On Tue, May 9, 2023 at 9:41 AM Andy Chiu <andy.chiu@sifive.com> wrote:
>
> On Tue, May 9, 2023 at 7:05 PM Heiko Stübner <heiko@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@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)
> > >  #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.
-Evan