| Message ID | 1430314657-2552-5-git-send-email-nschichan@freebox.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote: > In that case, emit_udiv() will be called with rn == ARM_R0 (r_scratch) > and loading rm first into ARM_R0 will result in jit_udiv() function > being called the same dividend and divisor. Fix that by loading rn > first into ARM_R1 and then rm into ARM_R0. > > Signed-off-by: Nicolas Schichan <nschichan@freebox.fr> > --- > arch/arm/net/bpf_jit_32.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c > index b5f470d..ffaf311 100644 > --- a/arch/arm/net/bpf_jit_32.c > +++ b/arch/arm/net/bpf_jit_32.c > @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx) > return; > } > #endif > - if (rm != ARM_R0) > - emit(ARM_MOV_R(ARM_R0, rm), ctx); > if (rn != ARM_R1) > emit(ARM_MOV_R(ARM_R1, rn), ctx); > + if (rm != ARM_R0) > + emit(ARM_MOV_R(ARM_R0, rm), ctx); I don't think you've thought enough about this. What if rm is ARM_R1? What if rn = ARM_R0 and rm = ARM_R1? How about: if (rn == ARM_R0 && rm == ARM_R1) { emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn) emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm) emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3 } else if (rn == ARM_R0) { emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn if (rm != ARM_R0) emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm } else { if (rm != ARM_R0) emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm if (rn != ARM_R1) emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn }
On 05/01/2015 07:37 PM, Russell King - ARM Linux wrote: > On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote: [...] >> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c >> index b5f470d..ffaf311 100644 >> --- a/arch/arm/net/bpf_jit_32.c >> +++ b/arch/arm/net/bpf_jit_32.c >> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx) >> return; >> } >> #endif >> - if (rm != ARM_R0) >> - emit(ARM_MOV_R(ARM_R0, rm), ctx); >> if (rn != ARM_R1) >> emit(ARM_MOV_R(ARM_R1, rn), ctx); >> + if (rm != ARM_R0) >> + emit(ARM_MOV_R(ARM_R0, rm), ctx); > > I don't think you've thought enough about this. What if rm is ARM_R1? > What if rn = ARM_R0 and rm = ARM_R1? > > How about: > > if (rn == ARM_R0 && rm == ARM_R1) { > emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn) > emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm) > emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3 > } else if (rn == ARM_R0) { > emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn > if (rm != ARM_R0) > emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm > } else { > if (rm != ARM_R0) > emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm > if (rn != ARM_R1) > emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn > } > Hello Russell, In the current JIT, emit_udiv() is only being called with: - rm = ARM_R4 (r_A) and rn = ARM_R0 (r_scrach) for BPF_ALU | BPF_DIV | BPF_K - rm = ARM_R4 (r_A) and rn = ARM_R5 (r_X) for BPF_ALU | BPF_DIV | BPF_X so it should not cause any issue in the current code state. But yes, I'll rework the patch to avoid any other nasty surprises should the code change. Thanks,
On Mon, May 04, 2015 at 06:16:30PM +0200, Nicolas Schichan wrote: > On 05/01/2015 07:37 PM, Russell King - ARM Linux wrote: > > On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote: > [...] > >> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c > >> index b5f470d..ffaf311 100644 > >> --- a/arch/arm/net/bpf_jit_32.c > >> +++ b/arch/arm/net/bpf_jit_32.c > >> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx) > >> return; > >> } > >> #endif > >> - if (rm != ARM_R0) > >> - emit(ARM_MOV_R(ARM_R0, rm), ctx); > >> if (rn != ARM_R1) > >> emit(ARM_MOV_R(ARM_R1, rn), ctx); > >> + if (rm != ARM_R0) > >> + emit(ARM_MOV_R(ARM_R0, rm), ctx); > > > > I don't think you've thought enough about this. What if rm is ARM_R1? > > What if rn = ARM_R0 and rm = ARM_R1? > > > > How about: > > > > if (rn == ARM_R0 && rm == ARM_R1) { > > emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn) > > emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm) > > emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3 > > } else if (rn == ARM_R0) { > > emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn > > if (rm != ARM_R0) > > emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm > > } else { > > if (rm != ARM_R0) > > emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm > > if (rn != ARM_R1) > > emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn > > } > > > > Hello Russell, > > In the current JIT, emit_udiv() is only being called with: > > - rm = ARM_R4 (r_A) and rn = ARM_R0 (r_scrach) for BPF_ALU | BPF_DIV | BPF_K > > - rm = ARM_R4 (r_A) and rn = ARM_R5 (r_X) for BPF_ALU | BPF_DIV | BPF_X > > so it should not cause any issue in the current code state. > > But yes, I'll rework the patch to avoid any other nasty surprises should the > code change. Maybe then add a comment detailing the current conditions that this is coded for so that if you're not around when the code changes, others are aware of the issue.
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c index b5f470d..ffaf311 100644 --- a/arch/arm/net/bpf_jit_32.c +++ b/arch/arm/net/bpf_jit_32.c @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx) return; } #endif - if (rm != ARM_R0) - emit(ARM_MOV_R(ARM_R0, rm), ctx); if (rn != ARM_R1) emit(ARM_MOV_R(ARM_R1, rn), ctx); + if (rm != ARM_R0) + emit(ARM_MOV_R(ARM_R0, rm), ctx); ctx->seen |= SEEN_CALL; emit_mov_i(ARM_R3, (u32)jit_udiv, ctx);
In that case, emit_udiv() will be called with rn == ARM_R0 (r_scratch) and loading rm first into ARM_R0 will result in jit_udiv() function being called the same dividend and divisor. Fix that by loading rn first into ARM_R1 and then rm into ARM_R0. Signed-off-by: Nicolas Schichan <nschichan@freebox.fr> --- arch/arm/net/bpf_jit_32.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)