On 23/05/2019 15:26, Daniel Borkmann wrote:
> On 05/23/2019 04:16 PM, Will Deacon wrote:
>> On Thu, May 23, 2019 at 02:54:54PM +0100, Jean-Philippe Brucker wrote:
>>> On 23/05/2019 14:02, Daniel Borkmann wrote:
>>>> On 05/23/2019 12:36 PM, Will Deacon wrote:
>>>>> [+Daniel and Jean-Philippe]
>>>>> On Thu, May 23, 2019 at 05:12:00PM +0900, Yoshihiro Shimoda wrote:
>>>>>> The following build warning happens on gcc 8.1.0.
>>>>>>
>>>>>> 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]
>>>>>> __AARCH64_INSN_FUNCS(ldadd, 0x3F20FC00, 0xB8200000)
>>>>>>
>>>>>> Since the second argument is mask value and compare with the third
>>>>>> argument value, the bit 31 is always masked and then this macro is
>>>>>> always false. So, this patch fixes the issue.
>>>>>>
>>>>>> Reported-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>>>>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>>>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>>>>>> ---
>>>>>> I'm not sure the second argument "0xBF20FC00" is OK or not (we can set
>>>>>> to 0xFF20FC00 instead). So, I marked RFC on this patch.
>>>>>>
>>>>>> 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 ec894de..c9e3cdc 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, 0xBF20FC00, 0xB8200000)
>>>>>
>>>>> Looking at the ISA encoding, I think that top digit should indeed be 'B',
>>>>> but I haven't checked the rest of the instruction.
>>>>>
>>>>> However, I'm fairly sure we tested this so now I'm a bit worried that I'm
>>>>> missing something :/
>>>>
>>>> Hmm, good catch, the mask aka aarch64_insn_is_ldadd() is not used anywhere
>>>> in the tree, just the aarch64_insn_get_ldadd_value(). Latter was runtime
>>>> tested via BPF JIT as well as through disassembler that it emits ldadd. I
>>>> initially had a different mask value than Jean-Philippe, but that was probably
>>>> due to confusion on my side. In any case, value should be correct though.
>>>
>>> I suggested that mask and forgot to change val, sorry about that. My
>>> intent was to stay consistent with ldr_reg and str_reg, which mask out
>>> the two size bits [31:30]. The proposed fix works but won't take into
>>> account ldaddb and ldaddh, so maybe we could change val to 0x38200000
>>> instead?
>>>
>>> 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
>>> @@ -279,3 +279,3 @@ __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)
>>
>> Yes, this is better. I didn't realise we wanted to catch the sub-word
>> instructions as well, but that's what we do for other memory access
>> instructions so we should be consistent.
>>
>> If you post this as a proper patch, I can queue it as a fix in the arm64
>> tree.
>
> If you're at it, it might also be good to add a comment to document such
> conventions for the mask right above the __AARCH64_INSN_FUNCS() definition
> or such. Would definitely be helpful for adding other insns there in future.
Hmm, I couldn't come up with anything generic and useful to say here,
sorry. I'll send the patch so we can have it in -rc2
Thanks,
Jean
