Re: [PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[Atish Kumar Patra <atishp@rivosinc.com>]

On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote:
> > This series implements a generic framework to parse multi-letter ISA
> > extensions. This series is based on Tsukasa's v3 isa extension improvement
> > series[1]. I have fixed few bugs and improved comments from that series
> > (PATCH1-3). I have not used PATCH 4 from that series as we are not using
> > ISA extension versioning as of now. We can add that later if required.
> >
> > PATCH 4 allows the probing of multi-letter extensions via a macro.
> > It continues to use the common isa extensions between all the harts.
> > Thus hetergenous hart systems will only see the common ISA extensions.
> >
> > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions
> > via /proc/cpuinfo.
> >
> > Here is the example output of /proc/cpuinfo:
> > (with debug patches in Qemu and Linux kernel)
> >
> > # cat /proc/cpuinfo
> > processor     : 0
> > hart          : 0
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
>
> I know it might seem a bit pedantic, but I really don't want to
> introduce a new format for encoding ISA extensions -- doubly so if this
> is the only way we're giving this info to userspace, as then we're just
> asking folks to turn this into a defacto ABI.  Every time we try to do
> something that's sort of like an ISA string but not exactly what's in
> the spec we end up getting burned, and while I don't see a specific way

I agree that this is an ABI change/improvement which is impossible to
modify later.
However, this is a Linux specific ABI. Do you think the RISC-V spec
will ever say anything about how /proc/cpuinfo is shown to the user ?

and we do have similar precedence in other arch

/proc/cpuinfo output in  x86:
flags : fpu vme .... arch_capabilities
vmx flags : vnmi preemption_timer ..tsc_scaling

/proc/cpuinfo output in  arm64:

Features  : fp asimd evtstrm aes pmull sha1 sha2 crc32


> that could happen here that's what's happened so many times before I
> just don't want to risk it.
>
> I've gone ahead and removed some of this information (isa-ext, and all
> the single-letter extensions we can't properly turn on yet) from
> /proc/cpuinfo, modifying the last patch to do so.  It's at
> palmer/riscv-isa, LMK if that's OK.
>

Few changes required on your tree:

riscv_isa_ext_data definition is no longer required

and this should be to show hypervisor extension:
+static const char *base_riscv_exts = "imafdch";

> I'm not opposed to doing something here, I just really don't want to
> rush into it as we've already got enough complexity around the various
> flavors of ISA strings.  I don't see a big pressing need to provide this
> information to userspace, but having the rest of this sorted out is
> great (and there's some dependencies on this) so I'd prefer to just
> stick to what we know is good.
>

"isa-ext" row is kind of helpful to inform the developer to verify
that a specific extension is available
without looking at extension specific dmesg logs. It proved to be
useful while developing sstc/sscofpmf.

Is it better if we just dump the entire ISA string as before so that
we know which extensions are available at least ?

> > mmu           : sv48
> >
> > processor     : 1
> > hart          : 1
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
> > mmu           : sv48
> >
> > processor     : 2
> > hart          : 2
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
> > mmu           : sv48
> >
> > processor     : 3
> > hart          : 3
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
> > mmu           : sv48
> >
> > Anybody adding support for any new multi-letter extensions should add an
> > entry to the riscv_isa_ext_id and the isa extension array.
> > E.g. The patch[2] adds the support for various ISA extensions.
> >
> > [1] https://lore.kernel.org/all/0f568515-a05e-8204-aae3-035975af3ee8@irq.a4lg.com/T/
> > [2] https://github.com/atishp04/linux/commit/e9e240c9a854dceb434ceb53bdbe82a657bee5f2
> >
> > Changes from v4->v5:
> > 1. Improved the /proc/cpuinfo to include only valid & enabled extensions
> > 2. Improved the multi-letter parsing by skipping the 'su' modes generated in
> >    Qemu as suggested by Tsukasa.
> >
> > Changes from v3->v4:
> > 1. Changed temporary variable for current hart isa to a bitmap
> > 2. Added reviewed-by tags.
> > 3. Improved comments
> >
> > Changes from v2->v3:
> > 1. Updated comments to mark clearly a fix required for Qemu only.
> > 2. Fixed a bug where the 1st multi-letter extension can be present without _
> > 3. Added Tested by tags.
> >
> > Changes from v1->v2:
> > 1. Instead of adding a separate DT property use the riscv,isa property.
> > 2. Based on Tsukasa's v3 isa extension improvement series.
> >
> > Atish Patra (3):
> > RISC-V: Implement multi-letter ISA extension probing framework
> > RISC-V: Do no continue isa string parsing without correct XLEN
> > RISC-V: Improve /proc/cpuinfo output for ISA extensions
> >
> > Tsukasa OI (3):
> > RISC-V: Correctly print supported extensions
> > RISC-V: Minimal parser for "riscv, isa" strings
> > RISC-V: Extract multi-letter extension names from "riscv, isa"
> >
> > arch/riscv/include/asm/hwcap.h |  25 +++++++
> > arch/riscv/kernel/cpu.c        |  51 ++++++++++++-
> > arch/riscv/kernel/cpufeature.c | 130 +++++++++++++++++++++++++++------
> > 3 files changed, 183 insertions(+), 23 deletions(-)