[DTrace-devel] [PATCH] Add 'endianness conversion' BPF instruction macro

[Kris Van Hees]

On Wed, Jul 07, 2021 at 04:46:39PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  include/bpf_asm.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/bpf_asm.h b/include/bpf_asm.h
> index cac60c24..11e8ad93 100644
> --- a/include/bpf_asm.h
> +++ b/include/bpf_asm.h
> @@ -36,6 +36,15 @@
>  		.imm = 0						\
>  	})
>  
> +#define BPF_END_REG(dst, dir, sz)					\
> +	((struct bpf_insn) {						\
> +		.code = BPF_ALU | BPF_END | (dir),			\
> +		.dst_reg = (dst),					\
> +		.src_reg = 0,						\
> +		.off = 0,						\
> +		.imm = (sz)						\
> +	})

This is one of those cases where the BPF instruction set has been hopelessly
abused to fit something into it and we're stuck with it.  But I would prefer
tp hide the kludge of these instructions, and instead present ourselves with
a more sensible way to provide this instruction.

Two things bother me with the way the BPF instruction is encoded in the kernel
implementation:

1. the width of the value is stored as an integer in the imm field while all
   other instructions use an encoded width (BPF_DW, BPF_W, BPF_H, ...)
2. the bits typically used to encode the width of an operation is now used to
   indicate the direction (to BE or to LE)

It would have made more sense (I think) if the width was encoded as it is with
all other instructions, and that the direction was encoded in the imm field.

So, what I would prefer to do here is:

   #define BPF_END_REG(sz, dst, dir)					\
	((struct bpf_insn) {						\
		.code = BPF_ALU | BPF_END | (dir),			\
		.dst_reg = (dst),					\
		.src_reg = 0,						\
		.off = 0,						\
		.imm = (sz)						\
		.imm = (sz) == BPF_DW ? 64 :				\
		       (sz) == BPF_W ? 32 :				\
		       (sz) == BPF_H ? 16 : 0				\
	})

This way you can write code like BPF_END_REG(BPF_DW, BPF_REG_5, BPF_TO_LE)
which is very much like other instructions.  I think it is worth having that
consistency, and since (sz) is pretty much always going to be a constant,
the compiler should optimize that nested conditional expression into the
proper constant value anyway, so there is no performance impact either.

>  #define BPF_NEG_REG(dst)						\
>  	((struct bpf_insn) {						\
>  		.code = BPF_ALU64 | BPF_NEG,				\
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel