* [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-23 8:12 Yoshihiro Shimoda
2019-05-23 10:36 ` Will Deacon
0 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-23 8:12 UTC (permalink / raw)
To: catalin.marinas, will.deacon
Cc: kuninori.morimoto.gx, linux-arm-kernel, linux-renesas-soc,
Yoshihiro Shimoda
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@renesas.com>
Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
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)
__AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
__AARCH64_INSN_FUNCS(ldr_lit, 0xBF000000, 0x18000000)
__AARCH64_INSN_FUNCS(ldrsw_lit, 0xFF000000, 0x98000000)
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
2019-05-23 8:12 [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...) Yoshihiro Shimoda
@ 2019-05-23 10:36 ` Will Deacon
0 siblings, 0 replies; 13+ messages in thread
From: Will Deacon @ 2019-05-23 10:36 UTC (permalink / raw)
To: Yoshihiro Shimoda
Cc: catalin.marinas, kuninori.morimoto.gx, linux-arm-kernel,
linux-renesas-soc, daniel, jean-philippe.brucker
[+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@renesas.com>
> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> 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 :/
Will
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-23 10:36 ` Will Deacon
0 siblings, 0 replies; 13+ messages in thread
From: Will Deacon @ 2019-05-23 10:36 UTC (permalink / raw)
To: Yoshihiro Shimoda
Cc: kuninori.morimoto.gx, daniel, jean-philippe.brucker,
catalin.marinas, linux-renesas-soc, linux-arm-kernel
[+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@renesas.com>
> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> 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 :/
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
2019-05-23 10:36 ` Will Deacon
@ 2019-05-23 13:02 ` Daniel Borkmann
-1 siblings, 0 replies; 13+ messages in thread
From: Daniel Borkmann @ 2019-05-23 13:02 UTC (permalink / raw)
To: Will Deacon, Yoshihiro Shimoda
Cc: catalin.marinas, kuninori.morimoto.gx, linux-arm-kernel,
linux-renesas-soc, jean-philippe.brucker
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@renesas.com>
>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>> 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.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-23 13:02 ` Daniel Borkmann
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Borkmann @ 2019-05-23 13:02 UTC (permalink / raw)
To: Will Deacon, Yoshihiro Shimoda
Cc: linux-renesas-soc, catalin.marinas, jean-philippe.brucker,
linux-arm-kernel, kuninori.morimoto.gx
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@renesas.com>
>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>> 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.
Thanks,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
2019-05-23 13:02 ` Daniel Borkmann
@ 2019-05-23 13:54 ` Jean-Philippe Brucker
-1 siblings, 0 replies; 13+ messages in thread
From: Jean-Philippe Brucker @ 2019-05-23 13:54 UTC (permalink / raw)
To: Daniel Borkmann, Will Deacon, Yoshihiro Shimoda
Cc: linux-renesas-soc, catalin.marinas, linux-arm-kernel,
kuninori.morimoto.gx
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@renesas.com>
>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>> 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)
__AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
Thanks,
Jean
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-23 13:54 ` Jean-Philippe Brucker
0 siblings, 0 replies; 13+ messages in thread
From: Jean-Philippe Brucker @ 2019-05-23 13:54 UTC (permalink / raw)
To: Daniel Borkmann, Will Deacon, Yoshihiro Shimoda
Cc: linux-renesas-soc, catalin.marinas, kuninori.morimoto.gx,
linux-arm-kernel
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@renesas.com>
>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>> 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)
__AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
Thanks,
Jean
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
2019-05-23 13:54 ` Jean-Philippe Brucker
@ 2019-05-23 14:16 ` Will Deacon
-1 siblings, 0 replies; 13+ messages in thread
From: Will Deacon @ 2019-05-23 14:16 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Daniel Borkmann, Yoshihiro Shimoda, linux-renesas-soc,
catalin.marinas, linux-arm-kernel, kuninori.morimoto.gx
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@renesas.com>
> >>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
> >>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >>> ---
> >>> 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.
Thanks both!
Will
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-23 14:16 ` Will Deacon
0 siblings, 0 replies; 13+ messages in thread
From: Will Deacon @ 2019-05-23 14:16 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: kuninori.morimoto.gx, Daniel Borkmann, catalin.marinas,
Yoshihiro Shimoda, linux-renesas-soc, linux-arm-kernel
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@renesas.com>
> >>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
> >>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >>> ---
> >>> 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.
Thanks both!
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
2019-05-23 14:16 ` Will Deacon
@ 2019-05-23 14:26 ` Daniel Borkmann
-1 siblings, 0 replies; 13+ messages in thread
From: Daniel Borkmann @ 2019-05-23 14:26 UTC (permalink / raw)
To: Will Deacon, Jean-Philippe Brucker
Cc: Yoshihiro Shimoda, linux-renesas-soc, catalin.marinas,
linux-arm-kernel, kuninori.morimoto.gx
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@renesas.com>
>>>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>> ---
>>>>> 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.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-23 14:26 ` Daniel Borkmann
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Borkmann @ 2019-05-23 14:26 UTC (permalink / raw)
To: Will Deacon, Jean-Philippe Brucker
Cc: linux-renesas-soc, catalin.marinas, Yoshihiro Shimoda,
kuninori.morimoto.gx, linux-arm-kernel
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@renesas.com>
>>>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>> ---
>>>>> 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.
Thanks,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
2019-05-23 14:26 ` Daniel Borkmann
@ 2019-05-24 9:24 ` Jean-Philippe Brucker
-1 siblings, 0 replies; 13+ messages in thread
From: Jean-Philippe Brucker @ 2019-05-24 9:24 UTC (permalink / raw)
To: Daniel Borkmann, Will Deacon
Cc: linux-renesas-soc, catalin.marinas, Yoshihiro Shimoda,
kuninori.morimoto.gx, linux-arm-kernel
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@renesas.com>
>>>>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>>>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>>> ---
>>>>>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...)
@ 2019-05-24 9:24 ` Jean-Philippe Brucker
0 siblings, 0 replies; 13+ messages in thread
From: Jean-Philippe Brucker @ 2019-05-24 9:24 UTC (permalink / raw)
To: Daniel Borkmann, Will Deacon
Cc: linux-renesas-soc, catalin.marinas, Yoshihiro Shimoda,
linux-arm-kernel, kuninori.morimoto.gx
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@renesas.com>
>>>>>> Fixes: 34b8ab091f9ef57a ("bpf, arm64: use more scalable stadd over ldxr / stxr loop in xadd")
>>>>>> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>>> ---
>>>>>> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-05-24 9:25 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23 8:12 [PATCH/RFC] arm64: fix build warning from __AARCH64_INSN_FUNCS(ldadd, ...) Yoshihiro Shimoda
2019-05-23 10:36 ` Will Deacon
2019-05-23 10:36 ` Will Deacon
2019-05-23 13:02 ` Daniel Borkmann
2019-05-23 13:02 ` Daniel Borkmann
2019-05-23 13:54 ` Jean-Philippe Brucker
2019-05-23 13:54 ` Jean-Philippe Brucker
2019-05-23 14:16 ` Will Deacon
2019-05-23 14:16 ` Will Deacon
2019-05-23 14:26 ` Daniel Borkmann
2019-05-23 14:26 ` Daniel Borkmann
2019-05-24 9:24 ` Jean-Philippe Brucker
2019-05-24 9:24 ` Jean-Philippe Brucker
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.