Re: [PATCH 4/9] powerpc/bpf: Handle large branch ranges with BPF_EXIT

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Christophe Leroy wrote:
> 
> 
> Le 01/10/2021 à 23:14, Naveen N. Rao a écrit :
>> In some scenarios, it is possible that the program epilogue is outside
>> the branch range for a BPF_EXIT instruction. Instead of rejecting such
>> programs, emit an indirect branch. We track the size of the bpf program
>> emitted after the initial run and do a second pass since BPF_EXIT can
>> end up emitting different number of instructions depending on the
>> program size.
>> 
>> Suggested-by: Jordan Niethe <jniethe5@gmail.com>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/net/bpf_jit.h        |  3 +++
>>   arch/powerpc/net/bpf_jit_comp.c   | 22 +++++++++++++++++++++-
>>   arch/powerpc/net/bpf_jit_comp32.c |  2 +-
>>   arch/powerpc/net/bpf_jit_comp64.c |  2 +-
>>   4 files changed, 26 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
>> index 89bd744c2bffd4..4023de1698b9f5 100644
>> --- a/arch/powerpc/net/bpf_jit.h
>> +++ b/arch/powerpc/net/bpf_jit.h
>> @@ -126,6 +126,7 @@
>>   
>>   #define SEEN_FUNC	0x20000000 /* might call external helpers */
>>   #define SEEN_TAILCALL	0x40000000 /* uses tail calls */
>> +#define SEEN_BIG_PROG	0x80000000 /* large prog, >32MB */
>>   
>>   #define SEEN_VREG_MASK	0x1ff80000 /* Volatile registers r3-r12 */
>>   #define SEEN_NVREG_MASK	0x0003ffff /* Non volatile registers r14-r31 */
>> @@ -179,6 +180,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>>   void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
>>   void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
>>   void bpf_jit_realloc_regs(struct codegen_context *ctx);
>> +int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx,
>> +					int tmp_reg, unsigned long exit_addr);
>>   
>>   #endif
>>   
>> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
>> index fcbf7a917c566e..3204872fbf2738 100644
>> --- a/arch/powerpc/net/bpf_jit_comp.c
>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>> @@ -72,6 +72,21 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
>>   	return 0;
>>   }
>>   
>> +int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx,
>> +					int tmp_reg, unsigned long exit_addr)
>> +{
>> +	if (!(ctx->seen & SEEN_BIG_PROG) && is_offset_in_branch_range(exit_addr)) {
>> +		PPC_JMP(exit_addr);
>> +	} else {
>> +		ctx->seen |= SEEN_BIG_PROG;
>> +		PPC_FUNC_ADDR(tmp_reg, (unsigned long)image + exit_addr);
>> +		EMIT(PPC_RAW_MTCTR(tmp_reg));
>> +		EMIT(PPC_RAW_BCTR());
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   struct powerpc64_jit_data {
>>   	struct bpf_binary_header *header;
>>   	u32 *addrs;
>> @@ -155,12 +170,17 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>>   		goto out_addrs;
>>   	}
>>   
>> +	if (!is_offset_in_branch_range((long)cgctx.idx * 4))
>> +		cgctx.seen |= SEEN_BIG_PROG;
>> +
>>   	/*
>>   	 * If we have seen a tail call, we need a second pass.
>>   	 * This is because bpf_jit_emit_common_epilogue() is called
>>   	 * from bpf_jit_emit_tail_call() with a not yet stable ctx->seen.
>> +	 * We also need a second pass if we ended up with too large
>> +	 * a program so as to fix branches.
>>   	 */
>> -	if (cgctx.seen & SEEN_TAILCALL) {
>> +	if (cgctx.seen & (SEEN_TAILCALL | SEEN_BIG_PROG)) {
>>   		cgctx.idx = 0;
>>   		if (bpf_jit_build_body(fp, 0, &cgctx, addrs, false)) {
>>   			fp = org_fp;
>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
>> index a74d52204f8da2..d2a67574a23066 100644
>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>> @@ -852,7 +852,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>>   			 * we'll just fall through to the epilogue.
>>   			 */
>>   			if (i != flen - 1)
>> -				PPC_JMP(exit_addr);
>> +				bpf_jit_emit_exit_insn(image, ctx, tmp_reg, exit_addr);
> 
> On ppc32, if you use tmp_reg you must flag it. But I think you could use 
> r0 instead.

Indeed. Can we drop tracking of the temp registers and using them while
remapping registers? Are you seeing significant benefits with re-use of 
those temp registers?

- Naveen