| Message ID | 4501050f6080f12bd3ba1b5d9d7bef8d3aa57d23.1641468127.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | powerpc/bpf: Some fixes and updates | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-PR | fail | merge-conflict |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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()); > + > /* > * 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 > > if (bpf_has_stack_frame(ctx)) { > /*
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
Le 11/01/2022 à 11:31, Naveen N. Rao a écrit : > 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? Yes, __is_defined() should work with any item. It is IS_ENABLED() which is supposed to work only with CONFIG options. See commit 5c189c523e78 ("powerpc/time: Fix mftb()/get_tb() for use with the compat VDSO") Or commit ca5999fde0a1 ("mm: introduce include/linux/pgtable.h") > >> >>> + >>> /* >>> * 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. > All this make me think about a discussion I had some time ago with Nic, and we ended up trying to get rid of PPC64_ELF_ABI_v2 macro and use a CONFIG item instead. For the result see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/ad0eb12f6e3f49b4a3284fc54c4c4d70c496609e.1634457599.git.christophe.leroy@csgroup.eu/ For the discussion see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/4fda65cda906e56aa87806b658e0828c64792403.1634190022.git.christophe.leroy@csgroup.eu/ Christophe
Le 11/01/2022 à 15:35, Christophe Leroy a écrit : > > > Le 11/01/2022 à 11:31, Naveen N. Rao a écrit : >> 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? > > Yes, __is_defined() should work with any item. > > It is IS_ENABLED() which is supposed to work only with CONFIG options. > > See commit 5c189c523e78 ("powerpc/time: Fix mftb()/get_tb() for use with > the compat VDSO") > > Or commit ca5999fde0a1 ("mm: introduce include/linux/pgtable.h") Ah ... wait. It seems it expects a macro set to 1. So it would require arch/powerpc/include/asm/types.h to be modified to define PPC64_ELF_ABI_v2 or PPC64_ELF_ABI_v1 as 1 Christophe
Christophe Leroy wrote: > > > Le 11/01/2022 à 15:35, Christophe Leroy a écrit : >> >> >> Le 11/01/2022 à 11:31, Naveen N. Rao a écrit : >>> 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? >> >> Yes, __is_defined() should work with any item. >> >> It is IS_ENABLED() which is supposed to work only with CONFIG options. I suppose you are saying that due to the name? Since IS_ENABLED() and IS_BUILTIN() seem to work fine too, once I define the macro as 1. Along those lines, it would have been nice to have IS_DEFINED(). >> >> See commit 5c189c523e78 ("powerpc/time: Fix mftb()/get_tb() for use with >> the compat VDSO") >> >> Or commit ca5999fde0a1 ("mm: introduce include/linux/pgtable.h") > > Ah ... wait. > > It seems it expects a macro set to 1. > > So it would require arch/powerpc/include/asm/types.h to be modified to > define PPC64_ELF_ABI_v2 or PPC64_ELF_ABI_v1 as 1 Thanks, that works. - Naveen
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 + /* * 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 if (bpf_has_stack_frame(ctx)) { /*
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(-)