Message-ID: <CALs-HssqaOjvUOdBVn=oN+uzkkmjguys2UttTYgdcqJwJB0HnQ@mail.gmail.com>
Date:   Thu, 24 Aug 2023 15:06:53 -0700
From:   Evan Green <evan@...osinc.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     Conor Dooley <conor.dooley@...rochip.com>,
        Anup Patel <apatel@...tanamicro.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, Palmer Dabbelt <palmer@...osinc.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        linux-riscv@...ts.infradead.org,
        Heiko Stuebner <heiko.stuebner@...ll.eu>,
        Andrew Jones <ajones@...tanamicro.com>
Subject: Re: [PATCH v4] RISC-V: Show accurate per-hart isa in /proc/cpuinfo

On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <conor@...nel.org> wrote:
>
> On Thu, Aug 24, 2023 at 09:18:16AM -0700, Evan Green wrote:
> > On Thu, Aug 24, 2023 at 5:20 AM Conor Dooley <conor.dooley@...rochip.com> wrote:
> > >
> > > On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
> > > > In /proc/cpuinfo, most of the information we show for each processor is
> > > > specific to that hart: marchid, mvendorid, mimpid, processor, hart,
> > > > compatible, and the mmu size. But the ISA string gets filtered through a
> > > > lowest common denominator mask, so that if one CPU is missing an ISA
> > > > extension, no CPUs will show it.
> > > >
> > > > Now that we track the ISA extensions for each hart, let's report ISA
> > > > extension info accurately per-hart in /proc/cpuinfo. We cannot change
> > > > the "isa:" line, as usermode may be relying on that line to show only
> > > > the common set of extensions supported across all harts. Add a new "hart
> > > > isa" line instead, which reports the true set of extensions for that
> > > > hart. This matches what is returned in riscv_hwprobe() when querying a
> > > > given hart.
> > > >
> > > > Signed-off-by: Evan Green <evan@...osinc.com>
> > > > Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
> > > > Reviewed-by: Conor Dooley <conor.dooley@...rochip.com>
> > > >
> > > > ---
> > > >
> > > > Changes in v4:
> > > >  - Documentation: Made the underline match the text line (Conor)
> > > >  - Documentation: hanged "in question" to "being described" (Andrew)
> > > >
> > > > Changes in v3:
> > > >  - Add some documentation (Conor)
> > > >
> > > > Changes in v2:
> > > >  - Added new "hart isa" line rather than altering behavior of existing
> > > >    "isa" line (Conor, Palmer)
> > > >
> > > >
> > > > I based this series on top of Conor's riscv-extensions-strings branch
> > > > from July 3rd, since otherwise this change gets hopelessly entangled
> > > > with that series.
> > > >
> > > > ---
> > > >  Documentation/riscv/uabi.rst | 10 ++++++++++
> > > >  arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
> > > >  2 files changed, 28 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > > > index 8960fac42c40..afdda580e5a2 100644
> > > > --- a/Documentation/riscv/uabi.rst
> > > > +++ b/Documentation/riscv/uabi.rst
> > > > @@ -42,6 +42,16 @@ An example string following the order is::
> > > >
> > > >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > > >
> > > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > > +------------------------------------------
> > > > +
> > > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > > +kernel on the particular hart being described, even if those extensions may not
> > > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > >
> > > Thinking about this again, I don't think this is true. hwprobe uses
> > > has_fpu(), has_vector() etc that interact with Kconfig options but the
> > > percpu isa bitmap isn't affected by these.
> >
> > Ugh yeah it's kind of a mishmash isn't it. hwprobe_isa_ext0() uses the
> > lowest common denominator for FD, C, V, but per-hart info for
> > Zba,Zbb,Zbs. Given the interface, per-hart info seems like what we
> > should have done there, and the FD, C, and V were my bad. The good
> > news is we can define new bits that do the right thing, though maybe
> > we should wait until someone actually wants them. For this patch we
> > should just remove this sentence. We can also correct the
> > documentation in hwprobe to mention the shortcoming in FD,C,V.
>
> I'm not really sure it's all that much of a shortcoming for V or FD,
> since without the kernel support you shouldn't be using those extensions
> anyway. A hwprobe thing for that sounds like a footgun to me & I think
> the current behaviour is how it should be for these extensions.
> It not being per-cpu is arguably a bug I suppose? But I would contend

Yeah it was mostly the not being per-cpu I was pointing to in my previous email.

> that we are conveying support for the extension on a per-hart level,
> with it then also gated by the kernel supporting V or FD, which is on a
> system-wide basis.
> Any other extensions that require Kconfig-gated kernel support should
> also not report via hwprobe that the extension is supported when the
> Kconfig option is disabled. It just so happens that the set of
> extensions that hwprobe supports that are Kconfig-gated and those that
> require all-hart support are one and the same right now, so we can kinda
> just conflate the two & use has_vector() et al that handles both
> kconfig-gating and all-hart support. Until something comes along that needs
> anything different, I'd leave well enough alone for hwprobe...

Sounds good.

>
> > Palmer, do you want a spin of this patch or a followup on top to
> > remove the above sentence?
>
> It's not actually been applied yet, right?
>
> Do you want to have this new thing in cpuinfo tell the user "this hart
> has xyz extensions that are supported by a kernel, but maybe not this
> kernel" or to tell the user "this hart has xyz extensions that are
> supported by this kernel"? Your text above says "understood by the
> kernel", but I think that's a poor definition that needs to be improved
> to spell out exactly what you mean. IOW does "understood" mean the
> kernel will parse them into a structure, or does it mean "yes you can
> use this extension on this particular hart".

I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
kernel at least vaguely understands it, but may not have full support
for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
humans wanting to know if they have hardware support for a feature,
and 2) administrators collecting telemetry to manage fleets (ie do I
have any hardware deployed that supports X). Programmers looking to
see "is the kernel support for this feature ready right now" would
ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
like specific hwprobe bits for "am I fully ready to go" would be
easier to work with. Feel free to yell at me if this overall vision
seems flawed.

I tried to look to see if there was consensus among the other
architectures. Aarch64 seems to go with "supported and fully enabled",
as their cpu_has_feature() directly tests elf_hwcap, and elements in
arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
more along the lines of "hardware has it". They have two macros,
cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
can do it too", and they use cpu_has() for /proc/cpuinfo flags.

-Evan

Powered by blists - more mailing lists