diff mbox series

[v1] riscv: Using TOOLCHAIN_HAS_ZIHINTPAUSE marco replace zihintpause

Message ID 20230802064215.31111-1-minda.chen@starfivetech.com
State New
Headers
Series [v1] riscv: Using TOOLCHAIN_HAS_ZIHINTPAUSE marco replace zihintpause |

Commit Message

Minda Chen Aug. 2, 2023, 6:42 a.m. UTC 
  Actually it is a part of Conor's
commit aae538cd03bc ("riscv: fix detection of toolchain
Zihintpause support").
It is looks like a merge issue. Samuel's
commit 0b1d60d6dd9e ("riscv: Fix build with
CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
revert to __riscv_zihintpause. So this patch can fix it.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 arch/riscv/include/asm/vdso/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Conor Dooley Aug. 2, 2023, 6:54 a.m. UTC | #1 
Hey Minda,

On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
> Actually it is a part of Conor's
> commit aae538cd03bc ("riscv: fix detection of toolchain
> Zihintpause support").
> It is looks like a merge issue.

Yup, spot on.

> Samuel's
> commit 0b1d60d6dd9e ("riscv: Fix build with
> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
> revert to __riscv_zihintpause. So this patch can fix it.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>

Did you actually manage to trigger this, or was this by inspection?
clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
what the clang-built-linux CI uses to test the LTS kernels from before
LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
satisfied there is that zihintpause doesn't appear in -march so this has
gone unnoticed.

Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
>  arch/riscv/include/asm/vdso/processor.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 14f5d27783b8..96b65a5396df 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -14,7 +14,7 @@ static inline void cpu_relax(void)
>  	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>  #endif
>  
> -#ifdef __riscv_zihintpause
> +#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
>  	/*
>  	 * Reduce instruction retirement.
>  	 * This assumes the PC changes.
> -- 
> 2.17.1
>
Minda Chen Aug. 2, 2023, 7:32 a.m. UTC | #2 
On 2023/8/2 14:54, Conor Dooley wrote:
> Hey Minda,
> 
> On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
>> Actually it is a part of Conor's
>> commit aae538cd03bc ("riscv: fix detection of toolchain
>> Zihintpause support").
>> It is looks like a merge issue.
> 
> Yup, spot on.
> 
>> Samuel's
>> commit 0b1d60d6dd9e ("riscv: Fix build with
>> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
>> revert to __riscv_zihintpause. So this patch can fix it.
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> 
> Did you actually manage to trigger this, or was this by inspection?
> clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
> what the clang-built-linux CI uses to test the LTS kernels from before
> LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
> satisfied there is that zihintpause doesn't appear in -march so this has
> gone unnoticed.
> 
> Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks,
> Conor.
> 
Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
of processor.h found this issue.
>> ---
>>  arch/riscv/include/asm/vdso/processor.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
>> index 14f5d27783b8..96b65a5396df 100644
>> --- a/arch/riscv/include/asm/vdso/processor.h
>> +++ b/arch/riscv/include/asm/vdso/processor.h
>> @@ -14,7 +14,7 @@ static inline void cpu_relax(void)
>>  	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>  #endif
>>  
>> -#ifdef __riscv_zihintpause
>> +#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
>>  	/*
>>  	 * Reduce instruction retirement.
>>  	 * This assumes the PC changes.
>> -- 
>> 2.17.1
>>
Conor Dooley Aug. 2, 2023, 7:48 a.m. UTC | #3 
On Wed, Aug 02, 2023 at 03:32:15PM +0800, Minda Chen wrote:
> 
> 
> On 2023/8/2 14:54, Conor Dooley wrote:
> > Hey Minda,
> > 
> > On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
> >> Actually it is a part of Conor's
> >> commit aae538cd03bc ("riscv: fix detection of toolchain
> >> Zihintpause support").
> >> It is looks like a merge issue.
> > 
> > Yup, spot on.
> > 
> >> Samuel's
> >> commit 0b1d60d6dd9e ("riscv: Fix build with
> >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
> >> revert to __riscv_zihintpause. So this patch can fix it.
> >> 
> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> > 
> > Did you actually manage to trigger this, or was this by inspection?
> > clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
> > what the clang-built-linux CI uses to test the LTS kernels from before
> > LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
> > satisfied there is that zihintpause doesn't appear in -march so this has
> > gone unnoticed.
> > 
> > Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Thanks,
> > Conor.
> > 
> Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
> cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
> of processor.h found this issue.

That doesn't look like it is fixed in later stable kernels (we are at
6.1.42-rcN right now I think). It sounds we should ask Greg to backport
0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")
to 6.1. Does that make sense to you?
Minda Chen Aug. 2, 2023, 8:17 a.m. UTC | #4 
On 2023/8/2 15:48, Conor Dooley wrote:
> On Wed, Aug 02, 2023 at 03:32:15PM +0800, Minda Chen wrote:
>> 
>> 
>> On 2023/8/2 14:54, Conor Dooley wrote:
>> > Hey Minda,
>> > 
>> > On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
>> >> Actually it is a part of Conor's
>> >> commit aae538cd03bc ("riscv: fix detection of toolchain
>> >> Zihintpause support").
>> >> It is looks like a merge issue.
>> > 
>> > Yup, spot on.
>> > 
>> >> Samuel's
>> >> commit 0b1d60d6dd9e ("riscv: Fix build with
>> >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
>> >> revert to __riscv_zihintpause. So this patch can fix it.
>> >> 
>> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> > 
>> > Did you actually manage to trigger this, or was this by inspection?
>> > clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
>> > what the clang-built-linux CI uses to test the LTS kernels from before
>> > LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
>> > satisfied there is that zihintpause doesn't appear in -march so this has
>> > gone unnoticed.
>> > 
>> > Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
>> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> > 
>> > Thanks,
>> > Conor.
>> > 
>> Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
>> cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
>> of processor.h found this issue.
> 
> That doesn't look like it is fixed in later stable kernels (we are at
> 6.1.42-rcN right now I think). It sounds we should ask Greg to backport
> 0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")
> to 6.1. Does that make sense to you?
Yes. 6.1 is lts kernel. Starfive will use this kernel for a long time. Thanks.
Greg KH Aug. 2, 2023, 8:33 a.m. UTC | #5 
On Wed, Aug 02, 2023 at 04:17:51PM +0800, Minda Chen wrote:
> 
> 
> On 2023/8/2 15:48, Conor Dooley wrote:
> > On Wed, Aug 02, 2023 at 03:32:15PM +0800, Minda Chen wrote:
> >> 
> >> 
> >> On 2023/8/2 14:54, Conor Dooley wrote:
> >> > Hey Minda,
> >> > 
> >> > On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
> >> >> Actually it is a part of Conor's
> >> >> commit aae538cd03bc ("riscv: fix detection of toolchain
> >> >> Zihintpause support").
> >> >> It is looks like a merge issue.
> >> > 
> >> > Yup, spot on.
> >> > 
> >> >> Samuel's
> >> >> commit 0b1d60d6dd9e ("riscv: Fix build with
> >> >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
> >> >> revert to __riscv_zihintpause. So this patch can fix it.
> >> >> 
> >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> >> > 
> >> > Did you actually manage to trigger this, or was this by inspection?
> >> > clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
> >> > what the clang-built-linux CI uses to test the LTS kernels from before
> >> > LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
> >> > satisfied there is that zihintpause doesn't appear in -march so this has
> >> > gone unnoticed.
> >> > 
> >> > Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
> >> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >> > 
> >> > Thanks,
> >> > Conor.
> >> > 
> >> Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
> >> cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
> >> of processor.h found this issue.
> > 
> > That doesn't look like it is fixed in later stable kernels (we are at
> > 6.1.42-rcN right now I think). It sounds we should ask Greg to backport
> > 0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")
> > to 6.1. Does that make sense to you?
> Yes. 6.1 is lts kernel. Starfive will use this kernel for a long time. Thanks.

What is preventing you from moving to a newer kernel version?  All of
your kernel changes are already properly merged into Linus's tree,
right?

thanks,

greg k-h
Conor Dooley Aug. 2, 2023, 8:52 a.m. UTC | #6 
On Wed, Aug 02, 2023 at 10:33:27AM +0200, Greg KH wrote:
> On Wed, Aug 02, 2023 at 04:17:51PM +0800, Minda Chen wrote:
> > 
> > 
> > On 2023/8/2 15:48, Conor Dooley wrote:
> > > On Wed, Aug 02, 2023 at 03:32:15PM +0800, Minda Chen wrote:
> > >> 
> > >> 
> > >> On 2023/8/2 14:54, Conor Dooley wrote:
> > >> > Hey Minda,
> > >> > 
> > >> > On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
> > >> >> Actually it is a part of Conor's
> > >> >> commit aae538cd03bc ("riscv: fix detection of toolchain
> > >> >> Zihintpause support").
> > >> >> It is looks like a merge issue.
> > >> > 
> > >> > Yup, spot on.
> > >> > 
> > >> >> Samuel's
> > >> >> commit 0b1d60d6dd9e ("riscv: Fix build with
> > >> >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
> > >> >> revert to __riscv_zihintpause. So this patch can fix it.
> > >> >> 
> > >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> > >> > 
> > >> > Did you actually manage to trigger this, or was this by inspection?
> > >> > clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
> > >> > what the clang-built-linux CI uses to test the LTS kernels from before
> > >> > LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
> > >> > satisfied there is that zihintpause doesn't appear in -march so this has
> > >> > gone unnoticed.
> > >> > 
> > >> > Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
> > >> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > >> > 
> > >> > Thanks,
> > >> > Conor.
> > >> > 
> > >> Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
> > >> cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
> > >> of processor.h found this issue.
> > > 
> > > That doesn't look like it is fixed in later stable kernels (we are at
> > > 6.1.42-rcN right now I think). It sounds we should ask Greg to backport
> > > 0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")
> > > to 6.1. Does that make sense to you?
> > Yes. 6.1 is lts kernel. Starfive will use this kernel for a long time. Thanks.
> 
> What is preventing you from moving to a newer kernel version?  All of
> your kernel changes are already properly merged into Linus's tree,
> right?

Regardless of their reasons, "vdso.so call cpu_relax cause application
core dump" is something that we should fix in stable kernels, no?
Greg KH Aug. 2, 2023, 9:42 a.m. UTC | #7 
On Wed, Aug 02, 2023 at 09:52:45AM +0100, Conor Dooley wrote:
> On Wed, Aug 02, 2023 at 10:33:27AM +0200, Greg KH wrote:
> > On Wed, Aug 02, 2023 at 04:17:51PM +0800, Minda Chen wrote:
> > > 
> > > 
> > > On 2023/8/2 15:48, Conor Dooley wrote:
> > > > On Wed, Aug 02, 2023 at 03:32:15PM +0800, Minda Chen wrote:
> > > >> 
> > > >> 
> > > >> On 2023/8/2 14:54, Conor Dooley wrote:
> > > >> > Hey Minda,
> > > >> > 
> > > >> > On Wed, Aug 02, 2023 at 02:42:15PM +0800, Minda Chen wrote:
> > > >> >> Actually it is a part of Conor's
> > > >> >> commit aae538cd03bc ("riscv: fix detection of toolchain
> > > >> >> Zihintpause support").
> > > >> >> It is looks like a merge issue.
> > > >> > 
> > > >> > Yup, spot on.
> > > >> > 
> > > >> >> Samuel's
> > > >> >> commit 0b1d60d6dd9e ("riscv: Fix build with
> > > >> >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
> > > >> >> revert to __riscv_zihintpause. So this patch can fix it.
> > > >> >> 
> > > >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> > > >> > 
> > > >> > Did you actually manage to trigger this, or was this by inspection?
> > > >> > clang-15 + binutils 2.35 was, IIRC, how we spotted this because that's
> > > >> > what the clang-built-linux CI uses to test the LTS kernels from before
> > > >> > LLVM's IAS was supported for RISC-V. Seemingly all that needs to be
> > > >> > satisfied there is that zihintpause doesn't appear in -march so this has
> > > >> > gone unnoticed.
> > > >> > 
> > > >> > Fixes: 3c349eacc559 ("Merge patch "riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y"")
> > > >> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > >> > 
> > > >> > Thanks,
> > > >> > Conor.
> > > >> > 
> > > >> Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
> > > >> cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
> > > >> of processor.h found this issue.
> > > > 
> > > > That doesn't look like it is fixed in later stable kernels (we are at
> > > > 6.1.42-rcN right now I think). It sounds we should ask Greg to backport
> > > > 0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")
> > > > to 6.1. Does that make sense to you?
> > > Yes. 6.1 is lts kernel. Starfive will use this kernel for a long time. Thanks.
> > 
> > What is preventing you from moving to a newer kernel version?  All of
> > your kernel changes are already properly merged into Linus's tree,
> > right?
> 
> Regardless of their reasons, "vdso.so call cpu_relax cause application
> core dump" is something that we should fix in stable kernels, no?

Yes.
Conor Dooley Aug. 2, 2023, 9:50 a.m. UTC | #8 
On Wed, Aug 02, 2023 at 11:42:59AM +0200, Greg KH wrote:
> On Wed, Aug 02, 2023 at 09:52:45AM +0100, Conor Dooley wrote:
> > On Wed, Aug 02, 2023 at 10:33:27AM +0200, Greg KH wrote:
> > > On Wed, Aug 02, 2023 at 04:17:51PM +0800, Minda Chen wrote:
> > > > On 2023/8/2 15:48, Conor Dooley wrote:
> > > > > On Wed, Aug 02, 2023 at 03:32:15PM +0800, Minda Chen wrote:
> > > > >> On 2023/8/2 14:54, Conor Dooley wrote:

> > > > >> Thanks, Conor. I found this just by inspection. I found a issue that vdso.so call cpu_relax
> > > > >> cause application core dump in kernel 6.1.31. I need Samuel'patch to fix this. And I search the log
> > > > >> of processor.h found this issue.
> > > > > 
> > > > > That doesn't look like it is fixed in later stable kernels (we are at
> > > > > 6.1.42-rcN right now I think). It sounds we should ask Greg to backport
> > > > > 0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")
> > > > > to 6.1. Does that make sense to you?
> > > > Yes. 6.1 is lts kernel. Starfive will use this kernel for a long time. Thanks.
> > > 
> > > What is preventing you from moving to a newer kernel version?  All of
> > > your kernel changes are already properly merged into Linus's tree,
> > > right?
> > 
> > Regardless of their reasons, "vdso.so call cpu_relax cause application
> > core dump" is something that we should fix in stable kernels, no?
> 
> Yes.

It doesn't apply cleanly as a cherry-pick onto linux-6.1, so it'll need
to be submitted. Maybe Minda can do that, since they've got an already
tested version of the patch. Failing that, I will.
patchwork-bot+linux-riscv@kernel.org Oct. 27, 2023, 1:20 p.m. UTC | #9 
Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed, 2 Aug 2023 14:42:15 +0800 you wrote:
> Actually it is a part of Conor's
> commit aae538cd03bc ("riscv: fix detection of toolchain
> Zihintpause support").
> It is looks like a merge issue. Samuel's
> commit 0b1d60d6dd9e ("riscv: Fix build with
> CONFIG_CC_OPTIMIZE_FOR_SIZE=y") do not base on Conor's commit and
> revert to __riscv_zihintpause. So this patch can fix it.
> 
> [...]

Here is the summary with links:
  - [v1] riscv: Using TOOLCHAIN_HAS_ZIHINTPAUSE marco replace zihintpause
    https://git.kernel.org/riscv/c/a09560a7b160

You are awesome, thank you!
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
index 14f5d27783b8..96b65a5396df 100644
--- a/arch/riscv/include/asm/vdso/processor.h
+++ b/arch/riscv/include/asm/vdso/processor.h
@@ -14,7 +14,7 @@  static inline void cpu_relax(void)
 	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
 #endif
 
-#ifdef __riscv_zihintpause
+#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
 	/*
 	 * Reduce instruction retirement.
 	 * This assumes the PC changes.