LinuxLists.cc - [PATCH v4] arch/riscv: add Zihintpause support

2022-06-20 20:28:44

Subject: [PATCH v4] arch/riscv: add Zihintpause support

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 <[email protected]>
Tested-by: Heiko Stuebner <[email protected]>
Signed-off-by: Dao Lu <[email protected]>
---

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

2022-07-05 17:04:59

by Dao Lu

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

ping

On Mon, Jun 20, 2022 at 1:15 PM Dao Lu <[email protected]> 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 <[email protected]>
> Tested-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Dao Lu <[email protected]>
> ---
>
> 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
>

2022-07-05 23:57:00

by Guo Ren

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

Reviewed-by: Guo Ren <[email protected]>

On Wed, Jul 6, 2022 at 12:57 AM Dao Lu <[email protected]> wrote:
>
> ping
>
> On Mon, Jun 20, 2022 at 1:15 PM Dao Lu <[email protected]> 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 <[email protected]>
> > Tested-by: Heiko Stuebner <[email protected]>
> > Signed-off-by: Dao Lu <[email protected]>
> > ---
> >
> > 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
> >

--
Best Regards
Guo Ren

2022-07-11 20:58:26

by Atish Patra

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

On Mon, Jun 20, 2022 at 1:15 PM Dao Lu <[email protected]> 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 <[email protected]>
> Tested-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Dao Lu <[email protected]>
> ---
>
> 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 <[email protected]>
--
Regards,
Atish

2022-07-11 21:00:49

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

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 <[email protected]>
> Tested-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Dao Lu <[email protected]>
> ---
>
> 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
> }

2022-07-16 13:37:03

by Jisheng Zhang

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

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 <[email protected]>
> Tested-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Dao Lu <[email protected]>

Reviewed-by: Jisheng Zhang<[email protected]>

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

2022-07-16 14:28:06

by Heiko Stuebner

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

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 <[email protected]>
> > Tested-by: Heiko Stuebner <[email protected]>
> > Signed-off-by: Dao Lu <[email protected]>
> > ---
> >
> > 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
> > }
>

2022-07-16 14:43:38

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

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 <[email protected]>
>>> Tested-by: Heiko Stuebner <[email protected]>
>>> Signed-off-by: Dao Lu <[email protected]>
>>> ---
>>>
>>> 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
>>> }
>>
>
>
>
>

2022-08-11 15:48:24

by Palmer Dabbelt

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

On Mon, 20 Jun 2022 13:15:25 PDT (-0700), [email protected] 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 <[email protected]>
> Tested-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Dao Lu <[email protected]>
> ---
>
> 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.

2022-08-12 07:19:37

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

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), [email protected] 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 <[email protected]>
>> Tested-by: Heiko Stuebner <[email protected]>
>> Signed-off-by: Dao Lu <[email protected]>
>> ---
>>
>> 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

2022-08-12 07:41:02

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

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), [email protected] 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 <[email protected]>
>>> Tested-by: Heiko Stuebner <[email protected]>
>>> Signed-off-by: Dao Lu <[email protected]>
>>> ---
>>>
>>> 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

2022-08-16 16:08:10

by Palmer Dabbelt

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

On Fri, 12 Aug 2022 00:21:40 PDT (-0700), [email protected] 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), [email protected] 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 <[email protected]>
>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>> Signed-off-by: Dao Lu <[email protected]>
>>>> ---
>>>>
>>>> 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
>

2022-08-16 16:40:35

by Andrew Jones

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

On Tue, Aug 16, 2022 at 04:04:06PM +0000, [email protected] wrote:
> On 16/08/2022 16:54, Palmer Dabbelt wrote:
> > On Fri, 12 Aug 2022 00:21:40 PDT (-0700), [email protected] 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), [email protected] 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 <[email protected]>
> >>>>> Tested-by: Heiko Stuebner <[email protected]>
> >>>>> Signed-off-by: Dao Lu <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> 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/[email protected]/

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
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2022-08-16 16:53:27

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4] arch/riscv: add Zihintpause support

On 16/08/2022 16:54, Palmer Dabbelt wrote:
> On Fri, 12 Aug 2022 00:21:40 PDT (-0700), [email protected] 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), [email protected] 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 <[email protected]>
>>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>>> Signed-off-by: Dao Lu <[email protected]>
>>>>> ---
>>>>>
>>>>> 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/[email protected]/

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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2022-08-19 05:25:55

by Samuel Holland

[permalink] [raw]

Subject: Re: Re: [PATCH v4] arch/riscv: add Zihintpause support

On 8/16/22 11:04 AM, [email protected] wrote:
> On 16/08/2022 16:54, Palmer Dabbelt wrote:
>> On Fri, 12 Aug 2022 00:21:40 PDT (-0700), [email protected] 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), [email protected] 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 <[email protected]>
>>>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>>>> Signed-off-by: Dao Lu <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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/[email protected]/

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