Re: [PATCH 3/3] arm64: insn: Generate 64 bit mask immediates correctly

From: Marc Zyngier <maz@kernel.org>
To: James Morse <james.morse@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kselftest@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Shuah Khan <shuah@kernel.org>,
	Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH 3/3] arm64: insn: Generate 64 bit mask immediates correctly
Date: Thu, 27 Jan 2022 18:34:12 +0000	[thread overview]
Message-ID: <87sft958pn.wl-maz@kernel.org> (raw)
In-Reply-To: <20220127162127.2391947-4-james.morse@arm.com>

On Thu, 27 Jan 2022 16:21:27 +0000,
James Morse <james.morse@arm.com> wrote:
> 
> When the insn framework is used to encode an AND/ORR/EOR instruction,
> aarch64_encode_immediate() is used to pick the immr imms values.
> 
> If the immediate is a 64bit mask, with bit 63 set, and zeros in any
> of the upper 32 bits, the immr value is incorrectly calculated meaning
> the wrong mask is generated.
> For example, 0x8000000000000001 should have an immr of 1, but 32 is used,
> meaning the resulting mask is 0x0000000300000000.
> 
> It would appear eBPF is unable to hit these cases, as build_insn()'s
> imm value is a s32, so when used with BPF_ALU64, the sign-extended
> u64 immediate would always have all-1s or all-0s in the upper 32 bits.
> 
> KVM does not generate a va_mask with any of the top bits set as these
> VA wouldn't be usable with TTBR0_EL2.
> 
> This happens because the rotation is calculated from fls(~imm), which
> takes an unsigned int, but the immediate may be 64bit.
> 
> Use fls64() so the 64bit mask doesn't get truncated to a u32.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/lib/insn.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index 8888e407032f..90253af7e294 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -1381,7 +1381,7 @@ static u32 aarch64_encode_immediate(u64 imm,
>  		 * Compute the rotation to get a continuous set of
>  		 * ones, with the first bit set at position 0
>  		 */
> -		ror = fls(~imm);
> +		ror = fls64(~imm);
>  	}
>  
>  	/*

Oh crap, not again... :-( Clearly, my initial test harness wasn't as
good as I thought. Out for morbid curiosity, how was this found?

Brown-paper-bag-for: Marc Zyngier <maz@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel