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;
>> + }
>> + }
>> +}
>
> [...]
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
