diff mbox

[1/2] arm64: bpf: add 'store immediate' instruction

Message ID 1447195301-16757-2-git-send-email-yang.shi@linaro.org
State Superseded
Headers show

Commit Message

Yang Shi Nov. 10, 2015, 10:41 p.m. UTC
aarch64 doesn't have native store immediate instruction, such operation
has to be implemented by the below instruction sequence:

Load immediate to register
Store register

Signed-off-by: Yang Shi <yang.shi@linaro.org>

CC: Zi Shen Lim <zlim.lnx@gmail.com>
CC: Xi Wang <xi.wang@gmail.com>
---
 arch/arm64/net/bpf_jit_comp.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

-- 
2.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Zi Shen Lim Nov. 11, 2015, 2:45 a.m. UTC | #1
On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote:
> aarch64 doesn't have native store immediate instruction, such operation


Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can
consider using it as an optimization.

You may also want to consider adding a note about the corresponding test cases:
    commit cffc642d93f9 ("test_bpf: add 173 new testcases for eBPF").

Otherwise, the patch below looks good.
Reviewed-by: Zi Shen Lim <zlim.lnx@gmail.com>


> has to be implemented by the below instruction sequence:

>

> Load immediate to register

> Store register

>

> Signed-off-by: Yang Shi <yang.shi@linaro.org>

> CC: Zi Shen Lim <zlim.lnx@gmail.com>

> CC: Xi Wang <xi.wang@gmail.com>

> ---

>  arch/arm64/net/bpf_jit_comp.c | 20 +++++++++++++++++++-

>  1 file changed, 19 insertions(+), 1 deletion(-)

>

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c

> index 6809647..49c1f1b 100644

> --- a/arch/arm64/net/bpf_jit_comp.c

> +++ b/arch/arm64/net/bpf_jit_comp.c

> @@ -563,7 +563,25 @@ emit_cond_jmp:

>         case BPF_ST | BPF_MEM | BPF_H:

>         case BPF_ST | BPF_MEM | BPF_B:

>         case BPF_ST | BPF_MEM | BPF_DW:

> -               goto notyet;

> +               /* Load imm to a register then store it */

> +               ctx->tmp_used = 1;

> +               emit_a64_mov_i(1, tmp2, off, ctx);

> +               emit_a64_mov_i(1, tmp, imm, ctx);

> +               switch (BPF_SIZE(code)) {

> +               case BPF_W:

> +                       emit(A64_STR32(tmp, dst, tmp2), ctx);

> +                       break;

> +               case BPF_H:

> +                       emit(A64_STRH(tmp, dst, tmp2), ctx);

> +                       break;

> +               case BPF_B:

> +                       emit(A64_STRB(tmp, dst, tmp2), ctx);

> +                       break;

> +               case BPF_DW:

> +                       emit(A64_STR64(tmp, dst, tmp2), ctx);

> +                       break;

> +               }

> +               break;

>

>         /* STX: *(size *)(dst + off) = src */

>         case BPF_STX | BPF_MEM | BPF_W:

> --

> 2.0.2

>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Will Deacon Nov. 11, 2015, 12:12 p.m. UTC | #2
On Tue, Nov 10, 2015 at 06:45:39PM -0800, Z Lim wrote:
> On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote:

> > aarch64 doesn't have native store immediate instruction, such operation

> 

> Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can

> consider using it as an optimization.


Yes, I'd definitely like to see that in preference to moving via a
temporary register.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Will Deacon Nov. 11, 2015, 12:39 p.m. UTC | #3
On Wed, Nov 11, 2015 at 12:12:56PM +0000, Will Deacon wrote:
> On Tue, Nov 10, 2015 at 06:45:39PM -0800, Z Lim wrote:

> > On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote:

> > > aarch64 doesn't have native store immediate instruction, such operation

> > 

> > Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can

> > consider using it as an optimization.

> 

> Yes, I'd definitely like to see that in preference to moving via a

> temporary register.


Wait a second, we're both talking rubbish here :) The STR (immediate)
form is referring to the addressing mode, whereas this patch wants to
store an immediate value to memory, which does need moving to a register
first.

So the original patch is fine.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yang Shi Nov. 12, 2015, 7:33 p.m. UTC | #4
On 11/11/2015 4:39 AM, Will Deacon wrote:
> On Wed, Nov 11, 2015 at 12:12:56PM +0000, Will Deacon wrote:

>> On Tue, Nov 10, 2015 at 06:45:39PM -0800, Z Lim wrote:

>>> On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote:

>>>> aarch64 doesn't have native store immediate instruction, such operation

>>>

>>> Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can

>>> consider using it as an optimization.

>>

>> Yes, I'd definitely like to see that in preference to moving via a

>> temporary register.

>

> Wait a second, we're both talking rubbish here :) The STR (immediate)

> form is referring to the addressing mode, whereas this patch wants to

> store an immediate value to memory, which does need moving to a register

> first.


Yes, the immediate means immediate offset for addressing index. Doesn't 
mean to store immediate to memory.

I don't think any load-store architecture has store immediate instruction.

Thanks,
Yang

>

> So the original patch is fine.

>

> Will

>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Zi Shen Lim Nov. 13, 2015, 3:45 a.m. UTC | #5
On Thu, Nov 12, 2015 at 11:33 AM, Shi, Yang <yang.shi@linaro.org> wrote:
> On 11/11/2015 4:39 AM, Will Deacon wrote:

>>

>> Wait a second, we're both talking rubbish here :) The STR (immediate)

>> form is referring to the addressing mode, whereas this patch wants to

>> store an immediate value to memory, which does need moving to a register

>> first.

>

>

> Yes, the immediate means immediate offset for addressing index. Doesn't mean

> to store immediate to memory.

>

> I don't think any load-store architecture has store immediate instruction.

>


Indeed. Sorry for the noise.

Somehow Will caught a whiff of whatever I was smoking then :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yang Shi Nov. 23, 2015, 7:34 p.m. UTC | #6
Hi folks,

Any more comments on this patch (store immediate only)?

I need more time to add XADD (I'm supposed everyone agrees it is 
equivalent to atomic_add). However, this one is irrelevant to XADD, so 
we may be able to apply it first?

Thanks,
Yang


On 11/12/2015 7:45 PM, Z Lim wrote:
> On Thu, Nov 12, 2015 at 11:33 AM, Shi, Yang <yang.shi@linaro.org> wrote:

>> On 11/11/2015 4:39 AM, Will Deacon wrote:

>>>

>>> Wait a second, we're both talking rubbish here :) The STR (immediate)

>>> form is referring to the addressing mode, whereas this patch wants to

>>> store an immediate value to memory, which does need moving to a register

>>> first.

>>

>>

>> Yes, the immediate means immediate offset for addressing index. Doesn't mean

>> to store immediate to memory.

>>

>> I don't think any load-store architecture has store immediate instruction.

>>

>

> Indeed. Sorry for the noise.

>

> Somehow Will caught a whiff of whatever I was smoking then :)

>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 6809647..49c1f1b 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -563,7 +563,25 @@  emit_cond_jmp:
 	case BPF_ST | BPF_MEM | BPF_H:
 	case BPF_ST | BPF_MEM | BPF_B:
 	case BPF_ST | BPF_MEM | BPF_DW:
-		goto notyet;
+		/* Load imm to a register then store it */
+		ctx->tmp_used = 1;
+		emit_a64_mov_i(1, tmp2, off, ctx);
+		emit_a64_mov_i(1, tmp, imm, ctx);
+		switch (BPF_SIZE(code)) {
+		case BPF_W:
+			emit(A64_STR32(tmp, dst, tmp2), ctx);
+			break;
+		case BPF_H:
+			emit(A64_STRH(tmp, dst, tmp2), ctx);
+			break;
+		case BPF_B:
+			emit(A64_STRB(tmp, dst, tmp2), ctx);
+			break;
+		case BPF_DW:
+			emit(A64_STR64(tmp, dst, tmp2), ctx);
+			break;
+		}
+		break;
 
 	/* STX: *(size *)(dst + off) = src */
 	case BPF_STX | BPF_MEM | BPF_W: