All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johan Almbladh <johan.almbladh@anyfinetworks.com>
To: Yonghong Song <yhs@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Tony Ambardar <Tony.Ambardar@gmail.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH 08/14] bpf/tests: Add tests for ALU operations implemented with function calls
Date: Thu, 29 Jul 2021 23:17:43 +0200	[thread overview]
Message-ID: <CAM1=_QQRuH2K3fMDJCYJuDtTmziqcmtcr31hQeQe-kCkXVC4gA@mail.gmail.com> (raw)
In-Reply-To: <ba3656eb-500b-9f14-1c97-d27868f1c3e6@fb.com>

On Thu, Jul 29, 2021 at 1:52 AM Yonghong Song <yhs@fb.com> wrote:
> > +             /*
> > +              * Register (non-)clobbering test, in the case where a 32-bit
> > +              * JIT implements complex ALU64 operations via function calls.
> > +              */
> > +             "INT: Register clobbering, R1 updated",
> > +             .u.insns_int = {
> > +                     BPF_ALU32_IMM(BPF_MOV, R0, 0),
> > +                     BPF_ALU32_IMM(BPF_MOV, R1, 123456789),
> > +                     BPF_ALU32_IMM(BPF_MOV, R2, 2),
> > +                     BPF_ALU32_IMM(BPF_MOV, R3, 3),
> > +                     BPF_ALU32_IMM(BPF_MOV, R4, 4),
> > +                     BPF_ALU32_IMM(BPF_MOV, R5, 5),
> > +                     BPF_ALU32_IMM(BPF_MOV, R6, 6),
> > +                     BPF_ALU32_IMM(BPF_MOV, R7, 7),
> > +                     BPF_ALU32_IMM(BPF_MOV, R8, 8),
> > +                     BPF_ALU32_IMM(BPF_MOV, R9, 9),
> > +                     BPF_ALU64_IMM(BPF_DIV, R1, 123456789),
> > +                     BPF_JMP_IMM(BPF_JNE, R0, 0, 10),
> > +                     BPF_JMP_IMM(BPF_JNE, R1, 1, 9),
> > +                     BPF_JMP_IMM(BPF_JNE, R2, 2, 8),
> > +                     BPF_JMP_IMM(BPF_JNE, R3, 3, 7),
> > +                     BPF_JMP_IMM(BPF_JNE, R4, 4, 6),
> > +                     BPF_JMP_IMM(BPF_JNE, R5, 5, 5),
> > +                     BPF_JMP_IMM(BPF_JNE, R6, 6, 4),
> > +                     BPF_JMP_IMM(BPF_JNE, R7, 7, 3),
> > +                     BPF_JMP_IMM(BPF_JNE, R8, 8, 2),
> > +                     BPF_JMP_IMM(BPF_JNE, R9, 9, 1),
> > +                     BPF_ALU32_IMM(BPF_MOV, R0, 1),
> > +                     BPF_EXIT_INSN(),
> > +             },
> > +             INTERNAL,
> > +             { },
> > +             { { 0, 1 } }
> > +     },
> > +     {
> > +             "INT: Register clobbering, R2 updated",
> > +             .u.insns_int = {
> > +                     BPF_ALU32_IMM(BPF_MOV, R0, 0),
> > +                     BPF_ALU32_IMM(BPF_MOV, R1, 1),
> > +                     BPF_ALU32_IMM(BPF_MOV, R2, 2 * 123456789),
> > +                     BPF_ALU32_IMM(BPF_MOV, R3, 3),
> > +                     BPF_ALU32_IMM(BPF_MOV, R4, 4),
> > +                     BPF_ALU32_IMM(BPF_MOV, R5, 5),
> > +                     BPF_ALU32_IMM(BPF_MOV, R6, 6),
> > +                     BPF_ALU32_IMM(BPF_MOV, R7, 7),
> > +                     BPF_ALU32_IMM(BPF_MOV, R8, 8),
> > +                     BPF_ALU32_IMM(BPF_MOV, R9, 9),
> > +                     BPF_ALU64_IMM(BPF_DIV, R2, 123456789),
> > +                     BPF_JMP_IMM(BPF_JNE, R0, 0, 10),
> > +                     BPF_JMP_IMM(BPF_JNE, R1, 1, 9),
> > +                     BPF_JMP_IMM(BPF_JNE, R2, 2, 8),
> > +                     BPF_JMP_IMM(BPF_JNE, R3, 3, 7),
> > +                     BPF_JMP_IMM(BPF_JNE, R4, 4, 6),
> > +                     BPF_JMP_IMM(BPF_JNE, R5, 5, 5),
> > +                     BPF_JMP_IMM(BPF_JNE, R6, 6, 4),
> > +                     BPF_JMP_IMM(BPF_JNE, R7, 7, 3),
> > +                     BPF_JMP_IMM(BPF_JNE, R8, 8, 2),
> > +                     BPF_JMP_IMM(BPF_JNE, R9, 9, 1),
> > +                     BPF_ALU32_IMM(BPF_MOV, R0, 1),
> > +                     BPF_EXIT_INSN(),
> > +             },
> > +             INTERNAL,
> > +             { },
> > +             { { 0, 1 } }
> > +     },
>
> It looks like the above two tests, "R1 updated" and "R2 updated" should
> be very similar and the only difference is one immediate is 123456789
> and another is 2 * 123456789. But for generated code, they all just have
> the final immediate. Could you explain what the difference in terms of
> jit for the above two tests?

When a BPF_CALL instruction is executed, the eBPF assembler have
already saved any caller-saved registers that must be preserved, put
the arguments in R1-R5, and expects a return value in R0. It is just
for the JIT to emit the call.

Not so when an eBPF instruction is implemented by a function call,
like ALU64 DIV in a 32-bit JIT. In this case, the function call is
unexpected by the eBPF assembler, and must be invisible to it. Now the
JIT must take care of saving all caller-saved registers on stack, put
the operands in the right argument registers, put the return value in
the destination register, and finally restore all caller-saved
registers without overwriting the computed result.

The test checks that all other registers retain their value after such
a hidden function call. However, one register will contain the result.
In order to verify that all registers are saved and restored properly,
we must vary the destination and run it two times. It is not the
result of the operation that its tested, it is absence of possible
side effects.

I can put a more elaborate description in the comment to explain this.

>
> > +     {
> > +             /*
> > +              * Test 32-bit JITs that implement complex ALU64 operations as
> > +              * function calls R0 = f(R1, R2), and must re-arrange operands.
> > +              */
> > +#define NUMER 0xfedcba9876543210ULL
> > +#define DENOM 0x0123456789abcdefULL
> > +             "ALU64_DIV X: Operand register permutations",
> > +             .u.insns_int = {
> > +                     /* R0 / R2 */
> > +                     BPF_LD_IMM64(R0, NUMER),
> > +                     BPF_LD_IMM64(R2, DENOM),
> > +                     BPF_ALU64_REG(BPF_DIV, R0, R2),
> > +                     BPF_JMP_IMM(BPF_JEQ, R0, NUMER / DENOM, 1),
> > +                     BPF_EXIT_INSN(),
> > +                     /* R1 / R0 */
> > +                     BPF_LD_IMM64(R1, NUMER),
> > +                     BPF_LD_IMM64(R0, DENOM),
> > +                     BPF_ALU64_REG(BPF_DIV, R1, R0),
> > +                     BPF_JMP_IMM(BPF_JEQ, R1, NUMER / DENOM, 1),
> > +                     BPF_EXIT_INSN(),
> > +                     /* R0 / R1 */
> > +                     BPF_LD_IMM64(R0, NUMER),
> > +                     BPF_LD_IMM64(R1, DENOM),
> > +                     BPF_ALU64_REG(BPF_DIV, R0, R1),
> > +                     BPF_JMP_IMM(BPF_JEQ, R0, NUMER / DENOM, 1),
> > +                     BPF_EXIT_INSN(),
> > +                     /* R2 / R0 */
> > +                     BPF_LD_IMM64(R2, NUMER),
> > +                     BPF_LD_IMM64(R0, DENOM),
> > +                     BPF_ALU64_REG(BPF_DIV, R2, R0),
> > +                     BPF_JMP_IMM(BPF_JEQ, R2, NUMER / DENOM, 1),
> > +                     BPF_EXIT_INSN(),
> > +                     /* R2 / R1 */
> > +                     BPF_LD_IMM64(R2, NUMER),
> > +                     BPF_LD_IMM64(R1, DENOM),
> > +                     BPF_ALU64_REG(BPF_DIV, R2, R1),
> > +                     BPF_JMP_IMM(BPF_JEQ, R2, NUMER / DENOM, 1),
> > +                     BPF_EXIT_INSN(),
> > +                     /* R1 / R2 */
> > +                     BPF_LD_IMM64(R1, NUMER),
> > +                     BPF_LD_IMM64(R2, DENOM),
> > +                     BPF_ALU64_REG(BPF_DIV, R1, R2),
> > +                     BPF_JMP_IMM(BPF_JEQ, R1, NUMER / DENOM, 1),
> > +                     BPF_EXIT_INSN(),
> > +                     BPF_LD_IMM64(R0, 1),
>
> Do we need this BPF_LD_IMM64(R0, 1)?
> First, if we have it, and next "BPF_ALU64_REG(BPF_DIV, R1, R1)"
> generates incorrect value and exit and then you will get
> exit value 1, which will signal the test success.
>
> Second, if you don't have this R0 = 1, R0 will be DENOM
> and you will be fine.

Good catch! No, it should not be there. Maybe left from previous
debugging, or a copy-and-paste error. I'll remove it.

  reply	other threads:[~2021-07-29 21:17 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-28 17:04 [PATCH 00/14] bpf/tests: Extend the eBPF test suite Johan Almbladh
2021-07-28 17:04 ` [PATCH 01/14] bpf/tests: Add BPF_JMP32 test cases Johan Almbladh
2021-07-28 22:31   ` Yonghong Song
2021-07-29 21:30     ` Johan Almbladh
2021-07-28 17:04 ` [PATCH 02/14] bpf/tests: Add BPF_MOV tests for zero and sign extension Johan Almbladh
2021-07-28 22:36   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 03/14] bpf/tests: Fix typos in test case descriptions Johan Almbladh
2021-07-28 22:43   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 04/14] bpf/tests: Add more tests of ALU32 and ALU64 bitwise operations Johan Almbladh
2021-07-28 22:53   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 05/14] bpf/tests: Add more ALU32 tests for BPF_LSH/RSH/ARSH Johan Almbladh
2021-07-28 22:57   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 06/14] bpf/tests: Add more BPF_LSH/RSH/ARSH tests for ALU64 Johan Almbladh
2021-07-28 23:30   ` Yonghong Song
2021-07-29 12:34     ` Johan Almbladh
2021-07-29 15:39       ` Yonghong Song
2021-07-28 17:04 ` [PATCH 07/14] bpf/tests: Add more ALU64 BPF_MUL tests Johan Almbladh
2021-07-28 23:32   ` Yonghong Song
2021-07-29 21:21     ` Johan Almbladh
2021-07-28 17:04 ` [PATCH 08/14] bpf/tests: Add tests for ALU operations implemented with function calls Johan Almbladh
2021-07-28 23:52   ` Yonghong Song
2021-07-29 21:17     ` Johan Almbladh [this message]
2021-07-29 22:54       ` Yonghong Song
2021-07-28 17:04 ` [PATCH 09/14] bpf/tests: Add word-order tests for load/store of double words Johan Almbladh
2021-07-28 23:54   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 10/14] bpf/tests: Add branch conversion JIT test Johan Almbladh
2021-07-28 23:58   ` Yonghong Song
2021-07-29 12:45     ` Johan Almbladh
2021-07-29 15:46       ` Yonghong Song
2021-07-29  0:55   ` Yonghong Song
2021-07-29 13:24     ` Johan Almbladh
2021-07-29 15:50       ` Yonghong Song
2021-07-28 17:04 ` [PATCH 11/14] bpf/tests: Add test for 32-bit context pointer argument passing Johan Almbladh
2021-07-29  0:09   ` Yonghong Song
2021-07-29 13:29     ` Johan Almbladh
2021-07-29 15:50       ` Yonghong Song
2021-07-28 17:05 ` [PATCH 12/14] bpf/tests: Add tests for atomic operations Johan Almbladh
2021-07-29  0:36   ` Yonghong Song
2021-07-28 17:05 ` [PATCH 13/14] bpf/tests: Add tests for BPF_CMPXCHG Johan Almbladh
2021-07-29  0:45   ` Yonghong Song
2021-07-28 17:05 ` [PATCH 14/14] bpf/tests: Add tail call test suite Johan Almbladh
2021-07-29  2:56   ` Yonghong Song
2021-07-29 20:44     ` Johan Almbladh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAM1=_QQRuH2K3fMDJCYJuDtTmziqcmtcr31hQeQe-kCkXVC4gA@mail.gmail.com' \
    --to=johan.almbladh@anyfinetworks.com \
    --cc=Tony.Ambardar@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.