Re: [PATCH bpf-next v1 06/14] bpf, s390: Convert struct_ops arena arguments

> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index 20b6e53999b3f..964f1ad82637b 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -2607,6 +2607,34 @@ static void load_imm64(struct bpf_jit *jit, int dst_reg, u64 val)
>  	EMIT6_IMM(0xc00d0000, dst_reg, val);
>  }
>
> +/*
> + * Convert an arena kernel address into the arena pointer form on its way
> + * into the BPF ctx, dst = (u32)(src - kern_vm_start). A nullable arg
> + * preserves NULL, tested on the full 64-bit kernel pointer. The 32-bit
> + * subtraction followed by zero-extension keeps the upper half clear.
> + */
> +static void emit_arena_arg_conv(struct bpf_jit *jit, int dst, int src,
> +				bool nullable, u32 base_lo)
> +{

[ ... ]

> +	/* llilf %w1,base_lo */
> +	EMIT6_IMM(0xc00f0000, REG_W1, base_lo);
> +	/* sr %dst,%w1 */
> +	EMIT2(0x1b00, dst, REG_W1);

This isn't a bug, but could slfi %dst,base_lo replace the llilf %w1 +
sr %dst,%w1 pair here, dropping the per-argument reload of the base and
the %r1 clobber?

> +	/* llgfr %dst,%dst */
> +	EMIT4(0xb9160000, dst, dst);
> +	/* 1: */
> +}

[ ... ]

> @@ -2829,14 +2868,33 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>  		bpf_arg_off = tjit->bpf_args_off + j * sizeof(u64);
>  		if (m->arg_size[i] <= 8) {
> -			if (i < MAX_NR_REG_ARGS)
> +			bool arena_arg = arena_base &&
> +					 (m->arg_flags[i] & BTF_FMODEL_ARENA_ARG);
> +			bool nullable = m->arg_flags[i] & BTF_FMODEL_NULLABLE_ARG;
> +
> +			if (arena_arg) {
> +				if (i < MAX_NR_REG_ARGS) {
> +					emit_arena_arg_conv(jit, REG_W0, arg, nullable,
> +							    (u32)arena_base);
> +				} else {
> +					/* lg %w0,arg(%r15) */
> +					EMIT6_DISP_LH(0xe3000000, 0x0004, REG_W0,
> +						      REG_0, REG_15, arg);
> +					emit_arena_arg_conv(jit, REG_W0, REG_W0,
> +							    nullable, (u32)arena_base);
> +				}
> +				/* stg %w0,bpf_arg_off(%r15) */
> +				EMIT6_DISP_LH(0xe3000000, 0x0024, REG_W0,
> +						      REG_0, REG_15, bpf_arg_off);
> +			} else if (i < MAX_NR_REG_ARGS) {
>  				/* stg %arg,bpf_arg_off(%r15) */
>  				EMIT6_DISP_LH(0xe3000000, 0x0024, arg,
> -					      REG_0, REG_15, bpf_arg_off);
> -			else
> +						      REG_0, REG_15, bpf_arg_off);

This isn't a bug, but was the extra tab on the REG_0, REG_15, bpf_arg_off
continuation lines intended? Realigning them to the open parenthesis (as
the nearby lg %w0,arg(%r15) already is) would keep the diff to the
functional change.

> +			} else {
>  				/* mvc bpf_arg_off(8,%r15),arg(%r15) */
>  				_EMIT6(0xd207f000 | bpf_arg_off,
>  				       0xf000 | arg);
> +			}

[ ... ]

>     Keep the native argument index separate from the BPF ctx slot index. The
>     former selects r2-r6 or one caller stack slot per s390 ABI argument, while
>     the latter still expands 16-byte arguments to two slots. This also converts
>     arena pointers passed after the fifth argument without shifting later
>     values.

This isn't a bug, but since i and j were already separate before this
change, could this paragraph be trimmed to just the new part - that arena
pointers beyond the fifth argument are loaded, converted and stored in
place?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32538351576