[PATCH v6] coresight: etm4x: avoid build failure with unrolled loops

[PATCH v6] coresight: etm4x: avoid build failure with unrolled loops

[Nick Desaulniers]

When the following configs are enabled:
* CORESIGHT
* CORESIGHT_SOURCE_ETM4X
* UBSAN
* UBSAN_TRAP

Clang fails assemble the kernel with the error:
<instantiation>:1:7: error: expected constant expression in '.inst' directive
.inst (0xd5200000|((((2) << 19) | ((1) << 16) | (((((((((((0x160 + (i * 4))))) >> 2))) >> 7) & 0x7)) << 12) | ((((((((((0x160 + (i * 4))))) >> 2))) & 0xf)) << 8) | (((((((((((0x160 + (i * 4))))) >> 2))) >> 4) & 0x7)) << 5)))|(.L__reg_num_x8))
      ^
drivers/hwtracing/coresight/coresight-etm4x-core.c:702:4: note: while in
macro instantiation
etm4x_relaxed_read32(csa, TRCCNTVRn(i));
^
drivers/hwtracing/coresight/coresight-etm4x.h:403:4: note: expanded from
macro 'etm4x_relaxed_read32'
read_etm4x_sysreg_offset((offset), false)))
^
drivers/hwtracing/coresight/coresight-etm4x.h:383:12: note: expanded
from macro 'read_etm4x_sysreg_offset'
__val = read_etm4x_sysreg_const_offset((offset));       \
        ^
drivers/hwtracing/coresight/coresight-etm4x.h:149:2: note: expanded from
macro 'read_etm4x_sysreg_const_offset'
READ_ETM4x_REG(ETM4x_OFFSET_TO_REG(offset))
^
drivers/hwtracing/coresight/coresight-etm4x.h:144:2: note: expanded from
macro 'READ_ETM4x_REG'
read_sysreg_s(ETM4x_REG_NUM_TO_SYSREG((reg)))
^
arch/arm64/include/asm/sysreg.h:1108:15: note: expanded from macro
'read_sysreg_s'
asm volatile(__mrs_s("%0", r) : "=r" (__val));                  \
             ^
arch/arm64/include/asm/sysreg.h:1074:2: note: expanded from macro '__mrs_s'
"       mrs_s " v ", " __stringify(r) "\n"                      \
 ^

Consider the definitions of TRCSSCSRn and TRCCNTVRn:
drivers/hwtracing/coresight/coresight-etm4x.h:56
 #define TRCCNTVRn(n)      (0x160 + (n * 4))
drivers/hwtracing/coresight/coresight-etm4x.h:81
 #define TRCSSCSRn(n)      (0x2A0 + (n * 4))

Where the macro parameter is expanded to i; a loop induction variable
from etm4_disable_hw.

When any compiler can determine that loops may be unrolled, then the
__builtin_constant_p check in read_etm4x_sysreg_offset() defined in
drivers/hwtracing/coresight/coresight-etm4x.h may evaluate to true. This
can lead to the expression `(0x160 + (i * 4))` being passed to
read_etm4x_sysreg_const_offset. Via the trace above, this is passed
through READ_ETM4x_REG, read_sysreg_s, and finally to __mrs_s where it
is string-ified and used directly in inline asm.

Regardless of which compiler or compiler options determine whether a
loop can or can't be unrolled, which determines whether
__builtin_constant_p evaluates to true when passed an expression using a
loop induction variable, it is NEVER safe to allow the preprocessor to
construct inline asm like:
  asm volatile (".inst (0x160 + (i * 4))" : "=r"(__val));
                                 ^ expected constant expression

Instead of read_etm4x_sysreg_offset() using __builtin_constant_p(), use
__is_constexpr from include/linux/const.h instead to ensure only
expressions that are valid integer constant expressions get passed
through to read_sysreg_s().

This is not a bug in clang; it's a potentially unsafe use of the macro
arguments in read_etm4x_sysreg_offset dependent on __builtin_constant_p.

Link: https://github.com/ClangBuiltLinux/linux/issues/1310
Reported-by: Arnd Bergmann <arnd@kernel.org>
Reported-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v5 -> v6:
* Rework patch to replace use of __builtin_constant_p with __is_constexpr in
  read_etm4x_sysreg_offset().
* Drop Suzuki's suggested by tag, since this is a different approach.
* Convert Tao's suggested by tag to a reported by tag.
* Reword bottom part of commit message.

V5 (Nick):
https://lore.kernel.org/llvm/20220623174131.3818333-1-ndesaulniers@google.com/
V4 (Nick):
https://lore.kernel.org/llvm/20220614220229.1640085-1-ndesaulniers@google.com/
V3 (Tao):
https://lore.kernel.org/lkml/1632652550-26048-1-git-send-email-quic_taozha@quicinc.com/
V2 (Arnd):
https://lore.kernel.org/lkml/20210429145752.3218324-1-arnd@kernel.org/
V1 (Arnd):
https://lore.kernel.org/lkml/20210225094324.3542511-1-arnd@kernel.org/

 drivers/hwtracing/coresight/coresight-etm4x.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 33869c1d20c3..a7bfea31f7d8 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -7,6 +7,7 @@
 #define _CORESIGHT_CORESIGHT_ETM_H
 
 #include <asm/local.h>
+#include <linux/const.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include "coresight-priv.h"
@@ -515,7 +516,7 @@
 	({									\
 		u64 __val;							\
 										\
-		if (__builtin_constant_p((offset)))				\
+		if (__is_constexpr((offset)))					\
 			__val = read_etm4x_sysreg_const_offset((offset));	\
 		else								\
 			__val = etm4x_sysreg_read((offset), true, (_64bit));	\

base-commit: 525496a030de4ae64bb9e1d6bfc88eec6f5fe6e2
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH v6] coresight: etm4x: avoid build failure with unrolled loops

[Arnd Bergmann]

On Sat, Jul 9, 2022 at 1:15 AM Nick Desaulniers <ndesaulniers@google.com> wrote:

>         ({                                                                      \
>                 u64 __val;                                                      \
>                                                                                 \
> -               if (__builtin_constant_p((offset)))                             \
> +               if (__is_constexpr((offset)))                                   \
>                         __val = read_etm4x_sysreg_const_offset((offset));       \
>                 else                                                            \
>                         __val = etm4x_sysreg_read((offset), true, (_64bit));    \
>

This is clearly better than the current version using
__builtin_constant_p(), but
I don't think it's safe in all cases, since there are expressions that
are constant
expressions to the compiler but are not valid input to the assembler.

I would prefer to see this fixed differently, but doing this one first is also
fine with me:

Acked-by: Arnd Bergmann <arnd@arndb.de>

        Arnd

Re: [PATCH v6] coresight: etm4x: avoid build failure with unrolled loops

[Suzuki K Poulose]

On 10/07/2022 20:13, Arnd Bergmann wrote:
> On Sat, Jul 9, 2022 at 1:15 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
> 
>>          ({                                                                      \
>>                  u64 __val;                                                      \
>>                                                                                  \
>> -               if (__builtin_constant_p((offset)))                             \
>> +               if (__is_constexpr((offset)))                                   \
>>                          __val = read_etm4x_sysreg_const_offset((offset));       \
>>                  else                                                            \
>>                          __val = etm4x_sysreg_read((offset), true, (_64bit));    \
>>
> 
> This is clearly better than the current version using
> __builtin_constant_p(), but
> I don't think it's safe in all cases, since there are expressions that
> are constant
> expressions to the compiler but are not valid input to the assembler.
> 
> I would prefer to see this fixed differently, but doing this one first is also
> fine with me:
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
>          Arnd


Thanks, I have queued this.

Suzuki