Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
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
---
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
Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
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
---
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
Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
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
>>> ---
>>> 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.le...@csgroup.eu/
For the discussion see
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/4fda65cda906e56aa87806b658e0828c64792403.1634190022.git.christophe.le...@csgroup.eu/
Christophe
Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
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
---
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
Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
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
> ---
> 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)) {
> /*
[PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
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
---
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
+
/*
* 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)) {
/*
--
2.34.1
