diff mbox series

[v4] arch/riscv: add Zihintpause support

Message ID 20220620201530.3929352-1-daolu@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series [v4] arch/riscv: add Zihintpause support | expand

Commit Message

Dao Lu June 20, 2022, 8:15 p.m. UTC
Implement support for the ZiHintPause extension.

The PAUSE instruction is a HINT that indicates the current hart’s rate
of instruction retirement should be temporarily reduced or paused.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Dao Lu <daolu@rivosinc.com>
---

v1 -> v2:
 Remove the usage of static branch, use PAUSE if toolchain supports it
v2 -> v3:
 Added the static branch back, cpu_relax() behavior is kept the same for
systems that do not support ZiHintPause
v3 -> v4:
 Adopted the newly added unified static keys for extensions
---
 arch/riscv/Makefile                     |  4 ++++
 arch/riscv/include/asm/hwcap.h          |  5 +++++
 arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
 arch/riscv/kernel/cpu.c                 |  1 +
 arch/riscv/kernel/cpufeature.c          |  1 +
 5 files changed, 29 insertions(+), 3 deletions(-)

Comments

Dao Lu July 5, 2022, 4:57 p.m. UTC | #1
ping

On Mon, Jun 20, 2022 at 1:15 PM Dao Lu <daolu@rivosinc.com> wrote:
>
> Implement support for the ZiHintPause extension.
>
> The PAUSE instruction is a HINT that indicates the current hart’s rate
> of instruction retirement should be temporarily reduced or paused.
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Dao Lu <daolu@rivosinc.com>
> ---
>
> v1 -> v2:
>  Remove the usage of static branch, use PAUSE if toolchain supports it
> v2 -> v3:
>  Added the static branch back, cpu_relax() behavior is kept the same for
> systems that do not support ZiHintPause
> v3 -> v4:
>  Adopted the newly added unified static keys for extensions
> ---
>  arch/riscv/Makefile                     |  4 ++++
>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>  arch/riscv/kernel/cpu.c                 |  1 +
>  arch/riscv/kernel/cpufeature.c          |  1 +
>  5 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 34cf8a598617..6ddacc6f44b9 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)    := $(riscv-march-y)c
>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>
> +# Check if the toolchain supports Zihintpause extension
> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> +
>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e48eebdd2631..dc47019a0b38 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -8,6 +8,7 @@
>  #ifndef _ASM_RISCV_HWCAP_H
>  #define _ASM_RISCV_HWCAP_H
>
> +#include <asm/errno.h>
>  #include <linux/bits.h>
>  #include <uapi/asm/hwcap.h>
>
> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>  enum riscv_isa_ext_id {
>         RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>         RISCV_ISA_EXT_SVPBMT,
> +       RISCV_ISA_EXT_ZIHINTPAUSE,
>         RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>  };
>
> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>   */
>  enum riscv_isa_ext_key {
>         RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
> +       RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>         RISCV_ISA_EXT_KEY_MAX,
>  };
>
> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>                 return RISCV_ISA_EXT_KEY_FPU;
>         case RISCV_ISA_EXT_d:
>                 return RISCV_ISA_EXT_KEY_FPU;
> +       case RISCV_ISA_EXT_ZIHINTPAUSE:
> +               return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>         default:
>                 return -EINVAL;
>         }
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 134388cbaaa1..1e4f8b4aef79 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -4,15 +4,30 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include <linux/jump_label.h>
>  #include <asm/barrier.h>
> +#include <asm/hwcap.h>
>
>  static inline void cpu_relax(void)
>  {
> +       if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>  #ifdef __riscv_muldiv
> -       int dummy;
> -       /* In lieu of a halt instruction, induce a long-latency stall. */
> -       __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> +               int dummy;
> +               /* In lieu of a halt instruction, induce a long-latency stall. */
> +               __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>  #endif
> +       } else {
> +               /*
> +                * Reduce instruction retirement.
> +                * This assumes the PC changes.
> +                */
> +#ifdef __riscv_zihintpause
> +               __asm__ __volatile__ ("pause");
> +#else
> +               /* Encoding of the pause instruction */
> +               __asm__ __volatile__ (".4byte 0x100000F");
> +#endif
> +       }
>         barrier();
>  }
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index fba9e9f46a8c..a123e92b14dd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>         __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>         __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> +       __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>         __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>  };
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1b3ec44e25f5..708df2c0bc34 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>                         } else {
>                                 SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>                                 SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> +                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>                         }
>  #undef SET_ISA_EXT_MAP
>                 }
> --
> 2.25.1
>
Guo Ren July 5, 2022, 11:47 p.m. UTC | #2
Reviewed-by: Guo Ren <guoren@kernel.org>

On Wed, Jul 6, 2022 at 12:57 AM Dao Lu <daolu@rivosinc.com> wrote:
>
> ping
>
> On Mon, Jun 20, 2022 at 1:15 PM Dao Lu <daolu@rivosinc.com> wrote:
> >
> > Implement support for the ZiHintPause extension.
> >
> > The PAUSE instruction is a HINT that indicates the current hart’s rate
> > of instruction retirement should be temporarily reduced or paused.
> >
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > Signed-off-by: Dao Lu <daolu@rivosinc.com>
> > ---
> >
> > v1 -> v2:
> >  Remove the usage of static branch, use PAUSE if toolchain supports it
> > v2 -> v3:
> >  Added the static branch back, cpu_relax() behavior is kept the same for
> > systems that do not support ZiHintPause
> > v3 -> v4:
> >  Adopted the newly added unified static keys for extensions
> > ---
> >  arch/riscv/Makefile                     |  4 ++++
> >  arch/riscv/include/asm/hwcap.h          |  5 +++++
> >  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
> >  arch/riscv/kernel/cpu.c                 |  1 +
> >  arch/riscv/kernel/cpufeature.c          |  1 +
> >  5 files changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 34cf8a598617..6ddacc6f44b9 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)    := $(riscv-march-y)c
> >  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
> >  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> >
> > +# Check if the toolchain supports Zihintpause extension
> > +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> > +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> > +
> >  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
> >  KBUILD_AFLAGS += -march=$(riscv-march-y)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index e48eebdd2631..dc47019a0b38 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -8,6 +8,7 @@
> >  #ifndef _ASM_RISCV_HWCAP_H
> >  #define _ASM_RISCV_HWCAP_H
> >
> > +#include <asm/errno.h>
> >  #include <linux/bits.h>
> >  #include <uapi/asm/hwcap.h>
> >
> > @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
> >  enum riscv_isa_ext_id {
> >         RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> >         RISCV_ISA_EXT_SVPBMT,
> > +       RISCV_ISA_EXT_ZIHINTPAUSE,
> >         RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> >  };
> >
> > @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
> >   */
> >  enum riscv_isa_ext_key {
> >         RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
> > +       RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
> >         RISCV_ISA_EXT_KEY_MAX,
> >  };
> >
> > @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
> >                 return RISCV_ISA_EXT_KEY_FPU;
> >         case RISCV_ISA_EXT_d:
> >                 return RISCV_ISA_EXT_KEY_FPU;
> > +       case RISCV_ISA_EXT_ZIHINTPAUSE:
> > +               return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
> >         default:
> >                 return -EINVAL;
> >         }
> > diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> > index 134388cbaaa1..1e4f8b4aef79 100644
> > --- a/arch/riscv/include/asm/vdso/processor.h
> > +++ b/arch/riscv/include/asm/vdso/processor.h
> > @@ -4,15 +4,30 @@
> >
> >  #ifndef __ASSEMBLY__
> >
> > +#include <linux/jump_label.h>
> >  #include <asm/barrier.h>
> > +#include <asm/hwcap.h>
> >
> >  static inline void cpu_relax(void)
> >  {
> > +       if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
> >  #ifdef __riscv_muldiv
> > -       int dummy;
> > -       /* In lieu of a halt instruction, induce a long-latency stall. */
> > -       __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> > +               int dummy;
> > +               /* In lieu of a halt instruction, induce a long-latency stall. */
> > +               __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> >  #endif
> > +       } else {
> > +               /*
> > +                * Reduce instruction retirement.
> > +                * This assumes the PC changes.
> > +                */
> > +#ifdef __riscv_zihintpause
> > +               __asm__ __volatile__ ("pause");
> > +#else
> > +               /* Encoding of the pause instruction */
> > +               __asm__ __volatile__ (".4byte 0x100000F");
> > +#endif
> > +       }
> >         barrier();
> >  }
> >
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index fba9e9f46a8c..a123e92b14dd 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
> >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> >         __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >         __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> > +       __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> >         __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> >  };
> >
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 1b3ec44e25f5..708df2c0bc34 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
> >                         } else {
> >                                 SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> >                                 SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > +                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >                         }
> >  #undef SET_ISA_EXT_MAP
> >                 }
> > --
> > 2.25.1
> >
Atish Patra July 11, 2022, 8:39 p.m. UTC | #3
On Mon, Jun 20, 2022 at 1:15 PM Dao Lu <daolu@rivosinc.com> wrote:
>
> Implement support for the ZiHintPause extension.
>
> The PAUSE instruction is a HINT that indicates the current hart’s rate
> of instruction retirement should be temporarily reduced or paused.
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Dao Lu <daolu@rivosinc.com>
> ---
>
> v1 -> v2:
>  Remove the usage of static branch, use PAUSE if toolchain supports it
> v2 -> v3:
>  Added the static branch back, cpu_relax() behavior is kept the same for
> systems that do not support ZiHintPause
> v3 -> v4:
>  Adopted the newly added unified static keys for extensions
> ---
>  arch/riscv/Makefile                     |  4 ++++
>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>  arch/riscv/kernel/cpu.c                 |  1 +
>  arch/riscv/kernel/cpufeature.c          |  1 +
>  5 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 34cf8a598617..6ddacc6f44b9 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)    := $(riscv-march-y)c
>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>
> +# Check if the toolchain supports Zihintpause extension
> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> +
>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e48eebdd2631..dc47019a0b38 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -8,6 +8,7 @@
>  #ifndef _ASM_RISCV_HWCAP_H
>  #define _ASM_RISCV_HWCAP_H
>
> +#include <asm/errno.h>
>  #include <linux/bits.h>
>  #include <uapi/asm/hwcap.h>
>
> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>  enum riscv_isa_ext_id {
>         RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>         RISCV_ISA_EXT_SVPBMT,
> +       RISCV_ISA_EXT_ZIHINTPAUSE,
>         RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>  };
>
> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>   */
>  enum riscv_isa_ext_key {
>         RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
> +       RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>         RISCV_ISA_EXT_KEY_MAX,
>  };
>
> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>                 return RISCV_ISA_EXT_KEY_FPU;
>         case RISCV_ISA_EXT_d:
>                 return RISCV_ISA_EXT_KEY_FPU;
> +       case RISCV_ISA_EXT_ZIHINTPAUSE:
> +               return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>         default:
>                 return -EINVAL;
>         }
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 134388cbaaa1..1e4f8b4aef79 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -4,15 +4,30 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include <linux/jump_label.h>
>  #include <asm/barrier.h>
> +#include <asm/hwcap.h>
>
>  static inline void cpu_relax(void)
>  {
> +       if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>  #ifdef __riscv_muldiv
> -       int dummy;
> -       /* In lieu of a halt instruction, induce a long-latency stall. */
> -       __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> +               int dummy;
> +               /* In lieu of a halt instruction, induce a long-latency stall. */
> +               __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>  #endif
> +       } else {
> +               /*
> +                * Reduce instruction retirement.
> +                * This assumes the PC changes.
> +                */
> +#ifdef __riscv_zihintpause
> +               __asm__ __volatile__ ("pause");
> +#else
> +               /* Encoding of the pause instruction */
> +               __asm__ __volatile__ (".4byte 0x100000F");
> +#endif
> +       }
>         barrier();
>  }
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index fba9e9f46a8c..a123e92b14dd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>         __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>         __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> +       __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>         __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>  };
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1b3ec44e25f5..708df2c0bc34 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>                         } else {
>                                 SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>                                 SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> +                               SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>                         }
>  #undef SET_ISA_EXT_MAP
>                 }
> --
> 2.25.1
>

LGTM.

Reviewed-by: Atish Patra <atishp@rivosinc.com>
Conor Dooley July 11, 2022, 8:44 p.m. UTC | #4
On 20/06/2022 21:15, Dao Lu wrote:
> [PATCH v4] arch/riscv: add Zihintpause support

A little ornery/pedantic maybe, but the "arch/" isn't needed.
Guess it can easily be fixed on application or if there's a
v5 so there's prob no need to resend.

> Implement support for the ZiHintPause extension.
> 
> The PAUSE instruction is a HINT that indicates the current hart’s rate
> of instruction retirement should be temporarily reduced or paused.
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Dao Lu <daolu@rivosinc.com>
> ---
> 
> v1 -> v2:
>  Remove the usage of static branch, use PAUSE if toolchain supports it
> v2 -> v3:
>  Added the static branch back, cpu_relax() behavior is kept the same for
> systems that do not support ZiHintPause
> v3 -> v4:
>  Adopted the newly added unified static keys for extensions
> ---
>  arch/riscv/Makefile                     |  4 ++++
>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>  arch/riscv/kernel/cpu.c                 |  1 +
>  arch/riscv/kernel/cpufeature.c          |  1 +
>  5 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 34cf8a598617..6ddacc6f44b9 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>  
> +# Check if the toolchain supports Zihintpause extension
> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> +
>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>  
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e48eebdd2631..dc47019a0b38 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -8,6 +8,7 @@
>  #ifndef _ASM_RISCV_HWCAP_H
>  #define _ASM_RISCV_HWCAP_H
>  
> +#include <asm/errno.h>
>  #include <linux/bits.h>
>  #include <uapi/asm/hwcap.h>
>  
> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>  enum riscv_isa_ext_id {
>  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>  	RISCV_ISA_EXT_SVPBMT,
> +	RISCV_ISA_EXT_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>  };
>  
> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>   */
>  enum riscv_isa_ext_key {
>  	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
> +	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_KEY_MAX,
>  };
>  
> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>  		return RISCV_ISA_EXT_KEY_FPU;
>  	case RISCV_ISA_EXT_d:
>  		return RISCV_ISA_EXT_KEY_FPU;
> +	case RISCV_ISA_EXT_ZIHINTPAUSE:
> +		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 134388cbaaa1..1e4f8b4aef79 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -4,15 +4,30 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#include <linux/jump_label.h>
>  #include <asm/barrier.h>
> +#include <asm/hwcap.h>
>  
>  static inline void cpu_relax(void)
>  {
> +	if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>  #ifdef __riscv_muldiv
> -	int dummy;
> -	/* In lieu of a halt instruction, induce a long-latency stall. */
> -	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> +		int dummy;
> +		/* In lieu of a halt instruction, induce a long-latency stall. */
> +		__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>  #endif
> +	} else {
> +		/*
> +		 * Reduce instruction retirement.
> +		 * This assumes the PC changes.
> +		 */
> +#ifdef __riscv_zihintpause
> +		__asm__ __volatile__ ("pause");
> +#else
> +		/* Encoding of the pause instruction */
> +		__asm__ __volatile__ (".4byte 0x100000F");
> +#endif
> +	}
>  	barrier();
>  }
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index fba9e9f46a8c..a123e92b14dd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>  };
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1b3ec44e25f5..708df2c0bc34 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>  			} else {
>  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> +				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>  			}
>  #undef SET_ISA_EXT_MAP
>  		}
Jisheng Zhang July 16, 2022, 12:43 p.m. UTC | #5
On Mon, Jun 20, 2022 at 01:15:25PM -0700, Dao Lu wrote:
> Implement support for the ZiHintPause extension.
> 
> The PAUSE instruction is a HINT that indicates the current hart’s rate
> of instruction retirement should be temporarily reduced or paused.
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Dao Lu <daolu@rivosinc.com>

Reviewed-by: Jisheng Zhang<jszhang@kernel.org>

> ---
> 
> v1 -> v2:
>  Remove the usage of static branch, use PAUSE if toolchain supports it
> v2 -> v3:
>  Added the static branch back, cpu_relax() behavior is kept the same for
> systems that do not support ZiHintPause
> v3 -> v4:
>  Adopted the newly added unified static keys for extensions
> ---
>  arch/riscv/Makefile                     |  4 ++++
>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>  arch/riscv/kernel/cpu.c                 |  1 +
>  arch/riscv/kernel/cpufeature.c          |  1 +
>  5 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 34cf8a598617..6ddacc6f44b9 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>  
> +# Check if the toolchain supports Zihintpause extension
> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> +
>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>  
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e48eebdd2631..dc47019a0b38 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -8,6 +8,7 @@
>  #ifndef _ASM_RISCV_HWCAP_H
>  #define _ASM_RISCV_HWCAP_H
>  
> +#include <asm/errno.h>
>  #include <linux/bits.h>
>  #include <uapi/asm/hwcap.h>
>  
> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>  enum riscv_isa_ext_id {
>  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>  	RISCV_ISA_EXT_SVPBMT,
> +	RISCV_ISA_EXT_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>  };
>  
> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>   */
>  enum riscv_isa_ext_key {
>  	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
> +	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_KEY_MAX,
>  };
>  
> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>  		return RISCV_ISA_EXT_KEY_FPU;
>  	case RISCV_ISA_EXT_d:
>  		return RISCV_ISA_EXT_KEY_FPU;
> +	case RISCV_ISA_EXT_ZIHINTPAUSE:
> +		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 134388cbaaa1..1e4f8b4aef79 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -4,15 +4,30 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#include <linux/jump_label.h>
>  #include <asm/barrier.h>
> +#include <asm/hwcap.h>
>  
>  static inline void cpu_relax(void)
>  {
> +	if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>  #ifdef __riscv_muldiv
> -	int dummy;
> -	/* In lieu of a halt instruction, induce a long-latency stall. */
> -	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> +		int dummy;
> +		/* In lieu of a halt instruction, induce a long-latency stall. */
> +		__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>  #endif
> +	} else {
> +		/*
> +		 * Reduce instruction retirement.
> +		 * This assumes the PC changes.
> +		 */
> +#ifdef __riscv_zihintpause
> +		__asm__ __volatile__ ("pause");
> +#else
> +		/* Encoding of the pause instruction */
> +		__asm__ __volatile__ (".4byte 0x100000F");
> +#endif
> +	}
>  	barrier();
>  }
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index fba9e9f46a8c..a123e92b14dd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>  };
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1b3ec44e25f5..708df2c0bc34 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>  			} else {
>  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> +				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>  			}
>  #undef SET_ISA_EXT_MAP
>  		}
> -- 
> 2.25.1
>
Heiko Stübner July 16, 2022, 2:11 p.m. UTC | #6
Am Montag, 11. Juli 2022, 22:44:21 CEST schrieb Conor Dooley:
> On 20/06/2022 21:15, Dao Lu wrote:
> > [PATCH v4] arch/riscv: add Zihintpause support
> 
> A little ornery/pedantic maybe, but the "arch/" isn't needed.
> Guess it can easily be fixed on application or if there's a
> v5 so there's prob no need to resend.

I noticed that the patch prefix on riscv is all over the place
in a lot of cases. There are

- RISC-V:
-riscv:
- now arch/riscv:

and probably even more.

I guess someone should just decide on one prefix that then
gets used (and slightly enforced) all the time.

Even just modifying applied patches to one specific prefix should
already solve the issue in a short time, as I guess most people will
just get the appicable prefix from a "git log" ;-) .


Heiko

> > Implement support for the ZiHintPause extension.
> > 
> > The PAUSE instruction is a HINT that indicates the current hart’s rate
> > of instruction retirement should be temporarily reduced or paused.
> > 
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > Signed-off-by: Dao Lu <daolu@rivosinc.com>
> > ---
> > 
> > v1 -> v2:
> >  Remove the usage of static branch, use PAUSE if toolchain supports it
> > v2 -> v3:
> >  Added the static branch back, cpu_relax() behavior is kept the same for
> > systems that do not support ZiHintPause
> > v3 -> v4:
> >  Adopted the newly added unified static keys for extensions
> > ---
> >  arch/riscv/Makefile                     |  4 ++++
> >  arch/riscv/include/asm/hwcap.h          |  5 +++++
> >  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
> >  arch/riscv/kernel/cpu.c                 |  1 +
> >  arch/riscv/kernel/cpufeature.c          |  1 +
> >  5 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 34cf8a598617..6ddacc6f44b9 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
> >  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
> >  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> >  
> > +# Check if the toolchain supports Zihintpause extension
> > +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> > +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> > +
> >  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
> >  KBUILD_AFLAGS += -march=$(riscv-march-y)
> >  
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index e48eebdd2631..dc47019a0b38 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -8,6 +8,7 @@
> >  #ifndef _ASM_RISCV_HWCAP_H
> >  #define _ASM_RISCV_HWCAP_H
> >  
> > +#include <asm/errno.h>
> >  #include <linux/bits.h>
> >  #include <uapi/asm/hwcap.h>
> >  
> > @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
> >  enum riscv_isa_ext_id {
> >  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> >  	RISCV_ISA_EXT_SVPBMT,
> > +	RISCV_ISA_EXT_ZIHINTPAUSE,
> >  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> >  };
> >  
> > @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
> >   */
> >  enum riscv_isa_ext_key {
> >  	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
> > +	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
> >  	RISCV_ISA_EXT_KEY_MAX,
> >  };
> >  
> > @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
> >  		return RISCV_ISA_EXT_KEY_FPU;
> >  	case RISCV_ISA_EXT_d:
> >  		return RISCV_ISA_EXT_KEY_FPU;
> > +	case RISCV_ISA_EXT_ZIHINTPAUSE:
> > +		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> > index 134388cbaaa1..1e4f8b4aef79 100644
> > --- a/arch/riscv/include/asm/vdso/processor.h
> > +++ b/arch/riscv/include/asm/vdso/processor.h
> > @@ -4,15 +4,30 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > +#include <linux/jump_label.h>
> >  #include <asm/barrier.h>
> > +#include <asm/hwcap.h>
> >  
> >  static inline void cpu_relax(void)
> >  {
> > +	if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
> >  #ifdef __riscv_muldiv
> > -	int dummy;
> > -	/* In lieu of a halt instruction, induce a long-latency stall. */
> > -	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> > +		int dummy;
> > +		/* In lieu of a halt instruction, induce a long-latency stall. */
> > +		__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> >  #endif
> > +	} else {
> > +		/*
> > +		 * Reduce instruction retirement.
> > +		 * This assumes the PC changes.
> > +		 */
> > +#ifdef __riscv_zihintpause
> > +		__asm__ __volatile__ ("pause");
> > +#else
> > +		/* Encoding of the pause instruction */
> > +		__asm__ __volatile__ (".4byte 0x100000F");
> > +#endif
> > +	}
> >  	barrier();
> >  }
> >  
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index fba9e9f46a8c..a123e92b14dd 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
> >  static struct riscv_isa_ext_data isa_ext_arr[] = {
> >  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> > +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> >  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> >  };
> >  
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 1b3ec44e25f5..708df2c0bc34 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
> >  			} else {
> >  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> >  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > +				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >  			}
> >  #undef SET_ISA_EXT_MAP
> >  		}
>
Conor Dooley July 16, 2022, 2:24 p.m. UTC | #7
On 16/07/2022 15:11, Heiko Stuebner wrote:
> Am Montag, 11. Juli 2022, 22:44:21 CEST schrieb Conor Dooley:
>> On 20/06/2022 21:15, Dao Lu wrote:
>>> [PATCH v4] arch/riscv: add Zihintpause support
>>
>> A little ornery/pedantic maybe, but the "arch/" isn't needed.
>> Guess it can easily be fixed on application or if there's a
>> v5 so there's prob no need to resend.
> 
> I noticed that the patch prefix on riscv is all over the place
> in a lot of cases. There are
> 
> - RISC-V:
> -riscv:
> - now arch/riscv:
> 
> and probably even more.
> 
> I guess someone should just decide on one prefix that then
> gets used (and slightly enforced) all the time.

Yeah, I did see Palmer commenting on RISC-V/riscv once.
I think riscv is more common since it matches the arch directory
but Palmer himself uses RISC-V

I've seen patchsets that use both RISC-V and riscv haha

Maybe it is time to pick one? Palmer?

> 
> Even just modifying applied patches to one specific prefix should
> already solve the issue in a short time, as I guess most people will
> just get the appicable prefix from a "git log" ;-) .

Again, that's down to Palmer (and now me I guess to a lesser extent
given I've starting doing PRs).

My OCD would like a single prefix though ;)
Conor.

> 
> 
> Heiko
> 
>>> Implement support for the ZiHintPause extension.
>>>
>>> The PAUSE instruction is a HINT that indicates the current hart’s rate
>>> of instruction retirement should be temporarily reduced or paused.
>>>
>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
>>> ---
>>>
>>> v1 -> v2:
>>>  Remove the usage of static branch, use PAUSE if toolchain supports it
>>> v2 -> v3:
>>>  Added the static branch back, cpu_relax() behavior is kept the same for
>>> systems that do not support ZiHintPause
>>> v3 -> v4:
>>>  Adopted the newly added unified static keys for extensions
>>> ---
>>>  arch/riscv/Makefile                     |  4 ++++
>>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>>>  arch/riscv/kernel/cpu.c                 |  1 +
>>>  arch/riscv/kernel/cpufeature.c          |  1 +
>>>  5 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>> index 34cf8a598617..6ddacc6f44b9 100644
>>> --- a/arch/riscv/Makefile
>>> +++ b/arch/riscv/Makefile
>>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>>>  
>>> +# Check if the toolchain supports Zihintpause extension
>>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
>>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
>>> +
>>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>>>  
>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>> index e48eebdd2631..dc47019a0b38 100644
>>> --- a/arch/riscv/include/asm/hwcap.h
>>> +++ b/arch/riscv/include/asm/hwcap.h
>>> @@ -8,6 +8,7 @@
>>>  #ifndef _ASM_RISCV_HWCAP_H
>>>  #define _ASM_RISCV_HWCAP_H
>>>  
>>> +#include <asm/errno.h>
>>>  #include <linux/bits.h>
>>>  #include <uapi/asm/hwcap.h>
>>>  
>>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>>>  enum riscv_isa_ext_id {
>>>  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>>  	RISCV_ISA_EXT_SVPBMT,
>>> +	RISCV_ISA_EXT_ZIHINTPAUSE,
>>>  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>>>  };
>>>  
>>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>>>   */
>>>  enum riscv_isa_ext_key {
>>>  	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
>>> +	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>>>  	RISCV_ISA_EXT_KEY_MAX,
>>>  };
>>>  
>>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>>>  		return RISCV_ISA_EXT_KEY_FPU;
>>>  	case RISCV_ISA_EXT_d:
>>>  		return RISCV_ISA_EXT_KEY_FPU;
>>> +	case RISCV_ISA_EXT_ZIHINTPAUSE:
>>> +		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>>>  	default:
>>>  		return -EINVAL;
>>>  	}
>>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>>> index 134388cbaaa1..1e4f8b4aef79 100644
>>> --- a/arch/riscv/include/asm/vdso/processor.h
>>> +++ b/arch/riscv/include/asm/vdso/processor.h
>>> @@ -4,15 +4,30 @@
>>>  
>>>  #ifndef __ASSEMBLY__
>>>  
>>> +#include <linux/jump_label.h>
>>>  #include <asm/barrier.h>
>>> +#include <asm/hwcap.h>
>>>  
>>>  static inline void cpu_relax(void)
>>>  {
>>> +	if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>>>  #ifdef __riscv_muldiv
>>> -	int dummy;
>>> -	/* In lieu of a halt instruction, induce a long-latency stall. */
>>> -	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>> +		int dummy;
>>> +		/* In lieu of a halt instruction, induce a long-latency stall. */
>>> +		__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>  #endif
>>> +	} else {
>>> +		/*
>>> +		 * Reduce instruction retirement.
>>> +		 * This assumes the PC changes.
>>> +		 */
>>> +#ifdef __riscv_zihintpause
>>> +		__asm__ __volatile__ ("pause");
>>> +#else
>>> +		/* Encoding of the pause instruction */
>>> +		__asm__ __volatile__ (".4byte 0x100000F");
>>> +#endif
>>> +	}
>>>  	barrier();
>>>  }
>>>  
>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>> index fba9e9f46a8c..a123e92b14dd 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>> +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>>  };
>>>  
>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>> index 1b3ec44e25f5..708df2c0bc34 100644
>>> --- a/arch/riscv/kernel/cpufeature.c
>>> +++ b/arch/riscv/kernel/cpufeature.c
>>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>>  			} else {
>>>  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>>  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>>> +				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>>  			}
>>>  #undef SET_ISA_EXT_MAP
>>>  		}
>>
> 
> 
> 
>
Palmer Dabbelt Aug. 11, 2022, 3:17 p.m. UTC | #8
On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
> Implement support for the ZiHintPause extension.
>
> The PAUSE instruction is a HINT that indicates the current hart’s rate
> of instruction retirement should be temporarily reduced or paused.
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Dao Lu <daolu@rivosinc.com>
> ---
>
> v1 -> v2:
>  Remove the usage of static branch, use PAUSE if toolchain supports it
> v2 -> v3:
>  Added the static branch back, cpu_relax() behavior is kept the same for
> systems that do not support ZiHintPause
> v3 -> v4:
>  Adopted the newly added unified static keys for extensions
> ---
>  arch/riscv/Makefile                     |  4 ++++
>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>  arch/riscv/kernel/cpu.c                 |  1 +
>  arch/riscv/kernel/cpufeature.c          |  1 +
>  5 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 34cf8a598617..6ddacc6f44b9 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>
> +# Check if the toolchain supports Zihintpause extension
> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> +
>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e48eebdd2631..dc47019a0b38 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -8,6 +8,7 @@
>  #ifndef _ASM_RISCV_HWCAP_H
>  #define _ASM_RISCV_HWCAP_H
>
> +#include <asm/errno.h>
>  #include <linux/bits.h>
>  #include <uapi/asm/hwcap.h>
>
> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>  enum riscv_isa_ext_id {
>  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>  	RISCV_ISA_EXT_SVPBMT,
> +	RISCV_ISA_EXT_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>  };
>
> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>   */
>  enum riscv_isa_ext_key {
>  	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
> +	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>  	RISCV_ISA_EXT_KEY_MAX,
>  };
>
> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>  		return RISCV_ISA_EXT_KEY_FPU;
>  	case RISCV_ISA_EXT_d:
>  		return RISCV_ISA_EXT_KEY_FPU;
> +	case RISCV_ISA_EXT_ZIHINTPAUSE:
> +		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 134388cbaaa1..1e4f8b4aef79 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -4,15 +4,30 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include <linux/jump_label.h>
>  #include <asm/barrier.h>
> +#include <asm/hwcap.h>
>
>  static inline void cpu_relax(void)
>  {
> +	if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>  #ifdef __riscv_muldiv
> -	int dummy;
> -	/* In lieu of a halt instruction, induce a long-latency stall. */
> -	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> +		int dummy;
> +		/* In lieu of a halt instruction, induce a long-latency stall. */
> +		__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>  #endif
> +	} else {
> +		/*
> +		 * Reduce instruction retirement.
> +		 * This assumes the PC changes.
> +		 */
> +#ifdef __riscv_zihintpause
> +		__asm__ __volatile__ ("pause");
> +#else
> +		/* Encoding of the pause instruction */
> +		__asm__ __volatile__ (".4byte 0x100000F");
> +#endif
> +	}
>  	barrier();
>  }
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index fba9e9f46a8c..a123e92b14dd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>  };
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1b3ec44e25f5..708df2c0bc34 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>  			} else {
>  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> +				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>  			}
>  #undef SET_ISA_EXT_MAP
>  		}

Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.
Conor Dooley Aug. 12, 2022, 6:57 a.m. UTC | #9
On 11/08/2022 16:17, Palmer Dabbelt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
>> Implement support for the ZiHintPause extension.
>>
>> The PAUSE instruction is a HINT that indicates the current hart’s rate
>> of instruction retirement should be temporarily reduced or paused.
>>
>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
>> ---
>>
>> v1 -> v2:
>>  Remove the usage of static branch, use PAUSE if toolchain supports it
>> v2 -> v3:
>>  Added the static branch back, cpu_relax() behavior is kept the same for
>> systems that do not support ZiHintPause
>> v3 -> v4:
>>  Adopted the newly added unified static keys for extensions
>> ---
>>  arch/riscv/Makefile                     |  4 ++++
>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>>  arch/riscv/kernel/cpu.c                 |  1 +
>>  arch/riscv/kernel/cpufeature.c          |  1 +
>>  5 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>> index 34cf8a598617..6ddacc6f44b9 100644
>> --- a/arch/riscv/Makefile
>> +++ b/arch/riscv/Makefile
>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)  := $(riscv-march-y)c
>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>>
>> +# Check if the toolchain supports Zihintpause extension
>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
>> +
>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>>
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index e48eebdd2631..dc47019a0b38 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -8,6 +8,7 @@
>>  #ifndef _ASM_RISCV_HWCAP_H
>>  #define _ASM_RISCV_HWCAP_H
>>
>> +#include <asm/errno.h>
>>  #include <linux/bits.h>
>>  #include <uapi/asm/hwcap.h>
>>
>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>>  enum riscv_isa_ext_id {
>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>       RISCV_ISA_EXT_SVPBMT,
>> +     RISCV_ISA_EXT_ZIHINTPAUSE,
>>       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>>  };
>>
>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>>   */
>>  enum riscv_isa_ext_key {
>>       RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
>> +     RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>>       RISCV_ISA_EXT_KEY_MAX,
>>  };
>>
>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>>               return RISCV_ISA_EXT_KEY_FPU;
>>       case RISCV_ISA_EXT_d:
>>               return RISCV_ISA_EXT_KEY_FPU;
>> +     case RISCV_ISA_EXT_ZIHINTPAUSE:
>> +             return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>>       default:
>>               return -EINVAL;
>>       }
>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>> index 134388cbaaa1..1e4f8b4aef79 100644
>> --- a/arch/riscv/include/asm/vdso/processor.h
>> +++ b/arch/riscv/include/asm/vdso/processor.h
>> @@ -4,15 +4,30 @@
>>
>>  #ifndef __ASSEMBLY__
>>
>> +#include <linux/jump_label.h>
>>  #include <asm/barrier.h>
>> +#include <asm/hwcap.h>
>>
>>  static inline void cpu_relax(void)
>>  {
>> +     if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>>  #ifdef __riscv_muldiv
>> -     int dummy;
>> -     /* In lieu of a halt instruction, induce a long-latency stall. */
>> -     __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>> +             int dummy;
>> +             /* In lieu of a halt instruction, induce a long-latency stall. */
>> +             __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>  #endif
>> +     } else {
>> +             /*
>> +              * Reduce instruction retirement.
>> +              * This assumes the PC changes.
>> +              */
>> +#ifdef __riscv_zihintpause
>> +             __asm__ __volatile__ ("pause");
>> +#else
>> +             /* Encoding of the pause instruction */
>> +             __asm__ __volatile__ (".4byte 0x100000F");
>> +#endif
>> +     }
>>       barrier();
>>  }
>>
>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>> index fba9e9f46a8c..a123e92b14dd 100644
>> --- a/arch/riscv/kernel/cpu.c
>> +++ b/arch/riscv/kernel/cpu.c
>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>> +     __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>  };
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 1b3ec44e25f5..708df2c0bc34 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>                       } else {
>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>> +                             SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>                       }
>>  #undef SET_ISA_EXT_MAP
>>               }
> 
> Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.

This breaks the C=1 build for all toolchains, not just new ones as your sparse
patch suggests. I amn't 100% what my CI is running, but I replicated this on
my own machine with:

sparse --version
0.6.4 (Ubuntu: 0.6.4-2)

---8<---
   YACC    scripts/dtc/dtc-parser.tab.[ch]
   HOSTCC  scripts/dtc/libfdt/fdt.o
   HOSTCC  scripts/dtc/libfdt/fdt_ro.o
   HOSTCC  scripts/dtc/libfdt/fdt_wip.o
   UPD     include/generated/uapi/linux/version.h
   HOSTCC  scripts/dtc/libfdt/fdt_sw.o
   UPD     include/config/kernel.release
   HOSTCC  scripts/dtc/libfdt/fdt_rw.o
   HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
   HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
   HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
   HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
   HOSTCC  scripts/dtc/fdtoverlay.o
   HOSTCC  scripts/dtc/dtc-lexer.lex.o
   HOSTCC  scripts/dtc/dtc-parser.tab.o
   UPD     include/generated/utsrelease.h
   HOSTLD  scripts/dtc/fdtoverlay
   HOSTLD  scripts/dtc/dtc
   HOSTCC  scripts/kallsyms
   HOSTCC  scripts/sorttable
   HOSTCC  scripts/asn1_compiler
   HOSTCC  scripts/selinux/genheaders/genheaders
   HOSTCC  scripts/selinux/mdp/mdp
   CC      scripts/mod/empty.o
   HOSTCC  scripts/mod/mk_elfconfig
   CC      scripts/mod/devicetable-offsets.s
   CHECK   ../scripts/mod/empty.c
invalid argument to '-march': '_zihintpause'

make[2]: *** [../scripts/Makefile.build:250: scripts/mod/empty.o] Error 1
make[2]: *** Deleting file 'scripts/mod/empty.o'
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/Makefile:1287: prepare0] Error 2
make[1]: Leaving directory '/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/builddir'
make: *** [Makefile:231: __sub-make] Error 2
Conor Dooley Aug. 12, 2022, 7:21 a.m. UTC | #10
On 12/08/2022 07:57, Conor Dooley - M52691 wrote:
> On 11/08/2022 16:17, Palmer Dabbelt wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
>>> Implement support for the ZiHintPause extension.
>>>
>>> The PAUSE instruction is a HINT that indicates the current hart’s rate
>>> of instruction retirement should be temporarily reduced or paused.
>>>
>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
>>> ---
>>>
>>> v1 -> v2:
>>>  Remove the usage of static branch, use PAUSE if toolchain supports it
>>> v2 -> v3:
>>>  Added the static branch back, cpu_relax() behavior is kept the same for
>>> systems that do not support ZiHintPause
>>> v3 -> v4:
>>>  Adopted the newly added unified static keys for extensions
>>> ---
>>>  arch/riscv/Makefile                     |  4 ++++
>>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>>>  arch/riscv/kernel/cpu.c                 |  1 +
>>>  arch/riscv/kernel/cpufeature.c          |  1 +
>>>  5 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>> index 34cf8a598617..6ddacc6f44b9 100644
>>> --- a/arch/riscv/Makefile
>>> +++ b/arch/riscv/Makefile
>>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)  := $(riscv-march-y)c
>>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>>>
>>> +# Check if the toolchain supports Zihintpause extension
>>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
>>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
>>> +
>>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>>>
>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>> index e48eebdd2631..dc47019a0b38 100644
>>> --- a/arch/riscv/include/asm/hwcap.h
>>> +++ b/arch/riscv/include/asm/hwcap.h
>>> @@ -8,6 +8,7 @@
>>>  #ifndef _ASM_RISCV_HWCAP_H
>>>  #define _ASM_RISCV_HWCAP_H
>>>
>>> +#include <asm/errno.h>
>>>  #include <linux/bits.h>
>>>  #include <uapi/asm/hwcap.h>
>>>
>>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>>>  enum riscv_isa_ext_id {
>>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>>       RISCV_ISA_EXT_SVPBMT,
>>> +     RISCV_ISA_EXT_ZIHINTPAUSE,
>>>       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>>>  };
>>>
>>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>>>   */
>>>  enum riscv_isa_ext_key {
>>>       RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
>>> +     RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>>>       RISCV_ISA_EXT_KEY_MAX,
>>>  };
>>>
>>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>       case RISCV_ISA_EXT_d:
>>>               return RISCV_ISA_EXT_KEY_FPU;
>>> +     case RISCV_ISA_EXT_ZIHINTPAUSE:
>>> +             return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>>>       default:
>>>               return -EINVAL;
>>>       }
>>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>>> index 134388cbaaa1..1e4f8b4aef79 100644
>>> --- a/arch/riscv/include/asm/vdso/processor.h
>>> +++ b/arch/riscv/include/asm/vdso/processor.h
>>> @@ -4,15 +4,30 @@
>>>
>>>  #ifndef __ASSEMBLY__
>>>
>>> +#include <linux/jump_label.h>
>>>  #include <asm/barrier.h>
>>> +#include <asm/hwcap.h>
>>>
>>>  static inline void cpu_relax(void)
>>>  {
>>> +     if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>>>  #ifdef __riscv_muldiv
>>> -     int dummy;
>>> -     /* In lieu of a halt instruction, induce a long-latency stall. */
>>> -     __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>> +             int dummy;
>>> +             /* In lieu of a halt instruction, induce a long-latency stall. */
>>> +             __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>  #endif
>>> +     } else {
>>> +             /*
>>> +              * Reduce instruction retirement.
>>> +              * This assumes the PC changes.
>>> +              */
>>> +#ifdef __riscv_zihintpause
>>> +             __asm__ __volatile__ ("pause");
>>> +#else
>>> +             /* Encoding of the pause instruction */
>>> +             __asm__ __volatile__ (".4byte 0x100000F");
>>> +#endif
>>> +     }
>>>       barrier();
>>>  }
>>>
>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>> index fba9e9f46a8c..a123e92b14dd 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>> +     __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>>  };
>>>
>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>> index 1b3ec44e25f5..708df2c0bc34 100644
>>> --- a/arch/riscv/kernel/cpufeature.c
>>> +++ b/arch/riscv/kernel/cpufeature.c
>>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>>                       } else {
>>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>>> +                             SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>>                       }
>>>  #undef SET_ISA_EXT_MAP
>>>               }
>>
>> Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.
> 
> This breaks the C=1 build for all toolchains, not just new ones as your sparse
> patch suggests. I amn't 100% what my CI is running, but I replicated this on
> my own machine with:

Argh, I went poking around and my toolchain's binutils etc is newer than I thought.
Good for people searching on lore I suppose...
Sorry!
Conor.

> 
> sparse --version
> 0.6.4 (Ubuntu: 0.6.4-2)
> 
> ---8<---
>    YACC    scripts/dtc/dtc-parser.tab.[ch]
>    HOSTCC  scripts/dtc/libfdt/fdt.o
>    HOSTCC  scripts/dtc/libfdt/fdt_ro.o
>    HOSTCC  scripts/dtc/libfdt/fdt_wip.o
>    UPD     include/generated/uapi/linux/version.h
>    HOSTCC  scripts/dtc/libfdt/fdt_sw.o
>    UPD     include/config/kernel.release
>    HOSTCC  scripts/dtc/libfdt/fdt_rw.o
>    HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
>    HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
>    HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
>    HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
>    HOSTCC  scripts/dtc/fdtoverlay.o
>    HOSTCC  scripts/dtc/dtc-lexer.lex.o
>    HOSTCC  scripts/dtc/dtc-parser.tab.o
>    UPD     include/generated/utsrelease.h
>    HOSTLD  scripts/dtc/fdtoverlay
>    HOSTLD  scripts/dtc/dtc
>    HOSTCC  scripts/kallsyms
>    HOSTCC  scripts/sorttable
>    HOSTCC  scripts/asn1_compiler
>    HOSTCC  scripts/selinux/genheaders/genheaders
>    HOSTCC  scripts/selinux/mdp/mdp
>    CC      scripts/mod/empty.o
>    HOSTCC  scripts/mod/mk_elfconfig
>    CC      scripts/mod/devicetable-offsets.s
>    CHECK   ../scripts/mod/empty.c
> invalid argument to '-march': '_zihintpause'
> 
> make[2]: *** [../scripts/Makefile.build:250: scripts/mod/empty.o] Error 1
> make[2]: *** Deleting file 'scripts/mod/empty.o'
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/Makefile:1287: prepare0] Error 2
> make[1]: Leaving directory '/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/builddir'
> make: *** [Makefile:231: __sub-make] Error 2
Palmer Dabbelt Aug. 16, 2022, 3:54 p.m. UTC | #11
On Fri, 12 Aug 2022 00:21:40 PDT (-0700), Conor.Dooley@microchip.com wrote:
> On 12/08/2022 07:57, Conor Dooley - M52691 wrote:
>> On 11/08/2022 16:17, Palmer Dabbelt wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
>>>> Implement support for the ZiHintPause extension.
>>>>
>>>> The PAUSE instruction is a HINT that indicates the current hart’s rate
>>>> of instruction retirement should be temporarily reduced or paused.
>>>>
>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>  Remove the usage of static branch, use PAUSE if toolchain supports it
>>>> v2 -> v3:
>>>>  Added the static branch back, cpu_relax() behavior is kept the same for
>>>> systems that do not support ZiHintPause
>>>> v3 -> v4:
>>>>  Adopted the newly added unified static keys for extensions
>>>> ---
>>>>  arch/riscv/Makefile                     |  4 ++++
>>>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>>>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>>>>  arch/riscv/kernel/cpu.c                 |  1 +
>>>>  arch/riscv/kernel/cpufeature.c          |  1 +
>>>>  5 files changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>>> index 34cf8a598617..6ddacc6f44b9 100644
>>>> --- a/arch/riscv/Makefile
>>>> +++ b/arch/riscv/Makefile
>>>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)  := $(riscv-march-y)c
>>>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>>>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>>>>
>>>> +# Check if the toolchain supports Zihintpause extension
>>>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
>>>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
>>>> +
>>>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>>>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>>>>
>>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>>> index e48eebdd2631..dc47019a0b38 100644
>>>> --- a/arch/riscv/include/asm/hwcap.h
>>>> +++ b/arch/riscv/include/asm/hwcap.h
>>>> @@ -8,6 +8,7 @@
>>>>  #ifndef _ASM_RISCV_HWCAP_H
>>>>  #define _ASM_RISCV_HWCAP_H
>>>>
>>>> +#include <asm/errno.h>
>>>>  #include <linux/bits.h>
>>>>  #include <uapi/asm/hwcap.h>
>>>>
>>>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>>>>  enum riscv_isa_ext_id {
>>>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>>>       RISCV_ISA_EXT_SVPBMT,
>>>> +     RISCV_ISA_EXT_ZIHINTPAUSE,
>>>>       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>>>>  };
>>>>
>>>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>>>>   */
>>>>  enum riscv_isa_ext_key {
>>>>       RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
>>>> +     RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>>>>       RISCV_ISA_EXT_KEY_MAX,
>>>>  };
>>>>
>>>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>>       case RISCV_ISA_EXT_d:
>>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>> +     case RISCV_ISA_EXT_ZIHINTPAUSE:
>>>> +             return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>>>>       default:
>>>>               return -EINVAL;
>>>>       }
>>>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>>>> index 134388cbaaa1..1e4f8b4aef79 100644
>>>> --- a/arch/riscv/include/asm/vdso/processor.h
>>>> +++ b/arch/riscv/include/asm/vdso/processor.h
>>>> @@ -4,15 +4,30 @@
>>>>
>>>>  #ifndef __ASSEMBLY__
>>>>
>>>> +#include <linux/jump_label.h>
>>>>  #include <asm/barrier.h>
>>>> +#include <asm/hwcap.h>
>>>>
>>>>  static inline void cpu_relax(void)
>>>>  {
>>>> +     if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>>>>  #ifdef __riscv_muldiv
>>>> -     int dummy;
>>>> -     /* In lieu of a halt instruction, induce a long-latency stall. */
>>>> -     __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>> +             int dummy;
>>>> +             /* In lieu of a halt instruction, induce a long-latency stall. */
>>>> +             __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>>  #endif
>>>> +     } else {
>>>> +             /*
>>>> +              * Reduce instruction retirement.
>>>> +              * This assumes the PC changes.
>>>> +              */
>>>> +#ifdef __riscv_zihintpause
>>>> +             __asm__ __volatile__ ("pause");
>>>> +#else
>>>> +             /* Encoding of the pause instruction */
>>>> +             __asm__ __volatile__ (".4byte 0x100000F");
>>>> +#endif
>>>> +     }
>>>>       barrier();
>>>>  }
>>>>
>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>> index fba9e9f46a8c..a123e92b14dd 100644
>>>> --- a/arch/riscv/kernel/cpu.c
>>>> +++ b/arch/riscv/kernel/cpu.c
>>>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>>> +     __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>>>  };
>>>>
>>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>>> index 1b3ec44e25f5..708df2c0bc34 100644
>>>> --- a/arch/riscv/kernel/cpufeature.c
>>>> +++ b/arch/riscv/kernel/cpufeature.c
>>>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>>>                       } else {
>>>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>>>> +                             SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>>>                       }
>>>>  #undef SET_ISA_EXT_MAP
>>>>               }
>>>
>>> Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.
>> 
>> This breaks the C=1 build for all toolchains, not just new ones as your sparse
>> patch suggests. I amn't 100% what my CI is running, but I replicated this on
>> my own machine with:
> 
> Argh, I went poking around and my toolchain's binutils etc is newer than I thought.
> Good for people searching on lore I suppose...
> Sorry!

So just to be clear: you're saying this only breaks with new binutils?  
I ask because Dao is also seeing some crashes, if it's breaking 
arbitrary builds too then it's a stronger hint to revert it.

> Conor.
> 
>> 
>> sparse --version
>> 0.6.4 (Ubuntu: 0.6.4-2)
>> 
>> ---8<---
>>    YACC    scripts/dtc/dtc-parser.tab.[ch]
>>    HOSTCC  scripts/dtc/libfdt/fdt.o
>>    HOSTCC  scripts/dtc/libfdt/fdt_ro.o
>>    HOSTCC  scripts/dtc/libfdt/fdt_wip.o
>>    UPD     include/generated/uapi/linux/version.h
>>    HOSTCC  scripts/dtc/libfdt/fdt_sw.o
>>    UPD     include/config/kernel.release
>>    HOSTCC  scripts/dtc/libfdt/fdt_rw.o
>>    HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
>>    HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
>>    HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
>>    HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
>>    HOSTCC  scripts/dtc/fdtoverlay.o
>>    HOSTCC  scripts/dtc/dtc-lexer.lex.o
>>    HOSTCC  scripts/dtc/dtc-parser.tab.o
>>    UPD     include/generated/utsrelease.h
>>    HOSTLD  scripts/dtc/fdtoverlay
>>    HOSTLD  scripts/dtc/dtc
>>    HOSTCC  scripts/kallsyms
>>    HOSTCC  scripts/sorttable
>>    HOSTCC  scripts/asn1_compiler
>>    HOSTCC  scripts/selinux/genheaders/genheaders
>>    HOSTCC  scripts/selinux/mdp/mdp
>>    CC      scripts/mod/empty.o
>>    HOSTCC  scripts/mod/mk_elfconfig
>>    CC      scripts/mod/devicetable-offsets.s
>>    CHECK   ../scripts/mod/empty.c
>> invalid argument to '-march': '_zihintpause'
>> 
>> make[2]: *** [../scripts/Makefile.build:250: scripts/mod/empty.o] Error 1
>> make[2]: *** Deleting file 'scripts/mod/empty.o'
>> make[2]: *** Waiting for unfinished jobs....
>> make[1]: *** [/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/Makefile:1287: prepare0] Error 2
>> make[1]: Leaving directory '/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/builddir'
>> make: *** [Makefile:231: __sub-make] Error 2
>
Conor Dooley Aug. 16, 2022, 4:04 p.m. UTC | #12
On 16/08/2022 16:54, Palmer Dabbelt wrote:
> On Fri, 12 Aug 2022 00:21:40 PDT (-0700), Conor.Dooley@microchip.com wrote:
>> On 12/08/2022 07:57, Conor Dooley - M52691 wrote:
>>> On 11/08/2022 16:17, Palmer Dabbelt wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
>>>>> Implement support for the ZiHintPause extension.
>>>>>
>>>>> The PAUSE instruction is a HINT that indicates the current hart’s rate
>>>>> of instruction retirement should be temporarily reduced or paused.
>>>>>
>>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
>>>>> ---
>>>>>
>>>>> v1 -> v2:
>>>>>  Remove the usage of static branch, use PAUSE if toolchain supports it
>>>>> v2 -> v3:
>>>>>  Added the static branch back, cpu_relax() behavior is kept the same for
>>>>> systems that do not support ZiHintPause
>>>>> v3 -> v4:
>>>>>  Adopted the newly added unified static keys for extensions
>>>>> ---
>>>>>  arch/riscv/Makefile                     |  4 ++++
>>>>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>>>>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>>>>>  arch/riscv/kernel/cpu.c                 |  1 +
>>>>>  arch/riscv/kernel/cpufeature.c          |  1 +
>>>>>  5 files changed, 29 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>>>> index 34cf8a598617..6ddacc6f44b9 100644
>>>>> --- a/arch/riscv/Makefile
>>>>> +++ b/arch/riscv/Makefile
>>>>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)  := $(riscv-march-y)c
>>>>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>>>>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>>>>>
>>>>> +# Check if the toolchain supports Zihintpause extension
>>>>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
>>>>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
>>>>> +
>>>>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>>>>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>>>> index e48eebdd2631..dc47019a0b38 100644
>>>>> --- a/arch/riscv/include/asm/hwcap.h
>>>>> +++ b/arch/riscv/include/asm/hwcap.h
>>>>> @@ -8,6 +8,7 @@
>>>>>  #ifndef _ASM_RISCV_HWCAP_H
>>>>>  #define _ASM_RISCV_HWCAP_H
>>>>>
>>>>> +#include <asm/errno.h>
>>>>>  #include <linux/bits.h>
>>>>>  #include <uapi/asm/hwcap.h>
>>>>>
>>>>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>>>>>  enum riscv_isa_ext_id {
>>>>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>>>>       RISCV_ISA_EXT_SVPBMT,
>>>>> +     RISCV_ISA_EXT_ZIHINTPAUSE,
>>>>>       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>>>>>  };
>>>>>
>>>>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>>>>>   */
>>>>>  enum riscv_isa_ext_key {
>>>>>       RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
>>>>> +     RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>>>>>       RISCV_ISA_EXT_KEY_MAX,
>>>>>  };
>>>>>
>>>>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>>>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>>>       case RISCV_ISA_EXT_d:
>>>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>>> +     case RISCV_ISA_EXT_ZIHINTPAUSE:
>>>>> +             return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>>>>>       default:
>>>>>               return -EINVAL;
>>>>>       }
>>>>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>>>>> index 134388cbaaa1..1e4f8b4aef79 100644
>>>>> --- a/arch/riscv/include/asm/vdso/processor.h
>>>>> +++ b/arch/riscv/include/asm/vdso/processor.h
>>>>> @@ -4,15 +4,30 @@
>>>>>
>>>>>  #ifndef __ASSEMBLY__
>>>>>
>>>>> +#include <linux/jump_label.h>
>>>>>  #include <asm/barrier.h>
>>>>> +#include <asm/hwcap.h>
>>>>>
>>>>>  static inline void cpu_relax(void)
>>>>>  {
>>>>> +     if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>>>>>  #ifdef __riscv_muldiv
>>>>> -     int dummy;
>>>>> -     /* In lieu of a halt instruction, induce a long-latency stall. */
>>>>> -     __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>>> +             int dummy;
>>>>> +             /* In lieu of a halt instruction, induce a long-latency stall. */
>>>>> +             __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>>>  #endif
>>>>> +     } else {
>>>>> +             /*
>>>>> +              * Reduce instruction retirement.
>>>>> +              * This assumes the PC changes.
>>>>> +              */
>>>>> +#ifdef __riscv_zihintpause
>>>>> +             __asm__ __volatile__ ("pause");
>>>>> +#else
>>>>> +             /* Encoding of the pause instruction */
>>>>> +             __asm__ __volatile__ (".4byte 0x100000F");
>>>>> +#endif
>>>>> +     }
>>>>>       barrier();
>>>>>  }
>>>>>
>>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>>> index fba9e9f46a8c..a123e92b14dd 100644
>>>>> --- a/arch/riscv/kernel/cpu.c
>>>>> +++ b/arch/riscv/kernel/cpu.c
>>>>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>>>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>>>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>>>> +     __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>>>>  };
>>>>>
>>>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>>>> index 1b3ec44e25f5..708df2c0bc34 100644
>>>>> --- a/arch/riscv/kernel/cpufeature.c
>>>>> +++ b/arch/riscv/kernel/cpufeature.c
>>>>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>>>>                       } else {
>>>>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>>>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>>>>> +                             SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>>>>                       }
>>>>>  #undef SET_ISA_EXT_MAP
>>>>>               }
>>>>
>>>> Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.
>>>
>>> This breaks the C=1 build for all toolchains, not just new ones as your sparse
>>> patch suggests. I amn't 100% what my CI is running, but I replicated this on
>>> my own machine with:
>>
>> Argh, I went poking around and my toolchain's binutils etc is newer than I thought.
>> Good for people searching on lore I suppose...
>> Sorry!
> 
> So just to be clear: you're saying this only breaks with new binutils?

Yes, sorry - I *thought* the binutils in our CI predated Zihintpause but
it doesn't. It (and so would the Zicbom stuff) breaks the builds in our CI
as they run with C=1. I manually patched sparse to get that going again & I
/suspect/ it may have impacted LKP too.

"New" is relative though - it breaks C=1 for anyone running a toolchain
from riscv-collab/riscv-gnu-toolchain.

I guess it's just that Zicbom is newer so not many people will be on a
toolchain that supports that. My GCC doesn't only my clang-15.

> I ask because Dao is also seeing some crashes, if it's breaking arbitrary
> builds too then it's a stronger hint to revert it.

It is breaking module loading on RISC-V in general from what it seems.
See:
https://lore.kernel.org/linux-riscv/728ecbd5-975a-168e-efab-3c0030be21d5@w6rz.net/

Thanks,
Conor.

> 
>> Conor.
>>
>>>
>>> sparse --version
>>> 0.6.4 (Ubuntu: 0.6.4-2)
>>>
>>> ---8<---
>>>    YACC    scripts/dtc/dtc-parser.tab.[ch]
>>>    HOSTCC  scripts/dtc/libfdt/fdt.o
>>>    HOSTCC  scripts/dtc/libfdt/fdt_ro.o
>>>    HOSTCC  scripts/dtc/libfdt/fdt_wip.o
>>>    UPD     include/generated/uapi/linux/version.h
>>>    HOSTCC  scripts/dtc/libfdt/fdt_sw.o
>>>    UPD     include/config/kernel.release
>>>    HOSTCC  scripts/dtc/libfdt/fdt_rw.o
>>>    HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
>>>    HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
>>>    HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
>>>    HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
>>>    HOSTCC  scripts/dtc/fdtoverlay.o
>>>    HOSTCC  scripts/dtc/dtc-lexer.lex.o
>>>    HOSTCC  scripts/dtc/dtc-parser.tab.o
>>>    UPD     include/generated/utsrelease.h
>>>    HOSTLD  scripts/dtc/fdtoverlay
>>>    HOSTLD  scripts/dtc/dtc
>>>    HOSTCC  scripts/kallsyms
>>>    HOSTCC  scripts/sorttable
>>>    HOSTCC  scripts/asn1_compiler
>>>    HOSTCC  scripts/selinux/genheaders/genheaders
>>>    HOSTCC  scripts/selinux/mdp/mdp
>>>    CC      scripts/mod/empty.o
>>>    HOSTCC  scripts/mod/mk_elfconfig
>>>    CC      scripts/mod/devicetable-offsets.s
>>>    CHECK   ../scripts/mod/empty.c
>>> invalid argument to '-march': '_zihintpause'
>>>
>>> make[2]: *** [../scripts/Makefile.build:250: scripts/mod/empty.o] Error 1
>>> make[2]: *** Deleting file 'scripts/mod/empty.o'
>>> make[2]: *** Waiting for unfinished jobs....
>>> make[1]: *** [/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/Makefile:1287: prepare0] Error 2
>>> make[1]: Leaving directory '/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/builddir'
>>> make: *** [Makefile:231: __sub-make] Error 2
>>
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Andrew Jones Aug. 16, 2022, 4:18 p.m. UTC | #13
On Tue, Aug 16, 2022 at 04:04:06PM +0000, Conor.Dooley@microchip.com wrote:
> On 16/08/2022 16:54, Palmer Dabbelt wrote:
> > On Fri, 12 Aug 2022 00:21:40 PDT (-0700), Conor.Dooley@microchip.com wrote:
> >> On 12/08/2022 07:57, Conor Dooley - M52691 wrote:
> >>> On 11/08/2022 16:17, Palmer Dabbelt wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>
> >>>> On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
> >>>>> Implement support for the ZiHintPause extension.
> >>>>>
> >>>>> The PAUSE instruction is a HINT that indicates the current hart’s rate
> >>>>> of instruction retirement should be temporarily reduced or paused.
> >>>>>
> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
> >>>>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
> >>>>> ---
> >>>>>
> >>>>> v1 -> v2:
> >>>>>  Remove the usage of static branch, use PAUSE if toolchain supports it
> >>>>> v2 -> v3:
> >>>>>  Added the static branch back, cpu_relax() behavior is kept the same for
> >>>>> systems that do not support ZiHintPause
> >>>>> v3 -> v4:
> >>>>>  Adopted the newly added unified static keys for extensions
> >>>>> ---
> >>>>>  arch/riscv/Makefile                     |  4 ++++
> >>>>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
> >>>>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
> >>>>>  arch/riscv/kernel/cpu.c                 |  1 +
> >>>>>  arch/riscv/kernel/cpufeature.c          |  1 +
> >>>>>  5 files changed, 29 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> >>>>> index 34cf8a598617..6ddacc6f44b9 100644
> >>>>> --- a/arch/riscv/Makefile
> >>>>> +++ b/arch/riscv/Makefile
> >>>>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)  := $(riscv-march-y)c
> >>>>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
> >>>>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> >>>>>
> >>>>> +# Check if the toolchain supports Zihintpause extension
> >>>>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> >>>>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> >>>>> +
> >>>>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
> >>>>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
> >>>>>
> >>>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> >>>>> index e48eebdd2631..dc47019a0b38 100644
> >>>>> --- a/arch/riscv/include/asm/hwcap.h
> >>>>> +++ b/arch/riscv/include/asm/hwcap.h
> >>>>> @@ -8,6 +8,7 @@
> >>>>>  #ifndef _ASM_RISCV_HWCAP_H
> >>>>>  #define _ASM_RISCV_HWCAP_H
> >>>>>
> >>>>> +#include <asm/errno.h>
> >>>>>  #include <linux/bits.h>
> >>>>>  #include <uapi/asm/hwcap.h>
> >>>>>
> >>>>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
> >>>>>  enum riscv_isa_ext_id {
> >>>>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> >>>>>       RISCV_ISA_EXT_SVPBMT,
> >>>>> +     RISCV_ISA_EXT_ZIHINTPAUSE,
> >>>>>       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> >>>>>  };
> >>>>>
> >>>>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
> >>>>>   */
> >>>>>  enum riscv_isa_ext_key {
> >>>>>       RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
> >>>>> +     RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
> >>>>>       RISCV_ISA_EXT_KEY_MAX,
> >>>>>  };
> >>>>>
> >>>>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
> >>>>>               return RISCV_ISA_EXT_KEY_FPU;
> >>>>>       case RISCV_ISA_EXT_d:
> >>>>>               return RISCV_ISA_EXT_KEY_FPU;
> >>>>> +     case RISCV_ISA_EXT_ZIHINTPAUSE:
> >>>>> +             return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
> >>>>>       default:
> >>>>>               return -EINVAL;
> >>>>>       }
> >>>>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> >>>>> index 134388cbaaa1..1e4f8b4aef79 100644
> >>>>> --- a/arch/riscv/include/asm/vdso/processor.h
> >>>>> +++ b/arch/riscv/include/asm/vdso/processor.h
> >>>>> @@ -4,15 +4,30 @@
> >>>>>
> >>>>>  #ifndef __ASSEMBLY__
> >>>>>
> >>>>> +#include <linux/jump_label.h>
> >>>>>  #include <asm/barrier.h>
> >>>>> +#include <asm/hwcap.h>
> >>>>>
> >>>>>  static inline void cpu_relax(void)
> >>>>>  {
> >>>>> +     if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
> >>>>>  #ifdef __riscv_muldiv
> >>>>> -     int dummy;
> >>>>> -     /* In lieu of a halt instruction, induce a long-latency stall. */
> >>>>> -     __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> >>>>> +             int dummy;
> >>>>> +             /* In lieu of a halt instruction, induce a long-latency stall. */
> >>>>> +             __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> >>>>>  #endif
> >>>>> +     } else {
> >>>>> +             /*
> >>>>> +              * Reduce instruction retirement.
> >>>>> +              * This assumes the PC changes.
> >>>>> +              */
> >>>>> +#ifdef __riscv_zihintpause
> >>>>> +             __asm__ __volatile__ ("pause");
> >>>>> +#else
> >>>>> +             /* Encoding of the pause instruction */
> >>>>> +             __asm__ __volatile__ (".4byte 0x100000F");
> >>>>> +#endif
> >>>>> +     }
> >>>>>       barrier();
> >>>>>  }
> >>>>>
> >>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> >>>>> index fba9e9f46a8c..a123e92b14dd 100644
> >>>>> --- a/arch/riscv/kernel/cpu.c
> >>>>> +++ b/arch/riscv/kernel/cpu.c
> >>>>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
> >>>>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
> >>>>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >>>>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> >>>>> +     __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> >>>>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> >>>>>  };
> >>>>>
> >>>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >>>>> index 1b3ec44e25f5..708df2c0bc34 100644
> >>>>> --- a/arch/riscv/kernel/cpufeature.c
> >>>>> +++ b/arch/riscv/kernel/cpufeature.c
> >>>>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
> >>>>>                       } else {
> >>>>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> >>>>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> >>>>> +                             SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >>>>>                       }
> >>>>>  #undef SET_ISA_EXT_MAP
> >>>>>               }
> >>>>
> >>>> Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.
> >>>
> >>> This breaks the C=1 build for all toolchains, not just new ones as your sparse
> >>> patch suggests. I amn't 100% what my CI is running, but I replicated this on
> >>> my own machine with:
> >>
> >> Argh, I went poking around and my toolchain's binutils etc is newer than I thought.
> >> Good for people searching on lore I suppose...
> >> Sorry!
> > 
> > So just to be clear: you're saying this only breaks with new binutils?
> 
> Yes, sorry - I *thought* the binutils in our CI predated Zihintpause but
> it doesn't. It (and so would the Zicbom stuff) breaks the builds in our CI
> as they run with C=1. I manually patched sparse to get that going again & I
> /suspect/ it may have impacted LKP too.
> 
> "New" is relative though - it breaks C=1 for anyone running a toolchain
> from riscv-collab/riscv-gnu-toolchain.
> 
> I guess it's just that Zicbom is newer so not many people will be on a
> toolchain that supports that. My GCC doesn't only my clang-15.
> 
> > I ask because Dao is also seeing some crashes, if it's breaking arbitrary
> > builds too then it's a stronger hint to revert it.
> 
> It is breaking module loading on RISC-V in general from what it seems.
> See:
> https://lore.kernel.org/linux-riscv/728ecbd5-975a-168e-efab-3c0030be21d5@w6rz.net/

Hi Conor,

I have a patch for this which I'll send in just a second.

Thanks,
drew

> 
> Thanks,
> Conor.
> 
> > 
> >> Conor.
> >>
> >>>
> >>> sparse --version
> >>> 0.6.4 (Ubuntu: 0.6.4-2)
> >>>
> >>> ---8<---
> >>>    YACC    scripts/dtc/dtc-parser.tab.[ch]
> >>>    HOSTCC  scripts/dtc/libfdt/fdt.o
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_ro.o
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_wip.o
> >>>    UPD     include/generated/uapi/linux/version.h
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_sw.o
> >>>    UPD     include/config/kernel.release
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_rw.o
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
> >>>    HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
> >>>    HOSTCC  scripts/dtc/fdtoverlay.o
> >>>    HOSTCC  scripts/dtc/dtc-lexer.lex.o
> >>>    HOSTCC  scripts/dtc/dtc-parser.tab.o
> >>>    UPD     include/generated/utsrelease.h
> >>>    HOSTLD  scripts/dtc/fdtoverlay
> >>>    HOSTLD  scripts/dtc/dtc
> >>>    HOSTCC  scripts/kallsyms
> >>>    HOSTCC  scripts/sorttable
> >>>    HOSTCC  scripts/asn1_compiler
> >>>    HOSTCC  scripts/selinux/genheaders/genheaders
> >>>    HOSTCC  scripts/selinux/mdp/mdp
> >>>    CC      scripts/mod/empty.o
> >>>    HOSTCC  scripts/mod/mk_elfconfig
> >>>    CC      scripts/mod/devicetable-offsets.s
> >>>    CHECK   ../scripts/mod/empty.c
> >>> invalid argument to '-march': '_zihintpause'
> >>>
> >>> make[2]: *** [../scripts/Makefile.build:250: scripts/mod/empty.o] Error 1
> >>> make[2]: *** Deleting file 'scripts/mod/empty.o'
> >>> make[2]: *** Waiting for unfinished jobs....
> >>> make[1]: *** [/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/Makefile:1287: prepare0] Error 2
> >>> make[1]: Leaving directory '/mnt/automation/corp/workspace/ux-test_upstream-next-develop-cd@2/linux/builddir'
> >>> make: *** [Makefile:231: __sub-make] Error 2
> >>
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Samuel Holland Aug. 19, 2022, 5 a.m. UTC | #14
On 8/16/22 11:04 AM, Conor.Dooley@microchip.com wrote:
> On 16/08/2022 16:54, Palmer Dabbelt wrote:
>> On Fri, 12 Aug 2022 00:21:40 PDT (-0700), Conor.Dooley@microchip.com wrote:
>>> On 12/08/2022 07:57, Conor Dooley - M52691 wrote:
>>>> On 11/08/2022 16:17, Palmer Dabbelt wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On Mon, 20 Jun 2022 13:15:25 PDT (-0700), daolu@rivosinc.com wrote:
>>>>>> Implement support for the ZiHintPause extension.
>>>>>>
>>>>>> The PAUSE instruction is a HINT that indicates the current hart’s rate
>>>>>> of instruction retirement should be temporarily reduced or paused.
>>>>>>
>>>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>>>> Signed-off-by: Dao Lu <daolu@rivosinc.com>
>>>>>> ---
>>>>>>
>>>>>> v1 -> v2:
>>>>>>  Remove the usage of static branch, use PAUSE if toolchain supports it
>>>>>> v2 -> v3:
>>>>>>  Added the static branch back, cpu_relax() behavior is kept the same for
>>>>>> systems that do not support ZiHintPause
>>>>>> v3 -> v4:
>>>>>>  Adopted the newly added unified static keys for extensions
>>>>>> ---
>>>>>>  arch/riscv/Makefile                     |  4 ++++
>>>>>>  arch/riscv/include/asm/hwcap.h          |  5 +++++
>>>>>>  arch/riscv/include/asm/vdso/processor.h | 21 ++++++++++++++++++---
>>>>>>  arch/riscv/kernel/cpu.c                 |  1 +
>>>>>>  arch/riscv/kernel/cpufeature.c          |  1 +
>>>>>>  5 files changed, 29 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>>>>> index 34cf8a598617..6ddacc6f44b9 100644
>>>>>> --- a/arch/riscv/Makefile
>>>>>> +++ b/arch/riscv/Makefile
>>>>>> @@ -56,6 +56,10 @@ riscv-march-$(CONFIG_RISCV_ISA_C)  := $(riscv-march-y)c
>>>>>>  toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
>>>>>>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>>>>>>
>>>>>> +# Check if the toolchain supports Zihintpause extension
>>>>>> +toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
>>>>>> +riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
>>>>>> +
>>>>>>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>>>>>>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>>>>>>
>>>>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>>>>> index e48eebdd2631..dc47019a0b38 100644
>>>>>> --- a/arch/riscv/include/asm/hwcap.h
>>>>>> +++ b/arch/riscv/include/asm/hwcap.h
>>>>>> @@ -8,6 +8,7 @@
>>>>>>  #ifndef _ASM_RISCV_HWCAP_H
>>>>>>  #define _ASM_RISCV_HWCAP_H
>>>>>>
>>>>>> +#include <asm/errno.h>
>>>>>>  #include <linux/bits.h>
>>>>>>  #include <uapi/asm/hwcap.h>
>>>>>>
>>>>>> @@ -54,6 +55,7 @@ extern unsigned long elf_hwcap;
>>>>>>  enum riscv_isa_ext_id {
>>>>>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>>>>>       RISCV_ISA_EXT_SVPBMT,
>>>>>> +     RISCV_ISA_EXT_ZIHINTPAUSE,
>>>>>>       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>>>>>>  };
>>>>>>
>>>>>> @@ -64,6 +66,7 @@ enum riscv_isa_ext_id {
>>>>>>   */
>>>>>>  enum riscv_isa_ext_key {
>>>>>>       RISCV_ISA_EXT_KEY_FPU,          /* For 'F' and 'D' */
>>>>>> +     RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
>>>>>>       RISCV_ISA_EXT_KEY_MAX,
>>>>>>  };
>>>>>>
>>>>>> @@ -83,6 +86,8 @@ static __always_inline int riscv_isa_ext2key(int num)
>>>>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>>>>       case RISCV_ISA_EXT_d:
>>>>>>               return RISCV_ISA_EXT_KEY_FPU;
>>>>>> +     case RISCV_ISA_EXT_ZIHINTPAUSE:
>>>>>> +             return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
>>>>>>       default:
>>>>>>               return -EINVAL;
>>>>>>       }
>>>>>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>>>>>> index 134388cbaaa1..1e4f8b4aef79 100644
>>>>>> --- a/arch/riscv/include/asm/vdso/processor.h
>>>>>> +++ b/arch/riscv/include/asm/vdso/processor.h
>>>>>> @@ -4,15 +4,30 @@
>>>>>>
>>>>>>  #ifndef __ASSEMBLY__
>>>>>>
>>>>>> +#include <linux/jump_label.h>
>>>>>>  #include <asm/barrier.h>
>>>>>> +#include <asm/hwcap.h>
>>>>>>
>>>>>>  static inline void cpu_relax(void)
>>>>>>  {
>>>>>> +     if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
>>>>>>  #ifdef __riscv_muldiv
>>>>>> -     int dummy;
>>>>>> -     /* In lieu of a halt instruction, induce a long-latency stall. */
>>>>>> -     __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>>>> +             int dummy;
>>>>>> +             /* In lieu of a halt instruction, induce a long-latency stall. */
>>>>>> +             __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>>>>  #endif
>>>>>> +     } else {
>>>>>> +             /*
>>>>>> +              * Reduce instruction retirement.
>>>>>> +              * This assumes the PC changes.
>>>>>> +              */
>>>>>> +#ifdef __riscv_zihintpause
>>>>>> +             __asm__ __volatile__ ("pause");
>>>>>> +#else
>>>>>> +             /* Encoding of the pause instruction */
>>>>>> +             __asm__ __volatile__ (".4byte 0x100000F");
>>>>>> +#endif
>>>>>> +     }
>>>>>>       barrier();
>>>>>>  }
>>>>>>
>>>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>>>> index fba9e9f46a8c..a123e92b14dd 100644
>>>>>> --- a/arch/riscv/kernel/cpu.c
>>>>>> +++ b/arch/riscv/kernel/cpu.c
>>>>>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>>>>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>>>>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>>>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>>>>> +     __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>>>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>>>>>  };
>>>>>>
>>>>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>>>>> index 1b3ec44e25f5..708df2c0bc34 100644
>>>>>> --- a/arch/riscv/kernel/cpufeature.c
>>>>>> +++ b/arch/riscv/kernel/cpufeature.c
>>>>>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>>>>>                       } else {
>>>>>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>>>>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>>>>>> +                             SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>>>>>                       }
>>>>>>  #undef SET_ISA_EXT_MAP
>>>>>>               }
>>>>>
>>>>> Thanks, this is on for-next.  It needs a sparse patch, which I put in as a link.
>>>>
>>>> This breaks the C=1 build for all toolchains, not just new ones as your sparse
>>>> patch suggests. I amn't 100% what my CI is running, but I replicated this on
>>>> my own machine with:
>>>
>>> Argh, I went poking around and my toolchain's binutils etc is newer than I thought.
>>> Good for people searching on lore I suppose...
>>> Sorry!
>>
>> So just to be clear: you're saying this only breaks with new binutils?
> 
> Yes, sorry - I *thought* the binutils in our CI predated Zihintpause but
> it doesn't. It (and so would the Zicbom stuff) breaks the builds in our CI
> as they run with C=1. I manually patched sparse to get that going again & I
> /suspect/ it may have impacted LKP too.
> 
> "New" is relative though - it breaks C=1 for anyone running a toolchain
> from riscv-collab/riscv-gnu-toolchain.
> 
> I guess it's just that Zicbom is newer so not many people will be on a
> toolchain that supports that. My GCC doesn't only my clang-15.
> 
>> I ask because Dao is also seeing some crashes, if it's breaking arbitrary
>> builds too then it's a stronger hint to revert it.
> 
> It is breaking module loading on RISC-V in general from what it seems.
> See:
> https://lore.kernel.org/linux-riscv/728ecbd5-975a-168e-efab-3c0030be21d5@w6rz.net/

This patch also completely breaks the build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y:

In file included from <command-line>:
./arch/riscv/include/asm/jump_label.h: In function 'cpu_relax':
././include/linux/compiler_types.h:285:33: warning: 'asm' operand 0 probably
does not match constraints
  285 | #define asm_volatile_goto(x...) asm goto(x)
      |                                 ^~~
./arch/riscv/include/asm/jump_label.h:41:9: note: in expansion of macro
'asm_volatile_goto'
   41 |         asm_volatile_goto(
      |         ^~~~~~~~~~~~~~~~~
././include/linux/compiler_types.h:285:33: error: impossible constraint in 'asm'
  285 | #define asm_volatile_goto(x...) asm goto(x)
      |                                 ^~~
./arch/riscv/include/asm/jump_label.h:41:9: note: in expansion of macro
'asm_volatile_goto'
   41 |         asm_volatile_goto(
      |         ^~~~~~~~~~~~~~~~~
make[1]: *** [scripts/Makefile.build:249:
arch/riscv/kernel/vdso/vgettimeofday.o] Error 1
make: *** [arch/riscv/Makefile:128: vdso_prepare] Error 2

Putting a static branch in such a widely-inlined function seems like a bad idea
in general. (A quick measurement of my somewhat-minimal config shows this single
static branch as responsible for 40% of the jump table.)

I don't think it's even necessary in this case. Why can't we unconditionally run
both instructions? If Zihintpause is supported, we trade the nop from the static
branch for a div. If it is unsupported, we trade the jump from the static branch
for a nop.

Regards,
Samuel
diff mbox series

Patch

diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 34cf8a598617..6ddacc6f44b9 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -56,6 +56,10 @@  riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
 toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
 riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
 
+# Check if the toolchain supports Zihintpause extension
+toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
+riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
+
 KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
 KBUILD_AFLAGS += -march=$(riscv-march-y)
 
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e48eebdd2631..dc47019a0b38 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -8,6 +8,7 @@ 
 #ifndef _ASM_RISCV_HWCAP_H
 #define _ASM_RISCV_HWCAP_H
 
+#include <asm/errno.h>
 #include <linux/bits.h>
 #include <uapi/asm/hwcap.h>
 
@@ -54,6 +55,7 @@  extern unsigned long elf_hwcap;
 enum riscv_isa_ext_id {
 	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
 	RISCV_ISA_EXT_SVPBMT,
+	RISCV_ISA_EXT_ZIHINTPAUSE,
 	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
 };
 
@@ -64,6 +66,7 @@  enum riscv_isa_ext_id {
  */
 enum riscv_isa_ext_key {
 	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
+	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
 	RISCV_ISA_EXT_KEY_MAX,
 };
 
@@ -83,6 +86,8 @@  static __always_inline int riscv_isa_ext2key(int num)
 		return RISCV_ISA_EXT_KEY_FPU;
 	case RISCV_ISA_EXT_d:
 		return RISCV_ISA_EXT_KEY_FPU;
+	case RISCV_ISA_EXT_ZIHINTPAUSE:
+		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
 	default:
 		return -EINVAL;
 	}
diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
index 134388cbaaa1..1e4f8b4aef79 100644
--- a/arch/riscv/include/asm/vdso/processor.h
+++ b/arch/riscv/include/asm/vdso/processor.h
@@ -4,15 +4,30 @@ 
 
 #ifndef __ASSEMBLY__
 
+#include <linux/jump_label.h>
 #include <asm/barrier.h>
+#include <asm/hwcap.h>
 
 static inline void cpu_relax(void)
 {
+	if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
 #ifdef __riscv_muldiv
-	int dummy;
-	/* In lieu of a halt instruction, induce a long-latency stall. */
-	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
+		int dummy;
+		/* In lieu of a halt instruction, induce a long-latency stall. */
+		__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
 #endif
+	} else {
+		/*
+		 * Reduce instruction retirement.
+		 * This assumes the PC changes.
+		 */
+#ifdef __riscv_zihintpause
+		__asm__ __volatile__ ("pause");
+#else
+		/* Encoding of the pause instruction */
+		__asm__ __volatile__ (".4byte 0x100000F");
+#endif
+	}
 	barrier();
 }
 
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index fba9e9f46a8c..a123e92b14dd 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -89,6 +89,7 @@  int riscv_of_parent_hartid(struct device_node *node)
 static struct riscv_isa_ext_data isa_ext_arr[] = {
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
+	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
 	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
 };
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 1b3ec44e25f5..708df2c0bc34 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -198,6 +198,7 @@  void __init riscv_fill_hwcap(void)
 			} else {
 				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
 				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
+				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
 			}
 #undef SET_ISA_EXT_MAP
 		}