Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231017131456.2053396-1-cleger@rivosinc.com> <20231017131456.2053396-2-cleger@rivosinc.com>
 <CALs-HssL=wNwj9nRuZwpZhy1CB9p9-X=OqgwBw9zvgA7hA4fEg@mail.gmail.com>
 <20231018-scrap-bankable-a0f321d97a46@spud> <20231018-flagpole-footpad-07a6228485f3@spud>
In-Reply-To: <20231018-flagpole-footpad-07a6228485f3@spud>
From:   Evan Green <evan@rivosinc.com>
Date:   Wed, 18 Oct 2023 10:45:06 -0700
Message-ID: <CALs-HsteDO0PvAKKQje7wU0f4z8w2V3f7WiHh5+LvQeVaSua1w@mail.gmail.com>
Subject: Re: [PATCH v2 01/19] riscv: hwprobe: factorize hwprobe ISA extension reporting
To:     Conor Dooley <conor@kernel.org>
Cc:     =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>,
        linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        Palmer Dabbelt <palmer@rivosinc.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Jonathan Corbet <corbet@lwn.net>,
        Andrew Jones <ajones@ventanamicro.com>,
        Samuel Ortiz <sameo@rivosinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 18, 2023 at 10:37=E2=80=AFAM Conor Dooley <conor@kernel.org> wr=
ote:
>
> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
> > On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
> > > On Tue, Oct 17, 2023 at 6:15=E2=80=AFAM Cl=C3=A9ment L=C3=A9ger <cleg=
er@rivosinc.com> wrote:
> > > >
> > > > 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=C3=A9ment L=C3=A9ger <cleger@rivosinc.com>
> > > > ---
> > > >  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_hwp=
robe *pair,
> > > >         for_each_cpu(cpu, cpus) {
> > > >                 struct riscv_isainfo *isainfo =3D &hart_isa[cpu];
> > > >
> > > > -               if (riscv_isa_extension_available(isainfo->isa, ZBA=
))
> > > > -                       pair->value |=3D RISCV_HWPROBE_EXT_ZBA;
> > > > -               else
> > > > -                       missing |=3D RISCV_HWPROBE_EXT_ZBA;
> > > > -
> > > > -               if (riscv_isa_extension_available(isainfo->isa, ZBB=
))
> > > > -                       pair->value |=3D RISCV_HWPROBE_EXT_ZBB;
> > > > -               else
> > > > -                       missing |=3D RISCV_HWPROBE_EXT_ZBB;
> > > > -
> > > > -               if (riscv_isa_extension_available(isainfo->isa, ZBS=
))
> > > > -                       pair->value |=3D RISCV_HWPROBE_EXT_ZBS;
> > > > -               else
> > > > -                       missing |=3D RISCV_HWPROBE_EXT_ZBS;
> > > > +#define CHECK_ISA_EXT(__ext)                                      =
             \
> > > > +               do {                                               =
             \
> > > > +                       if (riscv_isa_extension_available(isainfo->=
isa, __ext)) \
> > > > +                               pair->value |=3D RISCV_HWPROBE_EXT_=
##__ext;       \
> > > > +                       else                                       =
             \
> > > > +                               missing |=3D RISCV_HWPROBE_EXT_##__=
ext;           \
> > > > +               } while (false)
> > > > +
> > > > +               /*
> > > > +                * Only use CHECK_ISA_EXT() for extensions which ca=
n be exposed
> > > > +                * to userspace, regardless of the kernel's configu=
ration, as no
> > > > +                * other checks, besides presence in the hart_isa b=
itmap, 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 o=
r
> > 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.


Ohh, got it. The word "can" is doing a lot of heavy lifting in that
comment. So maybe something like: "This macro performs little in the
way of extension-specific kernel readiness checks. It's assumed other
gating factors like required Kconfig settings have already been
confirmed to support exposing the given extension to usermode". ...
But, you know, make it sparkle.

-Evan