Hi Xu, thanks for the review
On Tue May 27, 2025 at 10:11 AM CEST, Xu Kuohai wrote:
On 5/22/2025 6:14 PM, Alexis Lothoré wrote:
[...]
-static void save_args(struct jit_ctx *ctx, int args_off, int nregs) +struct arg_aux {
- /* how many args are passed through registers, the rest of the args are
* passed through stack*/- int args_in_regs;
- /* how many registers are used to pass arguments */
- int regs_for_args;
- /* how much stack is used for additional args passed to bpf program
* that did not fit in original function registers**/nit: "**/" should be "*/"
ACK
[...]
- a->ostack_for_args = 0;
- /* the rest arguments are passed through stack */
- for (a->ostack_for_args = 0, a->bstack_for_args = 0;
i < m->nr_args; i++) {a->ostack_for_args is initialized twice.
move all initializations before the loop?
ACK
/* We can not know for sure about exact alignment needs for* struct passed on stack, so deny those*/if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)return -EOPNOTSUPP;leave the error code as is, namely, return -ENOTSUPP?
Actually this change follows a complaint from checkpatch:
"WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP"
stack_slots = (m->arg_size[i] + 7) / 8;/* AAPCS 64 C.14: arguments passed on stack must be aligned to* max(8, arg_natural_alignment)*/a->bstack_for_args += stack_slots * 8;a->ostack_for_args = round_up(a->ostack_for_args + stack_slots * 8, 8);since a->ostack_for_args starts from 0 and is always incremented by multiples of 8, round_up() to 8 is not needed.
True. This is a (partial) remnant from the first attempt to handle more exotic alignments like large structs or __int128, but that's indeed not needed for this current version. I'll clean it up.
[...]
- for (i = a->args_in_regs; i < m->nr_args; i++) {
slots = (m->arg_size[i] + 7) / 8;/* AAPCS C.14: additional arguments on stack must be* aligned on max(8, arg_natural_alignment)*/soff = round_up(soff, 8);if (for_call_origin)doff = round_up(doff, 8);since both soff and doff start from multiples of 8 and are incremented by 8 each time, the two round_up()s are also not needed.
ACK. I guess the small AAPCS mention can go too then.
/* verifier ensures arg_size <= 16, so slots equals 1 or 2 */while (slots-- > 0) {emit(A64_LDR64I(tmp, A64_FP, soff), ctx);/* if there is unused space in the last slot, clear* the garbage contained in the space.*/if (slots == 0 && !for_call_origin)clear_garbage(ctx, tmp, m->arg_size[i] % 8);emit(A64_STR64I(tmp, A64_SP, doff), ctx);soff += 8;doff += 8;}- }
+}
[...]