[bpf-next,2/5] riscv, bpf: Relax restrictions on Zbb instructions

Commit Message

Pu Lehui March 28, 2024, 12:49 p.m. UTC

From: Pu Lehui <[e-mail]>

This patch relaxes the restrictions on the Zbb instructions. The hardware
is capable of recognizing the Zbb instructions independently, eliminating
the need for reliance on kernel compile configurations.

Signed-off-by: Pu Lehui <[e-mail]>
---
 arch/riscv/net/bpf_jit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefan O'Rear March 28, 2024, 7:34 p.m. UTC | #1

On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
> From: Pu Lehui <[e-mail]>
>
> This patch relaxes the restrictions on the Zbb instructions. The hardware
> is capable of recognizing the Zbb instructions independently, eliminating
> the need for reliance on kernel compile configurations.

This doesn't make sense to me.

RISCV_ISA_ZBB is defined as:

           Adds support to dynamically detect the presence of the ZBB
           extension (basic bit manipulation) and enable its usage.

In other words, RISCV_ISA_ZBB=n should disable everything that attempts
to detect Zbb at runtime. It is mostly relevant for code size reduction,
which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
checks can be constant-folded.

If BPF needs to become an exception (why?), this should be mentioned in
Kconfig.

-s

> Signed-off-by: Pu Lehui <[e-mail]>
> ---
>  arch/riscv/net/bpf_jit.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> index 5fc374ed98ea..bcf109b88df5 100644
> --- a/arch/riscv/net/bpf_jit.h
> +++ b/arch/riscv/net/bpf_jit.h
> @@ -20,7 +20,7 @@ static inline bool rvc_enabled(void)
> 
>  static inline bool rvzbb_enabled(void)
>  {
> -	return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && 
> riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
> +	return riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
>  }
> 
>  enum {
> -- 
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley March 28, 2024, 10:07 p.m. UTC | #2

On Thu, Mar 28, 2024 at 03:34:31PM -0400, Stefan O'Rear wrote:
> On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
> > From: Pu Lehui <[e-mail]>
> >
> > This patch relaxes the restrictions on the Zbb instructions. The hardware
> > is capable of recognizing the Zbb instructions independently, eliminating
> > the need for reliance on kernel compile configurations.
> 
> This doesn't make sense to me.

It doesn't make sense to me either. Of course the hardware's capability
to understand an instruction is independent of whether or not a
toolchain is capable of actually emitting the instruction.

> RISCV_ISA_ZBB is defined as:
> 
>            Adds support to dynamically detect the presence of the ZBB
>            extension (basic bit manipulation) and enable its usage.
> 
> In other words, RISCV_ISA_ZBB=n should disable everything that attempts
> to detect Zbb at runtime. It is mostly relevant for code size reduction,
> which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
> checks can be constant-folded.
> 
> If BPF needs to become an exception (why?), this should be mentioned in
> Kconfig.

And in the commit message. On one hand I think this could be a reasonable
thing to do in bpf as it is acting as a jit here, and doesn't actually
need the alternatives that we are using elsewhere to enable the
optimisations nor the compiler support. On the other the intention of
that kconfig option is to control optimisations like rvzbb_enabled()
gates, so this is gonna need a proper justification as to

As I said on IRC to you earlier, I think the Kconfig options here are in
need of a bit of a spring cleaning - they should be modified to explain
their individual purposes, be that enabling optimisations in the kernel
or being required for userspace. I'll try to send a patch for that if
I remember tomorrow.

Thanks,
Conor.

> > Signed-off-by: Pu Lehui <[e-mail]>
> > ---
> >  arch/riscv/net/bpf_jit.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> > index 5fc374ed98ea..bcf109b88df5 100644
> > --- a/arch/riscv/net/bpf_jit.h
> > +++ b/arch/riscv/net/bpf_jit.h
> > @@ -20,7 +20,7 @@ static inline bool rvc_enabled(void)
> > 
> >  static inline bool rvzbb_enabled(void)
> >  {
> > -	return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && 
> > riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
> > +	return riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
> >  }
> > 
> >  enum {
> > -- 
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Pu Lehui March 29, 2024, 10:05 a.m. UTC | #3

On 2024/3/29 6:07, Conor Dooley wrote:
> On Thu, Mar 28, 2024 at 03:34:31PM -0400, Stefan O'Rear wrote:
>> On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
>>> From: Pu Lehui <[e-mail]>
>>>
>>> This patch relaxes the restrictions on the Zbb instructions. The hardware
>>> is capable of recognizing the Zbb instructions independently, eliminating
>>> the need for reliance on kernel compile configurations.
>>
>> This doesn't make sense to me.
> 
> It doesn't make sense to me either. Of course the hardware's capability
> to understand an instruction is independent of whether or not a
> toolchain is capable of actually emitting the instruction.
> 
>> RISCV_ISA_ZBB is defined as:
>>
>>             Adds support to dynamically detect the presence of the ZBB
>>             extension (basic bit manipulation) and enable its usage.
>>
>> In other words, RISCV_ISA_ZBB=n should disable everything that attempts
>> to detect Zbb at runtime. It is mostly relevant for code size reduction,
>> which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
>> checks can be constant-folded.

Thanks for review. My initial thought was the same as yours, but after 
discussions [0] and test verifications, the hardware can indeed 
recognize the zbb instruction even if the kernel has not enabled 
CONFIG_RISCV_ISA_ZBB. As Conor mentioned, we are just acting as a JIT to 
emit zbb instruction here. Maybe is_hw_zbb_capable() will be better?

Link: 
https://lore.kernel.org/bpf/20240129-d06c79a17a5091b3403fc5b6@orel/ [0]

>>
>> If BPF needs to become an exception (why?), this should be mentioned in
>> Kconfig.
> 
> And in the commit message. On one hand I think this could be a reasonable
> thing to do in bpf as it is acting as a jit here, and doesn't actually
> need the alternatives that we are using elsewhere to enable the
> optimisations nor the compiler support. On the other the intention of
> that kconfig option is to control optimisations like rvzbb_enabled()
> gates, so this is gonna need a proper justification as to
> 
> As I said on IRC to you earlier, I think the Kconfig options here are in
> need of a bit of a spring cleaning - they should be modified to explain
> their individual purposes, be that enabling optimisations in the kernel
> or being required for userspace. I'll try to send a patch for that if
> I remember tomorrow.
> 
> Thanks,
> Conor.
> 
>>> Signed-off-by: Pu Lehui <[e-mail]>
>>> ---
>>>   arch/riscv/net/bpf_jit.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
>>> index 5fc374ed98ea..bcf109b88df5 100644
>>> --- a/arch/riscv/net/bpf_jit.h
>>> +++ b/arch/riscv/net/bpf_jit.h
>>> @@ -20,7 +20,7 @@ static inline bool rvc_enabled(void)
>>>
>>>   static inline bool rvzbb_enabled(void)
>>>   {
>>> -	return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) &&
>>> riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
>>> +	return riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
>>>   }
>>>
>>>   enum {
>>> -- 
>>> 2.34.1
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> [e-mail]
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [e-mail]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley March 29, 2024, 11:23 a.m. UTC | #4

On Thu, Mar 28, 2024 at 10:07:23PM +0000, Conor Dooley wrote:

> As I said on IRC to you earlier, I think the Kconfig options here are in
> need of a bit of a spring cleaning - they should be modified to explain
> their individual purposes, be that enabling optimisations in the kernel
> or being required for userspace. I'll try to send a patch for that if
> I remember tomorrow.

Something like this:

-- >8 --
commit 5125504beaedd669b082bf74b02003a77360670f
Author: Conor Dooley <[e-mail]>
Date:   Fri Mar 29 11:13:22 2024 +0000

    RISC-V: clarify what some RISCV_ISA* config options do
    
    During some discussion on IRC yesterday and on Pu's bpf patch [1]
    I noticed that these RISCV_ISA* Kconfig options are not really clear
    about their implications. Many of these options have no impact on what
    userspace is allowed to do, for example an application can use Zbb
    regardless of whether or not the kernel does. Change the help text to
    try and clarify whether or not an option affects just the kernel, or
    also userspace. None of these options actually control whether or not an
    extension is detected dynamically as that's done regardless of Kconfig
    options, so drop any text that implies the option is required for
    dynamic detection, rewording them as "do x when y is detected".
    
    Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
    Signed-off-by: Conor Dooley <[e-mail]>
    ---
    I did this based on top of Samuel's changes dropping the MMU
    requurements just in case, but I don't think there's a conflict:
    https://lore.kernel.org/linux-riscv/[e-mail]/

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d8a777f59402..f327a8ac648f 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -501,8 +501,8 @@ config RISCV_ISA_SVNAPOT
 	depends on RISCV_ALTERNATIVE
 	default y
 	help
-	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
-	  time and enable its usage.
+	  Add support for the Svnapot ISA-extension when it is detected by
+	  the kernel at boot.
 
 	  The Svnapot extension is used to mark contiguous PTEs as a range
 	  of contiguous virtual-to-physical translations for a naturally
@@ -520,9 +520,9 @@ config RISCV_ISA_SVPBMT
 	depends on RISCV_ALTERNATIVE
 	default y
 	help
-	   Adds support to dynamically detect the presence of the Svpbmt
-	   ISA-extension (Supervisor-mode: page-based memory types) and
-	   enable its usage.
+	   Add support for the Svpbmt ISA-extension (Supervisor-mode:
+	   page-based memory types) when it is detected by the kernel at
+	   boot.
 
 	   The memory type for a page contains a combination of attributes
 	   that indicate the cacheability, idempotency, and ordering
@@ -541,14 +541,15 @@ config TOOLCHAIN_HAS_V
 	depends on AS_HAS_OPTION_ARCH
 
 config RISCV_ISA_V
-	bool "VECTOR extension support"
+	bool "Vector extension support"
 	depends on TOOLCHAIN_HAS_V
 	depends on FPU
 	select DYNAMIC_SIGFRAME
 	default y
 	help
 	  Say N here if you want to disable all vector related procedure
-	  in the kernel.
+	  in the kernel. Without this option enabled, neither the kernel nor
+	  userspace may use vector.
 
 	  If you don't know what to do here, say Y.
 
@@ -606,8 +607,8 @@ config RISCV_ISA_ZBB
 	depends on RISCV_ALTERNATIVE
 	default y
 	help
-	   Adds support to dynamically detect the presence of the ZBB
-	   extension (basic bit manipulation) and enable its usage.
+	   Add support for enabling optimisations in the kernel when the
+	   Zbb extension is detected at boot.
 
 	   The Zbb extension provides instructions to accelerate a number
 	   of bit-specific operations (count bit population, sign extending,
@@ -623,9 +624,9 @@ config RISCV_ISA_ZICBOM
 	select RISCV_DMA_NONCOHERENT
 	select DMA_DIRECT_REMAP
 	help
-	   Adds support to dynamically detect the presence of the ZICBOM
-	   extension (Cache Block Management Operations) and enable its
-	   usage.
+	   Add support for the Zicbom extension (Cache Block Management
+	   Operations) and enable its use in the kernel when it is detected
+	   at boot.
 
 	   The Zicbom extension can be used to handle for example
 	   non-coherent DMA support on devices that need it.
@@ -684,7 +685,8 @@ config FPU
 	default y
 	help
 	  Say N here if you want to disable all floating-point related procedure
-	  in the kernel.
+	  in the kernel. Without this option enabled, neither the kernel nor
+	  userspace may use vector.
 
 	  If you don't know what to do here, say Y.

Pu Lehui March 30, 2024, 10:19 a.m. UTC | #5

Thanks for the clarification, looks good.

On 2024/3/29 19:23, Conor Dooley wrote:
> On Thu, Mar 28, 2024 at 10:07:23PM +0000, Conor Dooley wrote:
> 
>> As I said on IRC to you earlier, I think the Kconfig options here are in
>> need of a bit of a spring cleaning - they should be modified to explain
>> their individual purposes, be that enabling optimisations in the kernel
>> or being required for userspace. I'll try to send a patch for that if
>> I remember tomorrow.
> 
> Something like this:
> 
> -- >8 --
> commit 5125504beaedd669b082bf74b02003a77360670f
> Author: Conor Dooley <[e-mail]>
> Date:   Fri Mar 29 11:13:22 2024 +0000
> 
>      RISC-V: clarify what some RISCV_ISA* config options do
>      
>      During some discussion on IRC yesterday and on Pu's bpf patch [1]
>      I noticed that these RISCV_ISA* Kconfig options are not really clear
>      about their implications. Many of these options have no impact on what
>      userspace is allowed to do, for example an application can use Zbb
>      regardless of whether or not the kernel does. Change the help text to
>      try and clarify whether or not an option affects just the kernel, or
>      also userspace. None of these options actually control whether or not an
>      extension is detected dynamically as that's done regardless of Kconfig
>      options, so drop any text that implies the option is required for
>      dynamic detection, rewording them as "do x when y is detected".
>      
>      Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
>      Signed-off-by: Conor Dooley <[e-mail]>
>      ---
>      I did this based on top of Samuel's changes dropping the MMU
>      requurements just in case, but I don't think there's a conflict:
>      https://lore.kernel.org/linux-riscv/[e-mail]/
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d8a777f59402..f327a8ac648f 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -501,8 +501,8 @@ config RISCV_ISA_SVNAPOT
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	help
> -	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
> -	  time and enable its usage.
> +	  Add support for the Svnapot ISA-extension when it is detected by
> +	  the kernel at boot.
>   
>   	  The Svnapot extension is used to mark contiguous PTEs as a range
>   	  of contiguous virtual-to-physical translations for a naturally
> @@ -520,9 +520,9 @@ config RISCV_ISA_SVPBMT
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	help
> -	   Adds support to dynamically detect the presence of the Svpbmt
> -	   ISA-extension (Supervisor-mode: page-based memory types) and
> -	   enable its usage.
> +	   Add support for the Svpbmt ISA-extension (Supervisor-mode:
> +	   page-based memory types) when it is detected by the kernel at
> +	   boot.
>   
>   	   The memory type for a page contains a combination of attributes
>   	   that indicate the cacheability, idempotency, and ordering
> @@ -541,14 +541,15 @@ config TOOLCHAIN_HAS_V
>   	depends on AS_HAS_OPTION_ARCH
>   
>   config RISCV_ISA_V
> -	bool "VECTOR extension support"
> +	bool "Vector extension support"
>   	depends on TOOLCHAIN_HAS_V
>   	depends on FPU
>   	select DYNAMIC_SIGFRAME
>   	default y
>   	help
>   	  Say N here if you want to disable all vector related procedure
> -	  in the kernel.
> +	  in the kernel. Without this option enabled, neither the kernel nor
> +	  userspace may use vector.
>   
>   	  If you don't know what to do here, say Y.
>   
> @@ -606,8 +607,8 @@ config RISCV_ISA_ZBB
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	help
> -	   Adds support to dynamically detect the presence of the ZBB
> -	   extension (basic bit manipulation) and enable its usage.
> +	   Add support for enabling optimisations in the kernel when the
> +	   Zbb extension is detected at boot.
>   
>   	   The Zbb extension provides instructions to accelerate a number
>   	   of bit-specific operations (count bit population, sign extending,
> @@ -623,9 +624,9 @@ config RISCV_ISA_ZICBOM
>   	select RISCV_DMA_NONCOHERENT
>   	select DMA_DIRECT_REMAP
>   	help
> -	   Adds support to dynamically detect the presence of the ZICBOM
> -	   extension (Cache Block Management Operations) and enable its
> -	   usage.
> +	   Add support for the Zicbom extension (Cache Block Management
> +	   Operations) and enable its use in the kernel when it is detected
> +	   at boot.
>   
>   	   The Zicbom extension can be used to handle for example
>   	   non-coherent DMA support on devices that need it.
> @@ -684,7 +685,8 @@ config FPU
>   	default y
>   	help
>   	  Say N here if you want to disable all floating-point related procedure
> -	  in the kernel.
> +	  in the kernel. Without this option enabled, neither the kernel nor
> +	  userspace may use vector.
>   
>   	  If you don't know what to do here, say Y.
>   
>

Samuel Holland March 31, 2024, 5:49 p.m. UTC | #6

Hi Conor,

Looks good except for one typo.

On 2024-03-29 6:23 AM, Conor Dooley wrote:
> On Thu, Mar 28, 2024 at 10:07:23PM +0000, Conor Dooley wrote:
> 
>> As I said on IRC to you earlier, I think the Kconfig options here are in
>> need of a bit of a spring cleaning - they should be modified to explain
>> their individual purposes, be that enabling optimisations in the kernel
>> or being required for userspace. I'll try to send a patch for that if
>> I remember tomorrow.
> 
> Something like this:
> 
> -- >8 --
> commit 5125504beaedd669b082bf74b02003a77360670f
> Author: Conor Dooley <[e-mail]>
> Date:   Fri Mar 29 11:13:22 2024 +0000
> 
>     RISC-V: clarify what some RISCV_ISA* config options do
>     
>     During some discussion on IRC yesterday and on Pu's bpf patch [1]
>     I noticed that these RISCV_ISA* Kconfig options are not really clear
>     about their implications. Many of these options have no impact on what
>     userspace is allowed to do, for example an application can use Zbb
>     regardless of whether or not the kernel does. Change the help text to
>     try and clarify whether or not an option affects just the kernel, or
>     also userspace. None of these options actually control whether or not an
>     extension is detected dynamically as that's done regardless of Kconfig
>     options, so drop any text that implies the option is required for
>     dynamic detection, rewording them as "do x when y is detected".
>     
>     Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
>     Signed-off-by: Conor Dooley <[e-mail]>
>     ---
>     I did this based on top of Samuel's changes dropping the MMU
>     requurements just in case, but I don't think there's a conflict:
>     https://lore.kernel.org/linux-riscv/[e-mail]/
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d8a777f59402..f327a8ac648f 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -501,8 +501,8 @@ config RISCV_ISA_SVNAPOT
>  	depends on RISCV_ALTERNATIVE
>  	default y
>  	help
> -	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
> -	  time and enable its usage.
> +	  Add support for the Svnapot ISA-extension when it is detected by
> +	  the kernel at boot.
>  
>  	  The Svnapot extension is used to mark contiguous PTEs as a range
>  	  of contiguous virtual-to-physical translations for a naturally
> @@ -520,9 +520,9 @@ config RISCV_ISA_SVPBMT
>  	depends on RISCV_ALTERNATIVE
>  	default y
>  	help
> -	   Adds support to dynamically detect the presence of the Svpbmt
> -	   ISA-extension (Supervisor-mode: page-based memory types) and
> -	   enable its usage.
> +	   Add support for the Svpbmt ISA-extension (Supervisor-mode:
> +	   page-based memory types) when it is detected by the kernel at
> +	   boot.
>  
>  	   The memory type for a page contains a combination of attributes
>  	   that indicate the cacheability, idempotency, and ordering
> @@ -541,14 +541,15 @@ config TOOLCHAIN_HAS_V
>  	depends on AS_HAS_OPTION_ARCH
>  
>  config RISCV_ISA_V
> -	bool "VECTOR extension support"
> +	bool "Vector extension support"
>  	depends on TOOLCHAIN_HAS_V
>  	depends on FPU
>  	select DYNAMIC_SIGFRAME
>  	default y
>  	help
>  	  Say N here if you want to disable all vector related procedure
> -	  in the kernel.
> +	  in the kernel. Without this option enabled, neither the kernel nor
> +	  userspace may use vector.
>  
>  	  If you don't know what to do here, say Y.
>  
> @@ -606,8 +607,8 @@ config RISCV_ISA_ZBB
>  	depends on RISCV_ALTERNATIVE
>  	default y
>  	help
> -	   Adds support to dynamically detect the presence of the ZBB
> -	   extension (basic bit manipulation) and enable its usage.
> +	   Add support for enabling optimisations in the kernel when the
> +	   Zbb extension is detected at boot.
>  
>  	   The Zbb extension provides instructions to accelerate a number
>  	   of bit-specific operations (count bit population, sign extending,
> @@ -623,9 +624,9 @@ config RISCV_ISA_ZICBOM
>  	select RISCV_DMA_NONCOHERENT
>  	select DMA_DIRECT_REMAP
>  	help
> -	   Adds support to dynamically detect the presence of the ZICBOM
> -	   extension (Cache Block Management Operations) and enable its
> -	   usage.
> +	   Add support for the Zicbom extension (Cache Block Management
> +	   Operations) and enable its use in the kernel when it is detected
> +	   at boot.
>  
>  	   The Zicbom extension can be used to handle for example
>  	   non-coherent DMA support on devices that need it.
> @@ -684,7 +685,8 @@ config FPU
>  	default y
>  	help
>  	  Say N here if you want to disable all floating-point related procedure
> -	  in the kernel.
> +	  in the kernel. Without this option enabled, neither the kernel nor
> +	  userspace may use vector.

s/vector/floating point/ here.

Regards,
Samuel

>  
>  	  If you don't know what to do here, say Y.
>  
> 
> 
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Björn Töpel April 2, 2024, 2:18 p.m. UTC | #7

Conor Dooley <[e-mail]> writes:

> On Thu, Mar 28, 2024 at 10:07:23PM +0000, Conor Dooley wrote:
>
>> As I said on IRC to you earlier, I think the Kconfig options here are in
>> need of a bit of a spring cleaning - they should be modified to explain
>> their individual purposes, be that enabling optimisations in the kernel
>> or being required for userspace. I'll try to send a patch for that if
>> I remember tomorrow.
>
> Something like this:
>
> -- >8 --
> commit 5125504beaedd669b082bf74b02003a77360670f
> Author: Conor Dooley <[e-mail]>
> Date:   Fri Mar 29 11:13:22 2024 +0000
>
>     RISC-V: clarify what some RISCV_ISA* config options do
>     
>     During some discussion on IRC yesterday and on Pu's bpf patch [1]
>     I noticed that these RISCV_ISA* Kconfig options are not really clear
>     about their implications. Many of these options have no impact on what
>     userspace is allowed to do, for example an application can use Zbb
>     regardless of whether or not the kernel does. Change the help text to
>     try and clarify whether or not an option affects just the kernel, or
>     also userspace. None of these options actually control whether or not an
>     extension is detected dynamically as that's done regardless of Kconfig
>     options, so drop any text that implies the option is required for
>     dynamic detection, rewording them as "do x when y is detected".
>     
>     Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
>     Signed-off-by: Conor Dooley <[e-mail]>
>     ---
>     I did this based on top of Samuel's changes dropping the MMU
>     requurements just in case, but I don't think there's a conflict:
>     https://lore.kernel.org/linux-riscv/[e-mail]/
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d8a777f59402..f327a8ac648f 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -501,8 +501,8 @@ config RISCV_ISA_SVNAPOT
>  	depends on RISCV_ALTERNATIVE
>  	default y
>  	help
> -	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
> -	  time and enable its usage.
> +	  Add support for the Svnapot ISA-extension when it is detected by
> +	  the kernel at boot.
>  
>  	  The Svnapot extension is used to mark contiguous PTEs as a range
>  	  of contiguous virtual-to-physical translations for a naturally
> @@ -520,9 +520,9 @@ config RISCV_ISA_SVPBMT
>  	depends on RISCV_ALTERNATIVE
>  	default y
>  	help
> -	   Adds support to dynamically detect the presence of the Svpbmt
> -	   ISA-extension (Supervisor-mode: page-based memory types) and
> -	   enable its usage.
> +	   Add support for the Svpbmt ISA-extension (Supervisor-mode:
> +	   page-based memory types) when it is detected by the kernel at
> +	   boot.
>  
>  	   The memory type for a page contains a combination of attributes
>  	   that indicate the cacheability, idempotency, and ordering
> @@ -541,14 +541,15 @@ config TOOLCHAIN_HAS_V
>  	depends on AS_HAS_OPTION_ARCH
>  
>  config RISCV_ISA_V
> -	bool "VECTOR extension support"
> +	bool "Vector extension support"
>  	depends on TOOLCHAIN_HAS_V
>  	depends on FPU
>  	select DYNAMIC_SIGFRAME
>  	default y
>  	help
>  	  Say N here if you want to disable all vector related procedure
> -	  in the kernel.
> +	  in the kernel. Without this option enabled, neither the kernel nor
> +	  userspace may use vector.
>  
>  	  If you don't know what to do here, say Y.
>  
> @@ -606,8 +607,8 @@ config RISCV_ISA_ZBB
>  	depends on RISCV_ALTERNATIVE
>  	default y
>  	help
> -	   Adds support to dynamically detect the presence of the ZBB
> -	   extension (basic bit manipulation) and enable its usage.
> +	   Add support for enabling optimisations in the kernel when the

I don't care much, but "optimizations" here -- for consistency reasons!
[1] ;-)

Nice change!

Reviewed-by: Björn Töpel <[e-mail]>

[1] https://lwn.net/Articles/636286/

Björn Töpel April 2, 2024, 2:25 p.m. UTC | #8

Pu Lehui <[e-mail]> writes:

> On 2024/3/29 6:07, Conor Dooley wrote:
>> On Thu, Mar 28, 2024 at 03:34:31PM -0400, Stefan O'Rear wrote:
>>> On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
>>>> From: Pu Lehui <[e-mail]>
>>>>
>>>> This patch relaxes the restrictions on the Zbb instructions. The hardware
>>>> is capable of recognizing the Zbb instructions independently, eliminating
>>>> the need for reliance on kernel compile configurations.
>>>
>>> This doesn't make sense to me.
>> 
>> It doesn't make sense to me either. Of course the hardware's capability
>> to understand an instruction is independent of whether or not a
>> toolchain is capable of actually emitting the instruction.
>> 
>>> RISCV_ISA_ZBB is defined as:
>>>
>>>             Adds support to dynamically detect the presence of the ZBB
>>>             extension (basic bit manipulation) and enable its usage.
>>>
>>> In other words, RISCV_ISA_ZBB=n should disable everything that attempts
>>> to detect Zbb at runtime. It is mostly relevant for code size reduction,
>>> which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
>>> checks can be constant-folded.
>
> Thanks for review. My initial thought was the same as yours, but after 
> discussions [0] and test verifications, the hardware can indeed 
> recognize the zbb instruction even if the kernel has not enabled 
> CONFIG_RISCV_ISA_ZBB. As Conor mentioned, we are just acting as a JIT to 
> emit zbb instruction here. Maybe is_hw_zbb_capable() will be better?

I still think Lehui's patch is correct; Building a kernel that can boot
on multiple platforms (w/ or w/o Zbb support) and not having Zbb insn in
the kernel proper, and iff Zbb is available at run-time the BPF JIT will
emit Zbb.

For these kind of optimizations, (IMO) it's better to let the BPF JIT
decide at run-time.


Björn

Björn Töpel April 2, 2024, 2:27 p.m. UTC | #9

Pu Lehui <[e-mail]> writes:

> From: Pu Lehui <[e-mail]>
>
> This patch relaxes the restrictions on the Zbb instructions. The hardware
> is capable of recognizing the Zbb instructions independently, eliminating
> the need for reliance on kernel compile configurations.
>
> Signed-off-by: Pu Lehui <[e-mail]>

Should this patch really be part of this series?


Björn

Daniel Borkmann April 2, 2024, 4:03 p.m. UTC | #10

On 4/2/24 4:27 PM, Björn Töpel wrote:
> Pu Lehui <[e-mail]> writes:
>> From: Pu Lehui <[e-mail]>
>>
>> This patch relaxes the restrictions on the Zbb instructions. The hardware
>> is capable of recognizing the Zbb instructions independently, eliminating
>> the need for reliance on kernel compile configurations.
>>
>> Signed-off-by: Pu Lehui <[e-mail]>
> 
> Should this patch really be part of this series?

No, that should be submitted independently.

Also, given Eduard's comment, it would be great if you could add the
instructions to tools/testing/selftests/bpf/README.rst even if not in
a perfect shape, but it would give developers a chance to get started.

Thanks,
Daniel

Conor Dooley April 2, 2024, 5:38 p.m. UTC | #11

On Tue, Apr 02, 2024 at 04:25:24PM +0200, Björn Töpel wrote:
> Pu Lehui <[e-mail]> writes:
> 
> > On 2024/3/29 6:07, Conor Dooley wrote:
> >> On Thu, Mar 28, 2024 at 03:34:31PM -0400, Stefan O'Rear wrote:
> >>> On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
> >>>> From: Pu Lehui <[e-mail]>
> >>>>
> >>>> This patch relaxes the restrictions on the Zbb instructions. The hardware
> >>>> is capable of recognizing the Zbb instructions independently, eliminating
> >>>> the need for reliance on kernel compile configurations.
> >>>
> >>> This doesn't make sense to me.
> >> 
> >> It doesn't make sense to me either. Of course the hardware's capability
> >> to understand an instruction is independent of whether or not a
> >> toolchain is capable of actually emitting the instruction.
> >> 
> >>> RISCV_ISA_ZBB is defined as:
> >>>
> >>>             Adds support to dynamically detect the presence of the ZBB
> >>>             extension (basic bit manipulation) and enable its usage.
> >>>
> >>> In other words, RISCV_ISA_ZBB=n should disable everything that attempts
> >>> to detect Zbb at runtime. It is mostly relevant for code size reduction,
> >>> which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
> >>> checks can be constant-folded.
> >
> > Thanks for review. My initial thought was the same as yours, but after 
> > discussions [0] and test verifications, the hardware can indeed 
> > recognize the zbb instruction even if the kernel has not enabled 
> > CONFIG_RISCV_ISA_ZBB. As Conor mentioned, we are just acting as a JIT to 
> > emit zbb instruction here. Maybe is_hw_zbb_capable() will be better?
> 
> I still think Lehui's patch is correct; Building a kernel that can boot
> on multiple platforms (w/ or w/o Zbb support) and not having Zbb insn in
> the kernel proper, and iff Zbb is available at run-time the BPF JIT will
> emit Zbb.

This sentence is -ENOPARSE to me, did you accidentally omit some words?
Additionally he config option has nothing to do with building kernels that
boot on multiple platforms, it only controls whether optimisations for Zbb
are built so that if Zbb is detected they can be used.

> For these kind of optimizations, (IMO) it's better to let the BPF JIT
> decide at run-time.

Why is bpf a different case to any other user in this regard?
I think that the commit message is misleading and needs to be changed,
because the point "the hardware is capable of recognising the Zbb
instructions independently..." is completely unrelated to the purpose
of the config option. Of course the hardware understanding the option
has nothing to do with kernel configuration. The commit message needs to
explain why bpf is a special case and is exempt from an 

I totally understand any point about bpf being different in terms of
needing toolchain support, but IIRC it was I who pointed out up-thread.
The part of the conversation that you're replying to here is about the
semantics of the Kconfig option and the original patch never mentioned
trying to avoid a dependency on toolchains at all, just kernel
configurations. The toolchain requirements I don't think are even super
hard to fulfill either - the last 3 versions of ld and lld all meet the
criteria.

Björn Töpel April 2, 2024, 7 p.m. UTC | #12

Hey Conor!

Conor Dooley <[e-mail]> writes:

> On Tue, Apr 02, 2024 at 04:25:24PM +0200, Björn Töpel wrote:
>> Pu Lehui <[e-mail]> writes:
>> 
>> > On 2024/3/29 6:07, Conor Dooley wrote:
>> >> On Thu, Mar 28, 2024 at 03:34:31PM -0400, Stefan O'Rear wrote:
>> >>> On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
>> >>>> From: Pu Lehui <[e-mail]>
>> >>>>
>> >>>> This patch relaxes the restrictions on the Zbb instructions. The hardware
>> >>>> is capable of recognizing the Zbb instructions independently, eliminating
>> >>>> the need for reliance on kernel compile configurations.
>> >>>
>> >>> This doesn't make sense to me.
>> >> 
>> >> It doesn't make sense to me either. Of course the hardware's capability
>> >> to understand an instruction is independent of whether or not a
>> >> toolchain is capable of actually emitting the instruction.
>> >> 
>> >>> RISCV_ISA_ZBB is defined as:
>> >>>
>> >>>             Adds support to dynamically detect the presence of the ZBB
>> >>>             extension (basic bit manipulation) and enable its usage.
>> >>>
>> >>> In other words, RISCV_ISA_ZBB=n should disable everything that attempts
>> >>> to detect Zbb at runtime. It is mostly relevant for code size reduction,
>> >>> which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
>> >>> checks can be constant-folded.
>> >
>> > Thanks for review. My initial thought was the same as yours, but after 
>> > discussions [0] and test verifications, the hardware can indeed 
>> > recognize the zbb instruction even if the kernel has not enabled 
>> > CONFIG_RISCV_ISA_ZBB. As Conor mentioned, we are just acting as a JIT to 
>> > emit zbb instruction here. Maybe is_hw_zbb_capable() will be better?
>> 
>> I still think Lehui's patch is correct; Building a kernel that can boot
>> on multiple platforms (w/ or w/o Zbb support) and not having Zbb insn in
>> the kernel proper, and iff Zbb is available at run-time the BPF JIT will
>> emit Zbb.
>
> This sentence is -ENOPARSE to me, did you accidentally omit some words?
> Additionally he config option has nothing to do with building kernels that
> boot on multiple platforms, it only controls whether optimisations for Zbb
> are built so that if Zbb is detected they can be used.

Ugh, sorry about that! I'm probably confused myself.

>> For these kind of optimizations, (IMO) it's better to let the BPF JIT
>> decide at run-time.
>
> Why is bpf a different case to any other user in this regard?
> I think that the commit message is misleading and needs to be changed,
> because the point "the hardware is capable of recognising the Zbb
> instructions independently..." is completely unrelated to the purpose
> of the config option. Of course the hardware understanding the option
> has nothing to do with kernel configuration. The commit message needs to
> explain why bpf is a special case and is exempt from an 
>
> I totally understand any point about bpf being different in terms of
> needing toolchain support, but IIRC it was I who pointed out up-thread.
> The part of the conversation that you're replying to here is about the
> semantics of the Kconfig option and the original patch never mentioned
> trying to avoid a dependency on toolchains at all, just kernel
> configurations. The toolchain requirements I don't think are even super
> hard to fulfill either - the last 3 versions of ld and lld all meet the
> criteria.

Thanks for making it more clear, and I agree that the toolchain
requirements are not hard to fulfull.

My view has been that "BPF is like userland", but I realize now that's
odd. Let's make BPF similar to the rest of the RV kernel. If ZBB=n, then
the BPF JIT doesn't know about emitting Zbb.


Björn

Conor Dooley April 3, 2024, 1:20 a.m. UTC | #13

On Tue, Apr 02, 2024 at 09:00:45PM +0200, Björn Töpel wrote:

> >> I still think Lehui's patch is correct; Building a kernel that can boot
> >> on multiple platforms (w/ or w/o Zbb support) and not having Zbb insn in
> >> the kernel proper, and iff Zbb is available at run-time the BPF JIT will
> >> emit Zbb.
> >
> > This sentence is -ENOPARSE to me, did you accidentally omit some words?
> > Additionally he config option has nothing to do with building kernels that
> > boot on multiple platforms, it only controls whether optimisations for Zbb
> > are built so that if Zbb is detected they can be used.
> 
> Ugh, sorry about that! I'm probably confused myself.

Reading this back, I a bunch of words too, so no worries...

> >> For these kind of optimizations, (IMO) it's better to let the BPF JIT
> >> decide at run-time.
> >
> > Why is bpf a different case to any other user in this regard?
> > I think that the commit message is misleading and needs to be changed,
> > because the point "the hardware is capable of recognising the Zbb
> > instructions independently..." is completely unrelated to the purpose
> > of the config option. Of course the hardware understanding the option

This should have been "understanding the instructions"...

> > has nothing to do with kernel configuration. The commit message needs to
> > explain why bpf is a special case and is exempt from an

And this s/from an//...

> > I totally understand any point about bpf being different in terms of
> > needing toolchain support, but IIRC it was I who pointed out up-thread.

And "pointed that out".

I always make a mess of these emails that I re-write several times :)

> > The part of the conversation that you're replying to here is about the
> > semantics of the Kconfig option and the original patch never mentioned
> > trying to avoid a dependency on toolchains at all, just kernel
> > configurations. The toolchain requirements I don't think are even super
> > hard to fulfill either - the last 3 versions of ld and lld all meet the
> > criteria.
> 
> Thanks for making it more clear, and I agree that the toolchain
> requirements are not hard to fulfull.
> 
> My view has been that "BPF is like userland", but I realize now that's
> odd.

Yeah, I can understand that perspective, but it does seem rather odd to
someone that isn't a bpf-ist.

> Let's make BPF similar to the rest of the RV kernel. If ZBB=n, then
> the BPF JIT doesn't know about emitting Zbb.

Pu Lehui April 3, 2024, 10:05 a.m. UTC | #14

On 2024/4/3 9:20, Conor Dooley wrote:
> On Tue, Apr 02, 2024 at 09:00:45PM +0200, Björn Töpel wrote:
> 
>>>> I still think Lehui's patch is correct; Building a kernel that can boot
>>>> on multiple platforms (w/ or w/o Zbb support) and not having Zbb insn in
>>>> the kernel proper, and iff Zbb is available at run-time the BPF JIT will
>>>> emit Zbb.
>>>
>>> This sentence is -ENOPARSE to me, did you accidentally omit some words?
>>> Additionally he config option has nothing to do with building kernels that
>>> boot on multiple platforms, it only controls whether optimisations for Zbb
>>> are built so that if Zbb is detected they can be used.
>>
>> Ugh, sorry about that! I'm probably confused myself.
> 
> Reading this back, I a bunch of words too, so no worries...
> 
>>>> For these kind of optimizations, (IMO) it's better to let the BPF JIT
>>>> decide at run-time.
>>>
>>> Why is bpf a different case to any other user in this regard?
>>> I think that the commit message is misleading and needs to be changed,
>>> because the point "the hardware is capable of recognising the Zbb
>>> instructions independently..." is completely unrelated to the purpose
>>> of the config option. Of course the hardware understanding the option
> 
> This should have been "understanding the instructions"...
> 
>>> has nothing to do with kernel configuration. The commit message needs to
>>> explain why bpf is a special case and is exempt from an
> 
> And this s/from an//...
> 
>>> I totally understand any point about bpf being different in terms of
>>> needing toolchain support, but IIRC it was I who pointed out up-thread.
> 
> And "pointed that out".
> 
> I always make a mess of these emails that I re-write several times :)
> 
>>> The part of the conversation that you're replying to here is about the
>>> semantics of the Kconfig option and the original patch never mentioned
>>> trying to avoid a dependency on toolchains at all, just kernel
>>> configurations. The toolchain requirements I don't think are even super
>>> hard to fulfill either - the last 3 versions of ld and lld all meet the
>>> criteria.
>>
>> Thanks for making it more clear, and I agree that the toolchain
>> requirements are not hard to fulfull.
>>
>> My view has been that "BPF is like userland", but I realize now that's
>> odd.
> 
> Yeah, I can understand that perspective, but it does seem rather odd to
> someone that isn't a bpf-ist.
> 
>> Let's make BPF similar to the rest of the RV kernel. If ZBB=n, then
>> the BPF JIT doesn't know about emitting Zbb.
> 

Hi Conor and Björn,

Thanks for your explanation. I totally agree with what you said, 
"CONFIG_RISCV_ISA_ZBB only controls whether optimizations for Zbb are 
built so that if Zbb is detected they can be used.".

Since the instructions emited by bpf jit are in kernel space, they 
should indeed be aligned in this regard.

PS: It's a bit difficult to understand this,

Pu Lehui April 3, 2024, 10:19 a.m. UTC | #15

On 2024/4/3 0:03, Daniel Borkmann wrote:
> On 4/2/24 4:27 PM, Björn Töpel wrote:
>> Pu Lehui <[e-mail]> writes:
>>> From: Pu Lehui <[e-mail]>
>>>
>>> This patch relaxes the restrictions on the Zbb instructions. The 
>>> hardware
>>> is capable of recognizing the Zbb instructions independently, 
>>> eliminating
>>> the need for reliance on kernel compile configurations.
>>>
>>> Signed-off-by: Pu Lehui <[e-mail]>
>>
>> Should this patch really be part of this series?
> 
> No, that should be submitted independently.
> 

My initial thought was that I didn't enable CONFIG_RISCV_ISA_ZBB in 
config.riscv64, so I should loosen this restriction to enable zbb 
optimization. It could not be part of this series.

By the way, after reading what Conor and Björn said, I think we should 
align with kernel sematic, that is, emit zbb when CONFIG_RISCV_ISA_ZBB 
is enabled and so that if Zbb is detected they can be used.

> Also, given Eduard's comment, it would be great if you could add the
> instructions to tools/testing/selftests/bpf/README.rst even if not in
> a perfect shape, but it would give developers a chance to get started.
> 
> Thanks,
> Daniel

Conor Dooley April 3, 2024, 12:29 p.m. UTC | #16

On Wed, Apr 03, 2024 at 06:05:38PM +0800, Pu Lehui wrote:
> Hi Conor and Björn,
> 
> Thanks for your explanation. I totally agree with what you said,
> "CONFIG_RISCV_ISA_ZBB only controls whether optimizations for Zbb are built
> so that if Zbb is detected they can be used.".
> 
> Since the instructions emited by bpf jit are in kernel space, they should
> indeed be aligned in this regard.
> 
> PS: It's a bit difficult to understand this,

Patch

diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 5fc374ed98ea..bcf109b88df5 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -20,7 +20,7 @@  static inline bool rvc_enabled(void)
 
 static inline bool rvzbb_enabled(void)
 {
-	return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
+	return riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
 }
 
 enum {