[v2,bpf-next,02/13] bpf: x86: Factor out emission of REX byte
diff mbox series

Message ID 20201127175738.1085417-3-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 JIT case for encoding atomic ops is about to get more
complicated. In order to make the review & resulting code easier,
let's factor out some shared helpers.

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

Comments

Alexei Starovoitov Nov. 29, 2020, 1:14 a.m. UTC | #1
On Fri, Nov 27, 2020 at 05:57:27PM +0000, Brendan Jackman wrote:
> The JIT case for encoding atomic ops is about to get more
> complicated. In order to make the review & resulting code easier,
> let's factor out some shared helpers.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 39 ++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 94b17bd30e00..a839c1a54276 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -702,6 +702,21 @@ static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
>  	*pprog = prog;
>  }
>  
> +/*
> + * Emit a REX byte if it will be necessary to address these registers

What is "REX byte" ?
May be rename it to maybe_emit_mod() ?

> + */
> +static void maybe_emit_rex(u8 **pprog, u32 reg_rm, u32 reg_reg, bool wide)

could you please keep original names as dst_reg/src_reg instead of reg_rm/reg_reg ?
reg_reg reads really odd and reg_rm is equally puzzling unless the reader studied
intel's manual. I didn't. All these new abbreviations are challenging for me.
> +{
> +	u8 *prog = *pprog;
> +	int cnt = 0;
> +
> +	if (wide)

what is 'wide' ? Why not to call it 'bool is_alu64' ?

> +		EMIT1(add_2mod(0x48, reg_rm, reg_reg));
> +	else if (is_ereg(reg_rm) || is_ereg(reg_reg))
> +		EMIT1(add_2mod(0x40, reg_rm, reg_reg));
> +	*pprog = prog;
> +}
Brendan Jackman Dec. 1, 2020, 12:12 p.m. UTC | #2
On Sat, Nov 28, 2020 at 05:14:05PM -0800, Alexei Starovoitov wrote:
> On Fri, Nov 27, 2020 at 05:57:27PM +0000, Brendan Jackman wrote:
> > The JIT case for encoding atomic ops is about to get more
> > complicated. In order to make the review & resulting code easier,
> > let's factor out some shared helpers.
> > 
> > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 39 ++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index 94b17bd30e00..a839c1a54276 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -702,6 +702,21 @@ static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> >  	*pprog = prog;
> >  }
> >  
> > +/*
> > + * Emit a REX byte if it will be necessary to address these registers
> 
> What is "REX byte" ?
> May be rename it to maybe_emit_mod() ?

Er, this is the REX prefix as described in
https://wiki.osdev.org/X86-64_Instruction_Encoding#REX_prefix

Would maybe_emit_mod be accurate? In my mind "mod" is a field in the
ModR/M byte which comes _after_ the opcode. Before developing this
patchset I knew almost nothing about x86, so maybe I'm missing something
about the general terminology?

> > + */
> > +static void maybe_emit_rex(u8 **pprog, u32 reg_rm, u32 reg_reg, bool wide)
> 
> could you please keep original names as dst_reg/src_reg instead of reg_rm/reg_reg ?
> reg_reg reads really odd and reg_rm is equally puzzling unless the reader studied
> intel's manual. I didn't. All these new abbreviations are challenging for me.

OK. I originally changed it to use the x86 names because in theory you
could do:

  maybe_emit_rex(&prog, src_reg, dst_reg);

so the names would look backwards when you jump into the function
implementation.

> > +{
> > +	u8 *prog = *pprog;
> > +	int cnt = 0;
> > +
> > +	if (wide)
> 
> what is 'wide' ? Why not to call it 'bool is_alu64' ?

Ack - there's precedent in the file for 'is64' so I'll go with that.
Alexei Starovoitov Dec. 2, 2020, 5:48 a.m. UTC | #3
On Tue, Dec 1, 2020 at 4:12 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Sat, Nov 28, 2020 at 05:14:05PM -0800, Alexei Starovoitov wrote:
> > On Fri, Nov 27, 2020 at 05:57:27PM +0000, Brendan Jackman wrote:
> > > The JIT case for encoding atomic ops is about to get more
> > > complicated. In order to make the review & resulting code easier,
> > > let's factor out some shared helpers.
> > >
> > > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 39 ++++++++++++++++++++++---------------
> > >  1 file changed, 23 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 94b17bd30e00..a839c1a54276 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -702,6 +702,21 @@ static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> > >     *pprog = prog;
> > >  }
> > >
> > > +/*
> > > + * Emit a REX byte if it will be necessary to address these registers
> >
> > What is "REX byte" ?
> > May be rename it to maybe_emit_mod() ?
>
> Er, this is the REX prefix as described in
> https://wiki.osdev.org/X86-64_Instruction_Encoding#REX_prefix
>
> Would maybe_emit_mod be accurate? In my mind "mod" is a field in the
> ModR/M byte which comes _after_ the opcode. Before developing this
> patchset I knew almost nothing about x86, so maybe I'm missing something
> about the general terminology?

I wrote the JIT without looking into the manual and without studying
the terminology.
Why? Because it was not necessary. I still don't see a reason why
that obscure terminology needs to be brought in into the code.
'mod' to me is a 'modifier'. Nothing to do with intel's modrm thing.
Brendan Jackman Dec. 2, 2020, 10:54 a.m. UTC | #4
On Tue, Dec 01, 2020 at 09:48:36PM -0800, Alexei Starovoitov wrote:
> On Tue, Dec 1, 2020 at 4:12 AM Brendan Jackman <jackmanb@google.com> wrote:
> >
> > On Sat, Nov 28, 2020 at 05:14:05PM -0800, Alexei Starovoitov wrote:
> > > On Fri, Nov 27, 2020 at 05:57:27PM +0000, Brendan Jackman wrote:
> > > > The JIT case for encoding atomic ops is about to get more
> > > > complicated. In order to make the review & resulting code easier,
> > > > let's factor out some shared helpers.
> > > >
> > > > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c | 39 ++++++++++++++++++++++---------------
> > > >  1 file changed, 23 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > > index 94b17bd30e00..a839c1a54276 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -702,6 +702,21 @@ static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
> > > >     *pprog = prog;
> > > >  }
> > > >
> > > > +/*
> > > > + * Emit a REX byte if it will be necessary to address these registers
> > >
> > > What is "REX byte" ?
> > > May be rename it to maybe_emit_mod() ?
> >
> > Er, this is the REX prefix as described in
> > https://wiki.osdev.org/X86-64_Instruction_Encoding#REX_prefix
> >
> > Would maybe_emit_mod be accurate? In my mind "mod" is a field in the
> > ModR/M byte which comes _after_ the opcode. Before developing this
> > patchset I knew almost nothing about x86, so maybe I'm missing something
> > about the general terminology?
> 
> I wrote the JIT without looking into the manual and without studying
> the terminology.
> Why? Because it was not necessary. I still don't see a reason why
> that obscure terminology needs to be brought in into the code.
> 'mod' to me is a 'modifier'. Nothing to do with intel's modrm thing.

OK, calling it maybe_emit_mod(pprog, dst_reg, src_reg)

Patch
diff mbox series

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 94b17bd30e00..a839c1a54276 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -702,6 +702,21 @@  static void emit_modrm_dstoff(u8 **pprog, u32 r1, u32 r2, int off)
 	*pprog = prog;
 }
 
+/*
+ * Emit a REX byte if it will be necessary to address these registers
+ */
+static void maybe_emit_rex(u8 **pprog, u32 reg_rm, u32 reg_reg, bool wide)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	if (wide)
+		EMIT1(add_2mod(0x48, reg_rm, reg_reg));
+	else if (is_ereg(reg_rm) || is_ereg(reg_reg))
+		EMIT1(add_2mod(0x40, reg_rm, reg_reg));
+	*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)
 {
@@ -854,10 +869,8 @@  static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			case BPF_OR: b2 = 0x09; break;
 			case BPF_XOR: b2 = 0x31; break;
 			}
-			if (BPF_CLASS(insn->code) == BPF_ALU64)
-				EMIT1(add_2mod(0x48, dst_reg, src_reg));
-			else if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT1(add_2mod(0x40, dst_reg, src_reg));
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_CLASS(insn->code) == BPF_ALU64);
 			EMIT2(b2, add_2reg(0xC0, dst_reg, src_reg));
 			break;
 
@@ -1301,20 +1314,16 @@  xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 		case BPF_JMP32 | BPF_JSGE | BPF_X:
 		case BPF_JMP32 | BPF_JSLE | BPF_X:
 			/* cmp dst_reg, src_reg */
-			if (BPF_CLASS(insn->code) == BPF_JMP)
-				EMIT1(add_2mod(0x48, dst_reg, src_reg));
-			else if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT1(add_2mod(0x40, dst_reg, src_reg));
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_CLASS(insn->code) == BPF_JMP);
 			EMIT2(0x39, add_2reg(0xC0, dst_reg, src_reg));
 			goto emit_cond_jmp;
 
 		case BPF_JMP | BPF_JSET | BPF_X:
 		case BPF_JMP32 | BPF_JSET | BPF_X:
 			/* test dst_reg, src_reg */
-			if (BPF_CLASS(insn->code) == BPF_JMP)
-				EMIT1(add_2mod(0x48, dst_reg, src_reg));
-			else if (is_ereg(dst_reg) || is_ereg(src_reg))
-				EMIT1(add_2mod(0x40, dst_reg, src_reg));
+			maybe_emit_rex(&prog, dst_reg, src_reg,
+				       BPF_CLASS(insn->code) == BPF_JMP);
 			EMIT2(0x85, add_2reg(0xC0, dst_reg, src_reg));
 			goto emit_cond_jmp;
 
@@ -1350,10 +1359,8 @@  xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);
 		case BPF_JMP32 | BPF_JSLE | BPF_K:
 			/* test dst_reg, dst_reg to save one extra byte */
 			if (imm32 == 0) {
-				if (BPF_CLASS(insn->code) == BPF_JMP)
-					EMIT1(add_2mod(0x48, dst_reg, dst_reg));
-				else if (is_ereg(dst_reg))
-					EMIT1(add_2mod(0x40, dst_reg, dst_reg));
+				maybe_emit_rex(&prog, dst_reg, dst_reg,
+					       BPF_CLASS(insn->code) == BPF_JMP);
 				EMIT2(0x85, add_2reg(0xC0, dst_reg, dst_reg));
 				goto emit_cond_jmp;
 			}