Re: [PATCH v1 06/10] powerpc/bpf: Perform complete extra passes to update addresses

[Naveen N. Rao]

Christophe Leroy wrote:
> Le 13/12/2022 à 11:23, Naveen N. Rao a écrit :
> > Christophe Leroy wrote:
> > > BPF core calls the jit compiler again for an extra pass in order
> > > to properly set subprog addresses.
> > >
> > > Unlike other architectures, powerpc only updates the addresses
> > > during that extra pass. It means that holes must have been left
> > > in the code in order to enable the maximum possible instruction
> > > size.
> > >
> > > In order avoid waste of space, and waste of CPU time on powerpc
> > > processors on which the NOP instruction is not 0-cycle, perform
> > > two real additional passes.
> > >
> > > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> > > ---
> > >  arch/powerpc/net/bpf_jit_comp.c | 85 ---------------------------------
> > >  1 file changed, 85 deletions(-)
> > >
> > > diff --git a/arch/powerpc/net/bpf_jit_comp.c 
> > > b/arch/powerpc/net/bpf_jit_comp.c
> > > index 43e634126514..8833bf23f5aa 100644
> > > --- a/arch/powerpc/net/bpf_jit_comp.c
> > > +++ b/arch/powerpc/net/bpf_jit_comp.c
> > > @@ -23,74 +23,6 @@ static void bpf_jit_fill_ill_insns(void *area, 
> > > unsigned int size)
> > >      memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
> > >  }
> > >
> > > -/* Fix updated addresses (for subprog calls, ldimm64, et al) during 
> > > extra pass */
> > > -static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
> > > -                   struct codegen_context *ctx, u32 *addrs)
> > > -{
> > > -    const struct bpf_insn *insn = fp->insnsi;
> > > -    bool func_addr_fixed;
> > > -    u64 func_addr;
> > > -    u32 tmp_idx;
> > > -    int i, j, ret;
> > > -
> > > -    for (i = 0; i < fp->len; i++) {
> > > -        /*
> > > -         * During the extra pass, only the branch target addresses for
> > > -         * the subprog calls need to be fixed. All other instructions
> > > -         * can left untouched.
> > > -         *
> > > -         * The JITed image length does not change because we already
> > > -         * ensure that the JITed instruction sequence for these calls
> > > -         * are of fixed length by padding them with NOPs.
> > > -         */
> > > -        if (insn[i].code == (BPF_JMP | BPF_CALL) &&
> > > -            insn[i].src_reg == BPF_PSEUDO_CALL) {
> > > -            ret = bpf_jit_get_func_addr(fp, &insn[i], true,
> > > -                            &func_addr,
> > > -                            &func_addr_fixed);
> >
> > I don't see you updating calls to bpf_jit_get_func_addr() in 
> > bpf_jit_build_body() to set extra_pass to true. Afaics, that's required 
> > to get the correct address to be branched to for subprogs.
>
> I don't understand what you mean.

I am referring to the third parameter we pass to 
bpf_jit_get_func_addr().

In bpf_jit_build_body(), we do:

		case BPF_JMP | BPF_CALL:
			ctx->seen |= SEEN_FUNC;

			ret = bpf_jit_get_func_addr(fp, &insn[i], false,
						    &func_addr, &func_addr_fixed);


The third parameter (extra_pass) to bpf_jit_get_func_addr() is set to 
false. In bpf_jit_get_func_addr(), we have:

	*func_addr_fixed = insn->src_reg != BPF_PSEUDO_CALL;
	if (!*func_addr_fixed) {
		/* Place-holder address till the last pass has collected
		 * all addresses for JITed subprograms in which case we
		 * can pick them up from prog->aux.
		 */
		if (!extra_pass)
			addr = NULL;

Before this patch series, in bpf_jit_fixup_addresses(), we were calling 
bpf_jit_get_func_addr() with the third parameter set to true.


- Naveen