[v2,bpf-next,01/13] bpf: x86: Factor out emission of ModR/M for *(reg + off)
diff mbox series

Message ID 20201127175738.1085417-2-jackmanb@google.com
State New, archived
Headers show
Series
  • Atomics for eBPF
Related show

Commit Message

Brendan Jackman Nov. 27, 2020, 5:57 p.m. UTC
The case for JITing atomics is about to get more complicated. Let's
factor out some common code to make the review and result more
readable.

NB the atomics code doesn't yet use the new helper - a subsequent
patch will add its use as a side-effect of other changes.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 arch/x86/net/bpf_jit_comp.c | 42 +++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 18 deletions(-)

Comments

Alexei Starovoitov Nov. 29, 2020, 1:15 a.m. UTC | #1
On Fri, Nov 27, 2020 at 05:57:26PM +0000, Brendan Jackman wrote:
> +/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
> +static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)

same concern as in the another patch. If you could avoid intel's puzzling names
like above it will make reviewing the patch easier.
Brendan Jackman Dec. 1, 2020, 12:14 p.m. UTC | #2
On Sat, Nov 28, 2020 at 05:15:52PM -0800, Alexei Starovoitov wrote:
> On Fri, Nov 27, 2020 at 05:57:26PM +0000, Brendan Jackman wrote:
> > +/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
> > +static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> 
> same concern as in the another patch. If you could avoid intel's puzzling names
> like above it will make reviewing the patch easier.

In this case there is actually a call like

  emit_modrm_dstoff(&prog, src_reg, dst_reg)

So calling the function args dst_reg, src_reg would be misleading.

I could call them ptr_reg and val_reg or something?
Alexei Starovoitov Dec. 2, 2020, 5:50 a.m. UTC | #3
On Tue, Dec 1, 2020 at 4:14 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Sat, Nov 28, 2020 at 05:15:52PM -0800, Alexei Starovoitov wrote:
> > On Fri, Nov 27, 2020 at 05:57:26PM +0000, Brendan Jackman wrote:
> > > +/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
> > > +static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> >
> > same concern as in the another patch. If you could avoid intel's puzzling names
> > like above it will make reviewing the patch easier.
>
> In this case there is actually a call like
>
>   emit_modrm_dstoff(&prog, src_reg, dst_reg)

emit_insn_prefix() ?
Brendan Jackman Dec. 2, 2020, 10:52 a.m. UTC | #4
Tue, Dec 01, 2020 at 09:50:00PM -0800, Alexei Starovoitov wrote:
> On Tue, Dec 1, 2020 at 4:14 AM Brendan Jackman <jackmanb@google.com> wrote:
> >
> > On Sat, Nov 28, 2020 at 05:15:52PM -0800, Alexei Starovoitov wrote:
> > > On Fri, Nov 27, 2020 at 05:57:26PM +0000, Brendan Jackman wrote:
> > > > +/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
> > > > +static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> > >
> > > same concern as in the another patch. If you could avoid intel's puzzling names
> > > like above it will make reviewing the patch easier.
> >
> > In this case there is actually a call like
> >
> >   emit_modrm_dstoff(&prog, src_reg, dst_reg)
> 
> emit_insn_prefix() ?

Ah sorry, I thought you were talking about the _arg_ names.

This isn't a prefix, but emit_insn_suffix sounds good.
Alexei Starovoitov Dec. 2, 2020, 5:35 p.m. UTC | #5
On Wed, Dec 2, 2020 at 2:52 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> Tue, Dec 01, 2020 at 09:50:00PM -0800, Alexei Starovoitov wrote:
> > On Tue, Dec 1, 2020 at 4:14 AM Brendan Jackman <jackmanb@google.com> wrote:
> > >
> > > On Sat, Nov 28, 2020 at 05:15:52PM -0800, Alexei Starovoitov wrote:
> > > > On Fri, Nov 27, 2020 at 05:57:26PM +0000, Brendan Jackman wrote:
> > > > > +/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
> > > > > +static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> > > >
> > > > same concern as in the another patch. If you could avoid intel's puzzling names
> > > > like above it will make reviewing the patch easier.
> > >
> > > In this case there is actually a call like
> > >
> > >   emit_modrm_dstoff(&prog, src_reg, dst_reg)
> >
> > emit_insn_prefix() ?
>
> Ah sorry, I thought you were talking about the _arg_ names.

I meant both. Arg names and helper name. Sorry for the confusion.

Patch
diff mbox series

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 796506dcfc42..94b17bd30e00 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -681,6 +681,27 @@  static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
 	*pprog = prog;
 }
 
+/* Emit the ModR/M byte for addressing *(r1 + off) and r2 */
+static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	if (is_imm8(off)) {
+		/* 1-byte signed displacement.
+		 *
+		 * If off == 0 we could skip this and save one extra byte, but
+		 * special case of x86 R13 which always needs an offset is not
+		 * worth the hassle
+		 */
+		EMIT2(add_2reg(0x40, r1, r2), off);
+	} else {
+		/* 4-byte signed displacement */
+		EMIT1_off32(add_2reg(0x80, r1, r2), off);
+	}
+	*pprog = prog;
+}
+
 /* LDX: dst_reg = *(u8*)(src_reg + off) */
 static void emit_ldx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 {
@@ -708,15 +729,7 @@  static void emit_ldx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 		EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x8B);
 		break;
 	}
-	/*
-	 * If insn->off == 0 we can save one extra byte, but
-	 * special case of x86 R13 which always needs an offset
-	 * is not worth the hassle
-	 */
-	if (is_imm8(off))
-		EMIT2(add_2reg(0x40, src_reg, dst_reg), off);
-	else
-		EMIT1_off32(add_2reg(0x80, src_reg, dst_reg), off);
+	emit_modrm_dstoff(&prog, src_reg, dst_reg, off);
 	*pprog = prog;
 }
 
@@ -751,10 +764,7 @@  static void emit_stx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 		EMIT2(add_2mod(0x48, dst_reg, src_reg), 0x89);
 		break;
 	}
-	if (is_imm8(off))
-		EMIT2(add_2reg(0x40, dst_reg, src_reg), off);
-	else
-		EMIT1_off32(add_2reg(0x80, dst_reg, src_reg), off);
+	emit_modrm_dstoff(&prog, dst_reg, src_reg, off);
 	*pprog = prog;
 }
 
@@ -1240,11 +1250,7 @@  st:			if (is_imm8(insn->off))
 			goto xadd;
 		case BPF_STX | BPF_XADD | BPF_DW:
 			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
-xadd:			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
-			else
-				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
-					    insn->off);
+xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 			break;
 
 			/* call */