Re: [PATCH 06/14] bpf/tests: Add more BPF_LSH/RSH/ARSH tests for ALU64

[Johan Almbladh <johan.almbladh@anyfinetworks.com>]

On Thu, Jul 29, 2021 at 1:30 AM Yonghong Song <yhs@fb.com> wrote:
> > @@ -4139,6 +4139,106 @@ static struct bpf_test tests[] = {
> >               { },
> >               { { 0, 0x80000000 } },
> >       },
> > +     {
> > +             "ALU64_LSH_X: Shift < 32, low word",
> > +             .u.insns_int = {
> > +                     BPF_LD_IMM64(R0, 0x0123456789abcdefLL),
> > +                     BPF_ALU32_IMM(BPF_MOV, R1, 12),
> > +                     BPF_ALU64_REG(BPF_LSH, R0, R1),
> > +                     BPF_EXIT_INSN(),
> > +             },
> > +             INTERNAL,
> > +             { },
> > +             { { 0, 0xbcdef000 } }
>
> In bpf_test struct, the result is defined as __u32
>          struct {
>                  int data_size;
>                  __u32 result;
>          } test[MAX_SUBTESTS];
>
> But the above result 0xbcdef000 does not really capture the bpf program
> return value, which should be 0x3456789abcdef000.
> Can we change "result" type to __u64 so the result truly captures the
> program return value?

This was also my though at first, but I don't think that is possible.
As I understand it, the eBPF functions have the prototype int
func(struct *ctx). While the context pointer will have a different
size on 32-bit and 64-bit architectures, the return value will always
be 32 bits on most, or all, platforms.

> We have several other similar cases for the rest of this patch.

I have used two ways to check the full 64-bit result in such cases.

1) Load the expected result as a 64-bit value in a register. Then jump
conditionally if the result matches this value or not. The jump
destinations each set a distinct value in R0, which is finally
examined as the result.

2) Run the test twice. The first one returns the low 32-bits of R0.
The second adds a 32-bit right shift to return the high 32 bits.

When I first wrote the tests I tried to use as few complex
instructions not under test as possible, in order to test each
instruction in isolation. Since the 32-bit right shift is a much
simpler operation than conditional jumps, at least in the 32-bit MIPS
JIT, I chose method (2) for most of the tests. Existing tests seem to
use method (1), so in some cases I used that instead when adding more
tests of the same operation. The motivation for the simple one-by-one
tests is mainly convenience and better diagnostics during JIT
development. Both methods (1) and (2) are equally valid of course.

By the way, thanks a lot for the review, Yonghong!

Johan