[PATCH v2 0/6] RISC-V Hardware Probing User Interface

[PATCH v2 0/6] RISC-V Hardware Probing User Interface

[Evan Green]

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.

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

[PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Evan Green]

From: Palmer Dabbelt <palmer@rivosinc.com>

This key allows device trees to specify the performance of misaligned
accesses to main memory regions from each CPU in the system.

Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
Signed-off-by: Evan Green <evan@rivosinc.com>
---

(no changes since v1)

 Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index c6720764e765..2c09bd6f2927 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -85,6 +85,21 @@ properties:
     $ref: "/schemas/types.yaml#/definitions/string"
     pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
 
+  riscv,misaligned-access-performance:
+    description:
+      Identifies the performance of misaligned memory accesses to main memory
+      regions.  There are three flavors of unaligned access performance: "emulated"
+      means that misaligned accesses are emulated via software and thus
+      extremely slow, "slow" means that misaligned accesses are supported by
+      hardware but still slower that aligned accesses sequences, and "fast"
+      means that misaligned accesses are as fast or faster than the
+      cooresponding aligned accesses sequences.
+    $ref: "/schemas/types.yaml#/definitions/string"
+    enum:
+      - emulated
+      - slow
+      - fast
+
   # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
   timebase-frequency: false
 
-- 
2.25.1

Re: [PATCH v2 0/6] RISC-V Hardware Probing User Interface

[Jessica Clarke]

On 6 Feb 2023, at 20:14, Evan Green <evan@rivosinc.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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Rob Herring]

On Mon, 06 Feb 2023 12:14:53 -0800, Evan Green wrote:
> From: Palmer Dabbelt <palmer@rivosinc.com>
> 
> This key allows device trees to specify the performance of misaligned
> accesses to main memory regions from each CPU in the system.
> 
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> Signed-off-by: Evan Green <evan@rivosinc.com>
> ---
> 
> (no changes since v1)
> 
>  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/riscv/cpus.yaml:91:72: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/riscv/cpus.example.dts'
Documentation/devicetree/bindings/riscv/cpus.yaml:91:72: mapping values are not allowed here
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/riscv/cpus.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/riscv/cpus.yaml:91:72: mapping values are not allowed here
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.yaml: ignoring, error parsing file
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206201455.1790329-5-evan@rivosinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 0/6] RISC-V Hardware Probing User Interface

[Conor Dooley]

Hey Evan,
Having been talking to Palmer about this series at FOSDEM,
it was a very pleasant surprise to see this when I saw this in my inbox when I landed back home.
I do very much intend reviewing this, but... 

On 6 February 2023 20:14:49 GMT, Evan Green <evan@rivosinc.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

This all looks to be the same cover message as Palmer submitted with the v1,
so, as I'd mentioned to him the other day, I'd like to do a bit
of an investigation into the sysfs approach drew suggested
on the v1.
So, if it's a little bit before you hear - I've certainly not forgotten about the series!

Thanks,
Conor.

>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.
>
>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
>

Re: [PATCH v2 0/6] RISC-V Hardware Probing User Interface

[Heinrich Schuchardt]

On 2/6/23 22:11, Jessica Clarke wrote:
> On 6 Feb 2023, at 20:14, Evan Green <evan@rivosinc.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.

Operating systems tend to reschedule threads moving them between harts. 
New threads may be created by processes at any time.

It is not clear to me what information the syscall shall convey in the 
heterogeneous case. I see the following alternatives:

* The syscall describes the current hart.
* The syscall provides individual properties of all harts.
* The syscall provides a set of properties that is valid for any hart on 
which the thread might be scheduled.
* The syscall provides a set of properties that is valid for any hart 
that any thread of the current process might be scheduled to.

Describing only the current hart would not be helpful as the thread 
might be rescheduled to a hart with a smaller set of available extensions.

Describing the properties of all harts would not be helpful if the 
thread has no control to which hart it is scheduled.

Processes that don't control scheduling would most benefit from a 
guaranteed set of properties valid for all threads of the process.

Processes that take control of scheduling would probably want 
information about all harts.

Best regards

Heinrich

> 
> 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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

Re: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Rob Herring]

On Mon, Feb 06, 2023 at 12:14:53PM -0800, Evan Green wrote:
> From: Palmer Dabbelt <palmer@rivosinc.com>
> 
> This key allows device trees to specify the performance of misaligned
> accesses to main memory regions from each CPU in the system.
> 
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> Signed-off-by: Evan Green <evan@rivosinc.com>
> ---
> 
> (no changes since v1)
> 
>  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index c6720764e765..2c09bd6f2927 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -85,6 +85,21 @@ properties:
>      $ref: "/schemas/types.yaml#/definitions/string"
>      pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
>  
> +  riscv,misaligned-access-performance:
> +    description:
> +      Identifies the performance of misaligned memory accesses to main memory
> +      regions.  There are three flavors of unaligned access performance: "emulated"
> +      means that misaligned accesses are emulated via software and thus
> +      extremely slow, "slow" means that misaligned accesses are supported by
> +      hardware but still slower that aligned accesses sequences, and "fast"
> +      means that misaligned accesses are as fast or faster than the
> +      cooresponding aligned accesses sequences.
> +    $ref: "/schemas/types.yaml#/definitions/string"
> +    enum:
> +      - emulated
> +      - slow
> +      - fast

I don't think this belongs in DT. (I'm not sure about a userspace 
interface either.)

Can't this be tested and determined at runtime? Do misaligned accesses 
and compare the performance. We already do this for things like memcpy 
or crypto implementation selection.

Rob

RE: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[David Laight]

From: Rob Herring
> Sent: 07 February 2023 17:06
> 
> On Mon, Feb 06, 2023 at 12:14:53PM -0800, Evan Green wrote:
> > From: Palmer Dabbelt <palmer@rivosinc.com>
> >
> > This key allows device trees to specify the performance of misaligned
> > accesses to main memory regions from each CPU in the system.
> >
> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Signed-off-by: Evan Green <evan@rivosinc.com>
> > ---
> >
> > (no changes since v1)
> >
> >  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml
> b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > index c6720764e765..2c09bd6f2927 100644
> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > @@ -85,6 +85,21 @@ properties:
> >      $ref: "/schemas/types.yaml#/definitions/string"
> >      pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
> >
> > +  riscv,misaligned-access-performance:
> > +    description:
> > +      Identifies the performance of misaligned memory accesses to main memory
> > +      regions.  There are three flavors of unaligned access performance: "emulated"
> > +      means that misaligned accesses are emulated via software and thus
> > +      extremely slow, "slow" means that misaligned accesses are supported by
> > +      hardware but still slower that aligned accesses sequences, and "fast"
> > +      means that misaligned accesses are as fast or faster than the
> > +      cooresponding aligned accesses sequences.
> > +    $ref: "/schemas/types.yaml#/definitions/string"
> > +    enum:
> > +      - emulated
> > +      - slow
> > +      - fast
> 
> I don't think this belongs in DT. (I'm not sure about a userspace
> interface either.)
> 
> Can't this be tested and determined at runtime? Do misaligned accesses
> and compare the performance. We already do this for things like memcpy
> or crypto implementation selection.

There is also an long discussion about misaligned accesses
for loooongarch.

Basically if you want to run a common kernel (and userspace)
you have to default to compiling everything with -mno-stict-align
so that the compiler generates byte accesses for anything
marked 'packed' (etc).

Run-time tests can optimise some hot-spots.

In any case 'slow' is probably pointless - unless the accesses
take more than 1 or 2 extra cycles.

Oh, and you really never, ever want to emulate them.

Technically misaligned reads on (some) x86-64 cpu are slower
than aligned ones, but the difference is marginal.
I've measured two 64bit misaligned reads every clock.
But it is consistently slower by much less than one clock
per cache line.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Palmer Dabbelt]

On Wed, 08 Feb 2023 04:45:10 PST (-0800), David.Laight@ACULAB.COM wrote:
> From: Rob Herring
>> Sent: 07 February 2023 17:06
>>
>> On Mon, Feb 06, 2023 at 12:14:53PM -0800, Evan Green wrote:
>> > From: Palmer Dabbelt <palmer@rivosinc.com>
>> >
>> > This key allows device trees to specify the performance of misaligned
>> > accesses to main memory regions from each CPU in the system.
>> >
>> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> > Signed-off-by: Evan Green <evan@rivosinc.com>
>> > ---
>> >
>> > (no changes since v1)
>> >
>> >  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
>> >  1 file changed, 15 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml
>> b/Documentation/devicetree/bindings/riscv/cpus.yaml
>> > index c6720764e765..2c09bd6f2927 100644
>> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
>> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
>> > @@ -85,6 +85,21 @@ properties:
>> >      $ref: "/schemas/types.yaml#/definitions/string"
>> >      pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
>> >
>> > +  riscv,misaligned-access-performance:
>> > +    description:
>> > +      Identifies the performance of misaligned memory accesses to main memory
>> > +      regions.  There are three flavors of unaligned access performance: "emulated"
>> > +      means that misaligned accesses are emulated via software and thus
>> > +      extremely slow, "slow" means that misaligned accesses are supported by
>> > +      hardware but still slower that aligned accesses sequences, and "fast"
>> > +      means that misaligned accesses are as fast or faster than the
>> > +      cooresponding aligned accesses sequences.
>> > +    $ref: "/schemas/types.yaml#/definitions/string"
>> > +    enum:
>> > +      - emulated
>> > +      - slow
>> > +      - fast
>>
>> I don't think this belongs in DT. (I'm not sure about a userspace
>> interface either.)

[Kind of answered below.]

>> Can't this be tested and determined at runtime? Do misaligned accesses
>> and compare the performance. We already do this for things like memcpy
>> or crypto implementation selection.

We've had a history of broken firmware emulation of misaligned accesses 
wreaking havoc.  We don't run into concrete bugs there because we avoid 
misaligned accesses as much as possible in the kernel, but I'd be 
worried that we'd trigger a lot of these when probing for misaligned 
accesses.

> There is also an long discussion about misaligned accesses
> for loooongarch.
>
> Basically if you want to run a common kernel (and userspace)
> you have to default to compiling everything with -mno-stict-align
> so that the compiler generates byte accesses for anything
> marked 'packed' (etc).
>
> Run-time tests can optimise some hot-spots.
>
> In any case 'slow' is probably pointless - unless the accesses
> take more than 1 or 2 extra cycles.

[Also below.]

> Oh, and you really never, ever want to emulate them.

Unfortunately we're kind of stuck with this one: the specs used to 
require that misaligned accesses were supported and thus there's a bunch 
of firmwares that emulate them (and various misaligned accesses spread 
around, though they're kind of a mess).  The specs no longer require 
this support, but just dropping it from firmware will break binaries.

There's been some vague plans to dig out of this, but it'd require some 
sort of firmware interface additions in order to turn off the emulation 
and that's going to take a while.  As it stands we've got a bunch of 
users that just want to know when they can emit misaligned accesses.

> Technically misaligned reads on (some) x86-64 cpu are slower
> than aligned ones, but the difference is marginal.
> I've measured two 64bit misaligned reads every clock.
> But it is consistently slower by much less than one clock
> per cache line.

The "fast" case is explicitly written to catch that flavor of 
implementation.

The "slow" one is a bit vaguer, but the general idea is to catch 
implementations that end up with some sort of pipeline flush on 
misaligned accesses.  We've got a lot of very small in-order processors 
in RISC-V land, and while I haven't gotten around to benchmarking them 
all my guess is that the spec requirement for support ended up with some 
simple implementations.

FWIW: I checked the c906 RTL and it's setting some exception-related 
info on misaligned accesses, but I'd need to actually benchmark on to 
know for sure and they're kind of a headache to deal with.

>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH v2 0/6] RISC-V Hardware Probing User Interface

[Palmer Dabbelt]

On Mon, 06 Feb 2023 14:47:35 PST (-0800), heinrich.schuchardt@canonical.com wrote:
> On 2/6/23 22:11, Jessica Clarke wrote:
>> On 6 Feb 2023, at 20:14, Evan Green <evan@rivosinc.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.
>
> Operating systems tend to reschedule threads moving them between harts.
> New threads may be created by processes at any time.
>
> It is not clear to me what information the syscall shall convey in the
> heterogeneous case. I see the following alternatives:
>
> * The syscall describes the current hart.
> * The syscall provides individual properties of all harts.
> * The syscall provides a set of properties that is valid for any hart on
> which the thread might be scheduled.
> * The syscall provides a set of properties that is valid for any hart
> that any thread of the current process might be scheduled to.
>
> Describing only the current hart would not be helpful as the thread
> might be rescheduled to a hart with a smaller set of available extensions.
>
> Describing the properties of all harts would not be helpful if the
> thread has no control to which hart it is scheduled.
>
> Processes that don't control scheduling would most benefit from a
> guaranteed set of properties valid for all threads of the process.
>
> Processes that take control of scheduling would probably want
> information about all harts.

There's a cpu_set_t argument.  We tried to answer this via the 
Documentation patch.  It's just the single sentence

    The CPU set is defined by CPU_SET(3), the indicated features will be 
    supported on all CPUs in the set.

so maybe it needs beefing up...  Do you mind commenting on the doc diff, 
if you've got any ideas as to how to word it better?  That way anyone 
else reviewing the docs will see too.

> Best regards
>
> Heinrich
>
>>
>> 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@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>

Re: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1966 bytes --]

On Mon, Feb 06, 2023 at 12:14:53PM -0800, Evan Green wrote:
> From: Palmer Dabbelt <palmer@rivosinc.com>
> 
> This key allows device trees to specify the performance of misaligned
> accesses to main memory regions from each CPU in the system.
> 
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> Signed-off-by: Evan Green <evan@rivosinc.com>
> ---
> 
> (no changes since v1)
> 
>  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index c6720764e765..2c09bd6f2927 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -85,6 +85,21 @@ properties:
>      $ref: "/schemas/types.yaml#/definitions/string"
>      pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
>  
> +  riscv,misaligned-access-performance:
> +    description:
> +      Identifies the performance of misaligned memory accesses to main memory
> +      regions.  There are three flavors of unaligned access performance: "emulated"

Is the performance: emulated the source of the dt_binding_check issues?
And the fix is as simple as:
-    description:
+    description: |
?

> +      means that misaligned accesses are emulated via software and thus
> +      extremely slow, "slow" means that misaligned accesses are supported by
> +      hardware but still slower that aligned accesses sequences, and "fast"
> +      means that misaligned accesses are as fast or faster than the
> +      cooresponding aligned accesses sequences.
> +    $ref: "/schemas/types.yaml#/definitions/string"
> +    enum:
> +      - emulated
> +      - slow
> +      - fast
> +
>    # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
>    timebase-frequency: false
>  
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Evan Green]

On Tue, Feb 14, 2023 at 1:26 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, Feb 06, 2023 at 12:14:53PM -0800, Evan Green wrote:
> > From: Palmer Dabbelt <palmer@rivosinc.com>
> >
> > This key allows device trees to specify the performance of misaligned
> > accesses to main memory regions from each CPU in the system.
> >
> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Signed-off-by: Evan Green <evan@rivosinc.com>
> > ---
> >
> > (no changes since v1)
> >
> >  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > index c6720764e765..2c09bd6f2927 100644
> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > @@ -85,6 +85,21 @@ properties:
> >      $ref: "/schemas/types.yaml#/definitions/string"
> >      pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
> >
> > +  riscv,misaligned-access-performance:
> > +    description:
> > +      Identifies the performance of misaligned memory accesses to main memory
> > +      regions.  There are three flavors of unaligned access performance: "emulated"
>
> Is the performance: emulated the source of the dt_binding_check issues?
> And the fix is as simple as:
> -    description:
> +    description: |
> ?

Yep, I can pass cleanly with that change. Thanks!

Re: [PATCH v2 4/6] dt-bindings: Add RISC-V misaligned access performance

[Rob Herring]

On Thu, Feb 09, 2023 at 08:51:22AM -0800, Palmer Dabbelt wrote:
> On Wed, 08 Feb 2023 04:45:10 PST (-0800), David.Laight@ACULAB.COM wrote:
> > From: Rob Herring
> > > Sent: 07 February 2023 17:06
> > > 
> > > On Mon, Feb 06, 2023 at 12:14:53PM -0800, Evan Green wrote:
> > > > From: Palmer Dabbelt <palmer@rivosinc.com>
> > > >
> > > > This key allows device trees to specify the performance of misaligned
> > > > accesses to main memory regions from each CPU in the system.
> > > >
> > > > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> > > > Signed-off-by: Evan Green <evan@rivosinc.com>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  Documentation/devicetree/bindings/riscv/cpus.yaml | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > > index c6720764e765..2c09bd6f2927 100644
> > > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > > @@ -85,6 +85,21 @@ properties:
> > > >      $ref: "/schemas/types.yaml#/definitions/string"
> > > >      pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:_[hsxz](?:[a-z])+)*$
> > > >
> > > > +  riscv,misaligned-access-performance:
> > > > +    description:
> > > > +      Identifies the performance of misaligned memory accesses to main memory
> > > > +      regions.  There are three flavors of unaligned access performance: "emulated"
> > > > +      means that misaligned accesses are emulated via software and thus
> > > > +      extremely slow, "slow" means that misaligned accesses are supported by
> > > > +      hardware but still slower that aligned accesses sequences, and "fast"
> > > > +      means that misaligned accesses are as fast or faster than the
> > > > +      cooresponding aligned accesses sequences.
> > > > +    $ref: "/schemas/types.yaml#/definitions/string"
> > > > +    enum:
> > > > +      - emulated
> > > > +      - slow
> > > > +      - fast
> > > 
> > > I don't think this belongs in DT. (I'm not sure about a userspace
> > > interface either.)
> 
> [Kind of answered below.]
> 
> > > Can't this be tested and determined at runtime? Do misaligned accesses
> > > and compare the performance. We already do this for things like memcpy
> > > or crypto implementation selection.
> 
> We've had a history of broken firmware emulation of misaligned accesses
> wreaking havoc.  We don't run into concrete bugs there because we avoid
> misaligned accesses as much as possible in the kernel, but I'd be worried
> that we'd trigger a lot of these when probing for misaligned accesses.

Then how do you distinguish between emulated and working vs. emulated 
and broken? Sounds like the kernel running things would motivate fixing 
firmware. :) If not, then broken platforms can disable the check with a 
kernel command line flag. 

> 
> > There is also an long discussion about misaligned accesses
> > for loooongarch.
> > 
> > Basically if you want to run a common kernel (and userspace)
> > you have to default to compiling everything with -mno-stict-align
> > so that the compiler generates byte accesses for anything
> > marked 'packed' (etc).
> > 
> > Run-time tests can optimise some hot-spots.
> > 
> > In any case 'slow' is probably pointless - unless the accesses
> > take more than 1 or 2 extra cycles.
> 
> [Also below.]
> 
> > Oh, and you really never, ever want to emulate them.
> 
> Unfortunately we're kind of stuck with this one: the specs used to require
> that misaligned accesses were supported and thus there's a bunch of
> firmwares that emulate them (and various misaligned accesses spread around,
> though they're kind of a mess).  The specs no longer require this support,
> but just dropping it from firmware will break binaries.
> 
> There's been some vague plans to dig out of this, but it'd require some sort
> of firmware interface additions in order to turn off the emulation and
> that's going to take a while.  As it stands we've got a bunch of users that
> just want to know when they can emit misaligned accesses.
> 
> > Technically misaligned reads on (some) x86-64 cpu are slower
> > than aligned ones, but the difference is marginal.
> > I've measured two 64bit misaligned reads every clock.
> > But it is consistently slower by much less than one clock
> > per cache line.
> 
> The "fast" case is explicitly written to catch that flavor of
> implementation.
> 
> The "slow" one is a bit vaguer, but the general idea is to catch
> implementations that end up with some sort of pipeline flush on misaligned
> accesses.  We've got a lot of very small in-order processors in RISC-V land,
> and while I haven't gotten around to benchmarking them all my guess is that
> the spec requirement for support ended up with some simple implementations.

If userspace wants to get into microarchitecture level optimizations, it 
should just look at the CPU model. IOW, use the CPU compatible to infer 
things rather than continuously adding properties in an adhoc manor 
trying to parameterize everything.

Rob