[PATCH -next v21 03/27] riscv: hwprobe: Add support for probing V in RISCV_HWPROBE_KEY_IMA_EXT_0
Stefan O'Rear
sorear at fastmail.com
Tue Jun 27 21:53:38 PDT 2023
On Tue, Jun 27, 2023, at 9:56 PM, Palmer Dabbelt wrote:
> On Tue, 27 Jun 2023 17:30:33 PDT (-0700), sorear at fastmail.com wrote:
>> On Mon, Jun 5, 2023, at 7:07 AM, Andy Chiu wrote:
>>> Probing kernel support for Vector extension is available now. This only
>>> add detection for V only. Extenions like Zvfh, Zk are not in this scope.
>>>
>>> Signed-off-by: Andy Chiu <andy.chiu at sifive.com>
>>> Reviewed-by: Conor Dooley <conor.dooley at microchip.com>
>>> Reviewed-by: Evan Green <evan at rivosinc.com>
>>> Reviewed-by: Palmer Dabbelt <palmer at rivosinc.com>
>>> ---
>>> Changelog v20:
>>> - Fix a typo in document, and remove duplicated probes (Heiko)
>>> - probe V extension in RISCV_HWPROBE_KEY_IMA_EXT_0 key only (Palmer,
>>> Evan)
>>> ---
>>> Documentation/riscv/hwprobe.rst | 3 +++
>>> arch/riscv/include/uapi/asm/hwprobe.h | 1 +
>>> arch/riscv/kernel/sys_riscv.c | 4 ++++
>>> 3 files changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
>>> index 9f0dd62dcb5d..7431d9d01c73 100644
>>> --- a/Documentation/riscv/hwprobe.rst
>>> +++ b/Documentation/riscv/hwprobe.rst
>>> @@ -64,6 +64,9 @@ The following keys are defined:
>>> * :c:macro:`RISCV_HWPROBE_IMA_C`: The C extension is supported, as defined
>>> by version 2.2 of the RISC-V ISA manual.
>>>
>>> + * :c:macro:`RISCV_HWPROBE_IMA_V`: The V extension is supported, as defined by
>>> + version 1.0 of the RISC-V Vector extension manual.
>>> +
>>> * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
>>> information about the selected set of processors.
>>>
>>> diff --git a/arch/riscv/include/uapi/asm/hwprobe.h
>>> b/arch/riscv/include/uapi/asm/hwprobe.h
>>> index 8d745a4ad8a2..7c6fdcf7ced5 100644
>>> --- a/arch/riscv/include/uapi/asm/hwprobe.h
>>> +++ b/arch/riscv/include/uapi/asm/hwprobe.h
>>> @@ -25,6 +25,7 @@ struct riscv_hwprobe {
>>> #define RISCV_HWPROBE_KEY_IMA_EXT_0 4
>>> #define RISCV_HWPROBE_IMA_FD (1 << 0)
>>> #define RISCV_HWPROBE_IMA_C (1 << 1)
>>> +#define RISCV_HWPROBE_IMA_V (1 << 2)
>>> #define RISCV_HWPROBE_KEY_CPUPERF_0 5
>>> #define RISCV_HWPROBE_MISALIGNED_UNKNOWN (0 << 0)
>>> #define RISCV_HWPROBE_MISALIGNED_EMULATED (1 << 0)
>>> diff --git a/arch/riscv/kernel/sys_riscv.c
>>> b/arch/riscv/kernel/sys_riscv.c
>>> index 5db29683ebee..88357a848797 100644
>>> --- a/arch/riscv/kernel/sys_riscv.c
>>> +++ b/arch/riscv/kernel/sys_riscv.c
>>> @@ -10,6 +10,7 @@
>>> #include <asm/cpufeature.h>
>>> #include <asm/hwprobe.h>
>>> #include <asm/sbi.h>
>>> +#include <asm/vector.h>
>>> #include <asm/switch_to.h>
>>> #include <asm/uaccess.h>
>>> #include <asm/unistd.h>
>>> @@ -171,6 +172,9 @@ static void hwprobe_one_pair(struct riscv_hwprobe
>>> *pair,
>>> if (riscv_isa_extension_available(NULL, c))
>>> pair->value |= RISCV_HWPROBE_IMA_C;
>>>
>>> + if (has_vector())
>>> + pair->value |= RISCV_HWPROBE_IMA_V;
>>> +
>>> break;
>>
>> I am concerned by the exception this is making. I believe the intention of
>> riscv_hwprobe is to replace AT_HWCAP as the single point of truth for userspace
>> to make instruction use decisions. Since this does not check riscv_v_vstate_ctrl_user_allowed,
>> application code which wants to know if V instructions are usable must use
>> AT_HWCAP instead, unlike all other extensions for which the relevant data is
>> available within the hwprobe return.
>
> I guess we were vague in the docs about what "supported" means, but IIRC
> the goal was for riscv_hwprobe() to indicate what's supported by both
> the HW and the kernel. In other words, hwprobe should indicate what's
Should this be "the HW, the firmware, and the kernel" in the cases where it
matters, or are you considering the firmware part of the kernel's view of
the hardware?
> possible to enable -- even if there's some additional steps necessary to
> enable it.
>
> We can at least make this a little more explicit with something like
>
> diff --git a/Documentation/riscv/hwprobe.rst
> b/Documentation/riscv/hwprobe.rst
> index 19165ebd82ba..7f82a5385bc3 100644
> --- a/Documentation/riscv/hwprobe.rst
> +++ b/Documentation/riscv/hwprobe.rst
> @@ -27,6 +27,13 @@ AND of the values for the specified CPUs.
> Usermode can supply NULL for cpus and
> 0 for cpu_count as a shortcut for all online CPUs. There are
> currently no flags,
> this value must be zero for future compatibility.
>
> +Calls to `sys_riscv_hwprobe()` indicate the features supported by
> both the
> +kernel and the hardware that the system is running on. For
> example, if the
> +hardware supports the V extension and the kernel has V support
> enabled then
> +`RISCV_HWPROBE_KEY_IMA_EXT_0`/`RISCV_HWPROBE_IMA_V` will be set
> even if the V
> +extension is disabled via a userspace-controlled tunable such as
> +`PR_RISCV_V_SET_CONTROL`.
> +
> On success 0 is returned, on failure a negative error code is
> returned.
>
> The following keys are defined:
> @@ -65,7 +72,10 @@ The following keys are defined:
> by version 2.2 of the RISC-V ISA manual.
>
> * :c:macro:`RISCV_HWPROBE_IMA_V`: The V extension is supported,
> as defined by
> - version 1.0 of the RISC-V Vector extension manual.
> + version 1.0 of the RISC-V Vector extension manual. For strict
> uABI
> + compatibility some systems may disable V by default even when
> the hardware
> + supports in, in which case users must call
> `prctl(PR_RISCV_V_SET_CONTROL,
> + ...` to explicitly allow V to be used.
>
> * :c:macro:`RISCV_HWPROBE_EXT_ZBA`: The Zba address generation
> extension is
> supported, as defined in version 1.0 of the
> Bit-Manipulation ISA
>
> IMO that's the better way to go that to require that userspace tries to enable
> V via the prctl() first, but we haven't released this yet so in theory we could
> still change it.
It's certainly a more precise definition but I'm arguing it's not a useful one.
The description of the prctl() in Documentation/riscv/vector.rst is fairly clear
that it is intended only for use by init systems, and not by libraries.
Would you agree that "V is supported by the hardware and kernel, and could have
been enabled by the init system but wasn't" is not actionable information for
most applications and libraries?
https://sourceware.org/pipermail/libc-alpha/2023-April/147062.html proposes to
use hwprobe to "do things like dynamically choose a memcpy implementation".
Would you agree that hwprobe as currently defined in for-next is not suitable
for the purpose described in that message, since it describes features that could
be enabled, not features that are enabled?
> We'd have a similar discussion for some of the counters that need to feed
> through the perf interface, though those are still in flight...
The documented intent of the vector prctl is to enable or disable vector use as
a policy for a tree of processes. If I understand them correctly the perf
counter user access patches require _individual processes_ to enable perf
counters for their own use, which makes it a very different story from the
perspective of the hwprobe API consumers.
-s
>> Assuming this is intentional, what is the path forward for future extensions
>> that cannot be used from userspace without additional conditions being met?
>> For instance, if we add support in the future for the Zve* extensions, the V
>> bit would not be set in HWCAP for them, which would require library code to
>> use the prctl interface unless we define the hwcap bits to imply userspace
>> usability.
>
> In this case a system that supports some of the Zve extensions but not
> the full V extension would not be probably from userspace, as V would
> not be set anywhere. The way to support that would be to add new bits
> into hwprobe to indicate those extensions, it just wasn't clear that
> anyone was interested in building Linux-flavored systems that supported
> only some a strict subset of V.
>
> Happy to see patches if you know of some hardware in the pipeline, though ;)
>
>>
>> -s
>>
>>> case RISCV_HWPROBE_KEY_CPUPERF_0:
>>> --
>>> 2.17.1
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
More information about the linux-riscv
mailing list