* [PATCH net-next 04/13] nfp: bpf: flag jump destination to guide insn combine optimizations
From: Jakub Kicinski @ 2017-12-01 5:32 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Jiong Wang
In-Reply-To: <20171201053300.17503-1-jakub.kicinski@netronome.com>
From: Jiong Wang <jiong.wang@netronome.com>
NFP eBPF offload JIT engine is doing some instruction combine based
optimizations which however must not be safe if the combined sequences
are across basic block boarders.
Currently, there are post checks during fixing jump destinations. If the
jump destination is found to be eBPF insn that has been combined into
another one, then JIT engine will raise error and abort.
This is not optimal. The JIT engine ought to disable the optimization on
such cross-bb-border sequences instead of abort.
As there is no control flow information in eBPF infrastructure that we
can't do basic block based optimizations, this patch extends the existing
jump destination record pass to also flag the jump destination, then in
instruction combine passes we could skip the optimizations if insns in the
sequence are jump targets.
Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/main.h | 4 ++++
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index e488656f406c..99da1d34dd0e 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -89,6 +89,8 @@ typedef int (*instr_cb_t)(struct nfp_prog *, struct nfp_insn_meta *);
#define nfp_meta_next(meta) list_next_entry(meta, l)
#define nfp_meta_prev(meta) list_prev_entry(meta, l)
+#define FLAG_INSN_IS_JUMP_DST BIT(0)
+
/**
* struct nfp_insn_meta - BPF instruction wrapper
* @insn: BPF instruction
@@ -97,6 +99,7 @@ typedef int (*instr_cb_t)(struct nfp_prog *, struct nfp_insn_meta *);
* @jmp_dst: destination info for jump instructions
* @off: index of first generated machine instruction (in nfp_prog.prog)
* @n: eBPF instruction number
+ * @flags: eBPF instruction extra optimization flags
* @skip: skip this instruction (optimized out)
* @double_cb: callback for second part of the instruction
* @l: link on nfp_prog->insns list
@@ -112,6 +115,7 @@ struct nfp_insn_meta {
};
unsigned int off;
unsigned short n;
+ unsigned short flags;
bool skip;
instr_cb_t double_cb;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index 240db663d83f..377976ce92dd 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -83,6 +83,7 @@ nfp_prog_prepare(struct nfp_prog *nfp_prog, const struct bpf_insn *prog,
cnt);
meta->jmp_dst = dst_meta;
+ dst_meta->flags |= FLAG_INSN_IS_JUMP_DST;
}
}
--
2.15.0
^ permalink raw reply related
* [PATCH net-next 03/13] nfp: bpf: record jump destination to simplify jump fixup
From: Jakub Kicinski @ 2017-12-01 5:32 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Jiong Wang
In-Reply-To: <20171201053300.17503-1-jakub.kicinski@netronome.com>
From: Jiong Wang <jiong.wang@netronome.com>
eBPF insns are internally organized as dual-list inside NFP offload JIT.
Random access to an insn needs to be done by either forward or backward
traversal along the list.
One place we need to do such traversal is at nfp_fixup_branches where one
traversal is needed for each jump insn to find the destination. Such
traversals could be avoided if jump destinations are collected through a
single travesal in a pre-scan pass, and such information could also be
useful in other places where jump destination info are needed.
This patch adds such jump destination collection in nfp_prog_prepare.
Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/jit.c | 57 ++++-------------------
drivers/net/ethernet/netronome/nfp/bpf/main.h | 13 +++++-
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 22 +++++++--
drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 4 +-
4 files changed, 41 insertions(+), 55 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index 20daf6b95601..f76659ecb654 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -65,12 +65,6 @@
next = nfp_meta_next(pos), \
next2 = nfp_meta_next(next))
-static bool
-nfp_meta_has_next(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
-{
- return meta->l.next != &nfp_prog->insns;
-}
-
static bool
nfp_meta_has_prev(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
{
@@ -1864,9 +1858,8 @@ static void br_set_offset(u64 *instr, u16 offset)
/* --- Assembler logic --- */
static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
{
- struct nfp_insn_meta *meta, *next;
+ struct nfp_insn_meta *meta, *jmp_dst;
u32 idx, br_idx;
- int off;
list_for_each_entry(meta, &nfp_prog->insns, l) {
if (meta->skip)
@@ -1874,13 +1867,10 @@ static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
if (BPF_CLASS(meta->insn.code) != BPF_JMP)
continue;
- if (list_is_last(&meta->l, &nfp_prog->insns)) {
- next = NULL;
+ if (list_is_last(&meta->l, &nfp_prog->insns))
idx = nfp_prog->last_bpf_off;
- } else {
- next = list_next_entry(meta, l);
- idx = next->off - 1;
- }
+ else
+ idx = list_next_entry(meta, l)->off - 1;
br_idx = nfp_prog_offset_to_index(nfp_prog, idx);
@@ -1893,43 +1883,14 @@ static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
if (FIELD_GET(OP_BR_SPECIAL, nfp_prog->prog[br_idx]))
continue;
- /* Find the target offset in assembler realm */
- off = meta->insn.off;
- if (!off) {
- pr_err("Fixup found zero offset!!\n");
+ if (!meta->jmp_dst) {
+ pr_err("Non-exit jump doesn't have destination info recorded!!\n");
return -ELOOP;
}
- if (!next) {
- /* When "next" is NULL, "meta" is the last node in the
- * list. Given it is an JMP, it then must be a backward
- * jump.
- *
- * For eBPF, the jump offset is against pc + 1, so we
- * need to compensate the offset by 1 as we are pointing
- * "next" to the current node "meta".
- */
- if (WARN_ON_ONCE(off > -2))
- return -ELOOP;
-
- next = meta;
- off += 1;
- }
-
- while (off > 0 && nfp_meta_has_next(nfp_prog, next)) {
- next = nfp_meta_next(next);
- off--;
- }
- while (off < 0 && nfp_meta_has_prev(nfp_prog, next)) {
- next = nfp_meta_prev(next);
- off++;
- }
- if (off) {
- pr_err("Fixup found too large jump!! %d\n", off);
- return -ELOOP;
- }
+ jmp_dst = meta->jmp_dst;
- if (next->skip) {
+ if (jmp_dst->skip) {
pr_err("Branch landing on removed instruction!!\n");
return -ELOOP;
}
@@ -1938,7 +1899,7 @@ static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
idx <= br_idx; idx++) {
if (!nfp_is_br(nfp_prog->prog[idx]))
continue;
- br_set_offset(&nfp_prog->prog[idx], next->off);
+ br_set_offset(&nfp_prog->prog[idx], jmp_dst->off);
}
}
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 0f4d218fc77a..e488656f406c 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -94,6 +94,7 @@ typedef int (*instr_cb_t)(struct nfp_prog *, struct nfp_insn_meta *);
* @insn: BPF instruction
* @ptr: pointer type for memory operations
* @ptr_not_const: pointer is not always constant
+ * @jmp_dst: destination info for jump instructions
* @off: index of first generated machine instruction (in nfp_prog.prog)
* @n: eBPF instruction number
* @skip: skip this instruction (optimized out)
@@ -102,8 +103,13 @@ typedef int (*instr_cb_t)(struct nfp_prog *, struct nfp_insn_meta *);
*/
struct nfp_insn_meta {
struct bpf_insn insn;
- struct bpf_reg_state ptr;
- bool ptr_not_const;
+ union {
+ struct {
+ struct bpf_reg_state ptr;
+ bool ptr_not_const;
+ };
+ struct nfp_insn_meta *jmp_dst;
+ };
unsigned int off;
unsigned short n;
bool skip;
@@ -191,4 +197,7 @@ int nfp_bpf_translate(struct nfp_app *app, struct nfp_net *nn,
struct bpf_prog *prog);
int nfp_bpf_destroy(struct nfp_app *app, struct nfp_net *nn,
struct bpf_prog *prog);
+struct nfp_insn_meta *
+nfp_bpf_goto_meta(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
+ unsigned int insn_idx, unsigned int n_insns);
#endif
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index bc879aeb62d4..240db663d83f 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Netronome Systems, Inc.
+ * Copyright (C) 2016-2017 Netronome Systems, Inc.
*
* This software is dual licensed under the GNU General License Version 2,
* June 1991 as shown in the file COPYING in the top-level directory of this
@@ -55,11 +55,10 @@ static int
nfp_prog_prepare(struct nfp_prog *nfp_prog, const struct bpf_insn *prog,
unsigned int cnt)
{
+ struct nfp_insn_meta *meta;
unsigned int i;
for (i = 0; i < cnt; i++) {
- struct nfp_insn_meta *meta;
-
meta = kzalloc(sizeof(*meta), GFP_KERNEL);
if (!meta)
return -ENOMEM;
@@ -70,6 +69,23 @@ nfp_prog_prepare(struct nfp_prog *nfp_prog, const struct bpf_insn *prog,
list_add_tail(&meta->l, &nfp_prog->insns);
}
+ /* Another pass to record jump information. */
+ list_for_each_entry(meta, &nfp_prog->insns, l) {
+ u64 code = meta->insn.code;
+
+ if (BPF_CLASS(code) == BPF_JMP && BPF_OP(code) != BPF_EXIT &&
+ BPF_OP(code) != BPF_CALL) {
+ struct nfp_insn_meta *dst_meta;
+ unsigned short dst_indx;
+
+ dst_indx = meta->n + 1 + meta->insn.off;
+ dst_meta = nfp_bpf_goto_meta(nfp_prog, meta, dst_indx,
+ cnt);
+
+ meta->jmp_dst = dst_meta;
+ }
+ }
+
return 0;
}
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index 8d43491ddd6b..cca67730b91f 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Netronome Systems, Inc.
+ * Copyright (C) 2016-2017 Netronome Systems, Inc.
*
* This software is dual licensed under the GNU General License Version 2,
* June 1991 as shown in the file COPYING in the top-level directory of this
@@ -40,7 +40,7 @@
#include "main.h"
-static struct nfp_insn_meta *
+struct nfp_insn_meta *
nfp_bpf_goto_meta(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
unsigned int insn_idx, unsigned int n_insns)
{
--
2.15.0
^ permalink raw reply related
* [PATCH net-next 02/13] nfp: bpf: support backward jump
From: Jakub Kicinski @ 2017-12-01 5:32 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Jiong Wang, Jakub Kicinski
In-Reply-To: <20171201053300.17503-1-jakub.kicinski@netronome.com>
From: Jiong Wang <jiong.wang@netronome.com>
This patch adds support for backward jump on NFP.
- restrictions on backward jump in various functions have been removed.
- nfp_fixup_branches now supports backward jump.
There is one thing to note, currently an input eBPF JMP insn may generate
several NFP insns, for example,
NFP imm move insn A \
NFP compare insn B --> 3 NFP insn jited from eBPF JMP insn M
NFP branch insn C /
---
NFP insn X --> 1 NFP insn jited from eBPF insn N
---
...
therefore, we are doing sanity check to make sure the last jited insn from
an eBPF JMP is a NFP branch instruction.
Once backward jump is allowed, it is possible an eBPF JMP insn is at the
end of the program. This is however causing trouble for the sanity check.
Because the sanity check requires the end index of the NFP insns jited from
one eBPF insn while only the start index is recorded before this patch that
we can only get the end index by:
start_index_of_the_next_eBPF_insn - 1
or for the above example:
start_index_of_eBPF_insn_N (which is the index of NFP insn X) - 1
nfp_fixup_branches was using nfp_for_each_insn_walk2 to expose *next* insn
to each iteration during the traversal so the last index could be
calculated from which. Now, it needs some extra code to handle the last
insn. Meanwhile, the use of walk2 is actually unnecessary, we could simply
use generic single instruction walk to do this, the next insn could be
easily calculated using list_next_entry.
So, this patch migrates the jump fixup traversal method to
*list_for_each_entry*, this simplifies the code logic a little bit.
The other thing to note is a new state variable "last_bpf_off" is
introduced to track the index of the last jited NFP insn. This is necessary
because NFP is generating special purposes epilogue sequences, so the index
of the last jited NFP insn is *not* always nfp_prog->prog_len - 1.
Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/jit.c | 66 +++++++++++++++------------
drivers/net/ethernet/netronome/nfp/bpf/main.h | 4 +-
2 files changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index 995e95410b11..20daf6b95601 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Netronome Systems, Inc.
+ * Copyright (C) 2016-2017 Netronome Systems, Inc.
*
* This software is dual licensed under the GNU General License Version 2,
* June 1991 as shown in the file COPYING in the top-level directory of this
@@ -975,9 +975,6 @@ wrp_test_reg(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
{
const struct bpf_insn *insn = &meta->insn;
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
wrp_test_reg_one(nfp_prog, insn->dst_reg * 2, alu_op,
insn->src_reg * 2, br_mask, insn->off);
wrp_test_reg_one(nfp_prog, insn->dst_reg * 2 + 1, alu_op,
@@ -995,9 +992,6 @@ wrp_cmp_imm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
u8 reg = insn->dst_reg * 2;
swreg tmp_reg;
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
tmp_reg = ur_load_imm_any(nfp_prog, imm & ~0U, imm_b(nfp_prog));
if (!swap)
emit_alu(nfp_prog, reg_none(), reg_a(reg), ALU_OP_SUB, tmp_reg);
@@ -1027,9 +1021,6 @@ wrp_cmp_reg(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
areg = insn->dst_reg * 2;
breg = insn->src_reg * 2;
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
if (swap) {
areg ^= breg;
breg ^= areg;
@@ -1630,8 +1621,6 @@ static int mem_stx8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
static int jump(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
{
- if (meta->insn.off < 0) /* TODO */
- return -EOPNOTSUPP;
emit_br(nfp_prog, BR_UNC, meta->insn.off, 0);
return 0;
@@ -1646,9 +1635,6 @@ static int jeq_imm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
or1 = reg_a(insn->dst_reg * 2);
or2 = reg_b(insn->dst_reg * 2 + 1);
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
if (imm & ~0U) {
tmp_reg = ur_load_imm_any(nfp_prog, imm & ~0U, imm_b(nfp_prog));
emit_alu(nfp_prog, imm_a(nfp_prog),
@@ -1695,9 +1681,6 @@ static int jset_imm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
u64 imm = insn->imm; /* sign extend */
swreg tmp_reg;
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
if (!imm) {
meta->skip = true;
return 0;
@@ -1726,9 +1709,6 @@ static int jne_imm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
u64 imm = insn->imm; /* sign extend */
swreg tmp_reg;
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
if (!imm) {
emit_alu(nfp_prog, reg_none(), reg_a(insn->dst_reg * 2),
ALU_OP_OR, reg_b(insn->dst_reg * 2 + 1));
@@ -1753,9 +1733,6 @@ static int jeq_reg(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
{
const struct bpf_insn *insn = &meta->insn;
- if (insn->off < 0) /* TODO */
- return -EOPNOTSUPP;
-
emit_alu(nfp_prog, imm_a(nfp_prog), reg_a(insn->dst_reg * 2),
ALU_OP_XOR, reg_b(insn->src_reg * 2));
emit_alu(nfp_prog, imm_b(nfp_prog), reg_a(insn->dst_reg * 2 + 1),
@@ -1888,16 +1865,25 @@ static void br_set_offset(u64 *instr, u16 offset)
static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
{
struct nfp_insn_meta *meta, *next;
- u32 off, br_idx;
- u32 idx;
+ u32 idx, br_idx;
+ int off;
- nfp_for_each_insn_walk2(nfp_prog, meta, next) {
+ list_for_each_entry(meta, &nfp_prog->insns, l) {
if (meta->skip)
continue;
if (BPF_CLASS(meta->insn.code) != BPF_JMP)
continue;
- br_idx = nfp_prog_offset_to_index(nfp_prog, next->off) - 1;
+ if (list_is_last(&meta->l, &nfp_prog->insns)) {
+ next = NULL;
+ idx = nfp_prog->last_bpf_off;
+ } else {
+ next = list_next_entry(meta, l);
+ idx = next->off - 1;
+ }
+
+ br_idx = nfp_prog_offset_to_index(nfp_prog, idx);
+
if (!nfp_is_br(nfp_prog->prog[br_idx])) {
pr_err("Fixup found block not ending in branch %d %02x %016llx!!\n",
br_idx, meta->insn.code, nfp_prog->prog[br_idx]);
@@ -1914,10 +1900,30 @@ static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
return -ELOOP;
}
- while (off && nfp_meta_has_next(nfp_prog, next)) {
+ if (!next) {
+ /* When "next" is NULL, "meta" is the last node in the
+ * list. Given it is an JMP, it then must be a backward
+ * jump.
+ *
+ * For eBPF, the jump offset is against pc + 1, so we
+ * need to compensate the offset by 1 as we are pointing
+ * "next" to the current node "meta".
+ */
+ if (WARN_ON_ONCE(off > -2))
+ return -ELOOP;
+
+ next = meta;
+ off += 1;
+ }
+
+ while (off > 0 && nfp_meta_has_next(nfp_prog, next)) {
next = nfp_meta_next(next);
off--;
}
+ while (off < 0 && nfp_meta_has_prev(nfp_prog, next)) {
+ next = nfp_meta_prev(next);
+ off++;
+ }
if (off) {
pr_err("Fixup found too large jump!! %d\n", off);
return -ELOOP;
@@ -2105,6 +2111,8 @@ static int nfp_translate(struct nfp_prog *nfp_prog)
nfp_prog->n_translated++;
}
+ nfp_prog->last_bpf_off = nfp_prog_current_offset(nfp_prog) - 1;
+
nfp_outro(nfp_prog);
if (nfp_prog->error)
return nfp_prog->error;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 082a15f6dfb5..0f4d218fc77a 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Netronome Systems, Inc.
+ * Copyright (C) 2016-2017 Netronome Systems, Inc.
*
* This software is dual licensed under the GNU General License Version 2,
* June 1991 as shown in the file COPYING in the top-level directory of this
@@ -142,6 +142,7 @@ static inline u8 mbpf_mode(const struct nfp_insn_meta *meta)
* @verifier_meta: temporary storage for verifier's insn meta
* @type: BPF program type
* @start_off: address of the first instruction in the memory
+ * @last_bpf_off: address of the last instruction translated from BPF
* @tgt_out: jump target for normal exit
* @tgt_abort: jump target for abort (e.g. access outside of packet buffer)
* @tgt_done: jump target to get the next packet
@@ -160,6 +161,7 @@ struct nfp_prog {
enum bpf_prog_type type;
unsigned int start_off;
+ unsigned int last_bpf_off;
unsigned int tgt_out;
unsigned int tgt_abort;
unsigned int tgt_done;
--
2.15.0
^ permalink raw reply related
* [PATCH net-next 01/13] nfp: fix old kdoc issues
From: Jakub Kicinski @ 2017-12-01 5:32 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Jakub Kicinski
In-Reply-To: <20171201053300.17503-1-jakub.kicinski@netronome.com>
Since commit 3a025e1d1c2e ("Add optional check for bad kernel-doc
comments") when built with W=1 build will complain about kdoc errors.
Fix the kdoc issues we have. kdoc is still confused by defines in
nfp_net_ctrl.h but those are not really errors.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/nfp_net.h | 2 ++
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c | 9 +++------
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h
index 7f9857c276b1..3801c52098d5 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
@@ -548,6 +548,8 @@ struct nfp_net_dp {
* @max_r_vecs: Number of allocated interrupt vectors for RX/TX
* @max_tx_rings: Maximum number of TX rings supported by the Firmware
* @max_rx_rings: Maximum number of RX rings supported by the Firmware
+ * @stride_rx: Queue controller RX queue spacing
+ * @stride_tx: Queue controller TX queue spacing
* @r_vecs: Pre-allocated array of ring vectors
* @irq_entries: Pre-allocated array of MSI-X entries
* @lsc_handler: Handler for Link State Change interrupt
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c
index 04dd5758ecf5..3fcb522d2e85 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c
@@ -372,8 +372,7 @@ nfp_cpp_area_alloc(struct nfp_cpp *cpp, u32 dest,
* that it can be accessed directly.
*
* NOTE: @address and @size must be 32-bit aligned values.
- *
- * NOTE: The area must also be 'released' when the structure is freed.
+ * The area must also be 'released' when the structure is freed.
*
* Return: NFP CPP Area handle, or NULL
*/
@@ -536,8 +535,7 @@ void nfp_cpp_area_release_free(struct nfp_cpp_area *area)
* Read data from indicated CPP region.
*
* NOTE: @offset and @length must be 32-bit aligned values.
- *
- * NOTE: Area must have been locked down with an 'acquire'.
+ * Area must have been locked down with an 'acquire'.
*
* Return: length of io, or -ERRNO
*/
@@ -558,8 +556,7 @@ int nfp_cpp_area_read(struct nfp_cpp_area *area,
* Write data to indicated CPP region.
*
* NOTE: @offset and @length must be 32-bit aligned values.
- *
- * NOTE: Area must have been locked down with an 'acquire'.
+ * Area must have been locked down with an 'acquire'.
*
* Return: length of io, or -ERRNO
*/
--
2.15.0
^ permalink raw reply related
* [PATCH net-next 00/13] nfp: bpf: jump resolution and memcpy update
From: Jakub Kicinski @ 2017-12-01 5:32 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Jakub Kicinski
Hi!
Jiong says:
Currently, compiler will lower memcpy function call in XDP/eBPF C program
into a sequence of eBPF load/store pairs for some scenarios.
Compiler is thinking this "inline" optimiation is beneficial as it could
avoid function call and also increase code locality.
However, Netronome NPU is not an tranditional load/store architecture that
doing a sequence of individual load/store actions are not efficient.
This patch set tries to identify the load/store sequences composed of
load/store pairs that comes from memcpy lowering, then accelerates them
through NPU's Command Push Pull (CPP) instruction.
This patch set registered an new optimization pass before doing the actual
JIT work, it traverse through eBPF IR, once found candidate sequence then
record the memory copy source, destination and length information in the
first load instruction starting the sequence and marks all remaining
instructions in the sequence into skipable status. Later, when JITing the
first load instructoin, optimal instructions will be generated using those
record information.
For this safety of this transformation:
- jump into the middle of the sequence will cancel the optimization.
- overlapped memory access will cancel the optimization.
- the load destination register still contains the same value as before
the transformation.
Jakub Kicinski (2):
nfp: fix old kdoc issues
nfp: bpf: encode indirect commands
Jiong Wang (11):
nfp: bpf: support backward jump
nfp: bpf: record jump destination to simplify jump fixup
nfp: bpf: flag jump destination to guide insn combine optimizations
nfp: bpf: don't do ld/mask combination if mask is jump destination
nfp: bpf: don't do ld/shifts combination if shifts are jump
destination
nfp: bpf: relax source operands check
nfp: bpf: correct the encoding for No-Dest immed
nfp: bpf: factor out is_mbpf_load & is_mbpf_store
nfp: bpf: implement memory bulk copy for length within 32-bytes
nfp: bpf: implement memory bulk copy for length bigger than 32-bytes
nfp: bpf: detect load/store sequences lowered from memory copy
drivers/net/ethernet/netronome/nfp/bpf/jit.c | 489 ++++++++++++++++++---
drivers/net/ethernet/netronome/nfp/bpf/main.h | 35 +-
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 23 +-
drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 8 +-
drivers/net/ethernet/netronome/nfp/nfp_asm.c | 7 +-
drivers/net/ethernet/netronome/nfp/nfp_asm.h | 7 +-
drivers/net/ethernet/netronome/nfp/nfp_net.h | 2 +
.../ethernet/netronome/nfp/nfpcore/nfp_cppcore.c | 9 +-
8 files changed, 505 insertions(+), 75 deletions(-)
--
2.15.0
^ permalink raw reply
* [PATCH net-next 4/7] bpf: improve verifier liveness marks
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
registers with pointers filled from stack were missing live_written marks
which caused liveness propagation to unnecessary mark more registers as
live_read and miss state pruning opportunities later on.
before after
bpf_lb-DLB_L3.o 2285 2270
bpf_lb-DLB_L4.o 3723 3682
bpf_lb-DUNKNOWN.o 1110 1110
bpf_lxc-DDROP_ALL.o 27954 27876
bpf_lxc-DUNKNOWN.o 38954 38780
bpf_netdev.o 16943 16937
bpf_overlay.o 7929 7929
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 14ad7c6e806a..46ff4e5b3fb7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -795,6 +795,11 @@ static int check_stack_read(struct bpf_verifier_env *env,
if (value_regno >= 0) {
/* restore register state from stack */
state->regs[value_regno] = state->stack[spi].spilled_ptr;
+ /* mark reg as written since spilled pointer state likely
+ * has its liveness marks cleared by is_state_visited()
+ * which resets stack/reg liveness for state transitions
+ */
+ state->regs[value_regno].live |= REG_LIVE_WRITTEN;
mark_stack_slot_read(state, spi);
}
return 0;
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 7/7] selftests/bpf: adjust test_align expected output
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
since verifier started to print liveness state of the registers
adjust expected output of test_align.
Now this test checks for both proper alignment handling by verifier
and correctness of liveness marks.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
tools/testing/selftests/bpf/test_align.c | 156 +++++++++++++++----------------
1 file changed, 78 insertions(+), 78 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_align.c b/tools/testing/selftests/bpf/test_align.c
index 8591c89c0828..fe916d29e166 100644
--- a/tools/testing/selftests/bpf/test_align.c
+++ b/tools/testing/selftests/bpf/test_align.c
@@ -64,11 +64,11 @@ static struct bpf_align_test tests[] = {
.matches = {
{1, "R1=ctx(id=0,off=0,imm=0)"},
{1, "R10=fp0"},
- {1, "R3=inv2"},
- {2, "R3=inv4"},
- {3, "R3=inv8"},
- {4, "R3=inv16"},
- {5, "R3=inv32"},
+ {1, "R3_w=inv2"},
+ {2, "R3_w=inv4"},
+ {3, "R3_w=inv8"},
+ {4, "R3_w=inv16"},
+ {5, "R3_w=inv32"},
},
},
{
@@ -92,17 +92,17 @@ static struct bpf_align_test tests[] = {
.matches = {
{1, "R1=ctx(id=0,off=0,imm=0)"},
{1, "R10=fp0"},
- {1, "R3=inv1"},
- {2, "R3=inv2"},
- {3, "R3=inv4"},
- {4, "R3=inv8"},
- {5, "R3=inv16"},
- {6, "R3=inv1"},
- {7, "R4=inv32"},
- {8, "R4=inv16"},
- {9, "R4=inv8"},
- {10, "R4=inv4"},
- {11, "R4=inv2"},
+ {1, "R3_w=inv1"},
+ {2, "R3_w=inv2"},
+ {3, "R3_w=inv4"},
+ {4, "R3_w=inv8"},
+ {5, "R3_w=inv16"},
+ {6, "R3_w=inv1"},
+ {7, "R4_w=inv32"},
+ {8, "R4_w=inv16"},
+ {9, "R4_w=inv8"},
+ {10, "R4_w=inv4"},
+ {11, "R4_w=inv2"},
},
},
{
@@ -121,12 +121,12 @@ static struct bpf_align_test tests[] = {
.matches = {
{1, "R1=ctx(id=0,off=0,imm=0)"},
{1, "R10=fp0"},
- {1, "R3=inv4"},
- {2, "R3=inv8"},
- {3, "R3=inv10"},
- {4, "R4=inv8"},
- {5, "R4=inv12"},
- {6, "R4=inv14"},
+ {1, "R3_w=inv4"},
+ {2, "R3_w=inv8"},
+ {3, "R3_w=inv10"},
+ {4, "R4_w=inv8"},
+ {5, "R4_w=inv12"},
+ {6, "R4_w=inv14"},
},
},
{
@@ -143,10 +143,10 @@ static struct bpf_align_test tests[] = {
.matches = {
{1, "R1=ctx(id=0,off=0,imm=0)"},
{1, "R10=fp0"},
- {1, "R3=inv7"},
- {2, "R3=inv7"},
- {3, "R3=inv14"},
- {4, "R3=inv56"},
+ {1, "R3_w=inv7"},
+ {2, "R3_w=inv7"},
+ {3, "R3_w=inv14"},
+ {4, "R3_w=inv56"},
},
},
@@ -185,18 +185,18 @@ static struct bpf_align_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.matches = {
{7, "R0=pkt(id=0,off=8,r=8,imm=0)"},
- {7, "R3=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {8, "R3=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
- {9, "R3=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
- {10, "R3=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
- {11, "R3=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
+ {7, "R3_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {8, "R3_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
+ {9, "R3_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {10, "R3_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
+ {11, "R3_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
{18, "R3=pkt_end(id=0,off=0,imm=0)"},
- {18, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {19, "R4=inv(id=0,umax_value=8160,var_off=(0x0; 0x1fe0))"},
- {20, "R4=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
- {21, "R4=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
- {22, "R4=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
- {23, "R4=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
+ {18, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {19, "R4_w=inv(id=0,umax_value=8160,var_off=(0x0; 0x1fe0))"},
+ {20, "R4_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
+ {21, "R4_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
+ {22, "R4_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {23, "R4_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
},
},
{
@@ -217,16 +217,16 @@ static struct bpf_align_test tests[] = {
},
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.matches = {
- {7, "R3=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {8, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {9, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {10, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {11, "R4=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
- {12, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {13, "R4=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
- {14, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {15, "R4=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
- {16, "R4=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
+ {7, "R3_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {8, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {9, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {10, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {11, "R4_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
+ {12, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {13, "R4_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {14, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {15, "R4_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
+ {16, "R4_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
},
},
{
@@ -257,14 +257,14 @@ static struct bpf_align_test tests[] = {
},
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.matches = {
- {4, "R5=pkt(id=0,off=0,r=0,imm=0)"},
- {5, "R5=pkt(id=0,off=14,r=0,imm=0)"},
- {6, "R4=pkt(id=0,off=14,r=0,imm=0)"},
+ {4, "R5_w=pkt(id=0,off=0,r=0,imm=0)"},
+ {5, "R5_w=pkt(id=0,off=14,r=0,imm=0)"},
+ {6, "R4_w=pkt(id=0,off=14,r=0,imm=0)"},
{10, "R2=pkt(id=0,off=0,r=18,imm=0)"},
{10, "R5=pkt(id=0,off=14,r=18,imm=0)"},
- {10, "R4=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
- {14, "R4=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff))"},
- {15, "R4=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff))"},
+ {10, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
+ {14, "R4_w=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff))"},
+ {15, "R4_w=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff))"},
},
},
{
@@ -320,11 +320,11 @@ static struct bpf_align_test tests[] = {
* alignment of 4.
*/
{8, "R2=pkt(id=0,off=0,r=8,imm=0)"},
- {8, "R6=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {8, "R6_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Offset is added to packet pointer R5, resulting in
* known fixed offset, and variable offset from R6.
*/
- {11, "R5=pkt(id=1,off=14,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {11, "R5_w=pkt(id=1,off=14,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* At the time the word size load is performed from R5,
* it's total offset is NET_IP_ALIGN + reg->off (0) +
* reg->aux_off (14) which is 16. Then the variable
@@ -336,11 +336,11 @@ static struct bpf_align_test tests[] = {
/* Variable offset is added to R5 packet pointer,
* resulting in auxiliary alignment of 4.
*/
- {18, "R5=pkt(id=2,off=0,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {18, "R5_w=pkt(id=2,off=0,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Constant offset is added to R5, resulting in
* reg->off of 14.
*/
- {19, "R5=pkt(id=2,off=14,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {19, "R5_w=pkt(id=2,off=14,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* At the time the word size load is performed from R5,
* its total fixed offset is NET_IP_ALIGN + reg->off
* (14) which is 16. Then the variable offset is 4-byte
@@ -352,18 +352,18 @@ static struct bpf_align_test tests[] = {
/* Constant offset is added to R5 packet pointer,
* resulting in reg->off value of 14.
*/
- {26, "R5=pkt(id=0,off=14,r=8"},
+ {26, "R5_w=pkt(id=0,off=14,r=8"},
/* Variable offset is added to R5, resulting in a
* variable offset of (4n).
*/
- {27, "R5=pkt(id=3,off=14,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {27, "R5_w=pkt(id=3,off=14,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Constant is added to R5 again, setting reg->off to 18. */
- {28, "R5=pkt(id=3,off=18,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {28, "R5_w=pkt(id=3,off=18,r=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* And once more we add a variable; resulting var_off
* is still (4n), fixed offset is not changed.
* Also, we create a new reg->id.
*/
- {29, "R5=pkt(id=4,off=18,r=0,umax_value=2040,var_off=(0x0; 0x7fc))"},
+ {29, "R5_w=pkt(id=4,off=18,r=0,umax_value=2040,var_off=(0x0; 0x7fc))"},
/* At the time the word size load is performed from R5,
* its total fixed offset is NET_IP_ALIGN + reg->off (18)
* which is 20. Then the variable offset is (4n), so
@@ -410,11 +410,11 @@ static struct bpf_align_test tests[] = {
* alignment of 4.
*/
{8, "R2=pkt(id=0,off=0,r=8,imm=0)"},
- {8, "R6=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {8, "R6_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Adding 14 makes R6 be (4n+2) */
- {9, "R6=inv(id=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
+ {9, "R6_w=inv(id=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
/* Packet pointer has (4n+2) offset */
- {11, "R5=pkt(id=1,off=0,r=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
+ {11, "R5_w=pkt(id=1,off=0,r=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
{13, "R4=pkt(id=1,off=4,r=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
/* At the time the word size load is performed from R5,
* its total fixed offset is NET_IP_ALIGN + reg->off (0)
@@ -426,11 +426,11 @@ static struct bpf_align_test tests[] = {
/* Newly read value in R6 was shifted left by 2, so has
* known alignment of 4.
*/
- {18, "R6=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {18, "R6_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Added (4n) to packet pointer's (4n+2) var_off, giving
* another (4n+2).
*/
- {19, "R5=pkt(id=2,off=0,r=0,umin_value=14,umax_value=2054,var_off=(0x2; 0xffc))"},
+ {19, "R5_w=pkt(id=2,off=0,r=0,umin_value=14,umax_value=2054,var_off=(0x2; 0xffc))"},
{21, "R4=pkt(id=2,off=4,r=0,umin_value=14,umax_value=2054,var_off=(0x2; 0xffc))"},
/* At the time the word size load is performed from R5,
* its total fixed offset is NET_IP_ALIGN + reg->off (0)
@@ -473,11 +473,11 @@ static struct bpf_align_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.result = REJECT,
.matches = {
- {4, "R5=pkt(id=0,off=0,r=0,imm=0)"},
+ {4, "R5_w=pkt(id=0,off=0,r=0,imm=0)"},
/* ptr & 0x40 == either 0 or 0x40 */
- {5, "R5=inv(id=0,umax_value=64,var_off=(0x0; 0x40))"},
+ {5, "R5_w=inv(id=0,umax_value=64,var_off=(0x0; 0x40))"},
/* ptr << 2 == unknown, (4n) */
- {7, "R5=inv(id=0,smax_value=9223372036854775804,umax_value=18446744073709551612,var_off=(0x0; 0xfffffffffffffffc))"},
+ {7, "R5_w=inv(id=0,smax_value=9223372036854775804,umax_value=18446744073709551612,var_off=(0x0; 0xfffffffffffffffc))"},
/* (4n) + 14 == (4n+2). We blow our bounds, because
* the add could overflow.
*/
@@ -485,7 +485,7 @@ static struct bpf_align_test tests[] = {
/* Checked s>=0 */
{10, "R5=inv(id=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"},
/* packet pointer + nonnegative (4n+2) */
- {12, "R6=pkt(id=1,off=0,r=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"},
+ {12, "R6_w=pkt(id=1,off=0,r=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"},
{14, "R4=pkt(id=1,off=4,r=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"},
/* NET_IP_ALIGN + (4n+2) == (4n), alignment is fine.
* We checked the bounds, but it might have been able
@@ -530,11 +530,11 @@ static struct bpf_align_test tests[] = {
* alignment of 4.
*/
{7, "R2=pkt(id=0,off=0,r=8,imm=0)"},
- {9, "R6=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {9, "R6_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Adding 14 makes R6 be (4n+2) */
- {10, "R6=inv(id=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
+ {10, "R6_w=inv(id=0,umin_value=14,umax_value=1034,var_off=(0x2; 0x7fc))"},
/* New unknown value in R7 is (4n) */
- {11, "R7=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
+ {11, "R7_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
/* Subtracting it from R6 blows our unsigned bounds */
{12, "R6=inv(id=0,smin_value=-1006,smax_value=1034,var_off=(0x2; 0xfffffffffffffffc))"},
/* Checked s>= 0 */
@@ -583,15 +583,15 @@ static struct bpf_align_test tests[] = {
* alignment of 4.
*/
{7, "R2=pkt(id=0,off=0,r=8,imm=0)"},
- {10, "R6=inv(id=0,umax_value=60,var_off=(0x0; 0x3c))"},
+ {10, "R6_w=inv(id=0,umax_value=60,var_off=(0x0; 0x3c))"},
/* Adding 14 makes R6 be (4n+2) */
- {11, "R6=inv(id=0,umin_value=14,umax_value=74,var_off=(0x2; 0x7c))"},
+ {11, "R6_w=inv(id=0,umin_value=14,umax_value=74,var_off=(0x2; 0x7c))"},
/* Subtracting from packet pointer overflows ubounds */
- {13, "R5=pkt(id=1,off=0,r=8,umin_value=18446744073709551542,umax_value=18446744073709551602,var_off=(0xffffffffffffff82; 0x7c))"},
+ {13, "R5_w=pkt(id=1,off=0,r=8,umin_value=18446744073709551542,umax_value=18446744073709551602,var_off=(0xffffffffffffff82; 0x7c))"},
/* New unknown value in R7 is (4n), >= 76 */
- {15, "R7=inv(id=0,umin_value=76,umax_value=1096,var_off=(0x0; 0x7fc))"},
+ {15, "R7_w=inv(id=0,umin_value=76,umax_value=1096,var_off=(0x0; 0x7fc))"},
/* Adding it to packet pointer gives nice bounds again */
- {16, "R5=pkt(id=2,off=0,r=0,umin_value=2,umax_value=1082,var_off=(0x2; 0x7fc))"},
+ {16, "R5_w=pkt(id=2,off=0,r=0,umin_value=2,umax_value=1082,var_off=(0x2; 0x7fc))"},
/* At the time the word size load is performed from R5,
* its total fixed offset is NET_IP_ALIGN + reg->off (0)
* which is 2. Then the variable offset is (4n+2), so
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 1/7] bpf: fix stack state printing in verifier log
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
fix incorrect stack state prints in print_verifier_state()
Fixes: 638f5b90d460 ("bpf: reduce verifier memory consumption")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d4593571c404..71a9429fdbb5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -279,7 +279,7 @@ static void print_verifier_state(struct bpf_verifier_env *env,
for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) {
if (state->stack[i].slot_type[0] == STACK_SPILL)
verbose(env, " fp%d=%s",
- -MAX_BPF_STACK + i * BPF_REG_SIZE,
+ (-i - 1) * BPF_REG_SIZE,
reg_type_str[state->stack[i].spilled_ptr.type]);
}
verbose(env, "\n");
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 0/7] bpf: verifier improvements and cleanups
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
Small set of verifier improvements and cleanups which is
necessary for bigger patch set of bpf-to-bpf calls coming later.
See individual patches for details.
Tested on x86 and arm64 hw.
Alexei Starovoitov (7):
bpf: fix stack state printing in verifier log
bpf: print liveness info to verifier log
bpf: don't mark FP reg as uninit
bpf: improve verifier liveness marks
bpf: improve JEQ/JNE path walking
bpf: cleanup register_is_null()
selftests/bpf: adjust test_align expected output
kernel/bpf/verifier.c | 62 ++++++++----
tools/testing/selftests/bpf/test_align.c | 156 +++++++++++++++----------------
2 files changed, 120 insertions(+), 98 deletions(-)
--
2.9.5
^ permalink raw reply
* [PATCH net-next 3/7] bpf: don't mark FP reg as uninit
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
when verifier hits an internal bug don't mark register R10==FP as uninit,
since it's read only register and it's not technically correct to let
verifier run further, since it may assume that R10 has valid auxiliary state.
While developing subsequent patches this issue was discovered,
though the code eventually changed that aux reg state doesn't have
pointers any more it is still safer to avoid clearing readonly register.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f7229390c279..14ad7c6e806a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -584,8 +584,8 @@ static void mark_reg_unknown(struct bpf_verifier_env *env,
{
if (WARN_ON(regno >= MAX_BPF_REG)) {
verbose(env, "mark_reg_unknown(regs, %u)\n", regno);
- /* Something bad happened, let's kill all regs */
- for (regno = 0; regno < MAX_BPF_REG; regno++)
+ /* Something bad happened, let's kill all regs except FP */
+ for (regno = 0; regno < BPF_REG_FP; regno++)
__mark_reg_not_init(regs + regno);
return;
}
@@ -603,8 +603,8 @@ static void mark_reg_not_init(struct bpf_verifier_env *env,
{
if (WARN_ON(regno >= MAX_BPF_REG)) {
verbose(env, "mark_reg_not_init(regs, %u)\n", regno);
- /* Something bad happened, let's kill all regs */
- for (regno = 0; regno < MAX_BPF_REG; regno++)
+ /* Something bad happened, let's kill all regs except FP */
+ for (regno = 0; regno < BPF_REG_FP; regno++)
__mark_reg_not_init(regs + regno);
return;
}
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 5/7] bpf: improve JEQ/JNE path walking
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
verifier knows how to trim paths that are known not to be
taken at run-time when register containing run-time constant
is compared with another constant.
It was done only for JEQ comparison.
Extend it to include JNE as well.
More cases can be added in the future.
before after
bpf_lb-DLB_L3.o 2270 2051
bpf_lb-DLB_L4.o 3682 3287
bpf_lb-DUNKNOWN.o 1110 1080
bpf_lxc-DDROP_ALL.o 27876 24980
bpf_lxc-DUNKNOWN.o 38780 34308
bpf_netdev.o 16937 15404
bpf_overlay.o 7929 7191
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 46ff4e5b3fb7..afe9a1a0a5fe 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2955,8 +2955,9 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
if (BPF_SRC(insn->code) == BPF_K &&
(opcode == BPF_JEQ || opcode == BPF_JNE) &&
dst_reg->type == SCALAR_VALUE &&
- tnum_equals_const(dst_reg->var_off, insn->imm)) {
- if (opcode == BPF_JEQ) {
+ tnum_is_const(dst_reg->var_off)) {
+ if ((opcode == BPF_JEQ && dst_reg->var_off.value == insn->imm) ||
+ (opcode == BPF_JNE && dst_reg->var_off.value != insn->imm)) {
/* if (imm == imm) goto pc+off;
* only follow the goto, ignore fall-through
*/
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 6/7] bpf: cleanup register_is_null()
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
don't pass large struct bpf_reg_state by value.
Instead pass it by pointer.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index afe9a1a0a5fe..7afa92e9b409 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1265,9 +1265,9 @@ static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_ins
}
/* Does this register contain a constant zero? */
-static bool register_is_null(struct bpf_reg_state reg)
+static bool register_is_null(struct bpf_reg_state *reg)
{
- return reg.type == SCALAR_VALUE && tnum_equals_const(reg.var_off, 0);
+ return reg->type == SCALAR_VALUE && tnum_equals_const(reg->var_off, 0);
}
/* when register 'regno' is passed into function that will read 'access_size'
@@ -1280,31 +1280,31 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
int access_size, bool zero_size_allowed,
struct bpf_call_arg_meta *meta)
{
+ struct bpf_reg_state *reg = cur_regs(env) + regno;
struct bpf_verifier_state *state = env->cur_state;
- struct bpf_reg_state *regs = state->regs;
int off, i, slot, spi;
- if (regs[regno].type != PTR_TO_STACK) {
+ if (reg->type != PTR_TO_STACK) {
/* Allow zero-byte read from NULL, regardless of pointer type */
if (zero_size_allowed && access_size == 0 &&
- register_is_null(regs[regno]))
+ register_is_null(reg))
return 0;
verbose(env, "R%d type=%s expected=%s\n", regno,
- reg_type_str[regs[regno].type],
+ reg_type_str[reg->type],
reg_type_str[PTR_TO_STACK]);
return -EACCES;
}
/* Only allow fixed-offset stack reads */
- if (!tnum_is_const(regs[regno].var_off)) {
+ if (!tnum_is_const(reg->var_off)) {
char tn_buf[48];
- tnum_strn(tn_buf, sizeof(tn_buf), regs[regno].var_off);
+ tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
verbose(env, "invalid variable stack read R%d var_off=%s\n",
regno, tn_buf);
}
- off = regs[regno].off + regs[regno].var_off.value;
+ off = reg->off + reg->var_off.value;
if (off >= 0 || off < -MAX_BPF_STACK || off + access_size > 0 ||
access_size < 0 || (access_size == 0 && !zero_size_allowed)) {
verbose(env, "invalid stack type R%d off=%d access_size=%d\n",
@@ -1412,7 +1412,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
* passed in as argument, it's a SCALAR_VALUE type. Final test
* happens during stack boundary checking.
*/
- if (register_is_null(*reg) &&
+ if (register_is_null(reg) &&
arg_type == ARG_PTR_TO_MEM_OR_NULL)
/* final test in check_stack_boundary() */;
else if (!type_is_pkt_pointer(type) &&
--
2.9.5
^ permalink raw reply related
* [PATCH net-next 2/7] bpf: print liveness info to verifier log
From: Alexei Starovoitov @ 2017-12-01 5:31 UTC (permalink / raw)
To: David S . Miller; +Cc: Daniel Borkmann, John Fastabend, netdev, kernel-team
In-Reply-To: <20171201053141.3992592-1-ast@fb.com>
let verifier print register and stack liveness information
into verifier log
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 71a9429fdbb5..f7229390c279 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -216,6 +216,17 @@ static const char * const reg_type_str[] = {
[PTR_TO_PACKET_END] = "pkt_end",
};
+static void print_liveness(struct bpf_verifier_env *env,
+ enum bpf_reg_liveness live)
+{
+ if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN))
+ verbose(env, "_");
+ if (live & REG_LIVE_READ)
+ verbose(env, "r");
+ if (live & REG_LIVE_WRITTEN)
+ verbose(env, "w");
+}
+
static void print_verifier_state(struct bpf_verifier_env *env,
struct bpf_verifier_state *state)
{
@@ -228,7 +239,9 @@ static void print_verifier_state(struct bpf_verifier_env *env,
t = reg->type;
if (t == NOT_INIT)
continue;
- verbose(env, " R%d=%s", i, reg_type_str[t]);
+ verbose(env, " R%d", i);
+ print_liveness(env, reg->live);
+ verbose(env, "=%s", reg_type_str[t]);
if ((t == SCALAR_VALUE || t == PTR_TO_STACK) &&
tnum_is_const(reg->var_off)) {
/* reg->off should be 0 for SCALAR_VALUE */
@@ -277,10 +290,13 @@ static void print_verifier_state(struct bpf_verifier_env *env,
}
}
for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) {
- if (state->stack[i].slot_type[0] == STACK_SPILL)
- verbose(env, " fp%d=%s",
- (-i - 1) * BPF_REG_SIZE,
+ if (state->stack[i].slot_type[0] == STACK_SPILL) {
+ verbose(env, " fp%d",
+ (-i - 1) * BPF_REG_SIZE);
+ print_liveness(env, state->stack[i].spilled_ptr.live);
+ verbose(env, "=%s",
reg_type_str[state->stack[i].spilled_ptr.type]);
+ }
}
verbose(env, "\n");
}
--
2.9.5
^ permalink raw reply related
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-01 5:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jason Wang, Jesse Brandeburg, virtualization, Sridhar Samudrala,
Achiad, Peter Waskiewicz Jr, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz, netdev, Hannes Frederic Sowa
In-Reply-To: <20171130124816.7d534cf3@cakuba.netronome.com>
On Thu, Nov 30, 2017 at 12:48:22PM -0800, Jakub Kicinski wrote:
> On Thu, 30 Nov 2017 15:54:40 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 29, 2017 at 07:51:38PM -0800, Jakub Kicinski wrote:
> > > On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> > > > On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > > > > Hi, I'd like to get some feedback on a proposal to enhance virtio-net
> > > > > to ease configuration of a VM and that would enable live migration of
> > > > > passthrough network SR-IOV devices.
> > > > >
> > > > > Today we have SR-IOV network devices (VFs) that can be passed into a VM
> > > > > in order to enable high performance networking direct within the VM.
> > > > > The problem I am trying to address is that this configuration is
> > > > > generally difficult to live-migrate. There is documentation [1]
> > > > > indicating that some OS/Hypervisor vendors will support live migration
> > > > > of a system with a direct assigned networking device. The problem I
> > > > > see with these implementations is that the network configuration
> > > > > requirements that are passed on to the owner of the VM are quite
> > > > > complicated. You have to set up bonding, you have to configure it to
> > > > > enslave two interfaces, those interfaces (one is virtio-net, the other
> > > > > is SR-IOV device/driver like ixgbevf) must support MAC address changes
> > > > > requested in the VM, and on and on...
> > > > >
> > > > > So, on to the proposal:
> > > > > Modify virtio-net driver to be a single VM network device that
> > > > > enslaves an SR-IOV network device (inside the VM) with the same MAC
> > > > > address. This would cause the virtio-net driver to appear and work like
> > > > > a simplified bonding/team driver. The live migration problem would be
> > > > > solved just like today's bonding solution, but the VM user's networking
> > > > > config would be greatly simplified.
> > > > >
> > > > > At it's simplest, it would appear something like this in the VM.
> > > > >
> > > > > ==========
> > > > > = vnet0 =
> > > > > =============
> > > > > (virtio- = |
> > > > > net) = |
> > > > > = ==========
> > > > > = = ixgbef =
> > > > > ========== ==========
> > > > >
> > > > > (forgive the ASCII art)
> > > > >
> > > > > The fast path traffic would prefer the ixgbevf or other SR-IOV device
> > > > > path, and fall back to virtio's transmit/receive when migrating.
> > > > >
> > > > > Compared to today's options this proposal would
> > > > > 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> > > > > speeds
> > > > > 2) simplify end user configuration in the VM (most if not all of the
> > > > > set up to enable migration would be done in the hypervisor)
> > > > > 3) allow live migration via a simple link down and maybe a PCI
> > > > > hot-unplug of the SR-IOV device, with failover to the virtio-net
> > > > > driver core
> > > > > 4) allow vendor agnostic hardware acceleration, and live migration
> > > > > between vendors if the VM os has driver support for all the required
> > > > > SR-IOV devices.
> > > > >
> > > > > Runtime operation proposed:
> > > > > - <in either order> virtio-net driver loads, SR-IOV driver loads
> > > > > - virtio-net finds other NICs that match it's MAC address by
> > > > > both examining existing interfaces, and sets up a new device notifier
> > > > > - virtio-net enslaves the first NIC with the same MAC address
> > > > > - virtio-net brings up the slave, and makes it the "preferred" path
> > > > > - virtio-net follows the behavior of an active backup bond/team
> > > > > - virtio-net acts as the interface to the VM
> > > > > - live migration initiates
> > > > > - link goes down on SR-IOV, or SR-IOV device is removed
> > > > > - failover to virtio-net as primary path
> > > > > - migration continues to new host
> > > > > - new host is started with virio-net as primary
> > > > > - if no SR-IOV, virtio-net stays primary
> > > > > - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> > > > > - virtio-net notices new NIC and starts over at enslave step above
> > > > >
> > > > > Future ideas (brainstorming):
> > > > > - Optimize Fast east-west by having special rules to direct east-west
> > > > > traffic through virtio-net traffic path
> > > > >
> > > > > Thanks for reading!
> > > > > Jesse
> > > >
> > > > Cc netdev.
> > > >
> > > > Interesting, and this method is actually used by netvsc now:
> > > >
> > > > commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> > > > Author: stephen hemminger <stephen@networkplumber.org>
> > > > Date: Tue Aug 1 19:58:53 2017 -0700
> > > >
> > > > netvsc: transparent VF management
> > > >
> > > > This patch implements transparent fail over from synthetic NIC to
> > > > SR-IOV virtual function NIC in Hyper-V environment. It is a better
> > > > alternative to using bonding as is done now. Instead, the receive and
> > > > transmit fail over is done internally inside the driver.
> > > >
> > > > Using bonding driver has lots of issues because it depends on the
> > > > script being run early enough in the boot process and with sufficient
> > > > information to make the association. This patch moves all that
> > > > functionality into the kernel.
> > > >
> > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > >
> > > > If my understanding is correct there's no need to for any extension of
> > > > virtio spec. If this is true, maybe you can start to prepare the patch?
> > >
> > > IMHO this is as close to policy in the kernel as one can get. User
> > > land has all the information it needs to instantiate that bond/team
> > > automatically.
> >
> > It does have this info (MAC addresses match) but where's the policy
> > here? IMHO the policy has been set by the hypervisor already.
> > From hypervisor POV adding passthrough is a commitment not to migrate
> > until guest stops using the passthrough device.
> >
> > Within the guest, the bond is required for purely functional reasons - just to
> > maintain a link up since we know SRIOV will will go away. Maintaining an
> > uninterrupted connection is not a policy - it's what networking is
> > about.
> >
> > > In fact I'm trying to discuss this with NetworkManager
> > > folks and Red Hat right now:
> > >
> > > https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
> >
> > I thought we should do it too, for a while.
> >
> > But now, I think that the real issue is this: kernel exposes what looks
> > like two network devices to userspace, but in fact it is just one
> > backend device, just exposed by hypervisor in a weird way for
> > compatibility reasons.
> >
> > For example you will not get a better reliability or throughput by using
> > both of them - the only bonding mode that makes sense is fail over.
>
> Yes, I'm talking about fail over.
>
> > As another example, if the underlying physical device lost its link, trying
> > to use virtio won't help - it's only useful when the passthrough device
> > is gone for good. As another example, there is no point in not
> > configuring a bond. As a last example, depending on how the backend is
> > configured, virtio might not even work when the pass-through device is
> > active.
> >
> > So from that point of view, showing two network devices to userspace is
> > a bug that we are asking userspace to work around.
>
> I'm confused by what you're saying here. IIRC the question is whether
> we expose 2 netdevs or 3. There will always be a virtio netdev and a
> VF netdev. I assume you're not suggesting hiding the VF netdev.
Passthrough is a better term, it does not have to be a VF.
All I am saying is these are not two independent devices.
It's a good point - ideally we would hide it completely. Not sure we
can.
> So
> the question is do we expose a VF netdev and a combo virtio netdev
> which is also a bond or do we expose a VF netdev a virtio netdev, and a
> active/passive bond/team which is a well understood and architecturally
> correct construct.
It's a well understood construct for bonding but it is not exactly what
we are dealing with here. What we have is a single device with two ways
to access it.
> > > Can we flip the argument and ask why is the kernel supposed to be
> > > responsible for this?
> >
> > Because if we show a single device to userspace the number of
> > misconfigured guests will go down, and we won't lose any useful
> > flexibility.
>
> Again, single device?
>
> > > It's not like we run DHCP out of the kernel
> > > on new interfaces...
> >
> > Because one can set up a static IP, IPv6 doesn't always need DHCP, etc.
>
> But we don't handle LACP, etc.
>
> Look, as much as I don't like this, I'm not going to argue about this to
> death. I just find it very dishonest to claim kernel *has to* do it,
kernel does not *have* to do it. It might be better to do it in kernel though.
> when no one seem to have made any honest attempts to solve this in user
> space for the last 10 years :/
Each time I tried to convince userspace maintainers I get the kind of
pushback you seem to have encountered. And the reason is that they are
used to manage bonding for actual multiple ethernet ports with all the
complexity this entails. Maybe it wasn't an honest attempt in that I
didn't actually post patches, but there you are.
--
MST
^ permalink raw reply
* Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info_v1'
From: Al Viro @ 2017-12-01 4:54 UTC (permalink / raw)
To: Kees Cook
Cc: Shmulik Ladkani, Willem de Bruijn, Daniel Borkmann,
Pablo Neira Ayuso, Linus Torvalds, David Miller, LKML,
Network Development, Christoph Hellwig, Thomas Garnier, Jann Horn
In-Reply-To: <20171201034859.GN21978@ZenIV.linux.org.uk>
On Fri, Dec 01, 2017 at 03:48:59AM +0000, Al Viro wrote:
> Something similar to get_prog_path_type() above might make for a usable
> primitive, IMO...
Incidentally, bpf_obj_get_user()/bpf_obj_do_get() should just use
user_path(), rather than wanking with getname()+kern_path(pname->name)+putname().
Note that kern_path() will do getname_kernel() to get struct pathname...
Would cause problems for tracepoints in there, though. And that, BTW,
is precisely why I don't want tracepoints in core VFS, TYVM - makes
restructuring the code harder...
^ permalink raw reply
* Re: [PATCH] netfilter: add overflow checks in xt_bpf.c
From: Willem de Bruijn @ 2017-12-01 4:11 UTC (permalink / raw)
To: Jann Horn
Cc: Willem de Bruijn, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal, David S. Miller, Network Development, coreteam,
netfilter-devel
In-Reply-To: <CAG48ez0QWNg5zqyxnrDnaxqfRv4umjP81CqQxrhS3i6Wxi4t9g@mail.gmail.com>
On Thu, Nov 30, 2017 at 11:08 PM, Jann Horn <jannh@google.com> wrote:
> On Fri, Dec 1, 2017 at 5:04 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Thu, Nov 30, 2017 at 7:46 PM, Jann Horn <jannh@google.com> wrote:
>>> Check whether inputs from userspace are too long (explicit length field too
>>> big or string not null-terminated) to avoid out-of-bounds reads.
>>>
>>> As far as I can tell, this can at worst lead to very limited kernel heap
>>> memory disclosure or oopses.
>>>
>>> This bug can be triggered by an unprivileged user even if the xt_bpf module
>>> is not loaded: iptables is available in network namespaces, and the xt_bpf
>>> module can be autoloaded.
>>>
>>> Triggering the bug with a classic BPF filter with fake length 0x1000 causes
>>> the following KASAN report:
>>>
>>> ==================================================================
>>> BUG: KASAN: slab-out-of-bounds in bpf_prog_create+0x84/0xf0
>>> Read of size 32768 at addr ffff8801eff2c494 by task test/4627
>>>
>>> CPU: 0 PID: 4627 Comm: test Not tainted 4.15.0-rc1+ #1
>>> [...]
>>> Call Trace:
>>> dump_stack+0x5c/0x85
>>> print_address_description+0x6a/0x260
>>> kasan_report+0x254/0x370
>>> ? bpf_prog_create+0x84/0xf0
>>> memcpy+0x1f/0x50
>>> bpf_prog_create+0x84/0xf0
>>> bpf_mt_check+0x90/0xd6 [xt_bpf]
>>> [...]
>>> Allocated by task 4627:
>>> kasan_kmalloc+0xa0/0xd0
>>> __kmalloc_node+0x47/0x60
>>> xt_alloc_table_info+0x41/0x70 [x_tables]
>>> [...]
>>> The buggy address belongs to the object at ffff8801eff2c3c0
>>> which belongs to the cache kmalloc-2048 of size 2048
>>> The buggy address is located 212 bytes inside of
>>> 2048-byte region [ffff8801eff2c3c0, ffff8801eff2cbc0)
>>> [...]
>>> ==================================================================
>>>
>>> Fixes: e6f30c731718 ("netfilter: x_tables: add xt_bpf match")
>>> Signed-off-by: Jann Horn <jannh@google.com>
>>> ---
>>> net/netfilter/xt_bpf.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/net/netfilter/xt_bpf.c b/net/netfilter/xt_bpf.c
>>> index 041da0d9c06f..1f7fbd3c7e5a 100644
>>> --- a/net/netfilter/xt_bpf.c
>>> +++ b/net/netfilter/xt_bpf.c
>>> @@ -27,6 +27,9 @@ static int __bpf_mt_check_bytecode(struct sock_filter *insns, __u16 len,
>>> {
>>> struct sock_fprog_kern program;
>>>
>>> + if (len > XT_BPF_MAX_NUM_INSTR)
>>> + return -EINVAL;
>>> +
>>> program.len = len;
>>> program.filter = insns;
>>
>> Next, this calls bpf_prog_create, which calls bpf_check_basics_ok to verify len.
>
> Irrelevant:
>
> - see the KASAN splat in the commit message
> - bpf_check_basics_ok checks against BPF_MAXINSNS (4096), but a check against
> XT_BPF_MAX_NUM_INSTR (64) is needed because that's the size of the
> member in the
> input struct
Argh, of course. Thanks.
^ permalink raw reply
* Re: [PATCH] netfilter: add overflow checks in xt_bpf.c
From: Jann Horn @ 2017-12-01 4:08 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal, David S. Miller, Network Development, coreteam,
netfilter-devel
In-Reply-To: <CAF=yD-JRfKGjvPq4cMQADbNJc2C+0FBxAO2v=xNHKc7y-O=rJQ@mail.gmail.com>
On Fri, Dec 1, 2017 at 5:04 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Thu, Nov 30, 2017 at 7:46 PM, Jann Horn <jannh@google.com> wrote:
>> Check whether inputs from userspace are too long (explicit length field too
>> big or string not null-terminated) to avoid out-of-bounds reads.
>>
>> As far as I can tell, this can at worst lead to very limited kernel heap
>> memory disclosure or oopses.
>>
>> This bug can be triggered by an unprivileged user even if the xt_bpf module
>> is not loaded: iptables is available in network namespaces, and the xt_bpf
>> module can be autoloaded.
>>
>> Triggering the bug with a classic BPF filter with fake length 0x1000 causes
>> the following KASAN report:
>>
>> ==================================================================
>> BUG: KASAN: slab-out-of-bounds in bpf_prog_create+0x84/0xf0
>> Read of size 32768 at addr ffff8801eff2c494 by task test/4627
>>
>> CPU: 0 PID: 4627 Comm: test Not tainted 4.15.0-rc1+ #1
>> [...]
>> Call Trace:
>> dump_stack+0x5c/0x85
>> print_address_description+0x6a/0x260
>> kasan_report+0x254/0x370
>> ? bpf_prog_create+0x84/0xf0
>> memcpy+0x1f/0x50
>> bpf_prog_create+0x84/0xf0
>> bpf_mt_check+0x90/0xd6 [xt_bpf]
>> [...]
>> Allocated by task 4627:
>> kasan_kmalloc+0xa0/0xd0
>> __kmalloc_node+0x47/0x60
>> xt_alloc_table_info+0x41/0x70 [x_tables]
>> [...]
>> The buggy address belongs to the object at ffff8801eff2c3c0
>> which belongs to the cache kmalloc-2048 of size 2048
>> The buggy address is located 212 bytes inside of
>> 2048-byte region [ffff8801eff2c3c0, ffff8801eff2cbc0)
>> [...]
>> ==================================================================
>>
>> Fixes: e6f30c731718 ("netfilter: x_tables: add xt_bpf match")
>> Signed-off-by: Jann Horn <jannh@google.com>
>> ---
>> net/netfilter/xt_bpf.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/net/netfilter/xt_bpf.c b/net/netfilter/xt_bpf.c
>> index 041da0d9c06f..1f7fbd3c7e5a 100644
>> --- a/net/netfilter/xt_bpf.c
>> +++ b/net/netfilter/xt_bpf.c
>> @@ -27,6 +27,9 @@ static int __bpf_mt_check_bytecode(struct sock_filter *insns, __u16 len,
>> {
>> struct sock_fprog_kern program;
>>
>> + if (len > XT_BPF_MAX_NUM_INSTR)
>> + return -EINVAL;
>> +
>> program.len = len;
>> program.filter = insns;
>
> Next, this calls bpf_prog_create, which calls bpf_check_basics_ok to verify len.
Irrelevant:
- see the KASAN splat in the commit message
- bpf_check_basics_ok checks against BPF_MAXINSNS (4096), but a check against
XT_BPF_MAX_NUM_INSTR (64) is needed because that's the size of the
member in the
input struct
^ permalink raw reply
* Re: [PATCH] netfilter: add overflow checks in xt_bpf.c
From: Willem de Bruijn @ 2017-12-01 4:04 UTC (permalink / raw)
To: Jann Horn
Cc: Willem de Bruijn, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal, David S. Miller, Network Development, coreteam,
netfilter-devel
In-Reply-To: <20171201004607.7389-1-jannh@google.com>
On Thu, Nov 30, 2017 at 7:46 PM, Jann Horn <jannh@google.com> wrote:
> Check whether inputs from userspace are too long (explicit length field too
> big or string not null-terminated) to avoid out-of-bounds reads.
>
> As far as I can tell, this can at worst lead to very limited kernel heap
> memory disclosure or oopses.
>
> This bug can be triggered by an unprivileged user even if the xt_bpf module
> is not loaded: iptables is available in network namespaces, and the xt_bpf
> module can be autoloaded.
>
> Triggering the bug with a classic BPF filter with fake length 0x1000 causes
> the following KASAN report:
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in bpf_prog_create+0x84/0xf0
> Read of size 32768 at addr ffff8801eff2c494 by task test/4627
>
> CPU: 0 PID: 4627 Comm: test Not tainted 4.15.0-rc1+ #1
> [...]
> Call Trace:
> dump_stack+0x5c/0x85
> print_address_description+0x6a/0x260
> kasan_report+0x254/0x370
> ? bpf_prog_create+0x84/0xf0
> memcpy+0x1f/0x50
> bpf_prog_create+0x84/0xf0
> bpf_mt_check+0x90/0xd6 [xt_bpf]
> [...]
> Allocated by task 4627:
> kasan_kmalloc+0xa0/0xd0
> __kmalloc_node+0x47/0x60
> xt_alloc_table_info+0x41/0x70 [x_tables]
> [...]
> The buggy address belongs to the object at ffff8801eff2c3c0
> which belongs to the cache kmalloc-2048 of size 2048
> The buggy address is located 212 bytes inside of
> 2048-byte region [ffff8801eff2c3c0, ffff8801eff2cbc0)
> [...]
> ==================================================================
>
> Fixes: e6f30c731718 ("netfilter: x_tables: add xt_bpf match")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> net/netfilter/xt_bpf.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/netfilter/xt_bpf.c b/net/netfilter/xt_bpf.c
> index 041da0d9c06f..1f7fbd3c7e5a 100644
> --- a/net/netfilter/xt_bpf.c
> +++ b/net/netfilter/xt_bpf.c
> @@ -27,6 +27,9 @@ static int __bpf_mt_check_bytecode(struct sock_filter *insns, __u16 len,
> {
> struct sock_fprog_kern program;
>
> + if (len > XT_BPF_MAX_NUM_INSTR)
> + return -EINVAL;
> +
> program.len = len;
> program.filter = insns;
Next, this calls bpf_prog_create, which calls bpf_check_basics_ok to verify len.
> @@ -55,6 +58,9 @@ static int __bpf_mt_check_path(const char *path, struct bpf_prog **ret)
> mm_segment_t oldfs = get_fs();
> int retval, fd;
>
> + if (strnlen(path, XT_BPF_PATH_MAX) == XT_BPF_PATH_MAX)
> + return -EINVAL;
> +
Good catch. It looks like this code needs a more thorough revision.
https://lkml.kernel.org/r/<20171201034859.GN21978@ZenIV.linux.org.uk>
^ permalink raw reply
* Re: [PATCH net-next 2/5] rhashtable: Add rhastable_walk_peek
From: Tom Herbert @ 2017-12-01 3:50 UTC (permalink / raw)
To: Herbert Xu
Cc: Tom Herbert, David S . Miller, Linux Kernel Network Developers,
Rohit LastName
In-Reply-To: <20171201012102.GA26965@gondor.apana.org.au>
On Thu, Nov 30, 2017 at 5:21 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Nov 30, 2017 at 05:15:16PM -0800, Tom Herbert wrote:
>>
>> We don't need a guarantee of stability, but what I am seeing is that
>> we're consisitently dropping entries on when doing a multi-part
>> netlink walk. We start iterating over the table filling in the netlink
>> info. But eventually the netlink info fills up and returns an error.
>> netlink dump gets called again but now the iter of the table returns
>> the object following the one that would have overflowed the netlink
>> buffer. So the result I was seeing is that we dropped one object in in
>> each pass.
>
> Thanks Tom! This information is very useful.
>
> It sounds like this problem isn't specific to ila and would exist
> for all rhashtable users that dump through netlink. Let me think
> about this a little bit more.
>
Right. Also note that the first patch is inspired by netlink dump
handling also. When we reach the end of the table (walk_next returns
NULL), we'll return a non-zero skb->len if some records have been
written to the buffer. On the next call to the dump we need to bounce
out immediately with zero length returned. Resetting the walker table
in walk start because it's NULL results in infinite loop if -EAGAIN is
ignored by the caller (rhashtable_walk_start returning void is nice
side effect of this).
Tom
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info_v1'
From: Al Viro @ 2017-12-01 3:48 UTC (permalink / raw)
To: Kees Cook
Cc: Shmulik Ladkani, Willem de Bruijn, Daniel Borkmann,
Pablo Neira Ayuso, Linus Torvalds, David Miller, LKML,
Network Development, Christoph Hellwig, Thomas Garnier, Jann Horn
In-Reply-To: <20171201013304.GM21978@ZenIV.linux.org.uk>
On Fri, Dec 01, 2017 at 01:33:04AM +0000, Al Viro wrote:
> Use of file descriptors should be limited to "got a number from userland,
> convert to struct file *" on the way in and "install struct file * into
> descriptor table and return the descriptor to userland" on the way out.
> And the latter - *ONLY* after the last possible point of failure. Once
> a file reference is inserted into descriptor table, that's it - you
> can't undo that.
>
> The only way to use bpf_obj_get_user() is to pass its return value to
> userland. As return value of syscall - not even put_user() (for that
> you'd need to reserve the descriptor, copy it to userland and only
> then attach struct file * to it).
>
> The whole approach stinks - what it needs is something that would
> take struct filename * and return struct bpf_prog * or struct file *
> reference. With bpf_obj_get_user() and this thing implemented
> via that.
>
> I'm looking into that thing...
What it tries to pull off is something not far from
static struct bpf_prog *__get_prog(struct inode *inode, enum bpf_prog_type type)
{
struct bpf_prog *prog;
int err = inode_permission(inode, FMODE_READ | FMODE_WRITE);
if (err)
return ERR_PTR(err);
if (inode->i_op == &bpf_map_iops)
return ERR_PTR(-EINVAL);
if (inode->i_op != &bpf_prog_iops)
return ERR_PTR(-EACCES);
prog = inode->i_private;
err = security_bpf_prog(prog);
if (err < 0)
return ERR_PTR(err);
if (!bpf_prog_get_ok(prog, &type, false))
return ERR_PTR(-EINVAL);
return bpf_prog_inc(prog);
}
struct bpf_prog *get_prog_path_type(const char *name, enum bpf_prog_type type)
{
struct path path;
struct bpf_prog *prog;
int err = kern_path(name, LOOKUP_FOLLOW, &path);
if (err)
return ERR_PTR(err);
prog = __get_prog(d_backing_inode(path.dentry), type);
if (!IS_ERR(prog))
touch_atime(&path);
path_put(&path);
return prog;
}
static int __bpf_mt_check_path(const char *path, struct bpf_prog **ret)
{
*ret = get_prog_path_type(path, BPF_PROG_TYPE_SOCKET_FILTER);
return PTR_ERR_OR_ZERO(*ret);
}
That skips all tracepoint random shite (pardon the triple redundance) and makes
a somewhat arbitrary change for touch_atime() logics. And, of course, it is
not even compile-tested.
Something similar to get_prog_path_type() above might make for a usable
primitive, IMO...
^ permalink raw reply
* [PATCH net-next] net: hns3: Refactors "reset" handling code in HCLGE layer of HNS3 driver
From: Salil Mehta @ 2017-12-01 3:37 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: salil.mehta-hv44wF8Li93QT0dZR+AlfA,
yisen.zhuang-hv44wF8Li93QT0dZR+AlfA,
lipeng321-hv44wF8Li93QT0dZR+AlfA,
mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linuxarm-hv44wF8Li93QT0dZR+AlfA
This patch refactors the code of the reset feature in HCLGE layer
of HNS3 PF driver. Prime motivation to do this change is:
1. To reduce the time for which common miscellaneous Vector 0
interrupt is disabled because of the reset.
2. Simplification of reset request submission and pending reset
logic.
3. Simplification of the common miscellaneous interrupt handler
routine(for Vector 0) used to handle reset and other sources
of Vector 0 interrupt.
To achieve above below few things have been done:
1. Interrupt is disabled while common miscellaneous interrupt
handler is entered and re-enabled before it is exit. This
reduces the interrupt handling latency as compared to older
interrupt handling scheme where interrupt was being disabled
in interrupt handler context and re-enabled in task context
some time later.
2. Introduces new reset service task for honoring software reset
requests like from network stack related to timeout and serving
the pending reset request(to reset the driver and associated
clients).
3. Made Miscellaneous interrupt handler more generic to handle
all sources including reset interrupt source.
Signed-off-by: Salil Mehta <salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: lipeng <lipeng321-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
.../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 229 ++++++++++++++-------
.../ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 12 +-
2 files changed, 167 insertions(+), 74 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 59ed806a52c3..063be1c50a1d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -17,7 +17,7 @@
#include <linux/netdevice.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
-
+#include <net/rtnetlink.h>
#include "hclge_cmd.h"
#include "hclge_dcb.h"
#include "hclge_main.h"
@@ -2226,6 +2226,12 @@ static int hclge_mac_init(struct hclge_dev *hdev)
return hclge_cfg_func_mta_filter(hdev, 0, hdev->accept_mta_mc);
}
+static void hclge_reset_task_schedule(struct hclge_dev *hdev)
+{
+ if (!test_and_set_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev->state))
+ schedule_work(&hdev->rst_service_task);
+}
+
static void hclge_task_schedule(struct hclge_dev *hdev)
{
if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
@@ -2362,6 +2368,42 @@ static void hclge_service_complete(struct hclge_dev *hdev)
clear_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state);
}
+static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
+{
+ u32 rst_src_reg;
+
+ /* fetch the events from their corresponding regs */
+ rst_src_reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_RESET_STS_REG);
+
+ /* check for vector0 reset event sources */
+ if (BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B) & rst_src_reg) {
+ set_bit(HNAE3_GLOBAL_RESET, &hdev->reset_pending);
+ *clearval = BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B);
+ return HCLGE_VECTOR0_EVENT_RST;
+ }
+
+ if (BIT(HCLGE_VECTOR0_CORERESET_INT_B) & rst_src_reg) {
+ set_bit(HNAE3_CORE_RESET, &hdev->reset_pending);
+ *clearval = BIT(HCLGE_VECTOR0_CORERESET_INT_B);
+ return HCLGE_VECTOR0_EVENT_RST;
+ }
+
+ if (BIT(HCLGE_VECTOR0_IMPRESET_INT_B) & rst_src_reg) {
+ set_bit(HNAE3_IMP_RESET, &hdev->reset_pending);
+ *clearval = BIT(HCLGE_VECTOR0_IMPRESET_INT_B);
+ return HCLGE_VECTOR0_EVENT_RST;
+ }
+
+ return HCLGE_VECTOR0_EVENT_OTHER;
+}
+
+static void hclge_clear_event_cause(struct hclge_dev *hdev, u32 event_type,
+ u32 regclr)
+{
+ if (event_type == HCLGE_VECTOR0_EVENT_RST)
+ hclge_write_dev(&hdev->hw, HCLGE_MISC_RESET_STS_REG, regclr);
+}
+
static void hclge_enable_vector(struct hclge_misc_vector *vector, bool enable)
{
writel(enable ? 1 : 0, vector->addr);
@@ -2370,10 +2412,28 @@ static void hclge_enable_vector(struct hclge_misc_vector *vector, bool enable)
static irqreturn_t hclge_misc_irq_handle(int irq, void *data)
{
struct hclge_dev *hdev = data;
+ u32 event_cause;
+ u32 clearval;
hclge_enable_vector(&hdev->misc_vector, false);
- if (!test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
- schedule_work(&hdev->service_task);
+ event_cause = hclge_check_event_cause(hdev, &clearval);
+
+ /* vector 0 interrupt is shared with reset and mailbox source events.
+ * For now, we are not handling mailbox events.
+ */
+ switch (event_cause) {
+ case HCLGE_VECTOR0_EVENT_RST:
+ hclge_reset_task_schedule(hdev);
+ break;
+ default:
+ dev_dbg(&hdev->pdev->dev,
+ "received unknown or unhandled event of vector0\n");
+ break;
+ }
+
+ /* we should clear the source of interrupt */
+ hclge_clear_event_cause(hdev, event_cause, clearval);
+ hclge_enable_vector(&hdev->misc_vector, true);
return IRQ_HANDLED;
}
@@ -2404,9 +2464,9 @@ static int hclge_misc_irq_init(struct hclge_dev *hdev)
hclge_get_misc_vector(hdev);
- ret = devm_request_irq(&hdev->pdev->dev,
- hdev->misc_vector.vector_irq,
- hclge_misc_irq_handle, 0, "hclge_misc", hdev);
+ /* this would be explicitly freed in the end */
+ ret = request_irq(hdev->misc_vector.vector_irq, hclge_misc_irq_handle,
+ 0, "hclge_misc", hdev);
if (ret) {
hclge_free_vector(hdev, 0);
dev_err(&hdev->pdev->dev, "request misc irq(%d) fail\n",
@@ -2416,6 +2476,12 @@ static int hclge_misc_irq_init(struct hclge_dev *hdev)
return ret;
}
+static void hclge_misc_irq_uninit(struct hclge_dev *hdev)
+{
+ free_irq(hdev->misc_vector.vector_irq, hdev);
+ hclge_free_vector(hdev, 0);
+}
+
static int hclge_notify_client(struct hclge_dev *hdev,
enum hnae3_reset_notify_type type)
{
@@ -2471,12 +2537,6 @@ static int hclge_reset_wait(struct hclge_dev *hdev)
cnt++;
}
- /* must clear reset status register to
- * prevent driver detect reset interrupt again
- */
- reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_RESET_STS_REG);
- hclge_write_dev(&hdev->hw, HCLGE_MISC_RESET_STS_REG, reg);
-
if (cnt >= HCLGE_RESET_WAIT_CNT) {
dev_warn(&hdev->pdev->dev,
"Wait for reset timeout: %d\n", hdev->reset_type);
@@ -2505,12 +2565,12 @@ static int hclge_func_reset_cmd(struct hclge_dev *hdev, int func_id)
return ret;
}
-static void hclge_do_reset(struct hclge_dev *hdev, enum hnae3_reset_type type)
+static void hclge_do_reset(struct hclge_dev *hdev)
{
struct pci_dev *pdev = hdev->pdev;
u32 val;
- switch (type) {
+ switch (hdev->reset_type) {
case HNAE3_GLOBAL_RESET:
val = hclge_read_dev(&hdev->hw, HCLGE_GLOBAL_RESET_REG);
hnae_set_bit(val, HCLGE_GLOBAL_RESET_BIT, 1);
@@ -2526,30 +2586,17 @@ static void hclge_do_reset(struct hclge_dev *hdev, enum hnae3_reset_type type)
case HNAE3_FUNC_RESET:
dev_info(&pdev->dev, "PF Reset requested\n");
hclge_func_reset_cmd(hdev, 0);
+ /* schedule again to check later */
+ set_bit(HNAE3_FUNC_RESET, &hdev->reset_pending);
+ hclge_reset_task_schedule(hdev);
break;
default:
dev_warn(&pdev->dev,
- "Unsupported reset type: %d\n", type);
+ "Unsupported reset type: %d\n", hdev->reset_type);
break;
}
}
-static enum hnae3_reset_type hclge_detected_reset_event(struct hclge_dev *hdev)
-{
- enum hnae3_reset_type rst_level = HNAE3_NONE_RESET;
- u32 rst_reg_val;
-
- rst_reg_val = hclge_read_dev(&hdev->hw, HCLGE_MISC_RESET_STS_REG);
- if (BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B) & rst_reg_val)
- rst_level = HNAE3_GLOBAL_RESET;
- else if (BIT(HCLGE_VECTOR0_CORERESET_INT_B) & rst_reg_val)
- rst_level = HNAE3_CORE_RESET;
- else if (BIT(HCLGE_VECTOR0_IMPRESET_INT_B) & rst_reg_val)
- rst_level = HNAE3_IMP_RESET;
-
- return rst_level;
-}
-
static void hclge_reset_event(struct hnae3_handle *handle,
enum hnae3_reset_type reset)
{
@@ -2563,14 +2610,9 @@ static void hclge_reset_event(struct hnae3_handle *handle,
case HNAE3_FUNC_RESET:
case HNAE3_CORE_RESET:
case HNAE3_GLOBAL_RESET:
- if (test_bit(HCLGE_STATE_RESET_INT, &hdev->state)) {
- dev_err(&hdev->pdev->dev, "Already in reset state");
- return;
- }
- hdev->reset_type = reset;
- set_bit(HCLGE_STATE_RESET_INT, &hdev->state);
- set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state);
- schedule_work(&hdev->service_task);
+ /* request reset & schedule reset task */
+ set_bit(reset, &hdev->reset_request);
+ hclge_reset_task_schedule(hdev);
break;
default:
dev_warn(&hdev->pdev->dev, "Unsupported reset event:%d", reset);
@@ -2578,51 +2620,87 @@ static void hclge_reset_event(struct hnae3_handle *handle,
}
}
-static void hclge_reset_subtask(struct hclge_dev *hdev)
+static enum hnae3_reset_type hclge_get_reset_level(struct hclge_dev *hdev,
+ unsigned long *addr)
{
- bool do_reset;
+ enum hnae3_reset_type rst_level = HNAE3_NONE_RESET;
- do_reset = hdev->reset_type != HNAE3_NONE_RESET;
+ /* return the highest priority reset level amongst all */
+ if (test_bit(HNAE3_GLOBAL_RESET, addr))
+ rst_level = HNAE3_GLOBAL_RESET;
+ else if (test_bit(HNAE3_CORE_RESET, addr))
+ rst_level = HNAE3_CORE_RESET;
+ else if (test_bit(HNAE3_IMP_RESET, addr))
+ rst_level = HNAE3_IMP_RESET;
+ else if (test_bit(HNAE3_FUNC_RESET, addr))
+ rst_level = HNAE3_FUNC_RESET;
- /* Reset is detected by interrupt */
- if (hdev->reset_type == HNAE3_NONE_RESET)
- hdev->reset_type = hclge_detected_reset_event(hdev);
+ /* now, clear all other resets */
+ clear_bit(HNAE3_GLOBAL_RESET, addr);
+ clear_bit(HNAE3_CORE_RESET, addr);
+ clear_bit(HNAE3_IMP_RESET, addr);
+ clear_bit(HNAE3_FUNC_RESET, addr);
- if (hdev->reset_type == HNAE3_NONE_RESET)
- return;
+ return rst_level;
+}
- switch (hdev->reset_type) {
- case HNAE3_FUNC_RESET:
- case HNAE3_CORE_RESET:
- case HNAE3_GLOBAL_RESET:
- case HNAE3_IMP_RESET:
- hclge_notify_client(hdev, HNAE3_DOWN_CLIENT);
+static void hclge_reset(struct hclge_dev *hdev)
+{
+ /* perform reset of the stack & ae device for a client */
- if (do_reset)
- hclge_do_reset(hdev, hdev->reset_type);
- else
- set_bit(HCLGE_STATE_RESET_INT, &hdev->state);
+ hclge_notify_client(hdev, HNAE3_DOWN_CLIENT);
- if (!hclge_reset_wait(hdev)) {
- hclge_notify_client(hdev, HNAE3_UNINIT_CLIENT);
- hclge_reset_ae_dev(hdev->ae_dev);
- hclge_notify_client(hdev, HNAE3_INIT_CLIENT);
- clear_bit(HCLGE_STATE_RESET_INT, &hdev->state);
- }
- hclge_notify_client(hdev, HNAE3_UP_CLIENT);
- break;
- default:
- dev_err(&hdev->pdev->dev, "Unsupported reset type:%d\n",
- hdev->reset_type);
- break;
+ if (!hclge_reset_wait(hdev)) {
+ rtnl_lock();
+ hclge_notify_client(hdev, HNAE3_UNINIT_CLIENT);
+ hclge_reset_ae_dev(hdev->ae_dev);
+ hclge_notify_client(hdev, HNAE3_INIT_CLIENT);
+ rtnl_unlock();
+ } else {
+ /* schedule again to check pending resets later */
+ set_bit(hdev->reset_type, &hdev->reset_pending);
+ hclge_reset_task_schedule(hdev);
}
+
+ hclge_notify_client(hdev, HNAE3_UP_CLIENT);
+}
+
+static void hclge_reset_subtask(struct hclge_dev *hdev)
+{
+ /* check if there is any ongoing reset in the hardware. This status can
+ * be checked from reset_pending. If there is then, we need to wait for
+ * hardware to complete reset.
+ * a. If we are able to figure out in reasonable time that hardware
+ * has fully resetted then, we can proceed with driver, client
+ * reset.
+ * b. else, we can come back later to check this status so re-sched
+ * now.
+ */
+ hdev->reset_type = hclge_get_reset_level(hdev, &hdev->reset_pending);
+ if (hdev->reset_type != HNAE3_NONE_RESET)
+ hclge_reset(hdev);
+
+ /* check if we got any *new* reset requests to be honored */
+ hdev->reset_type = hclge_get_reset_level(hdev, &hdev->reset_request);
+ if (hdev->reset_type != HNAE3_NONE_RESET)
+ hclge_do_reset(hdev);
+
hdev->reset_type = HNAE3_NONE_RESET;
}
-static void hclge_misc_irq_service_task(struct hclge_dev *hdev)
+static void hclge_reset_service_task(struct work_struct *work)
{
+ struct hclge_dev *hdev =
+ container_of(work, struct hclge_dev, rst_service_task);
+
+ if (test_and_set_bit(HCLGE_STATE_RST_HANDLING, &hdev->state))
+ return;
+
+ clear_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev->state);
+
hclge_reset_subtask(hdev);
- hclge_enable_vector(&hdev->misc_vector, true);
+
+ clear_bit(HCLGE_STATE_RST_HANDLING, &hdev->state);
}
static void hclge_service_task(struct work_struct *work)
@@ -2630,7 +2708,6 @@ static void hclge_service_task(struct work_struct *work)
struct hclge_dev *hdev =
container_of(work, struct hclge_dev, service_task);
- hclge_misc_irq_service_task(hdev);
hclge_update_speed_duplex(hdev);
hclge_update_link_status(hdev);
hclge_update_stats_for_all(hdev);
@@ -4661,6 +4738,7 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
hdev->pdev = pdev;
hdev->ae_dev = ae_dev;
hdev->reset_type = HNAE3_NONE_RESET;
+ hdev->reset_request = 0;
ae_dev->priv = hdev;
ret = hclge_pci_init(hdev);
@@ -4772,12 +4850,15 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
timer_setup(&hdev->service_timer, hclge_service_timer, 0);
INIT_WORK(&hdev->service_task, hclge_service_task);
+ INIT_WORK(&hdev->rst_service_task, hclge_reset_service_task);
/* Enable MISC vector(vector0) */
hclge_enable_vector(&hdev->misc_vector, true);
set_bit(HCLGE_STATE_SERVICE_INITED, &hdev->state);
set_bit(HCLGE_STATE_DOWN, &hdev->state);
+ clear_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev->state);
+ clear_bit(HCLGE_STATE_RST_HANDLING, &hdev->state);
pr_info("%s driver initialization finished.\n", HCLGE_DRIVER_NAME);
return 0;
@@ -4889,14 +4970,16 @@ static void hclge_uninit_ae_dev(struct hnae3_ae_dev *ae_dev)
del_timer_sync(&hdev->service_timer);
if (hdev->service_task.func)
cancel_work_sync(&hdev->service_task);
+ if (hdev->rst_service_task.func)
+ cancel_work_sync(&hdev->rst_service_task);
if (mac->phydev)
mdiobus_unregister(mac->mdio_bus);
/* Disable MISC vector(vector0) */
hclge_enable_vector(&hdev->misc_vector, false);
- hclge_free_vector(hdev, 0);
hclge_destroy_cmd_queue(&hdev->hw);
+ hclge_misc_irq_uninit(hdev);
hclge_pci_uninit(hdev);
ae_dev->priv = NULL;
}
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 7027814ea5d7..aacec438b933 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -99,12 +99,19 @@ enum HCLGE_DEV_STATE {
HCLGE_STATE_REMOVING,
HCLGE_STATE_SERVICE_INITED,
HCLGE_STATE_SERVICE_SCHED,
+ HCLGE_STATE_RST_SERVICE_SCHED,
+ HCLGE_STATE_RST_HANDLING,
HCLGE_STATE_MBX_HANDLING,
HCLGE_STATE_MBX_IRQ,
- HCLGE_STATE_RESET_INT,
HCLGE_STATE_MAX
};
+enum hclge_evt_cause {
+ HCLGE_VECTOR0_EVENT_RST,
+ HCLGE_VECTOR0_EVENT_MBX,
+ HCLGE_VECTOR0_EVENT_OTHER,
+};
+
#define HCLGE_MPF_ENBALE 1
struct hclge_caps {
u16 num_tqp;
@@ -420,6 +427,8 @@ struct hclge_dev {
unsigned long state;
enum hnae3_reset_type reset_type;
+ unsigned long reset_request; /* reset has been requested */
+ unsigned long reset_pending; /* client rst is pending to be served */
u32 fw_version;
u16 num_vmdq_vport; /* Num vmdq vport this PF has set up */
u16 num_tqps; /* Num task queue pairs of this PF */
@@ -469,6 +478,7 @@ struct hclge_dev {
unsigned long service_timer_previous;
struct timer_list service_timer;
struct work_struct service_task;
+ struct work_struct rst_service_task;
bool cur_promisc;
int num_alloc_vfs; /* Actual number of VFs allocated */
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH net 1/2] bpf: set maximum number of attached progs to 64 for a single perf tp
From: Yonghong Song @ 2017-12-01 3:19 UTC (permalink / raw)
To: Daniel Borkmann, ast, netdev; +Cc: kernel-team
In-Reply-To: <08bfb1f5-6898-7021-f56c-5ad4b0053fcc@iogearbox.net>
On 11/30/17 6:07 PM, Daniel Borkmann wrote:
> On 11/30/2017 10:47 PM, Yonghong Song wrote:
>> cgropu+bpf prog array has a maximum number of 64 programs.
>> Let us apply the same limit here.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>
> Both applied to bpf tree, thanks! Please add a proper Fixes tags in the
> future; took care of it this time.
Will try to remember next time :-).
Thanks for taking care of this!
^ permalink raw reply
* Re: [PATCH net-next 3/5] bpftool: implement cgattach command
From: Jakub Kicinski @ 2017-12-01 3:13 UTC (permalink / raw)
To: Roman Gushchin; +Cc: netdev, linux-kernel, kernel-team, ast, daniel, kafai
In-Reply-To: <20171130134302.2840-4-guro@fb.com>
On Thu, 30 Nov 2017 13:43:00 +0000, Roman Gushchin wrote:
> + attach_type = parse_attach_type(argv[2]);
> + if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> + p_err("Invalid attach type\n");
> + return -1;
> + }
> +
> + if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, 0)) {
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> + p_err("Failed to attach program");
> + return -1;
> + }
> +
> + bpf_object__close(obj);
> + close(prog_fd);
> + close(cgroup_fd);
> +
> + return 0;
> +}
Could you try to consolidate the error paths into a one larger handler
and use gotos to jump to it? You can see it done in number of places,
grep for e.g. exit_free.
^ permalink raw reply
* Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations
From: Jakub Kicinski @ 2017-12-01 3:04 UTC (permalink / raw)
To: Roman Gushchin; +Cc: netdev, linux-kernel, kernel-team, ast, daniel, kafai
In-Reply-To: <20171130134302.2840-1-guro@fb.com>
Hi Roman!
On Thu, 30 Nov 2017 13:42:57 +0000, Roman Gushchin wrote:
> This patchset adds basic cgroup bpf operations to bpftool.
>
> Right now there is no convenient way to perform these operations.
> The /samples/bpf/load_sock_ops.c implements attach/detacg operations,
> but only for BPF_CGROUP_SOCK_OPS programs. Bps (part of bcc) implements
> bpf introspection, but lacks any cgroup-related specific.
>
> I find having a tool to perform these basic operations in the kernel tree
> very useful, as it can be used in the corresponding bpf documentation
> without creating additional dependencies. And bpftool seems to be
> a right tool to extend with such functionality.
Could you place your code in a new file and add a new "object level"?
I.e.
bpftool cgroup list
bpftool cgroup attach ...
bpftool cgroup help
etc? Note that you probably want the list to be first, so if someone
types "bpftool cg" it runs list by default.
Does it make sense to support pinned files and specifying programs by
id? I used the "id"/"pinned" keywords so that users can choose to use
either. Perhaps you should at least prefix the file to with "file"?
So:
$ bpftool cgattach file ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress
$ bpftool cgattach id 19 /sys/fs/cgroup/user.slice/ ingress
$ bpftool cgattach pin /bpf/prog /sys/fs/cgroup/user.slice/ ingress
Would this make sense?
Smaller nits on the coding style:
- please try to run checkpatch, perhaps you did, but some people
forget tools are in the kernel tree :)
- please keep includes in alphabetical order;
- please keep variable declarations in functions ordered longest to
shortest, if that's impossible because of dependency between
initializers - move the initializers to the code.
Please also don't forget to update/create new man page.
Thanks! :)
^ permalink raw reply
* Re: [PATCH net 1/2] bpf: set maximum number of attached progs to 64 for a single perf tp
From: Daniel Borkmann @ 2017-12-01 2:07 UTC (permalink / raw)
To: Yonghong Song, ast, netdev; +Cc: kernel-team
In-Reply-To: <20171130214755.3022117-1-yhs@fb.com>
On 11/30/2017 10:47 PM, Yonghong Song wrote:
> cgropu+bpf prog array has a maximum number of 64 programs.
> Let us apply the same limit here.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
Both applied to bpf tree, thanks! Please add a proper Fixes tags in the
future; took care of it this time.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox