Re: [PATCH -next v20 24/26] riscv: Add documentation for Vector

Re: [PATCH -next v20 24/26] riscv: Add documentation for Vector

[Rémi Denis-Courmont]

	Hi,

Le torstaina 18. toukokuuta 2023 19.19.47 EEST, vous avez écrit :
> This patch add a brief documentation of the userspace interface in
> regard to the RISC-V Vector extension.
> 
> Signed-off-by: Andy Chiu <andy.chiu at sifive.com>
> Reviewed-by: Greentime Hu <greentime.hu at sifive.com>
> Reviewed-by: Vincent Chen <vincent.chen at sifive.com>
> Co-developed-by: Bagas Sanjaya <bagasdotme at gmail.com>
> Signed-off-by: Bagas Sanjaya <bagasdotme at gmail.com>
> ---
> Changelog v20:
>  - Drop bit-field repressentation and typos (Bj?rn)
>  - Fix document styling (Bagas)
> ---
>  Documentation/riscv/index.rst  |   1 +
>  Documentation/riscv/vector.rst | 120 +++++++++++++++++++++++++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 Documentation/riscv/vector.rst
> 
> diff --git a/Documentation/riscv/index.rst b/Documentation/riscv/index.rst
> index 175a91db0200..95cf9c1e1da1 100644
> --- a/Documentation/riscv/index.rst
> +++ b/Documentation/riscv/index.rst
> @@ -10,6 +10,7 @@ RISC-V architecture
>      hwprobe
>      patch-acceptance
>      uabi
> +    vector
> 
>      features
> 
> diff --git a/Documentation/riscv/vector.rst b/Documentation/riscv/vector.rst
> new file mode 100644
> index 000000000000..5d37fd212720
> --- /dev/null
> +++ b/Documentation/riscv/vector.rst
> @@ -0,0 +1,120 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================================
> +Vector Extension Support for RISC-V Linux
> +=========================================
> +
> +This document briefly outlines the interface provided to userspace by Linux
> in +order to support the use of the RISC-V Vector Extension.
> +
> +1.  prctl() Interface
> +---------------------
> +
> +Two new prctl() calls are added to allow programs to manage the enablement
> +status for the use of Vector in userspace:
> +
> +* prctl(PR_RISCV_V_SET_CONTROL, unsigned long arg)
> +
> +    Sets the Vector enablement status of the calling thread, where the
> control
> +    argument consists of two 2-bit enablement statuses and a bit
> for inheritance
> +    mode. Other threads of the calling process are
> unaffected.

I somewhat wonder who is/are the intended users of this new prctl(). Are they 
the run-time (libc)? The main program? Libraries using RVV internally (think 
OpenSSL, Nettle, FFmpeg, etc)? The init system?

Library code doesn't typically know how stacks are allocated and how signal 
are handled (on alternate or normal stacks), since signal handlers are 
process-global state. So I figure that libraries should keep off off this one.

Conversely, it would be impractical for programs to call a Linux-specific RISC-
V-specific in or around their main(). And then libc presumably should not 
override the configured policy that comes from sysctl or from the parent 
process.

So I guess that that leaves just the init system (in a broad sense) then?

In any case, I think the intended use should be clarified with proper usage 
guidelines. Otherwise, what I bet happens is RVV-capable libraries just 
blindly invoke the prctl() to "enable RVV", deafeating the purpose of having 
the prctl() in the first place.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Rémi Denis-Courmont]

	Hi all,

Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
> This patch add two riscv-specific prctls, to allow usespace control the
> use of vector unit:
> 
>  * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
>    or all following execve for a thread. Turning off a thread's Vector
>    live is not possible since libraries may have registered ifunc that
>    may execute Vector instructions.
>  * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
>    current thread, and the setting for following execve(s).

So far the story was that if the nth bit in the ELF HWCAP auxillary vector was 
set, then the nth single lettered extension was supported. There is already 
userspace code out there that expects this of the V bit. (I know I have 
written such code, and I also know others did likewise.) This is how it 
already works for the D and F bits.

Admittedly, upstream Linux has never ever set that bit to this day. But still, 
if we end up with the bit set in a process that has had V support disabled by 
the parent (or the sysctl), existing userspace will encounter SIGILL and 
break.

IMO, the bit must be masked not only whence the kernel lacks V support (as 
PATCH 02 does), but also if the process starts with V disabled.

There are two ways to achieve this:
1) V is never ever set, and userspace is forced to use hwprobe() instead.
2) V is set only in processes starting with V enabled (and it's their own 
fault if they disabled it in future child threads).

Br,

-- 
レミ・デニ-クールモン
http://www.remlab.net/

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Andy Chiu]

On Sun, May 21, 2023 at 1:41 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>
>         Hi all,
>
> Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
> > This patch add two riscv-specific prctls, to allow usespace control the
> > use of vector unit:
> >
> >  * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
> >    or all following execve for a thread. Turning off a thread's Vector
> >    live is not possible since libraries may have registered ifunc that
> >    may execute Vector instructions.
> >  * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
> >    current thread, and the setting for following execve(s).
>
> So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
> set, then the nth single lettered extension was supported. There is already
> userspace code out there that expects this of the V bit. (I know I have
> written such code, and I also know others did likewise.) This is how it
> already works for the D and F bits.

Yes, the V bit in ELF_HWCAP becomes vague in this series.

>
> Admittedly, upstream Linux has never ever set that bit to this day. But still,
> if we end up with the bit set in a process that has had V support disabled by
> the parent (or the sysctl), existing userspace will encounter SIGILL and
> break.
>
> IMO, the bit must be masked not only whence the kernel lacks V support (as
> PATCH 02 does), but also if the process starts with V disabled.

This is going to change ELF_HWCAP from a macro to a function. The
function will turn on COMPAT_HWCAP_ISA_V iff V is supported and
allowed. I am going to do this in v21 If this looks sane. i.e.
Currently I don't see other architectures which give different
ELF_HWCAP values on each execve. If ELF_HWCAP is not a right place to
encode the information then userspace has to make the prctl() call to
be certain on whether V is usable.

>
> There are two ways to achieve this:
> 1) V is never ever set, and userspace is forced to use hwprobe() instead.
> 2) V is set only in processes starting with V enabled (and it's their own
> fault if they disabled it in future child threads).

The prctl() interface does not allow processes to turn off V once it
is enabled in its current (execve) context. The process can only
disable V when the next execve() happens. Then, if we implement
ELF_HWCAP as mentioned above, the kernel will reload a new HWCAP for
the process. By then, the new HWCAP will have V masked since it is not
allowed.

>
> Br,
>
> --
> レミ・デニ-クールモン
> http://www.remlab.net/
>
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks,
Andy

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Rémi Denis-Courmont]

Hi,

Le 22 mai 2023 11:28:28 GMT+03:00, Andy Chiu <andy.chiu@sifive.com> a écrit :
>On Sun, May 21, 2023 at 1:41 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>>
>>         Hi all,
>>
>> Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
>> > This patch add two riscv-specific prctls, to allow usespace control the
>> > use of vector unit:
>> >
>> >  * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
>> >    or all following execve for a thread. Turning off a thread's Vector
>> >    live is not possible since libraries may have registered ifunc that
>> >    may execute Vector instructions.
>> >  * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
>> >    current thread, and the setting for following execve(s).
>>
>> So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
>> set, then the nth single lettered extension was supported. There is already
>> userspace code out there that expects this of the V bit. (I know I have
>> written such code, and I also know others did likewise.) This is how it
>> already works for the D and F bits.
>
>Yes, the V bit in ELF_HWCAP becomes vague in this series.


>> Admittedly, upstream Linux has never ever set that bit to this day. But still,
>> if we end up with the bit set in a process that has had V support disabled by
>> the parent (or the sysctl), existing userspace will encounter SIGILL and
>> break.
>>
>> IMO, the bit must be masked not only whence the kernel lacks V support (as
>> PATCH 02 does), but also if the process starts with V disabled.
>
>This is going to change ELF_HWCAP from a macro to a function. The
>function will turn on COMPAT_HWCAP_ISA_V iff V is supported and
>allowed. I am going to do this in v21 If this looks sane. i.e.
>Currently I don't see other architectures which give different
>ELF_HWCAP values on each execve. If ELF_HWCAP is not a right place to
>encode the information then userspace has to make the prctl() call to
>be certain on whether V is usable.

I don't think the value of an auxillary vector entry can change in an existing process nor that we need that. If an application starts with V disabled, you can keep the V bit clear even if V gets enabled later on; that won't break existing userspace code, which simply won't use vectors.

What does break existing userspace is setting the V bit whilst vectors are disabled.



>
>>
>> There are two ways to achieve this:
>> 1) V is never ever set, and userspace is forced to use hwprobe() instead.
>> 2) V is set only in processes starting with V enabled (and it's their own
>> fault if they disabled it in future child threads).
>
>The prctl() interface does not allow processes to turn off V once it
>is enabled in its current (execve) context. The process can only
>disable V when the next execve() happens. Then, if we implement
>ELF_HWCAP as mentioned above, the kernel will reload a new HWCAP for
>the process. By then, the new HWCAP will have V masked since it is not
>allowed.
>
>>
>> Br,
>>
>> --
>> レミ・デニ-クールモン
>> http://www.remlab.net/
>>
>>
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>Thanks,
>Andy
>

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Palmer Dabbelt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3706 bytes --]

On Mon, 22 May 2023 02:58:45 PDT (-0700), remi@remlab.net wrote:
> Hi,
>
> Le 22 mai 2023 11:28:28 GMT+03:00, Andy Chiu <andy.chiu@sifive.com> a écrit :
>>On Sun, May 21, 2023 at 1:41 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>>>
>>>         Hi all,
>>>
>>> Le torstaina 18. toukokuuta 2023 19.19.43 EEST, vous avez écrit :
>>> > This patch add two riscv-specific prctls, to allow usespace control the
>>> > use of vector unit:
>>> >
>>> >  * PR_RISCV_V_SET_CONTROL: control the permission to use Vector at next,
>>> >    or all following execve for a thread. Turning off a thread's Vector
>>> >    live is not possible since libraries may have registered ifunc that
>>> >    may execute Vector instructions.
>>> >  * PR_RISCV_V_GET_CONTROL: get the same permission setting for the
>>> >    current thread, and the setting for following execve(s).
>>>
>>> So far the story was that if the nth bit in the ELF HWCAP auxillary vector was
>>> set, then the nth single lettered extension was supported. There is already
>>> userspace code out there that expects this of the V bit. (I know I have
>>> written such code, and I also know others did likewise.) This is how it
>>> already works for the D and F bits.
>>
>>Yes, the V bit in ELF_HWCAP becomes vague in this series.
>
>
>>> Admittedly, upstream Linux has never ever set that bit to this day. But still,
>>> if we end up with the bit set in a process that has had V support disabled by
>>> the parent (or the sysctl), existing userspace will encounter SIGILL and
>>> break.
>>>
>>> IMO, the bit must be masked not only whence the kernel lacks V support (as
>>> PATCH 02 does), but also if the process starts with V disabled.
>>
>>This is going to change ELF_HWCAP from a macro to a function. The
>>function will turn on COMPAT_HWCAP_ISA_V iff V is supported and
>>allowed. I am going to do this in v21 If this looks sane. i.e.
>>Currently I don't see other architectures which give different
>>ELF_HWCAP values on each execve. If ELF_HWCAP is not a right place to
>>encode the information then userspace has to make the prctl() call to
>>be certain on whether V is usable.
>
> I don't think the value of an auxillary vector entry can change in an existing process nor that we need that. If an application starts with V disabled, you can keep the V bit clear even if V gets enabled later on; that won't break existing userspace code, which simply won't use vectors.
>
> What does break existing userspace is setting the V bit whilst vectors are disabled.

So maybe the right answer is to just not set V at all?  The 
single-letter extensions are sort of defunct now, there's multi-letter 
sub extensions for most things, but V got ratified with those 
sub-extensions so we could just call it extra-ambiguous?

>
>
>
>>
>>>
>>> There are two ways to achieve this:
>>> 1) V is never ever set, and userspace is forced to use hwprobe() instead.
>>> 2) V is set only in processes starting with V enabled (and it's their own
>>> fault if they disabled it in future child threads).
>>
>>The prctl() interface does not allow processes to turn off V once it
>>is enabled in its current (execve) context. The process can only
>>disable V when the next execve() happens. Then, if we implement
>>ELF_HWCAP as mentioned above, the kernel will reload a new HWCAP for
>>the process. By then, the new HWCAP will have V masked since it is not
>>allowed.
>>
>>>
>>> Br,
>>>
>>> --
>>> レミ・デニ-クールモン
>>> http://www.remlab.net/
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>>Thanks,
>>Andy
>>

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Andy Chiu]

How about adding the following 2 patches to deal with the problem.
However, I am concerned if this makes ELF_HWCAP too complicated. e.g.
Future extensions may as well want to do something here after we create
an entry here.

Thanks,
Andy

[-- Attachment #1: patch.diff --]
From: Andy Chiu <andy.chiu@sifive.com>
Date: Wed, 24 May 2023 08:00:11 +0000
Subject: [PATCH 1/2] riscv: hwcap: change ELF_HWCAP to a function

Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 arch/riscv/include/asm/elf.h   | 2 +-
 arch/riscv/include/asm/hwcap.h | 2 ++
 arch/riscv/kernel/cpufeature.c | 5 +++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index ca23c4f6c440..c24280774caf 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -66,7 +66,7 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
  * via a bitmap that coorespends to each single-letter ISA extension.  This is
  * essentially defunct, but will remain for compatibility with userspace.
  */
-#define ELF_HWCAP	(elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1))
+#define ELF_HWCAP	riscv_get_elf_hwcap()
 extern unsigned long elf_hwcap;
 
 /*
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 574385930ba7..e6c288ac4581 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -61,6 +61,8 @@
 
 #include <linux/jump_label.h>
 
+unsigned long riscv_get_elf_hwcap(void);
+
 struct riscv_isa_ext_data {
 	/* Name of the extension displayed to userspace via /proc/cpuinfo */
 	char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 28032b083463..29c0680652a0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -293,6 +293,11 @@ void __init riscv_fill_hwcap(void)
 	pr_info("riscv: ELF capabilities %s\n", print_str);
 }
 
+unsigned long riscv_get_elf_hwcap(void)
+{
+	return (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1));
+}
+
 #ifdef CONFIG_RISCV_ALTERNATIVE
 /*
  * Alternative patch sites consider 48 bits when determining when to patch
-- 
2.17.1

[-- Attachment #2: patch2.diff --]
From: Andy Chiu <andy.chiu@sifive.com>
Date: Wed, 24 May 2023 08:00:11 +0000
Subject: [PATCH 1/2] riscv: hwcap: change ELF_HWCAP to a function

Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 arch/riscv/include/asm/elf.h   | 2 +-
 arch/riscv/include/asm/hwcap.h | 2 ++
 arch/riscv/kernel/cpufeature.c | 5 +++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index ca23c4f6c440..c24280774caf 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -66,7 +66,7 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
  * via a bitmap that coorespends to each single-letter ISA extension.  This is
  * essentially defunct, but will remain for compatibility with userspace.
  */
-#define ELF_HWCAP	(elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1))
+#define ELF_HWCAP	riscv_get_elf_hwcap()
 extern unsigned long elf_hwcap;
 
 /*
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 574385930ba7..e6c288ac4581 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -61,6 +61,8 @@
 
 #include <linux/jump_label.h>
 
+unsigned long riscv_get_elf_hwcap(void);
+
 struct riscv_isa_ext_data {
 	/* Name of the extension displayed to userspace via /proc/cpuinfo */
 	char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 28032b083463..29c0680652a0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -293,6 +293,11 @@ void __init riscv_fill_hwcap(void)
 	pr_info("riscv: ELF capabilities %s\n", print_str);
 }
 
+unsigned long riscv_get_elf_hwcap(void)
+{
+	return (elf_hwcap & ((1UL << RISCV_ISA_EXT_BASE) - 1));
+}
+
 #ifdef CONFIG_RISCV_ALTERNATIVE
 /*
  * Alternative patch sites consider 48 bits when determining when to patch
-- 
2.17.1

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Rémi Denis-Courmont]

Le keskiviikkona 24. toukokuuta 2023, 12.25.30 EEST Andy Chiu a écrit :
> How about adding the following 2 patches to deal with the problem.
> However, I am concerned if this makes ELF_HWCAP too complicated. e.g.
> Future extensions may as well want to do something here after we create
> an entry here.

Fine with me but I feel that we are missing the bigger picture here, as to 
what the usage pattern/guideline for the prctl() on userspace side should be?

Maybe it was already discussed, and I just missed it, but I couldn't find it in 
the documentation.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Andy Chiu]

On Thu, May 25, 2023 at 12:16 AM Rémi Denis-Courmont <remi@remlab.net> wrote:
>
> Le keskiviikkona 24. toukokuuta 2023, 12.25.30 EEST Andy Chiu a écrit :
> > How about adding the following 2 patches to deal with the problem.
> > However, I am concerned if this makes ELF_HWCAP too complicated. e.g.
> > Future extensions may as well want to do something here after we create
> > an entry here.
>
> Fine with me but I feel that we are missing the bigger picture here, as to
> what the usage pattern/guideline for the prctl() on userspace side should be?

Yes, I agree the use of this prctl is only meaningful for the init
system, or some ansenstor processes that make policy decisions. And
with ELF_HWCAP reflecting per-process's availability of V, which is
decided by either the prctl/sysctl, programs/libraries do not really
need to call this prctl unless they are doing aforementioned stuffs.

I am going to document these when rolling out the next revision.

>
> Maybe it was already discussed, and I just missed it, but I couldn't find it in
> the documentation.

We had discussed the potential theoretical abi breakage[1], and came
out with a need of a prctl/sysctl interface for distro's early
userspace programs.

>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
>
>
>

[1]:https://lore.kernel.org/all/mhng-47aa965a-2b25-4ac0-984a-c2e6f3a051ee@palmer-ri-x1c9/

Thanks,
Andy

Re: [PATCH -next v20 20/26] riscv: Add prctl controls for userspace vector management

[Rémi Denis-Courmont]

Le keskiviikkona 24. toukokuuta 2023, 3.18.26 EEST Palmer Dabbelt a écrit :
> > I don't think the value of an auxillary vector entry can change in an
> > existing process nor that we need that. If an application starts with V
> > disabled, you can keep the V bit clear even if V gets enabled later on;
> > that won't break existing userspace code, which simply won't use vectors.
> > 
> > What does break existing userspace is setting the V bit whilst vectors are
> > disabled.
> So maybe the right answer is to just not set V at all?

That is one possibility that I can live, although it feels unnecessarily user-
hostile compared to setting it only if the process _started_ with V enabled.

> The
> single-letter extensions are sort of defunct now, there's multi-letter
> sub extensions for most things, but V got ratified with those
> sub-extensions so we could just call it extra-ambiguous?

Maybe; I must admit I have zero visibility to RVI inner workings. At least C, 
D and F bits could work for JIT use cases, I suppose. E and M are totally 
impractical to support. G, I, X and Z cannot are already wasted by the design, 
and I guess we will now waste all 16 others.

But as for V, what is the user-space story for the prctl()? Who is intended to 
enablet V mode? If there is no clear story, it is all but guaranteed that 
random libraries will call it, and just _blindly_ assume that there is enough 
stack space for signal handling. If so, then there is not much point having a 
prctl() in the first place; might as well stick to just a kernel Kconfig with no 
runtime configuration.

-- 
Rémi Denis-Courmont
http://www.remlab.net/