Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry

From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Michael Ellerman <mpe@ellerman.id.au>
Cc: "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	Hari Bathini <hbathini@linux.ibm.com>,
	"johan.almbladh@anyfinetworks.com"
	<johan.almbladh@anyfinetworks.com>, Jiri Olsa <jolsa@redhat.com>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"song@kernel.org" <song@kernel.org>,
	"ykaliuta@redhat.com" <ykaliuta@redhat.com>
Subject: Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
Date: Tue, 11 Jan 2022 16:01:36 +0530	[thread overview]
Message-ID: <1641896867.1ukblu8135.naveen@linux.ibm.com> (raw)
In-Reply-To: <d0e28f07-c24c-200d-de04-5d27c651a5e6@csgroup.eu>

Christophe Leroy wrote:
> 
> 
> Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
>> In preparation for using kernel TOC, load the same in r2 on entry. With
>> elfv1, the kernel TOC is already setup by our caller so we just emit a
>> nop. We adjust the number of instructions to skip on a tail call
>> accordingly.
>> 
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/net/bpf_jit_comp64.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
>> index ce4fc59bbd6a92..e05b577d95bf11 100644
>> --- a/arch/powerpc/net/bpf_jit_comp64.c
>> +++ b/arch/powerpc/net/bpf_jit_comp64.c
>> @@ -73,6 +73,12 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
>>   {
>>   	int i;
>>   
>> +#ifdef PPC64_ELF_ABI_v2
>> +	PPC_BPF_LL(_R2, _R13, offsetof(struct paca_struct, kernel_toc));
>> +#else
>> +	EMIT(PPC_RAW_NOP());
>> +#endif
> 
> Can we avoid the #ifdef, using
> 
> 	if (__is_defined(PPC64_ELF_ABI_v2))
> 		PPC_BPF_LL(_R2, _R13, offsetof(struct paca_struct, kernel_toc));
> 	else
> 		EMIT(PPC_RAW_NOP());

Hmm... that doesn't work for me. Is __is_defined() expected to work with 
macros other than CONFIG options?

> 
>> +
>>   	/*
>>   	 * Initialize tail_call_cnt if we do tail calls.
>>   	 * Otherwise, put in NOPs so that it can be skipped when we are
>> @@ -87,7 +93,7 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
>>   		EMIT(PPC_RAW_NOP());
>>   	}
>>   
>> -#define BPF_TAILCALL_PROLOGUE_SIZE	8
>> +#define BPF_TAILCALL_PROLOGUE_SIZE	12
> 
> Why not change that for v2 ABI only instead of adding a NOP ? ABI won't 
> change during runtime AFAIU

Yeah, I wanted to keep this simple and I felt an additional nop 
shouldn't matter too much. But, I guess we can get rid of 
BPF_TAILCALL_PROLOGUE_SIZE since the only user is the function emitting 
a tail call. I will submit that as a separate cleanup unless I need to 
redo this series.

Thanks for the reviews!
- Naveen