[1/2] arm64: insn: Fix ldadd instruction encoding

Commit Message

Jean-Philippe Brucker May 24, 2019, 12:52 p.m. UTC

GCC 8.1.0 reports that the ldadd instruction encoding, recently added to
insn.c, doesn't match the mask and couldn't possibly be identified:

 linux/arch/arm64/include/asm/insn.h: In function 'aarch64_insn_is_ldadd':
 linux/arch/arm64/include/asm/insn.h:280:257: warning: bitwise comparison always evaluates to false [-Wtautological-compare]

Bits [31:30] normally encode the size of the instruction (1 to 8 bytes)
and the current instruction value only encodes the 4- and 8-byte
variants. At the moment only the BPF JIT needs this instruction, and
doesn't require the 1- and 2-byte variants, but to be consistent with
our other ldr and str instruction encodings, clear the size field in the
insn value.

Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
Reported-by: Kuninori Morimoto <[e-mail]>
Signed-off-by: Yoshihiro Shimoda <[e-mail]>
Signed-off-by: Jean-Philippe Brucker <[e-mail]>
---
Strictly speaking, to be consistent with the ldr/str instructions we
would also check the Acquire/Release bit to filter out the acquire
release variants. I'm not sure if that matters, I think taking them in
is harmless.
---
 arch/arm64/include/asm/insn.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Borkmann May 24, 2019, 1:29 p.m. UTC | #1

On 05/24/2019 02:52 PM, Jean-Philippe Brucker wrote:
> GCC 8.1.0 reports that the ldadd instruction encoding, recently added to
> insn.c, doesn't match the mask and couldn't possibly be identified:
> 
>  linux/arch/arm64/include/asm/insn.h: In function 'aarch64_insn_is_ldadd':
>  linux/arch/arm64/include/asm/insn.h:280:257: warning: bitwise comparison always evaluates to false [-Wtautological-compare]
> 
> Bits [31:30] normally encode the size of the instruction (1 to 8 bytes)
> and the current instruction value only encodes the 4- and 8-byte
> variants. At the moment only the BPF JIT needs this instruction, and
> doesn't require the 1- and 2-byte variants, but to be consistent with
> our other ldr and str instruction encodings, clear the size field in the
> insn value.
> 
> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
> Reported-by: Kuninori Morimoto <[e-mail]>
> Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> Signed-off-by: Jean-Philippe Brucker <[e-mail]>

Acked-by: Daniel Borkmann <[e-mail]>

> ---
> Strictly speaking, to be consistent with the ldr/str instructions we
> would also check the Acquire/Release bit to filter out the acquire
> release variants. I'm not sure if that matters, I think taking them in
> is harmless.
> ---
>  arch/arm64/include/asm/insn.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index ec894de0ed4e..f71b84d9f294 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -277,7 +277,7 @@ __AARCH64_INSN_FUNCS(adrp,	0x9F000000, 0x90000000)
>  __AARCH64_INSN_FUNCS(prfm,	0x3FC00000, 0x39800000)
>  __AARCH64_INSN_FUNCS(prfm_lit,	0xFF000000, 0xD8000000)
>  __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
> -__AARCH64_INSN_FUNCS(ldadd,	0x3F20FC00, 0xB8200000)
> +__AARCH64_INSN_FUNCS(ldadd,	0x3F20FC00, 0x38200000)

Right, so the 0xB8200000 encodes fixed '10' which denotes 4 byte
variant so if in aarch64_insn_gen_ldadd() we select 8 byte one,
we'll generate '11' which are the only two supported options right
now, but it would throw the tautological compare warn with the mask
which does not have pre-defined size encoded. Makes sense to remove
the encoding for ldadd itself. I think filtering out the acquire /
release variant bits might be good once there is an actual in-tree
user of aarch64_insn_is_ldadd() itself, or if the BPF JIT (or something
else) needs the aarch64_insn_get_ldadd_value() with that customization.

Thanks,
Daniel

>  __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
>  __AARCH64_INSN_FUNCS(ldr_lit,	0xBF000000, 0x18000000)
>  __AARCH64_INSN_FUNCS(ldrsw_lit,	0xFF000000, 0x98000000)
>

Will Deacon May 24, 2019, 2:39 p.m. UTC | #2

On Fri, May 24, 2019 at 01:52:19PM +0100, Jean-Philippe Brucker wrote:
> GCC 8.1.0 reports that the ldadd instruction encoding, recently added to
> insn.c, doesn't match the mask and couldn't possibly be identified:
> 
>  linux/arch/arm64/include/asm/insn.h: In function 'aarch64_insn_is_ldadd':
>  linux/arch/arm64/include/asm/insn.h:280:257: warning: bitwise comparison always evaluates to false [-Wtautological-compare]
> 
> Bits [31:30] normally encode the size of the instruction (1 to 8 bytes)
> and the current instruction value only encodes the 4- and 8-byte
> variants. At the moment only the BPF JIT needs this instruction, and
> doesn't require the 1- and 2-byte variants, but to be consistent with
> our other ldr and str instruction encodings, clear the size field in the
> insn value.
> 
> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
> Reported-by: Kuninori Morimoto <[e-mail]>
> Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> Signed-off-by: Jean-Philippe Brucker <[e-mail]>

Cheers, I've picked up this patch with Daniel's Ack, and also the other
patch as-is.

Will

Patch

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index ec894de0ed4e..f71b84d9f294 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -277,7 +277,7 @@  __AARCH64_INSN_FUNCS(adrp,	0x9F000000, 0x90000000)
 __AARCH64_INSN_FUNCS(prfm,	0x3FC00000, 0x39800000)
 __AARCH64_INSN_FUNCS(prfm_lit,	0xFF000000, 0xD8000000)
 __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
-__AARCH64_INSN_FUNCS(ldadd,	0x3F20FC00, 0xB8200000)
+__AARCH64_INSN_FUNCS(ldadd,	0x3F20FC00, 0x38200000)
 __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
 __AARCH64_INSN_FUNCS(ldr_lit,	0xBF000000, 0x18000000)
 __AARCH64_INSN_FUNCS(ldrsw_lit,	0xFF000000, 0x98000000)