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 <[email protected]>
> ---
> 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.
- Naveen
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 <[email protected]>
>> ---
>> 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.
My understanding is that bpf_int_jit_compile() is called twice by
jit_subprogs(), second call sets 'extra_pass" due to jit_data->addrs =
addrs being set at the end of first pass.
Christophe
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 <[email protected]>
>>> ---
>>> 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
Le 10/01/2023 à 09:44, Naveen N. Rao a écrit :
> 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 <[email protected]>
>>>> ---
>>>> 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.
Ah right, I see.
I will send out v2 shortly.
Thanks
Christophe