| Message ID | 20201127175738.1085417-2-jackmanb@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Atomics for eBPF | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 66 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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?
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() ?
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.
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.
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 */
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(-)