On 7/29/21 5:34 AM, Johan Almbladh wrote:

On Thu, Jul 29, 2021 at 1:30 AM Yonghong Song [off-list ref] wrote:

quoted

quoted

@@ -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.

Thanks for explanation. Yes, all BPF_PROG_RUN variables have bpf program
return type u32, so you are right, we cannot really check prog return
value against a 64bit R0.

quoted

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.

it is totally okay to use (2). Your tests are fine in that regard.

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

You are welcome!

Johan