Re: [RFC 10/12] arm64: Support TLBIP instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: tlbflush: ipas2e1is() calls __tlbi instead of __tlbip, dropping domain ID
- [High] arm64: tlbflush: macro precedence bug in __tlbip() destroys domain ID
--

commit f57328676d294e62d35d640905ead85c0b306e45
Author: Kristina Martšenko <kristina.martsenko@arm.com>

arm64: Support TLBIP instructions

This commit modifies the TLBI helper functions in tlbflush.h to support
issuing TLBIP instructions when requested. It introduces the __tlbip macro
to pass 128-bit arguments containing a TLBI domain ID in two consecutive GPRs.

> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> index b51df8e0ce631..dac35568570ff 100644
> --- a/arch/arm64/include/asm/tlbflush.h
> +++ b/arch/arm64/include/asm/tlbflush.h
> @@ -41,11 +41,26 @@
>  
>  #define __tlbi(op, ...)		__TLBI_N(op, ##__VA_ARGS__, 1, 0)
>  
> +#define __tlbip(op, arg) do {		\
> +	register u64 low asm ("x0") = (u64)arg;					\
> +	register u64 high asm ("x1") = (u64)(arg >> 64);			\

[Severity: High]
Does this macro expansion drop the upper 64 bits of arg when passed an
expression?

When __tlbip_user() calls this macro with a bitwise OR expression, the
expansion for the high register becomes:

    (u64)((arg) | (u128)USER_ASID_FLAG >> 64)

Because the bitwise shift operator (>>) has higher precedence than bitwise OR
(|), the shift applies only to USER_ASID_FLAG, yielding 0. This evaluates to
(u64)((arg) | 0), which truncates the 128-bit argument to its lower 64 bits and
causes the domain ID to be lost.

Should the macro definition enclose arg in parentheses, like ((arg) >> 64)?

> +										\
> +	asm (ARM64_ASM_PREAMBLE ".arch_extension tlbid\n"			\
> +	     "tlbip " #op ", %0, %1\n"						\
> +	  : : "r" (low), "r" (high));						\
> +} while (0)
> +
>  #define __tlbi_user(op, arg) do {						\
>  	if (arm64_kernel_unmapped_at_el0())					\
>  		__tlbi(op, (arg) | USER_ASID_FLAG);				\
>  } while (0)
>  
> +#define __tlbip_user(op, arg) do {						\
> +	if (arm64_kernel_unmapped_at_el0()) {					\
> +		__tlbip(op, (arg) | (u128)USER_ASID_FLAG);			\
> +	}									\
> +} while (0)
> +

[ ... ]

> -static __always_inline void ipas2e1is(u64 arg)
> +static __always_inline void ipas2e1is(bool tlbip, u128 arg)
>  {
> -	__tlbi(ipas2e1is, arg);
> +	if (tlbip)
> +		__tlbi(ipas2e1is, arg);

[Severity: High]
Does this branch mistakenly issue a regular TLBI instruction instead of a
TLBIP?

By calling __tlbi() instead of __tlbip() when tlbip is true, the 128-bit
argument is truncated, the domain ID is discarded, and domain-targeted stage-2
TLB invalidations will fail to apply to the correct domain.

Should this use __tlbip(ipas2e1is, arg) instead?

> +	else
> +		__tlbi(ipas2e1is, (u64)arg);
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001110655.461473-1-kristina.martsenko@arm.com?part=10