Message-Id: <212CC1BD-31FF-4B8B-B05D-89C5245EE8A7@jrtc27.com>
Date:   Mon, 6 Feb 2023 21:11:42 +0000
From:   Jessica Clarke <jrtc27@...c27.com>
To:     Evan Green <evan@...osinc.com>
Cc:     Palmer Dabbelt <palmer@...osinc.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Jisheng Zhang <jszhang@...nel.org>, linux-doc@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Andrew Bresticker <abrestic@...osinc.com>,
        Atish Patra <atishp@...osinc.com>,
        Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Celeste Liu <coelacanthus@...look.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Qinglin Pan <panqinglin2020@...as.ac.cn>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Shuah Khan <shuah@...nel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Jonathan Corbet <corbet@....net>,
        Xianting Tian <xianting.tian@...ux.alibaba.com>,
        Tsukasa OI <research_trasio@....a4lg.com>,
        Tobias Klauser <tklauser@...tanz.ch>,
        Andrew Jones <ajones@...tanamicro.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Albert Ou <aou@...s.berkeley.edu>,
        Arnd Bergmann <arnd@...db.de>,
        Vineet Gupta <vineetg@...osinc.com>,
        Mark Brown <broonie@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Ruizhe Pan <c141028@...il.com>,
        Anup Patel <apatel@...tanamicro.com>,
        linux-kselftest@...r.kernel.org, slewis@...osinc.com,
        Randy Dunlap <rdunlap@...radead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Conor Dooley <conor@...nel.org>, dram <dramforever@...e.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Heinrich Schuchardt <heinrich.schuchardt@...onical.com>,
        Guo Ren <guoren@...nel.org>, Dao Lu <daolu@...osinc.com>
Subject: Re: [PATCH v2 0/6] RISC-V Hardware Probing User Interface

On 6 Feb 2023, at 20:14, Evan Green <evan@...osinc.com> wrote:
> 
> 
> These are very much up for discussion, as it's a pretty big new user
> interface and it's quite a bit different from how we've historically
> done things: this isn't just providing an ISA string to userspace, this
> has its own format for providing information to userspace.
> 
> There's been a bunch of off-list discussions about this, including at
> Plumbers.  The original plan was to do something involving providing an
> ISA string to userspace, but ISA strings just aren't sufficient for a
> stable ABI any more: in order to parse an ISA string users need the
> version of the specifications that the string is written to, the version
> of each extension (sometimes at a finer granularity than the RISC-V
> releases/versions encode), and the expected use case for the ISA string
> (ie, is it a U-mode or M-mode string).  That's a lot of complexity to
> try and keep ABI compatible and it's probably going to continue to grow,
> as even if there's no more complexity in the specifications we'll have
> to deal with the various ISA string parsing oddities that end up all
> over userspace.
> 
> Instead this patch set takes a very different approach and provides a set
> of key/value pairs that encode various bits about the system.  The big
> advantage here is that we can clearly define what these mean so we can
> ensure ABI stability, but it also allows us to encode information that's
> unlikely to ever appear in an ISA string (see the misaligned access
> performance, for example).  The resulting interface looks a lot like
> what arm64 and x86 do, and will hopefully fit well into something like
> ACPI in the future.
> 
> The actual user interface is a syscall.  I'm not really sure that's the
> right way to go about this, but it makes for flexible prototying.
> Various other approaches have been talked about like making HWCAP2 a
> pointer, having a VDSO routine, or exposing this via sysfs.  Those seem
> like generally reasonable approaches, but I've yet to figure out a way
> to get the general case working without a syscall as that's the only way
> I've come up with to deal with the heterogenous CPU case.  Happy to hear
> if someone has a better idea, though, as I don't really want to add a
> syscall if we can avoid it.

Please work with https://github.com/riscv-non-isa/riscv-c-api-doc as
it’s crucial we have a portable standard interface for applications to
query this information that works on OSes other than Linux. This can be
backed by whatever you want, whether a syscall, magic VDSO thing,
sysfs, etc, but it’s key that the exposed interface outside of libc is
not Linux-specific otherwise we’re going to get fragmentation in this
space.

I would encourage figuring out the right shape for the exposed
interface first before continuing to refine details of how that
information gets communicated between the kernel and libc.

Jess

> An example series in glibc exposing this syscall and using it in an
> ifunc selector for memcpy can be found at [1].
> 
> [1] https://public-inbox.org/libc-alpha/20230206194819.1679472-1-evan@rivosinc.com/T/#t
> 
> Changes in v2:
> - Changed the interface to look more like poll(). Rather than supplying
>   key_offset and getting back an array of values with numerically
>   contiguous keys, have the user pre-fill the key members of the array,
>   and the kernel will fill in the corresponding values. For any key it
>   doesn't recognize, it will set the key of that element to -1. This
>   allows usermode to quickly ask for exactly the elements it cares
>   about, and not get bogged down in a back and forth about newer keys
>   that older kernels might not recognize. In other words, the kernel
>   can communicate that it doesn't recognize some of the keys while
>   still providing the data for the keys it does know.
> - Added a shortcut to the cpuset parameters that if a size of 0 and
>   NULL is provided for the CPU set, the kernel will use a cpu mask of
>   all online CPUs. This is convenient because I suspect most callers
>   will only want to act on a feature if it's supported on all CPUs, and
>   it's a headache to dynamically allocate an array of all 1s, not to
>   mention a waste to have the kernel loop over all of the offline bits.
> - Fixed logic error in if(of_property_read_string...) that caused crash
> - Include cpufeature.h in cpufeature.h to avoid undeclared variable
>   warning.
> - Added a _MASK define
> - Fix random checkpatch complaints
> - Updated the selftests to the new API and added some more.
> - Fixed indentation, comments in .S, and general checkpatch complaints.
> 
> Evan Green (4):
>  RISC-V: Move struct riscv_cpuinfo to new header
>  RISC-V: Add a syscall for HW probing
>  RISC-V: hwprobe: Support probing of misaligned access performance
>  selftests: Test the new RISC-V hwprobe interface
> 
> Palmer Dabbelt (2):
>  RISC-V: hwprobe: Add support for RISCV_HWPROBE_BASE_BEHAVIOR_IMA
>  dt-bindings: Add RISC-V misaligned access performance
> 
> .../devicetree/bindings/riscv/cpus.yaml       |  15 ++
> Documentation/riscv/hwprobe.rst               |  66 ++++++
> Documentation/riscv/index.rst                 |   1 +
> arch/riscv/include/asm/cpufeature.h           |  23 +++
> arch/riscv/include/asm/hwprobe.h              |  13 ++
> arch/riscv/include/asm/smp.h                  |   9 +
> arch/riscv/include/asm/syscall.h              |   3 +
> arch/riscv/include/uapi/asm/hwprobe.h         |  35 ++++
> arch/riscv/include/uapi/asm/unistd.h          |   8 +
> arch/riscv/kernel/cpu.c                       |  11 +-
> arch/riscv/kernel/cpufeature.c                |  31 ++-
> arch/riscv/kernel/sys_riscv.c                 | 192 +++++++++++++++++-
> tools/testing/selftests/Makefile              |   1 +
> tools/testing/selftests/riscv/Makefile        |  58 ++++++
> .../testing/selftests/riscv/hwprobe/Makefile  |  10 +
> .../testing/selftests/riscv/hwprobe/hwprobe.c |  89 ++++++++
> .../selftests/riscv/hwprobe/sys_hwprobe.S     |  12 ++
> tools/testing/selftests/riscv/libc.S          |  46 +++++
> 18 files changed, 613 insertions(+), 10 deletions(-)
> create mode 100644 Documentation/riscv/hwprobe.rst
> create mode 100644 arch/riscv/include/asm/cpufeature.h
> create mode 100644 arch/riscv/include/asm/hwprobe.h
> create mode 100644 arch/riscv/include/uapi/asm/hwprobe.h
> create mode 100644 tools/testing/selftests/riscv/Makefile
> create mode 100644 tools/testing/selftests/riscv/hwprobe/Makefile
> create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.c
> create mode 100644 tools/testing/selftests/riscv/hwprobe/sys_hwprobe.S
> create mode 100644 tools/testing/selftests/riscv/libc.S
> 
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists