| Message ID | f8d581e6a5d9555180c38e009f90d236f310f85e.1633104510.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | powerpc/bpf: Various fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf | success | VM_Test |
| bpf/vmtest-bpf-PR | success | PR summary |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next | success | VM_Test |
On Fri, Oct 1, 2021 at 2:16 PM Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote: > > Add a helper to check if a given offset is within the branch range for a > powerpc conditional branch instruction, and update some sites to use the > new helper. > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> Acked-by: Song Liu <songliubraving@fb.com> With one nitpick: > --- > arch/powerpc/include/asm/code-patching.h | 1 + > arch/powerpc/lib/code-patching.c | 7 ++++++- > arch/powerpc/net/bpf_jit.h | 7 +------ > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index a95f63788c6b14..4ba834599c4d4c 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -23,6 +23,7 @@ > #define BRANCH_ABSOLUTE 0x2 > > bool is_offset_in_branch_range(long offset); > +bool is_offset_in_cond_branch_range(long offset); > int create_branch(struct ppc_inst *instr, const u32 *addr, > unsigned long target, int flags); > int create_cond_branch(struct ppc_inst *instr, const u32 *addr, > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index f9a3019e37b43c..e2342b9a1ab9c9 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -228,6 +228,11 @@ bool is_offset_in_branch_range(long offset) > return (offset >= -0x2000000 && offset <= 0x1fffffc && !(offset & 0x3)); > } > > +bool is_offset_in_cond_branch_range(long offset) > +{ > + return offset >= -0x8000 && offset <= 0x7FFF && !(offset & 0x3); > +} Why not inline this one? > + > /* > * Helper to check if a given instruction is a conditional branch > * Derived from the conditional checks in analyse_instr() > @@ -280,7 +285,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr, > offset = offset - (unsigned long)addr; > > /* Check we can represent the target in the instruction format */ > - if (offset < -0x8000 || offset > 0x7FFF || offset & 0x3) > + if (!is_offset_in_cond_branch_range(offset)) > return 1; > > /* Mask out the flags and target, so they don't step on each other. */ > diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h > index 99fad093f43ec1..935ea95b66359e 100644 > --- a/arch/powerpc/net/bpf_jit.h > +++ b/arch/powerpc/net/bpf_jit.h > @@ -78,11 +78,6 @@ > #define PPC_FUNC_ADDR(d,i) do { PPC_LI32(d, i); } while(0) > #endif > > -static inline bool is_nearbranch(int offset) > -{ > - return (offset < 32768) && (offset >= -32768); > -} > - > /* > * The fly in the ointment of code size changing from pass to pass is > * avoided by padding the short branch case with a NOP. If code size differs > @@ -91,7 +86,7 @@ static inline bool is_nearbranch(int offset) > * state. > */ > #define PPC_BCC(cond, dest) do { \ > - if (is_nearbranch((dest) - (ctx->idx * 4))) { \ > + if (is_offset_in_cond_branch_range((long)(dest) - (ctx->idx * 4))) { \ > PPC_BCC_SHORT(cond, dest); \ > EMIT(PPC_RAW_NOP()); \ > } else { \ > -- > 2.33.0 >
Le 01/10/2021 à 23:14, Naveen N. Rao a écrit : > Add a helper to check if a given offset is within the branch range for a > powerpc conditional branch instruction, and update some sites to use the > new helper. > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/code-patching.h | 1 + > arch/powerpc/lib/code-patching.c | 7 ++++++- > arch/powerpc/net/bpf_jit.h | 7 +------ > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index a95f63788c6b14..4ba834599c4d4c 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -23,6 +23,7 @@ > #define BRANCH_ABSOLUTE 0x2 > > bool is_offset_in_branch_range(long offset); > +bool is_offset_in_cond_branch_range(long offset); > int create_branch(struct ppc_inst *instr, const u32 *addr, > unsigned long target, int flags); > int create_cond_branch(struct ppc_inst *instr, const u32 *addr, > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index f9a3019e37b43c..e2342b9a1ab9c9 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -228,6 +228,11 @@ bool is_offset_in_branch_range(long offset) > return (offset >= -0x2000000 && offset <= 0x1fffffc && !(offset & 0x3)); > } > > +bool is_offset_in_cond_branch_range(long offset) > +{ > + return offset >= -0x8000 && offset <= 0x7FFF && !(offset & 0x3); > +} Would be better without capital letters in numbers, in extenso 0x7fff instead of 0x7FFF > + > /* > * Helper to check if a given instruction is a conditional branch > * Derived from the conditional checks in analyse_instr() > @@ -280,7 +285,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr, > offset = offset - (unsigned long)addr; > > /* Check we can represent the target in the instruction format */ > - if (offset < -0x8000 || offset > 0x7FFF || offset & 0x3) > + if (!is_offset_in_cond_branch_range(offset)) > return 1; > > /* Mask out the flags and target, so they don't step on each other. */ > diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h > index 99fad093f43ec1..935ea95b66359e 100644 > --- a/arch/powerpc/net/bpf_jit.h > +++ b/arch/powerpc/net/bpf_jit.h > @@ -78,11 +78,6 @@ > #define PPC_FUNC_ADDR(d,i) do { PPC_LI32(d, i); } while(0) > #endif > > -static inline bool is_nearbranch(int offset) > -{ > - return (offset < 32768) && (offset >= -32768); > -} > - > /* > * The fly in the ointment of code size changing from pass to pass is > * avoided by padding the short branch case with a NOP. If code size differs > @@ -91,7 +86,7 @@ static inline bool is_nearbranch(int offset) > * state. > */ > #define PPC_BCC(cond, dest) do { \ > - if (is_nearbranch((dest) - (ctx->idx * 4))) { \ > + if (is_offset_in_cond_branch_range((long)(dest) - (ctx->idx * 4))) { \ > PPC_BCC_SHORT(cond, dest); \ > EMIT(PPC_RAW_NOP()); \ > } else { \ >
Hi Song, Thanks for the reviews. Song Liu wrote: > On Fri, Oct 1, 2021 at 2:16 PM Naveen N. Rao > <naveen.n.rao@linux.vnet.ibm.com> wrote: >> >> Add a helper to check if a given offset is within the branch range for a >> powerpc conditional branch instruction, and update some sites to use the >> new helper. >> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > Acked-by: Song Liu <songliubraving@fb.com> > > With one nitpick: > >> --- >> arch/powerpc/include/asm/code-patching.h | 1 + >> arch/powerpc/lib/code-patching.c | 7 ++++++- >> arch/powerpc/net/bpf_jit.h | 7 +------ >> 3 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h >> index a95f63788c6b14..4ba834599c4d4c 100644 >> --- a/arch/powerpc/include/asm/code-patching.h >> +++ b/arch/powerpc/include/asm/code-patching.h >> @@ -23,6 +23,7 @@ >> #define BRANCH_ABSOLUTE 0x2 >> >> bool is_offset_in_branch_range(long offset); >> +bool is_offset_in_cond_branch_range(long offset); >> int create_branch(struct ppc_inst *instr, const u32 *addr, >> unsigned long target, int flags); >> int create_cond_branch(struct ppc_inst *instr, const u32 *addr, >> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c >> index f9a3019e37b43c..e2342b9a1ab9c9 100644 >> --- a/arch/powerpc/lib/code-patching.c >> +++ b/arch/powerpc/lib/code-patching.c >> @@ -228,6 +228,11 @@ bool is_offset_in_branch_range(long offset) >> return (offset >= -0x2000000 && offset <= 0x1fffffc && !(offset & 0x3)); >> } >> >> +bool is_offset_in_cond_branch_range(long offset) >> +{ >> + return offset >= -0x8000 && offset <= 0x7FFF && !(offset & 0x3); >> +} > > Why not inline this one? Good point. This was modeled after the existing is_offset_in_branch_range(), and I guess both of those helpers can be inlined. I'll do a separate patch for that. - Naveen
Hi Christophe, Thanks for the reviews. Christophe Leroy wrote: > > > Le 01/10/2021 à 23:14, Naveen N. Rao a écrit : >> Add a helper to check if a given offset is within the branch range for a >> powerpc conditional branch instruction, and update some sites to use the >> new helper. >> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> arch/powerpc/include/asm/code-patching.h | 1 + >> arch/powerpc/lib/code-patching.c | 7 ++++++- >> arch/powerpc/net/bpf_jit.h | 7 +------ >> 3 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h >> index a95f63788c6b14..4ba834599c4d4c 100644 >> --- a/arch/powerpc/include/asm/code-patching.h >> +++ b/arch/powerpc/include/asm/code-patching.h >> @@ -23,6 +23,7 @@ >> #define BRANCH_ABSOLUTE 0x2 >> >> bool is_offset_in_branch_range(long offset); >> +bool is_offset_in_cond_branch_range(long offset); >> int create_branch(struct ppc_inst *instr, const u32 *addr, >> unsigned long target, int flags); >> int create_cond_branch(struct ppc_inst *instr, const u32 *addr, >> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c >> index f9a3019e37b43c..e2342b9a1ab9c9 100644 >> --- a/arch/powerpc/lib/code-patching.c >> +++ b/arch/powerpc/lib/code-patching.c >> @@ -228,6 +228,11 @@ bool is_offset_in_branch_range(long offset) >> return (offset >= -0x2000000 && offset <= 0x1fffffc && !(offset & 0x3)); >> } >> >> +bool is_offset_in_cond_branch_range(long offset) >> +{ >> + return offset >= -0x8000 && offset <= 0x7FFF && !(offset & 0x3); >> +} > > Would be better without capital letters in numbers, in extenso 0x7fff > instead of 0x7FFF Ack. - Naveen
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index a95f63788c6b14..4ba834599c4d4c 100644 --- a/arch/powerpc/include/asm/code-patching.h +++ b/arch/powerpc/include/asm/code-patching.h @@ -23,6 +23,7 @@ #define BRANCH_ABSOLUTE 0x2 bool is_offset_in_branch_range(long offset); +bool is_offset_in_cond_branch_range(long offset); int create_branch(struct ppc_inst *instr, const u32 *addr, unsigned long target, int flags); int create_cond_branch(struct ppc_inst *instr, const u32 *addr, diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index f9a3019e37b43c..e2342b9a1ab9c9 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -228,6 +228,11 @@ bool is_offset_in_branch_range(long offset) return (offset >= -0x2000000 && offset <= 0x1fffffc && !(offset & 0x3)); } +bool is_offset_in_cond_branch_range(long offset) +{ + return offset >= -0x8000 && offset <= 0x7FFF && !(offset & 0x3); +} + /* * Helper to check if a given instruction is a conditional branch * Derived from the conditional checks in analyse_instr() @@ -280,7 +285,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr, offset = offset - (unsigned long)addr; /* Check we can represent the target in the instruction format */ - if (offset < -0x8000 || offset > 0x7FFF || offset & 0x3) + if (!is_offset_in_cond_branch_range(offset)) return 1; /* Mask out the flags and target, so they don't step on each other. */ diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h index 99fad093f43ec1..935ea95b66359e 100644 --- a/arch/powerpc/net/bpf_jit.h +++ b/arch/powerpc/net/bpf_jit.h @@ -78,11 +78,6 @@ #define PPC_FUNC_ADDR(d,i) do { PPC_LI32(d, i); } while(0) #endif -static inline bool is_nearbranch(int offset) -{ - return (offset < 32768) && (offset >= -32768); -} - /* * The fly in the ointment of code size changing from pass to pass is * avoided by padding the short branch case with a NOP. If code size differs @@ -91,7 +86,7 @@ static inline bool is_nearbranch(int offset) * state. */ #define PPC_BCC(cond, dest) do { \ - if (is_nearbranch((dest) - (ctx->idx * 4))) { \ + if (is_offset_in_cond_branch_range((long)(dest) - (ctx->idx * 4))) { \ PPC_BCC_SHORT(cond, dest); \ EMIT(PPC_RAW_NOP()); \ } else { \
Add a helper to check if a given offset is within the branch range for a powerpc conditional branch instruction, and update some sites to use the new helper. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- arch/powerpc/include/asm/code-patching.h | 1 + arch/powerpc/lib/code-patching.c | 7 ++++++- arch/powerpc/net/bpf_jit.h | 7 +------ 3 files changed, 8 insertions(+), 7 deletions(-)