diff mbox series

arm64: mte: Make mte_check_tfsr_*() conditional on KASAN instead of MTE

Message ID 20240528225131.3577704-1-pcc@google.com (mailing list archive)
State New, archived
Headers show
Series arm64: mte: Make mte_check_tfsr_*() conditional on KASAN instead of MTE | expand

Commit Message

Peter Collingbourne May 28, 2024, 10:51 p.m. UTC
The check in mte_check_tfsr_el1() is only necessary if HW tag
based KASAN is enabled. However, we were also executing the check
if MTE is enabled and KASAN is enabled at build time but disabled
at runtime. This turned out to cause a measurable increase in
power consumption on a specific microarchitecture after enabling
MTE. Moreover, on the same system, an increase in invalid syscall
latency (as measured by [1]) of around 20-30% (depending on the
cluster) was observed after enabling MTE; this almost entirely goes
away after removing this check. Therefore, make the check conditional
on whether KASAN is enabled rather than on whether MTE is enabled.

[1] https://lore.kernel.org/all/CAMn1gO4MwRV8bmFJ_SeY5tsYNPn2ZP56LjAhafygjFaKuu5ouw@mail.gmail.com/

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/I22d98d1483dd400a95595946552b769a5a1ad7bd
---
 arch/arm64/include/asm/mte.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Peter Collingbourne June 10, 2024, 8:25 p.m. UTC | #1
On Tue, May 28, 2024 at 3:51 PM Peter Collingbourne <pcc@google.com> wrote:
>
> The check in mte_check_tfsr_el1() is only necessary if HW tag
> based KASAN is enabled. However, we were also executing the check
> if MTE is enabled and KASAN is enabled at build time but disabled
> at runtime. This turned out to cause a measurable increase in
> power consumption on a specific microarchitecture after enabling
> MTE. Moreover, on the same system, an increase in invalid syscall
> latency (as measured by [1]) of around 20-30% (depending on the
> cluster) was observed after enabling MTE; this almost entirely goes
> away after removing this check. Therefore, make the check conditional
> on whether KASAN is enabled rather than on whether MTE is enabled.

Ping.

Peter
Alexandru Elisei June 11, 2024, 10:41 a.m. UTC | #2
Hi,

On Tue, May 28, 2024 at 03:51:30PM -0700, Peter Collingbourne wrote:
> The check in mte_check_tfsr_el1() is only necessary if HW tag
> based KASAN is enabled. However, we were also executing the check
> if MTE is enabled and KASAN is enabled at build time but disabled
> at runtime. This turned out to cause a measurable increase in
> power consumption on a specific microarchitecture after enabling
> MTE. Moreover, on the same system, an increase in invalid syscall
> latency (as measured by [1]) of around 20-30% (depending on the
> cluster) was observed after enabling MTE; this almost entirely goes
> away after removing this check. Therefore, make the check conditional
> on whether KASAN is enabled rather than on whether MTE is enabled.
> 
> [1] https://lore.kernel.org/all/CAMn1gO4MwRV8bmFJ_SeY5tsYNPn2ZP56LjAhafygjFaKuu5ouw@mail.gmail.com/
> 
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/I22d98d1483dd400a95595946552b769a5a1ad7bd
> ---
>  arch/arm64/include/asm/mte.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 91fbd5c8a3911..0f84518632b4a 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -182,7 +182,7 @@ void mte_check_tfsr_el1(void);
>  
>  static inline void mte_check_tfsr_entry(void)
>  {
> -	if (!system_supports_mte())
> +	if (!kasan_hw_tags_enabled())
>  		return;
>  
>  	mte_check_tfsr_el1();
> @@ -190,7 +190,7 @@ static inline void mte_check_tfsr_entry(void)
>  
>  static inline void mte_check_tfsr_exit(void)
>  {
> -	if (!system_supports_mte())
> +	if (!kasan_hw_tags_enabled())
>  		return;

The change looks correct to me - tag check faults for the kernel are
enabled only when hw kasan is enabled. The functions are already compiled
only when CONFIG_KASAN_HW_TAGS=y, so it makes sense to further restrict
them based on kasan_hw_tags_enabled():

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Can't speak for the performance results, altough skipping the dsb + isb on
the exit path sure looks nice.

Thanks,
Alex

>  
>  	/*
> -- 
> 2.45.1.288.g0e0cd299f1-goog
>
Catalin Marinas June 12, 2024, 4:09 p.m. UTC | #3
On Tue, 28 May 2024 15:51:30 -0700, Peter Collingbourne wrote:
> The check in mte_check_tfsr_el1() is only necessary if HW tag
> based KASAN is enabled. However, we were also executing the check
> if MTE is enabled and KASAN is enabled at build time but disabled
> at runtime. This turned out to cause a measurable increase in
> power consumption on a specific microarchitecture after enabling
> MTE. Moreover, on the same system, an increase in invalid syscall
> latency (as measured by [1]) of around 20-30% (depending on the
> cluster) was observed after enabling MTE; this almost entirely goes
> away after removing this check. Therefore, make the check conditional
> on whether KASAN is enabled rather than on whether MTE is enabled.
> 
> [...]

Applied to arm64 (for-next/mte), thanks!

[1/1] arm64: mte: Make mte_check_tfsr_*() conditional on KASAN instead of MTE
      https://git.kernel.org/arm64/c/26ca4423604f
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 91fbd5c8a3911..0f84518632b4a 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -182,7 +182,7 @@  void mte_check_tfsr_el1(void);
 
 static inline void mte_check_tfsr_entry(void)
 {
-	if (!system_supports_mte())
+	if (!kasan_hw_tags_enabled())
 		return;
 
 	mte_check_tfsr_el1();
@@ -190,7 +190,7 @@  static inline void mte_check_tfsr_entry(void)
 
 static inline void mte_check_tfsr_exit(void)
 {
-	if (!system_supports_mte())
+	if (!kasan_hw_tags_enabled())
 		return;
 
 	/*