Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Apr 2023 12:52:27 -0700 (PDT)
Subject:     Re: [PATCH 0/4] Expose the isa-string via the AT_BASE_PLATFORM aux vector
In-Reply-To: <mhng-7497491e-b83c-42e4-8ef7-d18c847298e5@palmer-ri-x1c9>
CC:     heiko@sntech.de, linux-riscv@lists.infradead.org,
        Paul Walmsley <paul.walmsley@sifive.com>,
        kito.cheng@sifive.com, jrtc27@jrtc27.com,
        Conor Dooley <conor.dooley@microchip.com>,
        matthias.bgg@gmail.com, heinrich.schuchardt@canonical.com,
        greentime.hu@sifive.com, nick.knight@sifive.com,
        Richard Henderson <richard.henderson@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>, linux-kernel@vger.kernel.org,
        heiko.stuebner@vrull.eu
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     philipp.tomsich@vrull.eu, christoph.muellner@vrull.eu
Message-ID: <mhng-4019d84e-ad14-4563-b777-a8fdf4201d49@palmer-ri-x1c9>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 28 Apr 2023 12:28:09 PDT (-0700), Palmer Dabbelt wrote:
> On Fri, 28 Apr 2023 11:59:31 PDT (-0700), philipp.tomsich@vrull.eu wrote:
>> On Fri, 28 Apr 2023 at 20:48, Christoph Müllner
>> <christoph.muellner@vrull.eu> wrote:
>>>
>>> On Fri, Apr 28, 2023 at 4:57 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>> >
>>> > On Mon, 24 Apr 2023 12:49:07 PDT (-0700), heiko@sntech.de wrote:
>>> > > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
>>> > >
>>> > > The hwprobing infrastructure was merged recently [0] and contains a
>>> > > mechanism to probe both extensions but also microarchitecural features
>>> > > on a per-core level of detail.
>>> > >
>>> > > While discussing the solution internally we identified some possible issues,
>>> > > tried to understand the underlying issue and come up with a solution for it.
>>> > > All these deliberations overlapped with hwprobing being merged, but that
>>> > > shouldn't really be an issue, as both have their usability - see below.
>>> > >
>>> > > Also please see the "Things to consider" at the bottom!
>>> > >
>>> > >
>>> > > Possible issues:
>>> > > - very much limited to Linux
>>> > > - schedulers run processes on all cores by default, so will need
>>> > >   the common set of extensions in most cases
>>> > > - each new extensions requires an uapi change, requiring at least
>>> > >   two pieces of software to be changed
>>> > > - adding another extension requires a review process (only known
>>> > >   extensions can be exposed to user-space)
>>> > > - vendor extensions have special needs and therefore possibly
>>> > >   don’t fit well
>>> > >
>>> > >
>>> > > Limited to Linux:
>>> > > -----------------
>>> > >
>>> > > The syscall and its uapi is Linux-specific and other OSes probably
>>> > > will not defer to our review process and requirements just to get
>>> > > new bits in. Instead most likely they'll build their own systems,
>>> > > leading to fragmentation.
>>> > >
>>> > >
>>> > > Feature on all cores:
>>> > > ---------------------
>>> > >
>>> > > Arnd previously ([1]) commented in the discussion, that there
>>> > > should not be a need for optimization towards hardware with an
>>> > > asymmetric set of features. We believe so as well, especially
>>> > > when talking about an interface that helps processes to identify
>>> > > the optimized routines they can execute.
>>> > >
>>> > > Of course seeing it with this finality might not take into account
>>> > > the somewhat special nature of RISC-V, but nevertheless it describes
>>> > > the common case for programs very well.
>>> > >
>>> > > For starters the scheduler in its default behaviour, will try to use any
>>> > > available core, so the standard program behaviour will always need the
>>> > > intersection of available extensions over all cores.
>>> > >
>>> > >
>>> > > Limiting program execution to specific cores will likely always be a
>>> > > special use case and already requires Linux-specific syscalls to
>>> > > select the set of cores.
>>> > >
>>> > > So while it can come in handy to get per-core information down the road
>>> > > via the hwprobing interface, most programs will just want to know if
>>> > > they can use a extension on just any core.
>>> > >
>>> > >
>>> > > Review process:
>>> > > ---------------
>>> > >
>>> > > There are so many (multi-letter-)extensions already with even more in
>>> > > the pipeline. To expose all of them, each will require a review process
>>> > > and uapi change that will make defining all of them slow as the
>>> > > kernel needs patching after which userspace needs to sync in the new
>>> > > api header.
>>> >
>>> > The whole reason we're doing hwprobe with bitmaps is beacuse the ISA
>>> > strings are not a stable interface, and thus are not suitable for
>>> > building uABI around.
>>>
>>> The ISA string is the main description of the RISC-V ISA that a
>>> system/core/hart implements. It is suitable and stable enough for all toolchain
>>> components (-march string, ELF header, etc.).

Sorry to just reply to my own email here, but neither of those are ISA 
strings.  We started out with ISA strings in -march, but the rules 
changed enough times that they're no longer the case (which isn't 
documented in GCC, the LLVM folks do a better job there).  The ELF 
header uses a bitmap for ABI features.

>>> It is also used in the DTB that the kernel uses to identify available
>>> extensions.
>>> So it is reasonable to argue that it is good enough for all runtime components.
>>> Also, I don't see any evidence that users are affected by any stability issues
>>> of the ISA strings in the interfaces where it is used at the moment.
>>> Quite the opposite, users are expecting ISA string interfaces everywhere
>>> because of existing interfaces.
>>>
>>> Besides that, also the kernel stable ABI documentation allows changes:
>>>   "Userspace programs are free to use these
>>>   interfaces with no restrictions, and backward compatibility for
>>>   them will be guaranteed for at least 2 years." [1]
>>> I did not come across any issues in the RISC-V ISA string that would violate
>>> these requirements. Did you? Further, the vDSO is covered by the stable ABI
>>> requirements, but not the auxiliary vector. This does not imply that an ISA
>>> string interface in the aux vector does not have to be stable at all, but there
>>> is certainly enough room for any ISA extension errata that may pop up in the
>>> future. Other architectures can live with that risk as well.
>>
>> To provide a slightly different perspective, arriving at a similar conclusion...
>>
>> The ISA string is part of the psABI (see
>> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0)
>> to identify the target architecture through Tag_RISCV_arch.  As such,
>> it already provides an interface that the kernel will have to consume
>> (during process startup) and has to be reasonably stable. The ELF
>> auxiliary vector is closely related to and should generally follow the
>> lead of the psABI definitions (which already use this string), which
>> makes the ISA string a natural encoding for exposing this information
>> to userspace programs.
>
> There were so many breakages due to that tag we just turned it off.
>
>> Cheers,
>> Philipp.
>>
>>>
>>>
>>> BR
>>> Christoph
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/README
>>>
>>> >
>>> > >
>>> > >
>>> > > Vendor-extensions:
>>> > > ------------------
>>> > >
>>> > > Vendor extensions are special in their own right.
>>> > > Userspace probably will want to know about them, but we as the kernel
>>> > > don't want to care about them too much (except as errata), as they're
>>> > > not part of the official RISC-V ISA spec.
>>> > >
>>> > > Getting vendor extensions from the dt to userspace via hwprobe would
>>> > > require coordination efforts and as vendors have the tendency to invent
>>> > > things during their development process before trying to submit changes
>>> > > upstream this likely would result in conflicts with assigned ids down
>>> > > the road. Which in turn then may create compatibility-issues with
>>> > > userspace builds built on top of the mainline kernel or a pre-
>>> > > existing vendor kernel.
>>> > >
>>> > > The special case also is that vendor A could in theory implement an
>>> > > extension from vendor B. So this would require to actually assign
>>> > > separate hwprobe keys to vendors (key for xthead extensions, key for
>>> > > xventana extensions, etc). This in turn would require vendors to
>>> > > come to the mainline kernel to get assigned a key (which in reality
>>> > > probably won't happen), which would then make the kernel community
>>> > > sort of an id authority.
>>> > >
>>> > >
>>> > >
>>> > >
>>> > > To address these, the attached patch series adds a second interface
>>> > > for the common case and "just" exposes the isa-string via the
>>> > > AT_BASE_PLATFORM aux vector.
>>> > >
>>> > > In the total cost of program start, parsing the string does not create
>>> > > too much overhead. The extension list in the kernel already contains
>>> > > the extensions list limited to the ones available on all harts and
>>> > > the string form allows us to just pipe through additional stuff too, as
>>> > > can be seen in the example for T-Head extensions [2] .
>>> > >
>>> > > This of course does not handle the microarchitecture things that
>>> > > the hwprobe syscall provides but allows a more generalized view
>>> > > onto the available ISA extensions, so is not intended to replace
>>> > > hwprobe, but to supplement it.
>>> > >
>>> > > AT_BASE_PLATFORM itself is somewhat well established, with PPC already
>>> > > using it to also expose a platform-specific string that identifies
>>> > > the platform in finer grain so this aux-vector field could in theory
>>> > > be used by other OSes as well.
>>> > >
>>> > >
>>> > > A random riscv64-qemu could for example provide:
>>> > >     rv64imafdcvh_zicbom_zihintpause_zbb_sscofpmf_sstc_svpbmt
>>> > >
>>> > > where a d1-nezha provides:
>>> > >     rv64imafdc_xtheadba_xtheadbb_xtheadbs_xtheadcmo_xtheadcondmov_xtheadfmemidx_xtheadint_xtheadmac_xtheadmemidx_xtheadmempair_xtheadsync
>>> > >
>>> > >
>>> > > Things to still consider:
>>> > > -------------------------
>>> > >
>>> > > Right now both hwprobe and this approach will only pass through
>>> > > extensions the kernel actually knows about itself. This should not
>>> > > necessarily be needed (but could be an optional feature for e.g. virtualization).
>>> > >
>>> > > Most extensions don’t introduce new user-mode state that the kernel needs to manage (e.g. new registers). Extension that do introduce new user-mode state are usually disabled by default and have to be enabled by S mode or M mode (e.g. FS[1:0] for the floating-point extension). So there should not be a reason to filter any extensions that are unknown.
>>> > >
>>> > > So it might make more sense to just pass through a curated list (common
>>> > > set) created from the core's isa strings and remove state-handling
>>> > > extensions when they are not enabled in the kernel-side (sort of
>>> > > blacklisting extensions that need actual kernel support).
>>> > >
>>> > > However, this is a very related, but still independent discussion.
>>> > >
>>> > >
>>> > > [0] https://lore.kernel.org/lkml/168191462224.22791.2281450562691381145.git-patchwork-notify@kernel.org/
>>> > > [1] https://lore.kernel.org/all/605fb2fd-bda2-4922-92bf-e3e416d54398@app.fastmail.com/
>>> > > [2] These are the T-Head extensions available in _upstream_ toolchains
>>> > >
>>> > > Heiko Stuebner (4):
>>> > >   RISC-V: create ISA string separately - not as part of cpuinfo
>>> > >   RISC-V: don't parse dt isa string to get rv32/rv64
>>> > >   RISC-V: export the ISA string of the running machine in the aux vector
>>> > >   RISC-V: add support for vendor-extensions via AT_BASE_PLATFORM and
>>> > >     xthead
>>> > >
>>> > >  arch/riscv/errata/thead/errata.c     | 43 ++++++++++++
>>> > >  arch/riscv/include/asm/alternative.h |  4 ++
>>> > >  arch/riscv/include/asm/elf.h         | 10 +++
>>> > >  arch/riscv/kernel/alternative.c      | 21 ++++++
>>> > >  arch/riscv/kernel/cpu.c              | 98 +++++++++++++++++++++++++---
>>> > >  5 files changed, 168 insertions(+), 8 deletions(-)