Re: [RFC bpf-next 2/2] bpf, mips: Add BPF_MEMSX support to the JITs

[Johan Almbladh]

Hi Nicholas,

>  /* Narrow load operation: dst = *(size *)(src + off) */
>  static void emit_ldx_narrow(struct jit_context *ctx,
> -                           u8 dst, u8 src, s16 off, u8 size)
> +                           u8 dst, u8 src, s16 off, u8 size,
> +                           bool sign_extend)
>  {
>         switch (size) {
>         /* Load a byte */
>         case BPF_B:
> -               emit(ctx, lbu, dst, off, src);
> +               if (sign_extend)
> +                       emit(ctx, lb, dst, off, src);
> +               else
> +                       emit(ctx, lbu, dst, off, src);
>                 break;
>         /* Load a half word */
>         case BPF_H:
> -               emit(ctx, lhu, dst, off, src);
> +               if (sign_extend)
> +                       emit(ctx, lh, dst, off, src);
> +               else
> +                       emit(ctx, lhu, dst, off, src);
>                 break;
>         /* Load a word */
>         case BPF_W:
> -               emit(ctx, lwu, dst, off, src);
> +               if (sign_extend)
> +                       emit(ctx, lw, dst, off, src);
> +               else
> +                       emit(ctx, lwu, dst, off, src);
>                 break;
>         }
>  }

I would actually prefer to keep the original structure and not combine
ldx and ldsx into a single function with a boolean lever. The helper
does two completely different things depending on the boolean
argument, and the two modes have nothing (MIPS64) or very little
(MIPS32) code in common.

Another aspect to consider is this: An implementation principle has
been that each emittor should take care of its own "dirty laundry".
This means that the emittor should mark everything it touches as
clobbered, and emit necessary delay slot(s). You could easily call
clobber_reg() in the emit_ldx_narrow() helper, but it does not have
enough context to know if a load delay slot is needed or not. Without
the load delay slot in the helper, an implicit contract between with
the calling function is created. That is fragile and is easy to
overlook if the functionality is further extended in the future. If
you keep the loads inline in the ldx and ldsx emittors, the delay slot
lives together with load that needs it. Note that the clobber_reg()
helper is just idempotent JIT-time book keeping, whereas a delay slot
has a run time penalty.

Thanks,
Johan