* [PATCH v3 02/10] bpf: introduce extensible load API
From: Marat Khalili @ 2026-05-18 8:49 UTC (permalink / raw)
To: Konstantin Ananyev, Wathsala Vithanage; +Cc: dev
In-Reply-To: <20260518084912.57006-1-marat.khalili@huawei.com>
Introduce new BPF load parameters struct rte_bpf_prm_ex that can be
extended without breaking backward or forward compatibility. Introduce
new function rte_bpf_load_ex consolidating in one code path loading from
both ELF file and raw memory image, with possibility to add more options
in the future.
Some changes in code layout and sequence:
* Both old APIs now only forwarding calls to a new single entry point.
* There is now a centralized cleanup point for all temporary resources
created during the load process.
* External symbols (xsyms) are now checked for validity just after the
load started, not after they were already used for relocation.
* File bpf_load_elf.c now only handles opening ELF file and providing
patched instruction array to the load process. These are left as two
separate functions to support other ELF sources like memory image in
the future.
* Function stubs for the case libelf is not available are moved to
bpf_load_elf.c to make keeping track of them easier (forgetting to
update stubs is a common problem).
Signed-off-by: Marat Khalili <marat.khalili@huawei.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
lib/bpf/bpf_exec.c | 10 +--
lib/bpf/bpf_impl.h | 32 ++++++-
lib/bpf/bpf_jit_arm64.c | 12 +--
lib/bpf/bpf_jit_x86.c | 8 +-
lib/bpf/bpf_load.c | 182 +++++++++++++++++++++++++++++++++++-----
lib/bpf/bpf_load_elf.c | 151 +++++++++++++++++++--------------
lib/bpf/bpf_stub.c | 17 ----
lib/bpf/bpf_validate.c | 32 +++----
lib/bpf/meson.build | 4 +-
lib/bpf/rte_bpf.h | 68 ++++++++++++++-
10 files changed, 379 insertions(+), 137 deletions(-)
diff --git a/lib/bpf/bpf_exec.c b/lib/bpf/bpf_exec.c
index 18013753b1..e4668ba10b 100644
--- a/lib/bpf/bpf_exec.c
+++ b/lib/bpf/bpf_exec.c
@@ -47,7 +47,7 @@
RTE_BPF_LOG_LINE(ERR, \
"%s(%p): division by 0 at pc: %#zx;", \
__func__, bpf, \
- (uintptr_t)(ins) - (uintptr_t)(bpf)->prm.ins); \
+ (uintptr_t)(ins) - (uintptr_t)(bpf)->prm.raw.ins); \
return 0; \
} \
} while (0)
@@ -81,7 +81,7 @@
RTE_BPF_LOG_LINE(ERR, \
"%s(%p): unsupported atomic operation at pc: %#zx;", \
__func__, bpf, \
- (uintptr_t)(ins) - (uintptr_t)(bpf)->prm.ins); \
+ (uintptr_t)(ins) - (uintptr_t)(bpf)->prm.raw.ins); \
return 0; \
} \
} while (0)
@@ -157,7 +157,7 @@ bpf_ld_mbuf(const struct rte_bpf *bpf, uint64_t reg[EBPF_REG_NUM],
RTE_BPF_LOG_LINE(DEBUG, "%s(bpf=%p, mbuf=%p, ofs=%u, len=%u): "
"load beyond packet boundary at pc: %#zx;",
__func__, bpf, mb, off, len,
- (uintptr_t)(ins) - (uintptr_t)(bpf)->prm.ins);
+ (uintptr_t)(ins) - (uintptr_t)(bpf)->prm.raw.ins);
return p;
}
@@ -166,7 +166,7 @@ bpf_exec(const struct rte_bpf *bpf, uint64_t reg[EBPF_REG_NUM])
{
const struct ebpf_insn *ins;
- for (ins = bpf->prm.ins; ; ins++) {
+ for (ins = bpf->prm.raw.ins; ; ins++) {
switch (ins->code) {
/* 32 bit ALU IMM operations */
case (BPF_ALU | BPF_ADD | BPF_K):
@@ -483,7 +483,7 @@ bpf_exec(const struct rte_bpf *bpf, uint64_t reg[EBPF_REG_NUM])
RTE_BPF_LOG_LINE(ERR,
"%s(%p): invalid opcode %#x at pc: %#zx;",
__func__, bpf, ins->code,
- (uintptr_t)ins - (uintptr_t)bpf->prm.ins);
+ (uintptr_t)ins - (uintptr_t)bpf->prm.raw.ins);
return 0;
}
}
diff --git a/lib/bpf/bpf_impl.h b/lib/bpf/bpf_impl.h
index fb5ec3c4d6..1cee109bc9 100644
--- a/lib/bpf/bpf_impl.h
+++ b/lib/bpf/bpf_impl.h
@@ -11,17 +11,45 @@
#define MAX_BPF_STACK_SIZE 0x200
struct rte_bpf {
- struct rte_bpf_prm prm;
+ struct rte_bpf_prm_ex prm;
struct rte_bpf_jit jit;
size_t sz;
uint32_t stack_sz;
};
+/* Temporary copies etc. used by the load process. */
+struct __rte_bpf_load {
+ struct rte_bpf_prm_ex prm;
+
+ /* Loading ELF and applying relocations. */
+ int elf_fd; /* ELF fd, must be negative (not zero) by default. */
+ void *elf; /* Using void to avoid dependency on libelf. */
+
+ /* Value we are going to return, if any. */
+ struct rte_bpf *bpf;
+};
+
/*
* Use '__rte' prefix for non-static internal functions
* to avoid potential name conflict with other libraries.
*/
-int __rte_bpf_validate(struct rte_bpf *bpf);
+
+/* Free temporary resources created by opening ELF. */
+void
+__rte_bpf_load_elf_cleanup(struct __rte_bpf_load *load);
+
+/* Open the ELF file. */
+int
+__rte_bpf_load_elf_file(struct __rte_bpf_load *load);
+
+/* Get code from ELF and apply relocations to it. */
+int
+__rte_bpf_load_elf_code(struct __rte_bpf_load *load);
+
+/* Validate final BPF code and calculate stack size. */
+int
+__rte_bpf_validate(const struct rte_bpf_prm_ex *prm, uint32_t *stack_sz);
+
int __rte_bpf_jit(struct rte_bpf *bpf);
int __rte_bpf_jit_x86(struct rte_bpf *bpf);
int __rte_bpf_jit_arm64(struct rte_bpf *bpf);
diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c
index 4bbb97da1b..9e5e142c13 100644
--- a/lib/bpf/bpf_jit_arm64.c
+++ b/lib/bpf/bpf_jit_arm64.c
@@ -111,12 +111,12 @@ jump_offset_init(struct a64_jit_ctx *ctx, struct rte_bpf *bpf)
{
uint32_t i;
- ctx->map = malloc(bpf->prm.nb_ins * sizeof(ctx->map[0]));
+ ctx->map = malloc(bpf->prm.raw.nb_ins * sizeof(ctx->map[0]));
if (ctx->map == NULL)
return -ENOMEM;
/* Fill with fake offsets */
- for (i = 0; i != bpf->prm.nb_ins; i++) {
+ for (i = 0; i != bpf->prm.raw.nb_ins; i++) {
ctx->map[i].off = INT32_MAX;
ctx->map[i].off_to_b = 0;
}
@@ -1130,8 +1130,8 @@ check_program_has_call(struct a64_jit_ctx *ctx, struct rte_bpf *bpf)
uint8_t op;
uint32_t i;
- for (i = 0; i != bpf->prm.nb_ins; i++) {
- ins = bpf->prm.ins + i;
+ for (i = 0; i != bpf->prm.raw.nb_ins; i++) {
+ ins = bpf->prm.raw.ins + i;
op = ins->code;
switch (op) {
@@ -1168,10 +1168,10 @@ emit(struct a64_jit_ctx *ctx, struct rte_bpf *bpf)
emit_prologue(ctx);
- for (i = 0; i != bpf->prm.nb_ins; i++) {
+ for (i = 0; i != bpf->prm.raw.nb_ins; i++) {
jump_offset_update(ctx, i);
- ins = bpf->prm.ins + i;
+ ins = bpf->prm.raw.ins + i;
op = ins->code;
off = ins->off;
imm = ins->imm;
diff --git a/lib/bpf/bpf_jit_x86.c b/lib/bpf/bpf_jit_x86.c
index 88b1b5aeab..6f4235d434 100644
--- a/lib/bpf/bpf_jit_x86.c
+++ b/lib/bpf/bpf_jit_x86.c
@@ -1324,12 +1324,12 @@ emit(struct bpf_jit_state *st, const struct rte_bpf *bpf)
emit_prolog(st, bpf->stack_sz);
- for (i = 0; i != bpf->prm.nb_ins; i++) {
+ for (i = 0; i != bpf->prm.raw.nb_ins; i++) {
st->idx = i;
st->off[i] = st->sz;
- ins = bpf->prm.ins + i;
+ ins = bpf->prm.raw.ins + i;
dr = ebpf2x86[ins->dst_reg];
sr = ebpf2x86[ins->src_reg];
@@ -1532,13 +1532,13 @@ __rte_bpf_jit_x86(struct rte_bpf *bpf)
/* init state */
memset(&st, 0, sizeof(st));
- st.off = malloc(bpf->prm.nb_ins * sizeof(st.off[0]));
+ st.off = malloc(bpf->prm.raw.nb_ins * sizeof(st.off[0]));
if (st.off == NULL)
return -ENOMEM;
/* fill with fake offsets */
st.exit.off = INT32_MAX;
- for (i = 0; i != bpf->prm.nb_ins; i++)
+ for (i = 0; i != bpf->prm.raw.nb_ins; i++)
st.off[i] = INT32_MAX;
/*
diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c
index b8a0426fe2..6501841676 100644
--- a/lib/bpf/bpf_load.c
+++ b/lib/bpf/bpf_load.c
@@ -14,14 +14,14 @@
#include "bpf_impl.h"
static struct rte_bpf *
-bpf_load(const struct rte_bpf_prm *prm)
+bpf_load(const struct rte_bpf_prm_ex *prm)
{
uint8_t *buf;
struct rte_bpf *bpf;
size_t sz, bsz, insz, xsz;
xsz = prm->nb_xsym * sizeof(prm->xsym[0]);
- insz = prm->nb_ins * sizeof(prm->ins[0]);
+ insz = prm->raw.nb_ins * sizeof(prm->raw.ins[0]);
bsz = sizeof(bpf[0]);
sz = insz + xsz + bsz;
@@ -37,10 +37,10 @@ bpf_load(const struct rte_bpf_prm *prm)
if (xsz > 0)
memcpy(buf + bsz, prm->xsym, xsz);
- memcpy(buf + bsz + xsz, prm->ins, insz);
+ memcpy(buf + bsz + xsz, prm->raw.ins, insz);
bpf->prm.xsym = (void *)(buf + bsz);
- bpf->prm.ins = (void *)(buf + bsz + xsz);
+ bpf->prm.raw.ins = (void *)(buf + bsz + xsz);
return bpf;
}
@@ -80,37 +80,44 @@ bpf_check_xsym(const struct rte_bpf_xsym *xsym)
return 0;
}
-RTE_EXPORT_SYMBOL(rte_bpf_load)
-struct rte_bpf *
-rte_bpf_load(const struct rte_bpf_prm *prm)
+static int
+bpf_check_xsyms(const struct rte_bpf_xsym *xsym, uint32_t nb_xsym)
{
- struct rte_bpf *bpf;
int32_t rc;
uint32_t i;
- if (prm == NULL || prm->ins == NULL || prm->nb_ins == 0 ||
- (prm->nb_xsym != 0 && prm->xsym == NULL)) {
- rte_errno = EINVAL;
- return NULL;
- }
+ if (nb_xsym != 0 && xsym == NULL)
+ return -EINVAL;
rc = 0;
- for (i = 0; i != prm->nb_xsym && rc == 0; i++)
- rc = bpf_check_xsym(prm->xsym + i);
+ for (i = 0; i != nb_xsym && rc == 0; i++)
+ rc = bpf_check_xsym(xsym + i);
if (rc != 0) {
- rte_errno = -rc;
RTE_BPF_LOG_FUNC_LINE(ERR, "%d-th xsym is invalid", i);
- return NULL;
+ return rc;
}
+ return 0;
+}
+
+static int
+bpf_load_raw(struct __rte_bpf_load *load)
+{
+ const struct rte_bpf_prm_ex *const prm = &load->prm;
+ struct rte_bpf *bpf;
+ int32_t rc;
+
+ RTE_ASSERT(prm->origin == RTE_BPF_ORIGIN_RAW);
+
+ if (prm->raw.ins == NULL || prm->raw.nb_ins == 0)
+ return -EINVAL;
+
bpf = bpf_load(prm);
- if (bpf == NULL) {
- rte_errno = ENOMEM;
- return NULL;
- }
+ if (bpf == NULL)
+ return -ENOMEM;
- rc = __rte_bpf_validate(bpf);
+ rc = __rte_bpf_validate(&load->prm, &bpf->stack_sz);
if (rc == 0) {
__rte_bpf_jit(bpf);
if (mprotect(bpf, bpf->sz, PROT_READ) != 0)
@@ -119,9 +126,138 @@ rte_bpf_load(const struct rte_bpf_prm *prm)
if (rc != 0) {
rte_bpf_destroy(bpf);
+ return rc;
+ }
+
+ load->bpf = bpf;
+ return 0;
+}
+
+RTE_EXPORT_SYMBOL(rte_bpf_load)
+struct rte_bpf *
+rte_bpf_load(const struct rte_bpf_prm *prm)
+{
+ return rte_bpf_load_ex(&(struct rte_bpf_prm_ex){
+ .sz = sizeof(struct rte_bpf_prm_ex),
+ .origin = RTE_BPF_ORIGIN_RAW,
+ .raw.ins = prm->ins,
+ .raw.nb_ins = prm->nb_ins,
+ .xsym = prm->xsym,
+ .nb_xsym = prm->nb_xsym,
+ .prog_arg = prm->prog_arg,
+ });
+}
+
+RTE_EXPORT_SYMBOL(rte_bpf_elf_load)
+struct rte_bpf *
+rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
+ const char *sname)
+{
+ return rte_bpf_load_ex(&(struct rte_bpf_prm_ex){
+ .sz = sizeof(struct rte_bpf_prm_ex),
+ .origin = RTE_BPF_ORIGIN_ELF_FILE,
+ .elf_file.path = fname,
+ .elf_file.section = sname,
+ .xsym = prm->xsym,
+ .nb_xsym = prm->nb_xsym,
+ .prog_arg = prm->prog_arg,
+ });
+}
+
+/*
+ * Check extensible opts for invalid size or non-zero unsupported members.
+ *
+ * This code provides forward compatibility with applications compiled against
+ * newer version of this library. `opts_sz` is the size of struct `opts` in the
+ * version used for compiling the application, read from the member `sz`;
+ * `type_sz` is the size of same struct in the version used for compiling the
+ * library.
+ *
+ * If new fields were added to the struct in the application version, `opts_sz`
+ * will be greater than `type_sz`. In this case we are making sure all bytes we
+ * don't know how to interpret are zeroes, that is any new features that are
+ * there are not being used.
+ *
+ * This function can be used to check any struct following this convention.
+ */
+static bool
+opts_valid(const void *opts, size_t opts_sz, size_t type_sz)
+{
+ if (opts == NULL)
+ return true;
+
+ if (opts_sz < sizeof(opts_sz))
+ /* Size of the struct is too small even for sz member. */
+ return false;
+
+ /* Verify that all extra bytes are zeroed. */
+ for (size_t offset = type_sz; offset < opts_sz; ++offset)
+ if (((const char *)opts)[offset] != 0)
+ return false;
+
+ return true;
+}
+
+static int
+load_try(struct __rte_bpf_load *load, const struct rte_bpf_prm_ex *app_prm)
+{
+ int rc;
+
+ if (app_prm == NULL || !opts_valid(app_prm, app_prm->sz, sizeof(load->prm)))
+ return -EINVAL;
+
+ /*
+ * Convert extensible prm of application size to the size known to us.
+ *
+ * This code provides compatibility with applications compiled against
+ * different version of this library. `app_prm->sz` is the size of
+ * struct `rte_bpf_prm_ex` in the version used for compiling the
+ * application; `sizeof(load->prm)` is the size of the same struct in
+ * the version used for compiling the library.
+ *
+ * We are copying only the fields known to the application and leave
+ * the rest filled with zeroes. Any features that not known to the
+ * application will have backward-compatible default behaviour.
+ */
+ memcpy(&load->prm, app_prm, RTE_MIN(app_prm->sz, sizeof(load->prm)));
+ load->prm.sz = sizeof(load->prm);
+
+ rc = bpf_check_xsyms(load->prm.xsym, load->prm.nb_xsym);
+
+ /* Convert prm origin to raw unless it already is. */
+ switch (load->prm.origin) {
+ case RTE_BPF_ORIGIN_RAW:
+ break;
+ case RTE_BPF_ORIGIN_ELF_FILE:
+ rc = rc < 0 ? rc : __rte_bpf_load_elf_file(load);
+ rc = rc < 0 ? rc : __rte_bpf_load_elf_code(load);
+ break;
+ default:
+ rc = rc < 0 ? rc : -EINVAL;
+ }
+
+ /* Now that it is raw load it as such. */
+ rc = rc < 0 ? rc : bpf_load_raw(load);
+
+ return rc;
+}
+
+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bpf_load_ex, 26.11)
+struct rte_bpf *
+rte_bpf_load_ex(const struct rte_bpf_prm_ex *prm)
+{
+ struct __rte_bpf_load load = { .elf_fd = -1 };
+
+ const int rc = load_try(&load, prm);
+
+ __rte_bpf_load_elf_cleanup(&load);
+
+ RTE_ASSERT((rc < 0) == (load.bpf == NULL));
+
+ if (rc < 0) {
rte_errno = -rc;
return NULL;
}
- return bpf;
+ return load.bpf;
}
diff --git a/lib/bpf/bpf_load_elf.c b/lib/bpf/bpf_load_elf.c
index 2390823cbf..4ae7492351 100644
--- a/lib/bpf/bpf_load_elf.c
+++ b/lib/bpf/bpf_load_elf.c
@@ -2,6 +2,13 @@
* Copyright(c) 2018 Intel Corporation
*/
+#include "bpf_impl.h"
+
+#include <errno.h>
+
+#ifdef RTE_LIBRTE_BPF_ELF
+
+#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#include <string.h>
@@ -26,8 +33,6 @@
#include <rte_byteorder.h>
#include <rte_errno.h>
-#include "bpf_impl.h"
-
/* To overcome compatibility issue */
#ifndef EM_BPF
#define EM_BPF 247
@@ -56,7 +61,7 @@ bpf_find_xsym(const char *sn, enum rte_bpf_xtype type,
*/
static int
resolve_xsym(const char *sn, size_t ofs, struct ebpf_insn *ins, size_t ins_sz,
- const struct rte_bpf_prm *prm)
+ const struct rte_bpf_prm_ex *prm)
{
uint32_t idx, fidx;
enum rte_bpf_xtype type;
@@ -183,7 +188,7 @@ find_elf_code(Elf *elf, const char *section, Elf_Data **psd, size_t *pidx)
*/
static int
process_reloc(Elf *elf, size_t sym_idx, Elf64_Rel *re, size_t re_sz,
- struct ebpf_insn *ins, size_t ins_sz, const struct rte_bpf_prm *prm)
+ struct ebpf_insn *ins, size_t ins_sz, const struct rte_bpf_prm_ex *prm)
{
int32_t rc;
uint32_t i, n;
@@ -232,8 +237,8 @@ process_reloc(Elf *elf, size_t sym_idx, Elf64_Rel *re, size_t re_sz,
* and update bpf code.
*/
static int
-elf_reloc_code(Elf *elf, Elf_Data *ed, size_t sidx,
- const struct rte_bpf_prm *prm)
+elf_reloc_code(Elf *elf, struct ebpf_insn *ins, size_t ins_sz, size_t sidx,
+ const struct rte_bpf_prm_ex *prm)
{
Elf64_Rel *re;
Elf_Scn *sc;
@@ -256,7 +261,7 @@ elf_reloc_code(Elf *elf, Elf_Data *ed, size_t sidx,
sd->d_size % sizeof(re[0]) != 0)
return -EINVAL;
rc = process_reloc(elf, sh->sh_link,
- sd->d_buf, sd->d_size, ed->d_buf, ed->d_size,
+ sd->d_buf, sd->d_size, ins, ins_sz,
prm);
}
}
@@ -264,72 +269,96 @@ elf_reloc_code(Elf *elf, Elf_Data *ed, size_t sidx,
return rc;
}
-static struct rte_bpf *
-bpf_load_elf(const struct rte_bpf_prm *prm, int32_t fd, const char *section)
+void
+__rte_bpf_load_elf_cleanup(struct __rte_bpf_load *load)
{
- Elf *elf;
- Elf_Data *sd;
- size_t sidx;
- int32_t rc;
- struct rte_bpf *bpf;
- struct rte_bpf_prm np;
+ elf_end(load->elf);
- elf_version(EV_CURRENT);
- elf = elf_begin(fd, ELF_C_READ, NULL);
+ if (load->elf_fd >= 0 && close(load->elf_fd) < 0) {
+ const int close_errno = errno;
+ RTE_BPF_LOG_FUNC_LINE(ERR, "error %d closing: %s",
+ close_errno, strerror(close_errno));
+ }
+}
- rc = find_elf_code(elf, section, &sd, &sidx);
- if (rc == 0)
- rc = elf_reloc_code(elf, sd, sidx, prm);
+int
+__rte_bpf_load_elf_file(struct __rte_bpf_load *load)
+{
+ const struct rte_bpf_prm_ex *const prm = &load->prm;
- if (rc == 0) {
- np = prm[0];
- np.ins = sd->d_buf;
- np.nb_ins = sd->d_size / sizeof(struct ebpf_insn);
- bpf = rte_bpf_load(&np);
- } else {
- bpf = NULL;
- rte_errno = -rc;
+ RTE_ASSERT(prm->origin == RTE_BPF_ORIGIN_ELF_FILE);
+
+ if (prm->elf_file.path == NULL || prm->elf_file.section == NULL)
+ return -EINVAL;
+
+ if (elf_version(EV_CURRENT) == EV_NONE)
+ return -ENOTSUP;
+
+ load->elf_fd = open(prm->elf_file.path, O_RDONLY);
+ if (load->elf_fd < 0) {
+ const int open_errno = errno;
+ RTE_BPF_LOG_FUNC_LINE(ERR, "error %d opening \"%s\": %s",
+ open_errno, prm->elf_file.path, strerror(open_errno));
+ return -open_errno;
+ }
+
+ load->elf = elf_begin(load->elf_fd, ELF_C_READ, NULL);
+ if (load->elf == NULL) {
+ const int rc = elf_errno();
+ RTE_BPF_LOG_FUNC_LINE(ERR, "error %d opening ELF \"%s\": %s",
+ rc, prm->elf_file.path, elf_errmsg(rc));
+ return -EINVAL;
}
- elf_end(elf);
- return bpf;
+ return 0;
}
-RTE_EXPORT_SYMBOL(rte_bpf_elf_load)
-struct rte_bpf *
-rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
- const char *sname)
+int
+__rte_bpf_load_elf_code(struct __rte_bpf_load *load)
{
- int32_t fd, rc;
- struct rte_bpf *bpf;
+ struct rte_bpf_prm_ex *const prm = &load->prm;
+ Elf_Data *sd;
+ size_t sidx;
+ int rc;
- if (prm == NULL || fname == NULL || sname == NULL) {
- rte_errno = EINVAL;
- return NULL;
- }
+ rc = find_elf_code(load->elf, prm->elf_file.section, &sd, &sidx);
+ if (rc < 0)
+ return rc;
- fd = open(fname, O_RDONLY);
- if (fd < 0) {
- rc = errno;
- RTE_BPF_LOG_LINE(ERR, "%s(%s) error code: %d(%s)",
- __func__, fname, rc, strerror(rc));
- rte_errno = EINVAL;
- return NULL;
- }
+ prm->origin = RTE_BPF_ORIGIN_RAW;
+ prm->raw.ins = sd->d_buf;
+ prm->raw.nb_ins = sd->d_size / sizeof(struct ebpf_insn);
- bpf = bpf_load_elf(prm, fd, sname);
- close(fd);
+ rc = elf_reloc_code(load->elf, sd->d_buf, sd->d_size, sidx, prm);
+ if (rc < 0)
+ return -EINVAL;
- if (bpf == NULL) {
- RTE_BPF_LOG_LINE(ERR,
- "%s(fname=\"%s\", sname=\"%s\") failed, "
- "error code: %d",
- __func__, fname, sname, rte_errno);
- return NULL;
- }
+ return 0;
+}
+
+#else /* RTE_LIBRTE_BPF_ELF */
+
+void
+__rte_bpf_load_elf_cleanup(struct __rte_bpf_load *load)
+{
+ RTE_ASSERT(load->elf == NULL);
+ RTE_ASSERT(load->elf_fd < 0);
+}
- RTE_BPF_LOG_LINE(INFO, "%s(fname=\"%s\", sname=\"%s\") "
- "successfully creates %p(jit={.func=%p,.sz=%zu});",
- __func__, fname, sname, bpf, bpf->jit.func, bpf->jit.sz);
- return bpf;
+int
+__rte_bpf_load_elf_file(struct __rte_bpf_load *load)
+{
+ RTE_SET_USED(load);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "not supported, rebuild with libelf installed");
+ return -ENOTSUP;
}
+
+int
+__rte_bpf_load_elf_code(struct __rte_bpf_load *load)
+{
+ RTE_SET_USED(load);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "not supported, rebuild with libelf installed");
+ return -ENOTSUP;
+}
+
+#endif /* RTE_LIBRTE_BPF_ELF */
diff --git a/lib/bpf/bpf_stub.c b/lib/bpf/bpf_stub.c
index e06e820d83..4c329832c2 100644
--- a/lib/bpf/bpf_stub.c
+++ b/lib/bpf/bpf_stub.c
@@ -10,23 +10,6 @@
* Contains stubs for unimplemented public API functions
*/
-#ifndef RTE_LIBRTE_BPF_ELF
-RTE_EXPORT_SYMBOL(rte_bpf_elf_load)
-struct rte_bpf *
-rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
- const char *sname)
-{
- if (prm == NULL || fname == NULL || sname == NULL) {
- rte_errno = EINVAL;
- return NULL;
- }
-
- RTE_BPF_LOG_FUNC_LINE(ERR, "not supported, rebuild with libelf installed");
- rte_errno = ENOTSUP;
- return NULL;
-}
-#endif
-
#ifndef RTE_HAS_LIBPCAP
RTE_EXPORT_SYMBOL(rte_bpf_convert)
struct rte_bpf_prm *
diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c
index a7f4f576c9..5bfc59296d 100644
--- a/lib/bpf/bpf_validate.c
+++ b/lib/bpf/bpf_validate.c
@@ -80,7 +80,7 @@ struct evst_pool {
};
struct bpf_verifier {
- const struct rte_bpf_prm *prm;
+ const struct rte_bpf_prm_ex *prm;
struct inst_node *in;
uint64_t stack_sz;
uint32_t nb_nodes;
@@ -1837,7 +1837,7 @@ add_edge(struct bpf_verifier *bvf, struct inst_node *node, uint32_t nidx)
{
uint32_t ne;
- if (nidx >= bvf->prm->nb_ins) {
+ if (nidx >= bvf->prm->raw.nb_ins) {
RTE_BPF_LOG_FUNC_LINE(ERR,
"program boundary violation at pc: %u, next pc: %u",
get_node_idx(bvf, node), nidx);
@@ -1946,10 +1946,10 @@ log_unreachable(const struct bpf_verifier *bvf)
struct inst_node *node;
const struct ebpf_insn *ins;
- for (i = 0; i != bvf->prm->nb_ins; i++) {
+ for (i = 0; i != bvf->prm->raw.nb_ins; i++) {
node = bvf->in + i;
- ins = bvf->prm->ins + i;
+ ins = bvf->prm->raw.ins + i;
if (node->colour == WHITE &&
ins->code != (BPF_LD | BPF_IMM | EBPF_DW))
@@ -1966,7 +1966,7 @@ log_loop(const struct bpf_verifier *bvf)
uint32_t i, j;
struct inst_node *node;
- for (i = 0; i != bvf->prm->nb_ins; i++) {
+ for (i = 0; i != bvf->prm->raw.nb_ins; i++) {
node = bvf->in + i;
if (node->colour != BLACK)
@@ -1998,9 +1998,9 @@ validate(struct bpf_verifier *bvf)
const char *err;
rc = 0;
- for (i = 0; i < bvf->prm->nb_ins; i++) {
+ for (i = 0; i < bvf->prm->raw.nb_ins; i++) {
- ins = bvf->prm->ins + i;
+ ins = bvf->prm->raw.ins + i;
node = bvf->in + i;
err = check_syntax(ins);
@@ -2432,7 +2432,7 @@ evaluate(struct bpf_verifier *bvf)
bvf->evst->rv[EBPF_REG_10] = rvfp;
- ins = bvf->prm->ins;
+ ins = bvf->prm->raw.ins;
node = bvf->in;
next = node;
rc = 0;
@@ -2522,23 +2522,23 @@ evaluate(struct bpf_verifier *bvf)
}
int
-__rte_bpf_validate(struct rte_bpf *bpf)
+__rte_bpf_validate(const struct rte_bpf_prm_ex *prm, uint32_t *stack_sz)
{
int32_t rc;
struct bpf_verifier bvf;
/* check input argument type, don't allow mbuf ptr on 32-bit */
- if (bpf->prm.prog_arg.type != RTE_BPF_ARG_RAW &&
- bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR &&
+ if (prm->prog_arg.type != RTE_BPF_ARG_RAW &&
+ prm->prog_arg.type != RTE_BPF_ARG_PTR &&
(sizeof(uint64_t) != sizeof(uintptr_t) ||
- bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR_MBUF)) {
+ prm->prog_arg.type != RTE_BPF_ARG_PTR_MBUF)) {
RTE_BPF_LOG_FUNC_LINE(ERR, "unsupported argument type");
return -ENOTSUP;
}
memset(&bvf, 0, sizeof(bvf));
- bvf.prm = &bpf->prm;
- bvf.in = calloc(bpf->prm.nb_ins, sizeof(bvf.in[0]));
+ bvf.prm = prm;
+ bvf.in = calloc(prm->raw.nb_ins, sizeof(bvf.in[0]));
if (bvf.in == NULL)
return -ENOMEM;
@@ -2555,11 +2555,11 @@ __rte_bpf_validate(struct rte_bpf *bpf)
/* copy collected info */
if (rc == 0) {
- bpf->stack_sz = bvf.stack_sz;
+ *stack_sz = bvf.stack_sz;
/* for LD_ABS/LD_IND, we'll need extra space on the stack */
if (bvf.nb_ldmb_nodes != 0)
- bpf->stack_sz = RTE_ALIGN_CEIL(bpf->stack_sz +
+ *stack_sz = RTE_ALIGN_CEIL(*stack_sz +
sizeof(uint64_t), sizeof(uint64_t));
}
diff --git a/lib/bpf/meson.build b/lib/bpf/meson.build
index 28df7f469a..4901b6ee14 100644
--- a/lib/bpf/meson.build
+++ b/lib/bpf/meson.build
@@ -19,6 +19,7 @@ sources = files('bpf.c',
'bpf_dump.c',
'bpf_exec.c',
'bpf_load.c',
+ 'bpf_load_elf.c',
'bpf_pkt.c',
'bpf_stub.c',
'bpf_validate.c')
@@ -38,10 +39,9 @@ deps += ['mbuf', 'net', 'ethdev']
dep = dependency('libelf', required: false, method: 'pkg-config')
if dep.found()
dpdk_conf.set('RTE_LIBRTE_BPF_ELF', 1)
- sources += files('bpf_load_elf.c')
ext_deps += dep
else
- warning('libelf is missing, rte_bpf_elf_load API will be disabled')
+ warning('libelf is missing, ELF API will be disabled')
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
diff --git a/lib/bpf/rte_bpf.h b/lib/bpf/rte_bpf.h
index 309d84bc51..bf58a41819 100644
--- a/lib/bpf/rte_bpf.h
+++ b/lib/bpf/rte_bpf.h
@@ -86,7 +86,47 @@ struct rte_bpf_xsym {
};
/**
- * Input parameters for loading eBPF code.
+ * Possible origins of eBPF program code.
+ */
+enum rte_bpf_origin {
+ RTE_BPF_ORIGIN_RAW, /**< code loaded from raw array */
+ RTE_BPF_ORIGIN_RESERVED, /**< reserved for cBPF */
+ RTE_BPF_ORIGIN_ELF_FILE, /**< code loaded from elf_file */
+};
+
+/**
+ * Input parameters for loading eBPF code, extensible version.
+ *
+ * Follows libbpf conventions for extensible structs.
+ */
+struct rte_bpf_prm_ex {
+ size_t sz; /**< size of this struct for backward compatibility */
+
+ uint32_t flags; /**< flags controlling eBPF load and other options */
+
+ enum rte_bpf_origin origin; /**< origin of eBPF program code */
+
+ /** program origin parameters, member in use depends on origin */
+ union {
+ struct {
+ const struct ebpf_insn *ins; /**< eBPF instructions */
+ uint32_t nb_ins; /**< number of instructions in ins */
+ } raw;
+ struct {
+ const char *path; /**< path to the ELF file */
+ const char *section; /**< ELF section with the code */
+ } elf_file;
+ };
+
+ const struct rte_bpf_xsym *xsym;
+ /**< array of external symbols that eBPF code is allowed to reference */
+ uint32_t nb_xsym; /**< number of elements in xsym */
+
+ struct rte_bpf_arg prog_arg; /**< input arg description */
+};
+
+/**
+ * Input parameters for loading eBPF code, legacy version.
*/
struct rte_bpf_prm {
const struct ebpf_insn *ins; /**< array of eBPF instructions */
@@ -116,6 +156,32 @@ struct rte_bpf;
void
rte_bpf_destroy(struct rte_bpf *bpf);
+/**
+ * @warning
+ * @b EXPERIMENTAL: This API may change, or be removed, without prior notice.
+ *
+ * Create a new eBPF execution context, load code from specified origin into it.
+ *
+ * @param prm
+ * Parameters used to create and initialise the BPF execution context.
+ *
+ * Member sz must be set to the struct size as known to the application.
+ * If it exceeds the size known to the library, and the extra part has
+ * non-zero bytes, parameter is rejected. If it's smaller than the size known
+ * to the library, defaults are used for the members that are not present.
+ * @return
+ * BPF handle that is used in future BPF operations,
+ * or NULL on error, with error code set in rte_errno.
+ * Possible rte_errno errors include:
+ * - EINVAL - invalid parameter passed to function
+ * - ENOMEM - can't reserve enough memory
+ * - ENOTSUP - requested feature is not supported (e.g. no libelf to load ELF)
+ */
+__rte_experimental
+struct rte_bpf *
+rte_bpf_load_ex(const struct rte_bpf_prm_ex *prm)
+ __rte_malloc __rte_dealloc(rte_bpf_destroy, 1);
+
/**
* Create a new eBPF execution context and load given BPF code into it.
*
--
2.43.0
^ permalink raw reply related
* [PATCH v3 01/10] bpf: make logging prefixes more consistent
From: Marat Khalili @ 2026-05-18 8:49 UTC (permalink / raw)
To: Konstantin Ananyev, Wathsala Vithanage; +Cc: dev
In-Reply-To: <20260518084912.57006-1-marat.khalili@huawei.com>
Logging in lib/bpf is inconsistent: some places use `%s()`, other just
`%s` for `__func__`.
Introduce new macro for logging prefixed with function name and use it
everywhere function name without arguments is prefixed to the log line.
Signed-off-by: Marat Khalili <marat.khalili@huawei.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
lib/bpf/bpf_convert.c | 18 +++++++++---------
lib/bpf/bpf_impl.h | 3 +++
lib/bpf/bpf_jit_arm64.c | 4 ++--
lib/bpf/bpf_load.c | 2 +-
lib/bpf/bpf_load_elf.c | 2 +-
lib/bpf/bpf_stub.c | 6 ++----
lib/bpf/bpf_validate.c | 25 ++++++++++++-------------
7 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/lib/bpf/bpf_convert.c b/lib/bpf/bpf_convert.c
index 86e703299d..953ca80670 100644
--- a/lib/bpf/bpf_convert.c
+++ b/lib/bpf/bpf_convert.c
@@ -247,8 +247,8 @@ static int bpf_convert_filter(const struct bpf_insn *prog, size_t len,
uint8_t bpf_src;
if (len > BPF_MAXINSNS) {
- RTE_BPF_LOG_LINE(ERR, "%s: cBPF program too long (%zu insns)",
- __func__, len);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "cBPF program too long (%zu insns)",
+ len);
return -EINVAL;
}
@@ -483,8 +483,8 @@ static int bpf_convert_filter(const struct bpf_insn *prog, size_t len,
/* Unknown instruction. */
default:
- RTE_BPF_LOG_LINE(ERR, "%s: Unknown instruction!: %#x",
- __func__, fp->code);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "Unknown instruction!: %#x",
+ fp->code);
goto err;
}
@@ -528,7 +528,7 @@ rte_bpf_convert(const struct bpf_program *prog)
int ret;
if (prog == NULL) {
- RTE_BPF_LOG_LINE(ERR, "%s: NULL program", __func__);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "NULL program");
rte_errno = EINVAL;
return NULL;
}
@@ -536,13 +536,13 @@ rte_bpf_convert(const struct bpf_program *prog)
/* 1st pass: calculate the eBPF program length */
ret = bpf_convert_filter(prog->bf_insns, prog->bf_len, NULL, &ebpf_len);
if (ret < 0) {
- RTE_BPF_LOG_LINE(ERR, "%s: cannot get eBPF length", __func__);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "cannot get eBPF length");
rte_errno = -ret;
return NULL;
}
- RTE_BPF_LOG_LINE(DEBUG, "%s: prog len cBPF=%u -> eBPF=%u",
- __func__, prog->bf_len, ebpf_len);
+ RTE_BPF_LOG_FUNC_LINE(DEBUG, "prog len cBPF=%u -> eBPF=%u",
+ prog->bf_len, ebpf_len);
prm = rte_zmalloc("bpf_filter",
sizeof(*prm) + ebpf_len * sizeof(*ebpf), 0);
@@ -557,7 +557,7 @@ rte_bpf_convert(const struct bpf_program *prog)
/* 2nd pass: remap cBPF to eBPF instructions */
ret = bpf_convert_filter(prog->bf_insns, prog->bf_len, ebpf, &ebpf_len);
if (ret < 0) {
- RTE_BPF_LOG_LINE(ERR, "%s: cannot convert cBPF to eBPF", __func__);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "cannot convert cBPF to eBPF");
rte_free(prm);
rte_errno = -ret;
return NULL;
diff --git a/lib/bpf/bpf_impl.h b/lib/bpf/bpf_impl.h
index f5fa220984..fb5ec3c4d6 100644
--- a/lib/bpf/bpf_impl.h
+++ b/lib/bpf/bpf_impl.h
@@ -32,6 +32,9 @@ extern int rte_bpf_logtype;
#define RTE_BPF_LOG_LINE(lvl, ...) \
RTE_LOG_LINE(lvl, BPF, __VA_ARGS__)
+#define RTE_BPF_LOG_FUNC_LINE(lvl, fmt, ...) \
+ RTE_LOG_LINE(lvl, BPF, "%s(): " fmt, __func__, ##__VA_ARGS__)
+
static inline size_t
bpf_size(uint32_t bpf_op_sz)
{
diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c
index a04ef33a9c..4bbb97da1b 100644
--- a/lib/bpf/bpf_jit_arm64.c
+++ b/lib/bpf/bpf_jit_arm64.c
@@ -98,8 +98,8 @@ check_invalid_args(struct a64_jit_ctx *ctx, uint32_t limit)
for (idx = 0; idx < limit; idx++) {
if (rte_le_to_cpu_32(ctx->ins[idx]) == A64_INVALID_OP_CODE) {
- RTE_BPF_LOG_LINE(ERR,
- "%s: invalid opcode at %u;", __func__, idx);
+ RTE_BPF_LOG_FUNC_LINE(ERR,
+ "invalid opcode at %u;", idx);
return -EINVAL;
}
}
diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c
index 6983c026af..b8a0426fe2 100644
--- a/lib/bpf/bpf_load.c
+++ b/lib/bpf/bpf_load.c
@@ -100,7 +100,7 @@ rte_bpf_load(const struct rte_bpf_prm *prm)
if (rc != 0) {
rte_errno = -rc;
- RTE_BPF_LOG_LINE(ERR, "%s: %d-th xsym is invalid", __func__, i);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "%d-th xsym is invalid", i);
return NULL;
}
diff --git a/lib/bpf/bpf_load_elf.c b/lib/bpf/bpf_load_elf.c
index 1d30ba17e2..2390823cbf 100644
--- a/lib/bpf/bpf_load_elf.c
+++ b/lib/bpf/bpf_load_elf.c
@@ -122,7 +122,7 @@ check_elf_header(const Elf64_Ehdr *eh)
err = "unexpected machine type";
if (err != NULL) {
- RTE_BPF_LOG_LINE(ERR, "%s(): %s", __func__, err);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "%s", err);
return -EINVAL;
}
diff --git a/lib/bpf/bpf_stub.c b/lib/bpf/bpf_stub.c
index dea0d703ca..e06e820d83 100644
--- a/lib/bpf/bpf_stub.c
+++ b/lib/bpf/bpf_stub.c
@@ -21,8 +21,7 @@ rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
return NULL;
}
- RTE_BPF_LOG_LINE(ERR, "%s() is not supported, rebuild with libelf installed",
- __func__);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "not supported, rebuild with libelf installed");
rte_errno = ENOTSUP;
return NULL;
}
@@ -38,8 +37,7 @@ rte_bpf_convert(const struct bpf_program *prog)
return NULL;
}
- RTE_BPF_LOG_LINE(ERR, "%s() is not supported, rebuild with libpcap installed",
- __func__);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "not supported, rebuild with libpcap installed");
rte_errno = ENOTSUP;
return NULL;
}
diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c
index e8dbec2827..a7f4f576c9 100644
--- a/lib/bpf/bpf_validate.c
+++ b/lib/bpf/bpf_validate.c
@@ -1838,16 +1838,16 @@ add_edge(struct bpf_verifier *bvf, struct inst_node *node, uint32_t nidx)
uint32_t ne;
if (nidx >= bvf->prm->nb_ins) {
- RTE_BPF_LOG_LINE(ERR,
- "%s: program boundary violation at pc: %u, next pc: %u",
- __func__, get_node_idx(bvf, node), nidx);
+ RTE_BPF_LOG_FUNC_LINE(ERR,
+ "program boundary violation at pc: %u, next pc: %u",
+ get_node_idx(bvf, node), nidx);
return -EINVAL;
}
ne = node->nb_edge;
if (ne >= RTE_DIM(node->edge_dest)) {
- RTE_BPF_LOG_LINE(ERR, "%s: internal error at pc: %u",
- __func__, get_node_idx(bvf, node));
+ RTE_BPF_LOG_FUNC_LINE(ERR, "internal error at pc: %u",
+ get_node_idx(bvf, node));
return -EINVAL;
}
@@ -2005,8 +2005,7 @@ validate(struct bpf_verifier *bvf)
err = check_syntax(ins);
if (err != 0) {
- RTE_BPF_LOG_LINE(ERR, "%s: %s at pc: %u",
- __func__, err, i);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "%s at pc: %u", err, i);
rc |= -EINVAL;
}
@@ -2230,9 +2229,9 @@ save_cur_eval_state(struct bpf_verifier *bvf, struct inst_node *node)
/* get new eval_state for this node */
st = pull_eval_state(&bvf->evst_sr_pool);
if (st == NULL) {
- RTE_BPF_LOG_LINE(ERR,
- "%s: internal error (out of space) at pc: %u",
- __func__, get_node_idx(bvf, node));
+ RTE_BPF_LOG_FUNC_LINE(ERR,
+ "internal error (out of space) at pc: %u",
+ get_node_idx(bvf, node));
return -ENOMEM;
}
@@ -2462,8 +2461,8 @@ evaluate(struct bpf_verifier *bvf)
err = ins_chk[op].eval(bvf, ins + idx);
stats.nb_eval++;
if (err != NULL) {
- RTE_BPF_LOG_LINE(ERR, "%s: %s at pc: %u",
- __func__, err, idx);
+ RTE_BPF_LOG_FUNC_LINE(ERR,
+ "%s at pc: %u", err, idx);
rc = -EINVAL;
}
}
@@ -2533,7 +2532,7 @@ __rte_bpf_validate(struct rte_bpf *bpf)
bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR &&
(sizeof(uint64_t) != sizeof(uintptr_t) ||
bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR_MBUF)) {
- RTE_BPF_LOG_LINE(ERR, "%s: unsupported argument type", __func__);
+ RTE_BPF_LOG_FUNC_LINE(ERR, "unsupported argument type");
return -ENOTSUP;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v3 00/10] bpf: introduce extensible load API
From: Marat Khalili @ 2026-05-18 8:49 UTC (permalink / raw)
Cc: dev
In-Reply-To: <20260514093713.90118-1-marat.khalili@huawei.com>
This patchset introduces an extensible load API for the BPF library in
DPDK, addressing current limitations regarding ABI stability and feature
constraints.
Currently, `rte_bpf_load` relies on a fixed `struct rte_bpf_prm`, which
makes it difficult to add new loading options or parameters without
breaking the ABI.
To resolve these issues, this series introduces `rte_bpf_load_ex` taking
`struct rte_bpf_prm_ex`. The new parameter structure includes a `sz`
field for backward compatibility, allowing future extensions.
Taking advantage of the new extensible API, this patchset also adds
several new features:
* Support for loading and executing BPF programs with up to 5 arguments.
* Support for loading classic BPF (cBPF) directly.
* Support for loading ELF files directly from memory buffers.
* New API functions (`rte_bpf_eth_rx_install` and `rte_bpf_eth_tx_install`)
to install an already loaded BPF program as a port callback, decoupling
the loading phase from the installation phase.
v3:
* Appended Acked-by tags to all individual commits to align with
patchwork requirements.
v2:
* Fixed a potential segmentation fault in `exec_vm_burst_ex` by deferring
the dereference of `ctx[i].arg` until it is confirmed that `nb_prog_arg > 0`.
* Clarified documentation and code comments for `RTE_BPF_EXEC_FLAG_JIT`
requirements and fast-path expectations.
---
To the AI reviewer:
Thank you for the review. Several of your points were very helpful and
have been addressed in v2 (e.g. the potential null pointer dereference
in `exec_vm_burst_ex`, and documenting the `RTE_BPF_EXEC_FLAG_JIT`
expectations). The remaining points were investigated but do not require
code changes for the following reasons:
> Can the macro [RTE_BPF_LOG_FUNC_LINE] be enhanced for type safety?
> Does string literal concatenation support logging format modifiers
> consistently across all call sites?
`RTE_LOG_LINE` uses compiler attributes (`__attribute__((format(printf,
...)))`) to strictly enforce format type safety. Furthermore, standard C
preprocessors inherently support string literal concatenation prior to
format checking, ensuring it works safely across all macro invocations.
> Does `bpf_load()` always set `rte_errno` to `ENOMEM` when returning
> `NULL`, or could it mask other errors by hardcoding `-ENOMEM`?
By design, the internal `bpf_load()` function only returns `NULL` when
its internal memory allocation (`mmap`) fails. Returning `-ENOMEM`
perfectly accurately reflects the nature of this failure.
> Can the loop go out of bounds? When `opts_sz` is larger than
> `type_sz`, does this code read beyond the allocated memory region
> pointed to by `opts`?
The bounds are established by the application-supplied `opts_sz`.
Standard C semantics dictate that if a caller declares a size larger
than the actual allocated memory, it generates undefined behavior on
their end. `opts_valid` iterates exactly within the declared limits.
> Does the `memcpy()` handle the case where `app_prm->sz` is larger than
> `sizeof(load->prm)` correctly? Can partial copies lead to inconsistent
> state if the struct has pointer members?
The call to `opts_valid()` happens prior to `memcpy()` and guarantees
that any extra trailing space is definitively filled with zeroes. This
implies the application relies entirely on default functionality for the
newer unknown fields, making a truncated partial copy completely sound.
> Can `elf_end()` be called with `NULL` safely? Does it cause undefined
> behavior if `load->elf` is `NULL`?
`elf_end()` in `libelf` is explicitly defined to handle `NULL` pointers
as a safe no-op.
> Can this logic handle uninitialized or out-of-range `prog_arg->type`
> values? Does `RTE_BPF_ARG_PTR_MBUF` have a specific numeric value that
> should be validated before use?
The function utilizes strict positive equality checks against valid enum
values (`RTE_BPF_ARG_RAW`, `RTE_BPF_ARG_PTR`, `RTE_BPF_ARG_PTR_MBUF`).
Invalid or uninitialized types simply fall through and cleanly return
`false`.
> Does the `malloc` check for integer overflow in the size calculation
> `nb_ins * sizeof(load->ins[0])`?
`bpf_convert_filter()` acts as a strict gatekeeper and enforces an upper
bound limit (`BPF_MAXINSNS` = 4096) on instruction count. This extremely
small ceiling makes an arithmetic overflow in `malloc` mathematically
impossible.
> Does `bpf_eth_elf_install()` acquire any additional locks? Can this
> lead to a deadlock?
No secondary lock acquisitions exist inside `bpf_eth_elf_install()`. It
executes standard initialization logic and callback registrations
without touching threading constructs, ensuring a deadlock-free flow.
> Are we standardizing on this specific format ["%s(): "]?
Yes, we are standardizing on this specific formatting for BPF logs.
> Does this properly handle the error string in all cases?
Yes, `err` is a statically assigned string literal in this context, so
passing it to `%s` is perfectly safe.
> Does this code dereference app_prm->sz before checking if app_prm is
> NULL?
The code relies on standard C short-circuit evaluation
(`app_prm == NULL || !opts_valid(...)`), which guarantees safety.
> When app_prm->sz is smaller than sizeof(load->prm), does this code
> leave portions of load->prm uninitialized?
No. `load` is zero-initialized upon declaration (`struct __rte_bpf_load
load = { .elf_fd = -1 };`), meaning any tail space is already securely
zeroed.
> If the user-supplied paths are invalid, does this check prevent
> filesystem attacks like path traversal?
In DPDK, path validation and access control are strictly the caller's
responsibility.
> Does this code risk out-of-bounds access if the union or array layout
> changes?
Structural changes to ABI types like `ctx` are major ABI breaks, which
DPDK explicitly manages across releases.
> Does bpf_convert_filter modify nb_ins even on failure? Is checking ret
> sufficient?
The code explicitly checks if `ret < 0` and immediately bails out,
safely ignoring the state of `nb_ins` on error.
> Does this code leak load->ins if called twice without cleanup between
> invocations?
No. `load` is stack-allocated internally for a single `rte_bpf_load_ex`
invocation and never reused.
> When bpf_eth_elf_install returns an error, does this code still
> transfer ownership of the bpf pointer?
On error, ownership is not transferred. The caller retains
responsibility to call `rte_bpf_destroy()`.
> Does elf_memory() actually guarantee not to modify its input buffer?
Yes, `elf_memory` treats the buffer as read-only memory unless explicit
`ELF_C_WRITE` operations are executed, which DPDK does not do here.
Marat Khalili (10):
bpf: make logging prefixes more consistent
bpf: introduce extensible load API
bpf: support up to 5 arguments
bpf: add cBPF origin to rte_bpf_load_ex
bpf: support rte_bpf_prm_ex with port callbacks
bpf: support loading ELF files from memory
test/bpf: test loading cBPF directly
test/bpf: test loading ELF file from memory
doc: add release notes for new extensible BPF API
doc: add load API to BPF programmer's guide
app/test/test_bpf.c | 325 +++++++++++++++----------
doc/guides/prog_guide/bpf_lib.rst | 75 +++++-
doc/guides/rel_notes/release_26_07.rst | 20 ++
lib/bpf/bpf.c | 32 ++-
lib/bpf/bpf_convert.c | 97 +++++++-
lib/bpf/bpf_exec.c | 129 +++++++++-
lib/bpf/bpf_impl.h | 53 +++-
lib/bpf/bpf_jit_arm64.c | 18 +-
lib/bpf/bpf_jit_x86.c | 10 +-
lib/bpf/bpf_load.c | 200 +++++++++++++--
lib/bpf/bpf_load_elf.c | 189 +++++++++-----
lib/bpf/bpf_pkt.c | 65 +++--
lib/bpf/bpf_stub.c | 46 ----
lib/bpf/bpf_validate.c | 94 ++++---
lib/bpf/meson.build | 15 +-
lib/bpf/rte_bpf.h | 199 ++++++++++++++-
lib/bpf/rte_bpf_ethdev.h | 54 ++++
17 files changed, 1252 insertions(+), 369 deletions(-)
delete mode 100644 lib/bpf/bpf_stub.c
--
2.43.0
^ permalink raw reply
* [PATCH dpdk] fib,rib: implement full network order support for IPv4
From: Robin Jarry @ 2026-05-18 8:39 UTC (permalink / raw)
To: dev, Vladimir Medvedkin
Control plane stacks deal exclusively with network byte order addresses.
The existing RTE_FIB_F_LOOKUP_NETWORK_ORDER flag only covers datapath
lookups, forcing callers to convert every address back and forth when
adding or deleting routes.
Rename RTE_FIB_F_LOOKUP_NETWORK_ORDER to RTE_FIB_F_NETWORK_ORDER (keep
the old name with a deprecation notice) and add a new opt-in flag:
RTE_RIB_F_NETWORK_ORDER.
When these flags are specified, *all* user-facing IPv4 functions accept
and return addresses in network byte order. Internally, both libraries
keep operating in host order; the conversion happens at the public API
boundary only.
For rte_rib, a per-node flag bit tracks whether rte_rib_get_ip()
should convert back to network order, since that function does not
take a rib pointer.
When creating a FIB with RTE_FIB_F_NETWORK_ORDER, make sure that its
internal RIB object has the corresponding RTE_RIB_F_NETWORK_ORDER.
Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
app/test/test_fib.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
app/test/test_rib.c | 59 +++++++++++++++++++++++++++++++++
lib/fib/dir24_8.c | 29 ++++++++++++----
lib/fib/dir24_8.h | 1 +
lib/fib/rte_fib.c | 6 ++--
lib/fib/rte_fib.h | 8 +++--
lib/rib/rte_rib.c | 74 +++++++++++++++++++++++++++++++----------
lib/rib/rte_rib.h | 5 +++
8 files changed, 233 insertions(+), 29 deletions(-)
diff --git a/app/test/test_fib.c b/app/test/test_fib.c
index bd73399d565c..84b57eef3b5a 100644
--- a/app/test/test_fib.c
+++ b/app/test/test_fib.c
@@ -7,6 +7,7 @@
#include <stdint.h>
#include <stdlib.h>
+#include <rte_byteorder.h>
#include <rte_ip.h>
#include <rte_log.h>
#include <rte_fib.h>
@@ -24,6 +25,7 @@ static int32_t test_get_invalid(void);
static int32_t test_lookup(void);
static int32_t test_invalid_rcu(void);
static int32_t test_fib_rcu_sync_rw(void);
+static int32_t test_network_order(void);
#define MAX_ROUTES (1 << 16)
#define MAX_TBL8 (1 << 15)
@@ -588,6 +590,83 @@ test_fib_rcu_sync_rw(void)
return status == 0 ? TEST_SUCCESS : TEST_FAILED;
}
+int32_t
+test_network_order(void)
+{
+ struct rte_fib *fib = NULL;
+ struct rte_fib_conf config = { 0 };
+ uint32_t ip_he = RTE_IPV4(192, 0, 2, 0);
+ uint32_t ip_be = rte_cpu_to_be_32(ip_he);
+ uint32_t ip_miss_be = rte_cpu_to_be_32(RTE_IPV4(10, 0, 0, 1));
+ uint64_t def_nh = 100;
+ uint64_t nh_set = 42;
+ uint64_t nh_arr[3];
+ uint32_t ip_arr[3];
+ int ret;
+
+ config.max_routes = MAX_ROUTES;
+ config.rib_ext_sz = 0;
+ config.default_nh = def_nh;
+ config.type = RTE_FIB_DUMMY;
+ config.flags = RTE_FIB_F_NETWORK_ORDER;
+
+ fib = rte_fib_create(__func__, SOCKET_ID_ANY, &config);
+ RTE_TEST_ASSERT(fib != NULL, "Failed to create FIB\n");
+
+ ret = rte_fib_add(fib, ip_be, 24, nh_set);
+ RTE_TEST_ASSERT(ret == 0, "Failed to add route\n");
+
+ ip_arr[0] = ip_be;
+ ip_arr[1] = rte_cpu_to_be_32(RTE_IPV4(192, 0, 2, 123));
+ ip_arr[2] = ip_miss_be;
+
+ ret = rte_fib_lookup_bulk(fib, ip_arr, nh_arr, 3);
+ RTE_TEST_ASSERT(ret == 0, "Failed to lookup\n");
+ RTE_TEST_ASSERT(nh_arr[0] == nh_set,
+ "Failed to get proper nexthop for prefix\n");
+ RTE_TEST_ASSERT(nh_arr[1] == nh_set,
+ "Failed to get proper nexthop for covered IP\n");
+ RTE_TEST_ASSERT(nh_arr[2] == def_nh,
+ "Failed to get default nexthop for missing IP\n");
+
+ ret = rte_fib_delete(fib, ip_be, 24);
+ RTE_TEST_ASSERT(ret == 0, "Failed to delete route\n");
+
+ ret = rte_fib_lookup_bulk(fib, ip_arr, nh_arr, 1);
+ RTE_TEST_ASSERT(ret == 0, "Failed to lookup\n");
+ RTE_TEST_ASSERT(nh_arr[0] == def_nh,
+ "Failed to get default nexthop after delete\n");
+
+ rte_fib_free(fib);
+
+ /* repeat with DIR24_8 */
+ config.type = RTE_FIB_DIR24_8;
+ config.dir24_8.nh_sz = RTE_FIB_DIR24_8_4B;
+ config.dir24_8.num_tbl8 = MAX_TBL8;
+
+ fib = rte_fib_create(__func__, SOCKET_ID_ANY, &config);
+ RTE_TEST_ASSERT(fib != NULL, "Failed to create DIR24_8 FIB\n");
+
+ ret = rte_fib_add(fib, ip_be, 24, nh_set);
+ RTE_TEST_ASSERT(ret == 0, "Failed to add route\n");
+
+ ret = rte_fib_lookup_bulk(fib, ip_arr, nh_arr, 3);
+ RTE_TEST_ASSERT(ret == 0, "Failed to lookup\n");
+ RTE_TEST_ASSERT(nh_arr[0] == nh_set,
+ "Failed to get proper nexthop for prefix\n");
+ RTE_TEST_ASSERT(nh_arr[1] == nh_set,
+ "Failed to get proper nexthop for covered IP\n");
+ RTE_TEST_ASSERT(nh_arr[2] == def_nh,
+ "Failed to get default nexthop for missing IP\n");
+
+ ret = rte_fib_delete(fib, ip_be, 24);
+ RTE_TEST_ASSERT(ret == 0, "Failed to delete route\n");
+
+ rte_fib_free(fib);
+
+ return TEST_SUCCESS;
+}
+
static struct unit_test_suite fib_fast_tests = {
.suite_name = "fib autotest",
.setup = NULL,
@@ -600,6 +679,7 @@ static struct unit_test_suite fib_fast_tests = {
TEST_CASE(test_lookup),
TEST_CASE(test_invalid_rcu),
TEST_CASE(test_fib_rcu_sync_rw),
+ TEST_CASE(test_network_order),
TEST_CASES_END()
}
};
diff --git a/app/test/test_rib.c b/app/test/test_rib.c
index a4a683140df3..200533e84e09 100644
--- a/app/test/test_rib.c
+++ b/app/test/test_rib.c
@@ -21,6 +21,7 @@ static int32_t test_insert_invalid(void);
static int32_t test_get_fn(void);
static int32_t test_basic(void);
static int32_t test_tree_traversal(void);
+static int32_t test_network_order(void);
#define MAX_DEPTH 32
#define MAX_RULES (1 << 22)
@@ -323,6 +324,63 @@ test_tree_traversal(void)
return TEST_SUCCESS;
}
+int32_t
+test_network_order(void)
+{
+ struct rte_rib *rib = NULL;
+ struct rte_rib_node *node;
+ struct rte_rib_conf config;
+
+ uint32_t ip_he = RTE_IPV4(192, 0, 2, 0);
+ uint32_t ip_be = rte_cpu_to_be_32(ip_he);
+ uint32_t ip_ret;
+ uint64_t nh_set = 42;
+ uint64_t nh_ret;
+ uint8_t depth = 24;
+ int ret;
+
+ config.max_nodes = MAX_RULES;
+ config.ext_sz = 0;
+ config.flags = RTE_RIB_F_NETWORK_ORDER;
+
+ rib = rte_rib_create(__func__, SOCKET_ID_ANY, &config);
+ RTE_TEST_ASSERT(rib != NULL, "Failed to create RIB\n");
+
+ node = rte_rib_insert(rib, ip_be, depth);
+ RTE_TEST_ASSERT(node != NULL, "Failed to insert rule\n");
+
+ ret = rte_rib_set_nh(node, nh_set);
+ RTE_TEST_ASSERT(ret == 0, "Failed to set nh\n");
+
+ /* lookup with network-order IP */
+ node = rte_rib_lookup(rib, ip_be);
+ RTE_TEST_ASSERT(node != NULL, "Failed to lookup\n");
+
+ ret = rte_rib_get_nh(node, &nh_ret);
+ RTE_TEST_ASSERT((ret == 0) && (nh_ret == nh_set),
+ "Failed to get proper nexthop\n");
+
+ /* get_ip must return network-order IP */
+ ret = rte_rib_get_ip(node, &ip_ret);
+ RTE_TEST_ASSERT((ret == 0) && (ip_ret == ip_be),
+ "Failed to get proper IP in network order\n");
+
+ /* exact lookup with network-order IP */
+ node = rte_rib_lookup_exact(rib, ip_be, depth);
+ RTE_TEST_ASSERT(node != NULL, "Failed to exact lookup\n");
+
+ /* remove with network-order IP */
+ rte_rib_remove(rib, ip_be, depth);
+
+ node = rte_rib_lookup(rib, ip_be);
+ RTE_TEST_ASSERT(node == NULL,
+ "Lookup returns non existent rule\n");
+
+ rte_rib_free(rib);
+
+ return TEST_SUCCESS;
+}
+
static struct unit_test_suite rib_tests = {
.suite_name = "rib autotest",
.setup = NULL,
@@ -334,6 +392,7 @@ static struct unit_test_suite rib_tests = {
TEST_CASE(test_get_fn),
TEST_CASE(test_basic),
TEST_CASE(test_tree_traversal),
+ TEST_CASE(test_network_order),
TEST_CASES_END()
}
};
diff --git a/lib/fib/dir24_8.c b/lib/fib/dir24_8.c
index 489d2ef427c6..781ecbe9e64e 100644
--- a/lib/fib/dir24_8.c
+++ b/lib/fib/dir24_8.c
@@ -414,24 +414,33 @@ install_to_fib(struct dir24_8_tbl *dp, uint32_t ledge, uint32_t redge,
return 0;
}
+/*
+ * modify_fib operates in host byte order. When the RIB carries the
+ * network-order flag, rte_rib_get_nxt/rte_rib_get_ip return addresses
+ * in network order, so this helper converts them back to host order
+ * for DIR24_8 table operations.
+ */
static int
modify_fib(struct dir24_8_tbl *dp, struct rte_rib *rib, uint32_t ip,
uint8_t depth, uint64_t next_hop)
{
struct rte_rib_node *tmp = NULL;
uint32_t ledge, redge, tmp_ip;
+ uint32_t rib_ip = dp->be_addr ? rte_cpu_to_be_32(ip) : ip;
int ret;
uint8_t tmp_depth;
ledge = ip;
do {
- tmp = rte_rib_get_nxt(rib, ip, depth, tmp,
+ tmp = rte_rib_get_nxt(rib, rib_ip, depth, tmp,
RTE_RIB_GET_NXT_COVER);
if (tmp != NULL) {
rte_rib_get_depth(tmp, &tmp_depth);
if (tmp_depth == depth)
continue;
rte_rib_get_ip(tmp, &tmp_ip);
+ if (dp->be_addr)
+ tmp_ip = rte_be_to_cpu_32(tmp_ip);
redge = tmp_ip & rte_rib_depth_to_mask(tmp_depth);
if (ledge == redge) {
ledge = redge +
@@ -475,6 +484,7 @@ dir24_8_modify(struct rte_fib *fib, uint32_t ip, uint8_t depth,
struct rte_rib_node *parent;
int ret = 0;
uint64_t par_nh, node_nh;
+ uint32_t ip_he;
if ((fib == NULL) || (depth > RTE_FIB_MAXDEPTH))
return -EINVAL;
@@ -486,7 +496,13 @@ dir24_8_modify(struct rte_fib *fib, uint32_t ip, uint8_t depth,
if (next_hop > get_max_nh(dp->nh_sz))
return -EINVAL;
- ip &= rte_rib_depth_to_mask(depth);
+ /*
+ * The RIB API handles byte order conversion when the
+ * network-order flag is set. DIR24_8 table operations
+ * require host-order addresses.
+ */
+ ip_he = dp->be_addr ? rte_be_to_cpu_32(ip) : ip;
+ ip_he &= rte_rib_depth_to_mask(depth);
node = rte_rib_lookup_exact(rib, ip, depth);
switch (op) {
@@ -495,7 +511,7 @@ dir24_8_modify(struct rte_fib *fib, uint32_t ip, uint8_t depth,
rte_rib_get_nh(node, &node_nh);
if (node_nh == next_hop)
return 0;
- ret = modify_fib(dp, rib, ip, depth, next_hop);
+ ret = modify_fib(dp, rib, ip_he, depth, next_hop);
if (ret == 0)
rte_rib_set_nh(node, next_hop);
return 0;
@@ -518,7 +534,7 @@ dir24_8_modify(struct rte_fib *fib, uint32_t ip, uint8_t depth,
if (par_nh == next_hop)
goto successfully_added;
}
- ret = modify_fib(dp, rib, ip, depth, next_hop);
+ ret = modify_fib(dp, rib, ip_he, depth, next_hop);
if (ret != 0) {
rte_rib_remove(rib, ip, depth);
return ret;
@@ -536,9 +552,9 @@ dir24_8_modify(struct rte_fib *fib, uint32_t ip, uint8_t depth,
rte_rib_get_nh(parent, &par_nh);
rte_rib_get_nh(node, &node_nh);
if (par_nh != node_nh)
- ret = modify_fib(dp, rib, ip, depth, par_nh);
+ ret = modify_fib(dp, rib, ip_he, depth, par_nh);
} else
- ret = modify_fib(dp, rib, ip, depth, dp->def_nh);
+ ret = modify_fib(dp, rib, ip_he, depth, dp->def_nh);
if (ret == 0) {
rte_rib_remove(rib, ip, depth);
if (depth > 24) {
@@ -606,6 +622,7 @@ dir24_8_create(const char *name, int socket_id, struct rte_fib_conf *fib_conf)
dp->def_nh = def_nh;
dp->nh_sz = nh_sz;
dp->number_tbl8s = num_tbl8;
+ dp->be_addr = !!(fib_conf->flags & RTE_FIB_F_NETWORK_ORDER);
snprintf(mem_name, sizeof(mem_name), "TBL8_idxes_%p", dp);
dp->tbl8_idxes = rte_zmalloc_socket(mem_name,
diff --git a/lib/fib/dir24_8.h b/lib/fib/dir24_8.h
index b343b5d6866d..d4fa248cfb88 100644
--- a/lib/fib/dir24_8.h
+++ b/lib/fib/dir24_8.h
@@ -33,6 +33,7 @@ struct dir24_8_tbl {
uint32_t rsvd_tbl8s; /**< Number of reserved tbl8s */
uint32_t cur_tbl8s; /**< Current number of tbl8s */
enum rte_fib_dir24_8_nh_sz nh_sz; /**< Size of nexthop entry */
+ bool be_addr; /**< User addresses in network order */
/* RCU config. */
enum rte_fib_qsbr_mode rcu_mode;/* Blocking, defer queue. */
struct rte_rcu_qsbr *v; /* RCU QSBR variable. */
diff --git a/lib/fib/rte_fib.c b/lib/fib/rte_fib.c
index 184210f38070..3e4c15cfc809 100644
--- a/lib/fib/rte_fib.c
+++ b/lib/fib/rte_fib.c
@@ -109,7 +109,7 @@ init_dataplane(struct rte_fib *fib, __rte_unused int socket_id,
if (fib->dp == NULL)
return -rte_errno;
fib->lookup = dir24_8_get_lookup_fn(fib->dp,
- RTE_FIB_LOOKUP_DEFAULT, !!(fib->flags & RTE_FIB_F_LOOKUP_NETWORK_ORDER));
+ RTE_FIB_LOOKUP_DEFAULT, !!(fib->flags & RTE_FIB_F_NETWORK_ORDER));
fib->modify = dir24_8_modify;
return 0;
default:
@@ -172,6 +172,8 @@ rte_fib_create(const char *name, int socket_id, struct rte_fib_conf *conf)
rib_conf.ext_sz = conf->rib_ext_sz;
rib_conf.max_nodes = conf->max_routes * 2;
+ rib_conf.flags = (conf->flags & RTE_FIB_F_NETWORK_ORDER) ?
+ RTE_RIB_F_NETWORK_ORDER : 0;
rib = rte_rib_create(name, socket_id, &rib_conf);
if (rib == NULL) {
@@ -340,7 +342,7 @@ rte_fib_select_lookup(struct rte_fib *fib,
switch (fib->type) {
case RTE_FIB_DIR24_8:
fn = dir24_8_get_lookup_fn(fib->dp, type,
- !!(fib->flags & RTE_FIB_F_LOOKUP_NETWORK_ORDER));
+ !!(fib->flags & RTE_FIB_F_NETWORK_ORDER));
if (fn == NULL)
return -EINVAL;
fib->lookup = fn;
diff --git a/lib/fib/rte_fib.h b/lib/fib/rte_fib.h
index b16a653535cf..5542e735b6f3 100644
--- a/lib/fib/rte_fib.h
+++ b/lib/fib/rte_fib.h
@@ -91,9 +91,11 @@ enum rte_fib_lookup_type {
/**< Vector implementation using AVX512 */
};
-/** If set, fib lookup is expecting IPv4 address in network byte order */
-#define RTE_FIB_F_LOOKUP_NETWORK_ORDER 1
-#define RTE_FIB_ALLOWED_FLAGS (RTE_FIB_F_LOOKUP_NETWORK_ORDER)
+/** If set, all user-facing IPv4 addresses are in network byte order */
+#define RTE_FIB_F_NETWORK_ORDER 1
+#define RTE_FIB_F_LOOKUP_NETWORK_ORDER \
+ (RTE_DEPRECATED(RTE_FIB_F_LOOKUP_NETWORK_ORDER) RTE_FIB_F_NETWORK_ORDER)
+#define RTE_FIB_ALLOWED_FLAGS (RTE_FIB_F_NETWORK_ORDER)
/** FIB configuration structure */
struct rte_fib_conf {
diff --git a/lib/rib/rte_rib.c b/lib/rib/rte_rib.c
index 046db131ca00..c6dbe684d211 100644
--- a/lib/rib/rte_rib.c
+++ b/lib/rib/rte_rib.c
@@ -8,6 +8,7 @@
#include <sys/queue.h>
#include <eal_export.h>
+#include <rte_byteorder.h>
#include <rte_eal_memconfig.h>
#include <rte_errno.h>
#include <rte_malloc.h>
@@ -28,6 +29,8 @@ static struct rte_tailq_elem rte_rib_tailq = {
EAL_REGISTER_TAILQ(rte_rib_tailq)
#define RTE_RIB_VALID_NODE 1
+#define RIB_NODE_NET_ORDER 2
+#define RTE_RIB_ALLOWED_FLAGS (RTE_RIB_F_NETWORK_ORDER)
/* Maximum depth value possible for IPv4 RIB. */
#define RIB_MAXDEPTH 32
/* Maximum length of a RIB name. */
@@ -51,6 +54,7 @@ struct rte_rib {
uint32_t cur_nodes;
uint32_t cur_routes;
uint32_t max_nodes;
+ unsigned int flags;
};
static inline bool
@@ -112,6 +116,8 @@ rte_rib_lookup(struct rte_rib *rib, uint32_t ip)
rte_errno = EINVAL;
return NULL;
}
+ if (rib->flags & RTE_RIB_F_NETWORK_ORDER)
+ ip = rte_be_to_cpu_32(ip);
cur = rib->tree;
while ((cur != NULL) && is_covered(ip, cur->ip, cur->depth)) {
@@ -162,6 +168,8 @@ rte_rib_lookup_exact(struct rte_rib *rib, uint32_t ip, uint8_t depth)
rte_errno = EINVAL;
return NULL;
}
+ if (rib->flags & RTE_RIB_F_NETWORK_ORDER)
+ ip = rte_be_to_cpu_32(ip);
ip &= rte_rib_depth_to_mask(depth);
return __rib_lookup_exact(rib, ip, depth);
@@ -172,18 +180,12 @@ rte_rib_lookup_exact(struct rte_rib *rib, uint32_t ip, uint8_t depth)
* for a given in args ip/depth prefix
* last = NULL means the first invocation
*/
-RTE_EXPORT_SYMBOL(rte_rib_get_nxt)
-struct rte_rib_node *
-rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
+static struct rte_rib_node *
+__rib_get_nxt(struct rte_rib *rib, uint32_t ip,
uint8_t depth, struct rte_rib_node *last, int flag)
{
struct rte_rib_node *tmp, *prev = NULL;
- if (unlikely(rib == NULL || depth > RIB_MAXDEPTH)) {
- rte_errno = EINVAL;
- return NULL;
- }
-
if (last == NULL) {
tmp = rib->tree;
while ((tmp) && (tmp->depth < depth))
@@ -213,13 +215,28 @@ rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
return prev;
}
-RTE_EXPORT_SYMBOL(rte_rib_remove)
-void
-rte_rib_remove(struct rte_rib *rib, uint32_t ip, uint8_t depth)
+RTE_EXPORT_SYMBOL(rte_rib_get_nxt)
+struct rte_rib_node *
+rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
+ uint8_t depth, struct rte_rib_node *last, int flag)
+{
+ if (unlikely(rib == NULL || depth > RIB_MAXDEPTH)) {
+ rte_errno = EINVAL;
+ return NULL;
+ }
+ if (rib->flags & RTE_RIB_F_NETWORK_ORDER)
+ ip = rte_be_to_cpu_32(ip);
+
+ return __rib_get_nxt(rib, ip, depth, last, flag);
+}
+
+static void
+__rib_remove(struct rte_rib *rib, uint32_t ip, uint8_t depth)
{
struct rte_rib_node *cur, *prev, *child;
- cur = rte_rib_lookup_exact(rib, ip, depth);
+ ip &= rte_rib_depth_to_mask(depth);
+ cur = __rib_lookup_exact(rib, ip, depth);
if (cur == NULL)
return;
@@ -246,6 +263,17 @@ rte_rib_remove(struct rte_rib *rib, uint32_t ip, uint8_t depth)
}
}
+RTE_EXPORT_SYMBOL(rte_rib_remove)
+void
+rte_rib_remove(struct rte_rib *rib, uint32_t ip, uint8_t depth)
+{
+ if (unlikely(rib == NULL || depth > RIB_MAXDEPTH))
+ return;
+ if (rib->flags & RTE_RIB_F_NETWORK_ORDER)
+ ip = rte_be_to_cpu_32(ip);
+ __rib_remove(rib, ip, depth);
+}
+
RTE_EXPORT_SYMBOL(rte_rib_insert)
struct rte_rib_node *
rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
@@ -257,12 +285,18 @@ rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
int d = 0;
uint32_t common_prefix;
uint8_t common_depth;
+ uint8_t net_order = 0;
if (unlikely(rib == NULL || depth > RIB_MAXDEPTH)) {
rte_errno = EINVAL;
return NULL;
}
+ if (rib->flags & RTE_RIB_F_NETWORK_ORDER) {
+ ip = rte_be_to_cpu_32(ip);
+ net_order = RIB_NODE_NET_ORDER;
+ }
+
tmp = &rib->tree;
ip &= rte_rib_depth_to_mask(depth);
new_node = __rib_lookup_exact(rib, ip, depth);
@@ -281,7 +315,7 @@ rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
new_node->parent = NULL;
new_node->ip = ip;
new_node->depth = depth;
- new_node->flag = RTE_RIB_VALID_NODE;
+ new_node->flag = RTE_RIB_VALID_NODE | net_order;
/* traverse down the tree to find matching node or closest matching */
while (1) {
@@ -300,7 +334,7 @@ rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
*/
if ((ip == (*tmp)->ip) && (depth == (*tmp)->depth)) {
node_free(rib, new_node);
- (*tmp)->flag |= RTE_RIB_VALID_NODE;
+ (*tmp)->flag |= RTE_RIB_VALID_NODE | net_order;
++rib->cur_routes;
return *tmp;
}
@@ -336,7 +370,7 @@ rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
}
common_node->ip = common_prefix;
common_node->depth = common_depth;
- common_node->flag = 0;
+ common_node->flag = net_order;
common_node->parent = (*tmp)->parent;
new_node->parent = common_node;
(*tmp)->parent = common_node;
@@ -362,6 +396,8 @@ rte_rib_get_ip(const struct rte_rib_node *node, uint32_t *ip)
return -1;
}
*ip = node->ip;
+ if (node->flag & RIB_NODE_NET_ORDER)
+ *ip = rte_cpu_to_be_32(*ip);
return 0;
}
@@ -419,7 +455,8 @@ rte_rib_create(const char *name, int socket_id, const struct rte_rib_conf *conf)
struct rte_mempool *node_pool;
/* Check user arguments. */
- if (unlikely(name == NULL || conf == NULL || conf->max_nodes <= 0)) {
+ if (unlikely(name == NULL || conf == NULL || conf->max_nodes <= 0 ||
+ (conf->flags & ~RTE_RIB_ALLOWED_FLAGS))) {
rte_errno = EINVAL;
return NULL;
}
@@ -473,6 +510,7 @@ rte_rib_create(const char *name, int socket_id, const struct rte_rib_conf *conf)
rte_strlcpy(rib->name, name, sizeof(rib->name));
rib->tree = NULL;
rib->max_nodes = conf->max_nodes;
+ rib->flags = conf->flags;
rib->node_pool = node_pool;
te->data = (void *)rib;
TAILQ_INSERT_TAIL(rib_list, te, next);
@@ -541,9 +579,9 @@ rte_rib_free(struct rte_rib *rib)
rte_mcfg_tailq_write_unlock();
- while ((tmp = rte_rib_get_nxt(rib, 0, 0, tmp,
+ while ((tmp = __rib_get_nxt(rib, 0, 0, tmp,
RTE_RIB_GET_NXT_ALL)) != NULL)
- rte_rib_remove(rib, tmp->ip, tmp->depth);
+ __rib_remove(rib, tmp->ip, tmp->depth);
rte_mempool_free(rib->node_pool);
rte_free(rib);
diff --git a/lib/rib/rte_rib.h b/lib/rib/rte_rib.h
index c325b3e3618c..085e507508be 100644
--- a/lib/rib/rte_rib.h
+++ b/lib/rib/rte_rib.h
@@ -17,6 +17,7 @@
#include <stdlib.h>
#include <stdint.h>
+#include <rte_byteorder.h>
#include <rte_common.h>
#ifdef __cplusplus
@@ -36,6 +37,9 @@ enum {
struct rte_rib;
struct rte_rib_node;
+/** If set, all user-facing IPv4 addresses are in network byte order */
+#define RTE_RIB_F_NETWORK_ORDER 1
+
/** RIB configuration structure */
struct rte_rib_conf {
/**
@@ -46,6 +50,7 @@ struct rte_rib_conf {
size_t ext_sz;
/* size of rte_rib_node's pool */
int max_nodes;
+ unsigned int flags; /**< Optional feature flags from RTE_RIB_F_* */
};
/**
--
2.54.0
^ permalink raw reply related
* Memory management BoF summary (DPDK Summit 2026)
From: Morten Brørup @ 2026-05-18 8:35 UTC (permalink / raw)
To: dev; +Cc: techboard, mattias.ronnblom
There are two ways of allocating/freeing memory objects in DPDK:
1. From the memory heap, i.e. rte_malloc() etc.
2. From a memory pool, i.e. rte_mempool_get() etc.
The current memory heap in DPDK has two issues:
- It is too slow to be used in the fast path.
- There may be contention between control threads and
EAL threads for the same spinlocks in the heap.
The memory pools in the DPDK has multiple issues:
- It is designed for fixed size objects,
so individual mempools must be allocated for each type of object.
(In theory, mempools could be allocated per object size,
and shared across modules.
But that would require coordination across modules, including
the total number of objects required by these modules,
the choice of underlying mempool driver,
and the optimal mempool cache size.)
- The number of objects in each mempool is fixed,
so applications have to size their mempools to worst case usage.
- The memory for the objects in a mempool is pre-allocated.
In summary, mempools consume significantly more memory than effectively
being used.
We discussed the need for a new memory heap system,
and converged on the following features:
- Providing functions to allocate and free memory objects of varying
size, like the rte_malloc library, but usable in the fast path.
- Perhaps also a need for bulk alloc/free functions,
like the rte_mempool library.
- Building on top of memzones.
- No dependency on the current DPDK heap, so it can cleanly replace it.
This is a stretch requirement.
- NUMA aware.
- Using slabs for various block sizes like in the Linux Kernel,
possibly with object size 2^N.
- Using per-lcore caches to reduce contention,
resembling the mempool per-lcore caches.
Mattias Rönnblom has implemented a prototype with the above properties,
and this was a good starting point for the discussion.
Discussion:
- It does not seem to be a requirement to be able to free the memory
used by the heap back to the memzones.
- It is possible having header-less objects.
However, if the headers are relatively small compared to the size of
the objects themselves, having a header may offer some benefits.
- The mempool library has implementation details optimizing for
spreading usage across memory channels, cache alignment, etc..
Such performance optimization details might need consideration.
- There is a lower limit to how small objects will be allocated.
E.g. objects smaller than the size of a pointer is unlikely.
- It is acceptable to return an object larger than the size requested,
e.g. returning an object of 16 bytes when 12 bytes were requested.
- Preventing false sharing of allocated objects between CPU caches
may not be necessary, but must be considered.
- Optimally, the new heap should replace the existing heap.
As seen with the timer wheel library previously proposed, replacing
core libraries in DPDK is difficult, so adding the new heap library
in parallel with the existing heap, i.e. with separate APIs, might be
a path of less resistance.
It will then be an application choice to use this new memory heap.
And long term, it can replace the existing heap, if appropriate.
Mbuf discussion:
- As a stretch goal, mbufs should be dynamically allocated from
the new heap, instead of being pre-allocated in mempools.
- This might be achievable either
- by replacing the mempool library with a wrapper to the heap, or
- by providing a new mempool driver using the heap, as an
alternative to the existing ring and stack mempool drivers.
- In this context, it is important to note that mbuf objects have some
pre-initialized fields when held in their respective mempools.
If allocating an mbuf directly from the heap, these fields must
somehow be initialized.
We did not discuss solutions for this, but noted that using the new
heap for mbufs should be considered in the design choices.
Venlig hilsen / Kind regards,
-Morten Brørup
^ permalink raw reply
* [PATCH dpdk v5] net/tap: use offsets provided by rte_net_get_ptype
From: Robin Jarry @ 2026-05-18 8:27 UTC (permalink / raw)
To: dev, Stephen Hemminger
In-Reply-To: <20260422133615.680318-2-rjarry@redhat.com>
Instead of guessing what are the proper header lengths, pass
a rte_net_hdr_lens struct to rte_net_get_ptype and use it to get the
proper header lengths/offsets in tap_verify_csum.
This allows supporting stacked VLAN/QinQ tags and IPv6 extensions.
Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
Notes:
v5:
* Fixed invalid L4 checksum verification when there are IPv6 extension headers.
v4:
* Fixed stale comments.
v3:
* Fixed double count for ipv6 extension headers.
v2:
* Restored packet length checks in IPv4/IPv6 before checking L4
checksums.
* Moved struct rte_net_hdr_lens variable next to rte_net_get_ptype call
and initialize it to 0 instead of calling memset().
drivers/net/tap/rte_eth_tap.c | 41 +++++++++++++----------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index a5d460a0b3cb..3b8e19afb7af 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -327,52 +327,41 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive, int persistent)
}
static void
-tap_verify_csum(struct rte_mbuf *mbuf)
+tap_verify_csum(struct rte_mbuf *mbuf, const struct rte_net_hdr_lens *hlen)
{
- uint32_t l2 = mbuf->packet_type & RTE_PTYPE_L2_MASK;
uint32_t l3 = mbuf->packet_type & RTE_PTYPE_L3_MASK;
uint32_t l4 = mbuf->packet_type & RTE_PTYPE_L4_MASK;
- unsigned int l2_len = sizeof(struct rte_ether_hdr);
- unsigned int l3_len;
+ uint32_t l4_off = hlen->l2_len + hlen->l3_len;
uint16_t cksum = 0;
void *l3_hdr;
void *l4_hdr;
- struct rte_udp_hdr *udp_hdr;
- if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
- l2_len += 4;
- else if (l2 == RTE_PTYPE_L2_ETHER_QINQ)
- l2_len += 8;
/* Don't verify checksum for packets with discontinuous L2 header */
- if (unlikely(l2_len + sizeof(struct rte_ipv4_hdr) >
- rte_pktmbuf_data_len(mbuf)))
+ if (unlikely(l4_off > rte_pktmbuf_data_len(mbuf)))
return;
- l3_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len);
+
+ l3_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, hlen->l2_len);
if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
struct rte_ipv4_hdr *iph = l3_hdr;
- l3_len = rte_ipv4_hdr_len(iph);
- if (unlikely(l2_len + l3_len > rte_pktmbuf_data_len(mbuf)))
- return;
/* check that the total length reported by header is not
* greater than the total received size
*/
- if (l2_len + rte_be_to_cpu_16(iph->total_length) >
+ if (hlen->l2_len + rte_be_to_cpu_16(iph->total_length) >
rte_pktmbuf_data_len(mbuf))
return;
- cksum = ~rte_raw_cksum(iph, l3_len);
+ cksum = ~rte_raw_cksum(iph, hlen->l3_len);
mbuf->ol_flags |= cksum ?
RTE_MBUF_F_RX_IP_CKSUM_BAD :
RTE_MBUF_F_RX_IP_CKSUM_GOOD;
} else if (l3 == RTE_PTYPE_L3_IPV6) {
struct rte_ipv6_hdr *iph = l3_hdr;
- l3_len = sizeof(struct rte_ipv6_hdr);
/* check that the total length reported by header is not
* greater than the total received size
*/
- if (l2_len + l3_len + rte_be_to_cpu_16(iph->payload_len) >
+ if (hlen->l2_len + sizeof(*iph) + rte_be_to_cpu_16(iph->payload_len) >
rte_pktmbuf_data_len(mbuf))
return;
} else {
@@ -386,20 +375,19 @@ tap_verify_csum(struct rte_mbuf *mbuf)
if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
int cksum_ok;
- const unsigned int l4_min_len = (l4 == RTE_PTYPE_L4_UDP)
- ? sizeof(struct rte_udp_hdr) : sizeof(struct rte_tcp_hdr);
/* Don't verify checksum if L4 header is truncated */
- if (l2_len + l3_len + l4_min_len > rte_pktmbuf_data_len(mbuf))
+ if (l4_off + hlen->l4_len > rte_pktmbuf_data_len(mbuf))
return;
- l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len + l3_len);
/* Don't verify checksum for multi-segment packets. */
if (mbuf->nb_segs > 1)
return;
+
+ l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l4_off);
if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
if (l4 == RTE_PTYPE_L4_UDP) {
- udp_hdr = (struct rte_udp_hdr *)l4_hdr;
+ struct rte_udp_hdr *udp_hdr = l4_hdr;
if (udp_hdr->dgram_cksum == 0) {
/*
* For IPv4, a zero UDP checksum
@@ -561,10 +549,11 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
continue;
}
- mbuf->packet_type = rte_net_get_ptype(mbuf, NULL,
+ struct rte_net_hdr_lens hlen = {0};
+ mbuf->packet_type = rte_net_get_ptype(mbuf, &hlen,
RTE_PTYPE_ALL_MASK);
if (rxq->rxmode->offloads & RTE_ETH_RX_OFFLOAD_CHECKSUM)
- tap_verify_csum(mbuf);
+ tap_verify_csum(mbuf, &hlen);
/* account for the receive frame */
bufs[num_rx++] = mbuf;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 1/2] bus/fslmc: fix ignored return value in fslmc_bus_unplug
From: Maxime Leroy @ 2026-05-18 8:16 UTC (permalink / raw)
To: Md Shofiqul Islam; +Cc: dev, hemant.agrawal, sachin.saxena, g.singh, stable
In-Reply-To: <20260513203725.1905-2-shofiqtest@gmail.com>
Hi Shofiqul,
On Sat, May 16, 2026 at 11:20 AM Md Shofiqul Islam <shofiqtest@gmail.com> wrote:
>
> fslmc_bus_unplug() called drv->remove() and discarded the return value,
> unconditionally clearing driver references and reporting success even
> when the remove callback signalled failure. As a result, callers had
> no way to detect or react to removal errors.
>
> Capture the return value and propagate it to the caller. Only clear
> the driver references and log successful unplug when the callback
> returns zero.
>
> Fixes: b5721f271cbf ("bus/fslmc: support DPNI hotplug")
> Bugzilla ID: 1914
> Cc: hemant.agrawal@nxp.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
> ---
> drivers/bus/fslmc/fslmc_bus.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
> index cf881b3eec..9cfd8b10ba 100644
> --- a/drivers/bus/fslmc/fslmc_bus.c
> +++ b/drivers/bus/fslmc/fslmc_bus.c
> @@ -620,7 +620,9 @@ fslmc_bus_unplug(struct rte_device *rte_dev)
> struct rte_dpaa2_driver *drv = dev->driver;
>
> if (drv && drv->remove) {
> - drv->remove(dev);
> + int ret = drv->remove(dev);
> + if (ret)
We log when the function successfully unplugs the driver, but there is
no log for error. Adding one could help here.
> + return ret;
Could you please declare ret at the top of the function to match the
existing style in this file (fslmc_bus_plug() just above, plus
scan_one_fslmc_device, rte_fslmc_parse, rte_fslmc_scan,
rte_fslmc_close all declare int ret in the local-variable block) ?
> dev->driver = NULL;
> dev->device.driver = NULL;
> DPAA2_BUS_INFO("%s Un-Plugged", dev->device.name);
Regards,
Maxime
^ permalink raw reply
* RE: [PATCH v2 1/2] bus/fslmc: fix ignored return value in fslmc_bus_unplug
From: Hemant Agrawal @ 2026-05-18 7:19 UTC (permalink / raw)
To: Md Shofiqul Islam, dev@dpdk.org; +Cc: stable@dpdk.org
In-Reply-To: <20260516110828.35701-2-shofiqtest@gmail.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
^ permalink raw reply
* RE: [PATCH v2 2/2] dma/dpaa2: fix dpaa2_qdma_remove always returning success
From: Hemant Agrawal @ 2026-05-18 7:19 UTC (permalink / raw)
To: Md Shofiqul Islam, dev@dpdk.org; +Cc: stable@dpdk.org
In-Reply-To: <20260516110828.35701-3-shofiqtest@gmail.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
^ permalink raw reply
* Re: [PATCH V2 00/15] power: unify and improve lcore ID verification
From: lihuisong (C) @ 2026-05-18 7:02 UTC (permalink / raw)
To: anatoly.burakov, sivaprasad.tummala, stephen
Cc: dev, thomas, fengchengwen, yangxingui, zhanjie9, lihuisong
In-Reply-To: <20260507024230.1198111-1-lihuisong@huawei.com>
Kindly ping for reivew.
/Huisong
On 5/7/2026 10:42 AM, Huisong Li wrote:
> This patch series reworks the lcore ID verification logic within the
> power library to ensure consistency and improve maintainability.
>
> Currently, various cpufreq drivers implement their own lcore ID checks,
> which are limited to simple range validation and involve significant
> code duplication. Moreover, these checks do not account for whether the
> core is actually managed by the application.
>
> For the verification in cpufreq-related APIs and power QoS APIs, although
> service cores do not typically invoke these APIs, they may operate in
> polling modes where power management is required. To maintain compatibility
> with applications using service cores, the validation logic now explicitly
> allows both ROLE_RTE and ROLE_SERVICE.
>
> But the lcore ID in the pmd_mgmt library must be ROLE_RTE because it is
> mainly used together with the data plane of ethdev PMD. So use
> rte_lcore_is_enabled to verify.
>
> Key Changes:
> 1. Add lcore role verification to individual cpufreq drivers.
> 2. Introduces a unified macro in the power library to standardize lcore ID
> checks.
> 3. Moves verification logic from individualdrivers to the framework level.
> This reduces code duplication.
> 4. Allow the service cores to configure power QoS.
> 5. Use rte_lcore_is_enabled to verfify the lcore ID in pmd_mgmt.
>
> ---
> v2:
> - Allow the service cores to set power API.
>
> ---
>
> Huisong Li (15):
> eal: add interface to check if lcore is EAL managed
> power/kvm_vm: validate lcore role in cpufreq API
> power/acpi_cpufreq: validate lcore role in cpufreq API
> power/amd_pstate: validate lcore role in cpufreq API
> power/cppc_cpufreq: validate lcore role in cpufreq API
> power/intel_pstate: validate lcore role in cpufreq API
> power: add a common macro to verify lcore ID
> power/cpufreq: add the lcore ID verification to framework
> power/acpi_cpufreq: remove the verification of lcore ID
> power/amd_pstate: remove the verification of lcore ID
> power/cppc_cpufreq: remove the verification of lcore ID
> power/intel_pstate: remove the verification of lcore ID
> power/kvm_vm: remove the verification of lcore ID
> power: allow the service core to config power QoS
> power: add lcore ID check for PMD mgmt
>
> drivers/power/acpi/acpi_cpufreq.c | 65 -------------------
> drivers/power/amd_pstate/amd_pstate_cpufreq.c | 65 -------------------
> drivers/power/cppc/cppc_cpufreq.c | 65 -------------------
> .../power/intel_pstate/intel_pstate_cpufreq.c | 65 -------------------
> drivers/power/kvm_vm/kvm_vm.c | 10 ---
> lib/eal/common/eal_common_lcore.c | 11 ++++
> lib/eal/include/rte_lcore.h | 11 ++++
> lib/power/power_common.h | 7 ++
> lib/power/rte_power_cpufreq.c | 14 +++-
> lib/power/rte_power_pmd_mgmt.c | 21 +++---
> lib/power/rte_power_qos.c | 10 +--
> 11 files changed, 55 insertions(+), 289 deletions(-)
>
^ permalink raw reply
* [PATCH] net/iavf: fix VF reset race and stale ARQ message handling
From: Talluri Chaitanyababu @ 2026-05-18 5:42 UTC (permalink / raw)
To: dev, bruce.richardson, aman.deep.singh
Cc: shaiq.wani, Talluri Chaitanyababu, stable
During VF reset, multiple issues can lead to initialization instability.
The first issue is a race condition in iavf_check_vf_reset_done(),
where VFR state VFACTIVE is treated as both "reset not started" and
"reset completed". This can allow the VF to proceed before the PF
has acknowledged the reset, resulting in inconsistent initialization.
The second issue is the presence of stale messages in the Admin
Receive Queue (ARQ) after VF reset. These may include opcode 0
(VIRTCHNL_OP_UNKNOWN) or responses to commands issued before reset,
which can interfere with API negotiation and cause command mismatch
errors.
Additionally, opcode 0 messages generate excessive warning logs,
causing unnecessary noise during initialization.
The solution involves:
1. Introducing a two-phase reset wait mechanism in
iavf_check_vf_reset_done():
- Wait for RSTAT to leave VFACTIVE to confirm reset start.
- Wait for RSTAT to return to VFACTIVE or COMPLETED to confirm
reset completion.
2. Draining stale ARQ messages after admin queue initialization
and before issuing commands in iavf_init_vf().
3. Downgrading opcode 0 message logging to DEBUG level while
preserving mismatch detection for other opcodes.
4. Refactoring reset-start detection and ARQ drain logic into
helper functions (iavf_wait_for_reset_start() and
iavf_drain_arq()) to improve readability and reuse.
This ensures reliable VF reset handling and stable initialization.
Fixes: 28a1a72eac26 ("net/iavf: add VF initiated reset")
Cc: stable@dpdk.org
Signed-off-by: Talluri Chaitanyababu <chaitanyababux.talluri@intel.com>
---
drivers/net/intel/iavf/iavf_ethdev.c | 54 ++++++++++++++++++++++++++++
drivers/net/intel/iavf/iavf_vchnl.c | 16 +++++++--
2 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 1eca20bc9a..692e4455dc 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2002,11 +2002,36 @@ iavf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id)
return 0;
}
+/* Wait until PF acknowledges VF reset (RSTAT leaves VFACTIVE) */
+static int
+iavf_wait_for_reset_start(struct iavf_hw *hw)
+{
+ int i;
+ uint32_t rstat;
+
+ for (i = 0; i < 100; i++) {
+ rte_delay_ms(10);
+
+ rstat = IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT);
+ rstat &= IAVF_VFGEN_RSTAT_VFR_STATE_MASK;
+ rstat >>= IAVF_VFGEN_RSTAT_VFR_STATE_SHIFT;
+
+ if (rstat != VIRTCHNL_VFR_VFACTIVE)
+ return 0;
+ }
+
+ return -1;
+}
+
static int
iavf_check_vf_reset_done(struct iavf_hw *hw)
{
int i, reset;
+ /* Phase 1: wait for reset to start (leave VFACTIVE) */
+ if (iavf_wait_for_reset_start(hw) != 0)
+ PMD_DRV_LOG(DEBUG, "VF reset did not start within timeout");
+
for (i = 0; i < IAVF_RESET_WAIT_CNT; i++) {
reset = IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT) &
IAVF_VFGEN_RSTAT_VFR_STATE_MASK;
@@ -2517,6 +2542,31 @@ iavf_init_proto_xtr(struct rte_eth_dev *dev)
}
}
+/* Drain stale Admin Receive Queue messages after reset */
+static void
+iavf_drain_arq(struct iavf_hw *hw, struct iavf_info *vf)
+{
+ struct iavf_arq_event_info event;
+ int drain_count = 0;
+
+ memset(&event, 0, sizeof(event));
+ event.msg_buf = vf->aq_resp;
+
+ while (drain_count < IAVF_AQ_LEN) {
+ event.buf_len = IAVF_AQ_BUF_SZ;
+
+ if (iavf_clean_arq_element(hw, &event, NULL) != IAVF_SUCCESS)
+ break;
+
+ drain_count++;
+ }
+
+ if (drain_count > 0)
+ PMD_INIT_LOG(DEBUG,
+ "Drained %d stale ARQ messages",
+ drain_count);
+}
+
static int
iavf_init_vf(struct rte_eth_dev *dev)
{
@@ -2558,6 +2608,10 @@ iavf_init_vf(struct rte_eth_dev *dev)
PMD_INIT_LOG(ERR, "unable to allocate vf_aq_resp memory");
goto err_aq;
}
+
+ /* Drain stale ARQ messages after VF reset */
+ iavf_drain_arq(hw, vf);
+
if (iavf_check_api_version(adapter) != 0) {
PMD_INIT_LOG(ERR, "check_api version failed");
goto err_api;
diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
index 08dd6f2d7f..a014be8b57 100644
--- a/drivers/net/intel/iavf/iavf_vchnl.c
+++ b/drivers/net/intel/iavf/iavf_vchnl.c
@@ -296,11 +296,21 @@ iavf_read_msg_from_pf(struct iavf_adapter *adapter, uint16_t buf_len,
__func__, vpe->event);
}
} else {
- /* async reply msg on command issued by vf previously */
+ /* Async reply for previously issued VF command.
+ * Stale messages from before reset are ignored, and polling
+ * continues until the expected response is received.
+ */
result = IAVF_MSG_CMD;
if (opcode != vf->pend_cmd) {
- PMD_DRV_LOG(WARNING, "command mismatch, expect %u, get %u",
- vf->pend_cmd, opcode);
+ if (opcode == VIRTCHNL_OP_UNKNOWN)
+ PMD_DRV_LOG(DEBUG,
+ "Ignoring stale msg (opcode 0), pending cmd %u",
+ vf->pend_cmd);
+ else
+ PMD_DRV_LOG(WARNING,
+ "command mismatch, expect %u, get %u",
+ vf->pend_cmd, opcode);
+
result = IAVF_MSG_ERR;
}
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 1/1] net/mana: add device reset support
From: Stephen Hemminger @ 2026-05-18 3:48 UTC (permalink / raw)
To: Wei Hu; +Cc: dev, longli, weh
In-Reply-To: <20260509060113.61686-2-weh@linux.microsoft.com>
On Fri, 8 May 2026 23:01:13 -0700
Wei Hu <weh@linux.microsoft.com> wrote:
> From: Wei Hu <weh@microsoft.com>
>
> Add support for handling hardware reset events in the MANA driver.
> When the MANA kernel driver receives a hardware service event, it
> initiates a device reset and notifies userspace via
> IBV_EVENT_DEVICE_FATAL. The DPDK driver handles this by performing
> an automatic teardown and recovery sequence.
>
> The reset flow has two phases. In the enter phase, running on the
> EAL interrupt thread, the driver transitions the device state,
> waits for data path threads to reach a quiescent state using RCU,
> stops queues, tears down IB resources, and frees per-queue MR
> caches. A control thread is then spawned to handle the exit phase:
> it waits for the hardware to recover, unregisters the interrupt
> handler, re-probes the PCI device, reinitializes MR caches, and
> restarts queues.
>
> A per-device spinlock serializes the reset path with ethdev
> operations. Operations that cannot wait (configure, queue setup)
> return -EBUSY during reset, while dev_stop and dev_close join the
> reset thread and use a blocking lock to ensure proper sequencing.
>
> Multi-process support is included: secondary processes unmap and
> remap doorbell pages via IPC during the reset enter and exit
> phases. Data path functions in both primary and secondary
> processes check the device state atomically and return early when
> the device is not active.
>
> The driver uses ethdev recovery events to notify upper layers
> (e.g. netvsc) of the reset lifecycle: RTE_ETH_EVENT_ERR_RECOVERING
> on entry, RTE_ETH_EVENT_RECOVERY_SUCCESS or
> RTE_ETH_EVENT_RECOVERY_FAILED on completion. A PCI device removal
> event callback distinguishes hot-remove from service reset.
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
Lots of issues found during AI review..
Subject: Re: [PATCH v2 1/1] net/mana: add device reset support
To: Wei Hu <weh@linux.microsoft.com>
Cc: dev@dpdk.org, longli@microsoft.com, weh@microsoft.com
In-Reply-To: <20260509060113.61686-2-weh@linux.microsoft.com>
References: <20260509060113.61686-1-weh@linux.microsoft.com>
<20260509060113.61686-2-weh@linux.microsoft.com>
On Fri, 8 May 2026 23:01:13 -0700, Wei Hu wrote:
> Add support for handling hardware reset events in the MANA driver.
Several issues need attention before this can be merged.
Errors
------
1. Memory leak: priv->dev_state_qsv is never freed on close.
In mana_probe_port (non-reset path) the qsbr buffer is allocated:
priv->dev_state_qsv = rte_zmalloc_socket("mana_rcu", sz, ...);
It is only freed in the probe-failure goto label. mana_dev_close
has no corresponding rte_free, so every successful close leaks
the allocation.
2. Joinable reset thread is leaked when reset completes on its own.
Both mana_reset_thread (skip path) and mana_reset_exit_delay
(normal completion) do:
priv->reset_thread_active = false;
rte_spinlock_unlock(&priv->reset_ops_lock);
return 0;
The pthread is still joinable at this point. If mana_dev_close
runs afterward it sees active==false and skips rte_thread_join,
leaving the TCB/stack unreaped. The flag must only be cleared by
whoever actually joins the thread; otherwise the reset thread
should be detached (and then dev_close cannot wait for it, which
is its own problem). Either way the current pattern is wrong.
3. Spinlock held across blocking operations.
priv->reset_ops_lock is rte_spinlock_t and is held across:
- mana_reset_enter: mana_dev_stop, mana_dev_close,
ibv_close_device,
mana_mp_req_on_rxtx (5s IPC timeout)
- mana_reset_exit_delay: ibv_close_device, mana_pci_probe,
mana_mp_req_on_rxtx, mana_dev_start
IPC with a 5-second timeout and device probe under a spinlock is
not acceptable. Use a sleeping mutex (pthread_mutex_t initialized
with PTHREAD_PROCESS_SHARED, since priv is in shared memory), or
split the lock so the long operations run outside it.
4. pthread_mutex_init / pthread_cond_init without PTHREAD_PROCESS_SHARED.
priv is allocated with rte_zmalloc_socket, so reset_cond_mutex and
reset_cond live in hugepage memory mapped into every secondary.
The patch initializes them with NULL attributes:
pthread_mutex_init(&priv->reset_cond_mutex, NULL);
pthread_cond_init(&priv->reset_cond, NULL);
Even if only the primary uses them today, placement in shared
memory requires the process-shared attribute. Otherwise behavior
is undefined and the choice will silently break the day a
secondary starts touching these fields.
Warnings
--------
5. Secondary data path has no synchronization with the doorbell unmap.
MANA_MP_REQ_RESET_ENTER causes the secondary MP handler to munmap
proc_priv->db_page and set it to NULL. The secondary's rx_burst /
tx_burst protect themselves only with an atomic state check:
rte_rcu_qsbr_thread_online(dstate_qsv, tid);
if (state != MANA_DEV_ACTIVE || !db_page) { ... return 0; }
But the qsbr only has primary threads registered (registration
happens in mana_dev_configure, which never runs in secondary), so
thread_online/offline in secondary do not block the primary's
qsbr_check. The MP handler in secondary therefore unmaps db_page
while a peer secondary lcore can still be inside rx/tx_burst with
a stale pointer. Result: SIGSEGV on the next doorbell write.
The secondary needs its own quiescence mechanism (a per-process
qsbr or a reader-side rwlock around the data path that the MP
handler acquires before unmap).
6. Return value of rte_dev_event_callback_unregister is discarded.
In mana_intr_uninstall:
if (dev->device)
rte_dev_event_callback_unregister(dev->device->name,
mana_pci_remove_event_cb,
priv);
The API returns -EAGAIN if the callback is currently executing.
When that happens the unregister silently fails, the callback
stays on the list, and any later free of priv produces a
use-after-free the next time the EAL device-event thread
dispatches.
7. PCI-remove callback depends on a monitor nobody starts.
rte_dev_event_callback_register only wires the callback into the
dispatch table; events are only delivered when something calls
rte_dev_event_monitor_start(). The patch never calls it. Whether
this works in practice depends on the application (testpmd,
netvsc) calling it first. At minimum document the requirement;
better, have the driver start the monitor on first probe and stop
it on last remove.
8. No documentation or release-note update.
This is a substantial new feature. doc/guides/rel_notes/ and
doc/guides/nics/mana.rst should both be updated to describe the
reset lifecycle, the RTE_ETH_EVENT_ERR_RECOVERING /
RECOVERY_SUCCESS / RECOVERY_FAILED behavior, and the dependency
on rte_dev_event_monitor_start.
9. priv->reset_thread_active is bool, accessed without atomic ops.
It is read in mana_dev_stop_lock and mana_dev_close_lock before
the spinlock is acquired (intentionally, to avoid deadlock with
the reset thread), and written from at least three threads (intr
handler, reset thread, dev_close). It should be RTE_ATOMIC(bool)
with explicit ordering. This compounds issue #2.
10. mana_intr_handler races with mana_pci_remove_event_cb on dev_state.
intr_handler does:
if (state == MANA_DEV_ACTIVE) {
rte_spinlock_lock(&priv->reset_ops_lock);
mana_reset_enter(priv); /* stores RESET_ENTER */
pci_remove_event_cb stores RESET_FAILED before taking the lock.
If intr_handler reads ACTIVE, pci_remove fires (sets
RESET_FAILED) while intr_handler is waiting on the lock,
intr_handler then acquires it and overwrites RESET_FAILED with
RESET_ENTER, and the driver attempts a reset on a device that
has already been physically removed. The state-set in
pci_remove_event_cb must occur under the lock, or the
intr_handler must re-check state after taking the lock.
Info
----
11. (void *)0 is used in mp.c (proc_priv->db_page = (void *)0; and
if (proc_priv->db_page != 0)). Use NULL.
12. The hand-off of reset_ops_lock from mana_reset_enter to
mana_reset_thread to mana_reset_exit_delay (released in the
latter) is hard to follow. A short comment block at
mana_reset_enter listing which functions take the lock and which
release it would help future maintainers.
13. The widened TOCTOU window in mana_tx_burst (db_page is now read
at the top of the function rather than just before the doorbell)
is not itself a bug, but it makes #5 worse. Once #5 is fixed
via a proper reader-side primitive, consider keeping the db_page
read inside the critical section.
^ permalink raw reply
* Re: [PATCH v2] net/hns3: fix L4 checksum incorrect for tunnel packets
From: Stephen Hemminger @ 2026-05-18 3:30 UTC (permalink / raw)
To: Xingui Yang
Cc: dev, david.marchand, fengchengwen, lihuisong, liuyonglong,
kangfenglong
In-Reply-To: <20260509064753.3292074-1-yangxingui@huawei.com>
On Sat, 9 May 2026 14:47:53 +0800
Xingui Yang <yangxingui@huawei.com> wrote:
> In HIP09 simple BD mode, the driver incorrectly calculates L4_START
> position for tunnel packets. The current implementation only uses
> l2_len + l3_len without considering outer header lengths (outer_l2_len
> and outer_l3_len), causing hardware to calculate checksum from wrong
> offset.
>
> For a typical NvGRE tunnel packet with structure:
> Outer Eth(14) + Outer IPv6(40) + NvGRE(8) + Inner Eth(14) +
> Inner IPv6(40) + TCP(20)
>
> Expected L4_START: 116 bytes (from packet start to inner TCP header)
> Actual calculation: 62 bytes (missing outer headers)
>
> This results in incorrect TCP/UDP checksum for all tunnel packets when
> using simple BD mode, including NvGRE, VxLAN, GRE, GENEVE tunnels.
>
> Fix by adding outer_l2_len and outer_l3_len to L4_START calculation
> when RTE_MBUF_F_TX_TUNNEL_MASK flag is set.
>
> Fixes: 6393fc0b823c ("net/hns3: simplify hardware checksum offloading")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
> drivers/net/hns3/hns3_rxtx.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index 573604b0cd..4d48cbdc11 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -3982,6 +3982,7 @@ hns3_handle_simple_bd(struct hns3_tx_queue *txq, struct hns3_desc *desc,
> {
> #define HNS3_TCP_CSUM_OFFSET 16
> #define HNS3_UDP_CSUM_OFFSET 6
> + uint32_t l4_start;
>
> /*
> * In HIP09, NIC HW support Tx simple BD mode that the HW will
> @@ -4003,9 +4004,13 @@ hns3_handle_simple_bd(struct hns3_tx_queue *txq, struct hns3_desc *desc,
> ((m->ol_flags & RTE_MBUF_F_TX_L4_MASK) == RTE_MBUF_F_TX_TCP_CKSUM ||
> (m->ol_flags & RTE_MBUF_F_TX_L4_MASK) == RTE_MBUF_F_TX_UDP_CKSUM)) {
> /* set checksum start and offset, defined in 2 Bytes */
> + l4_start = m->l2_len + m->l3_len;
Applied to next-net; moved declaration of l4_start into that basic block.
^ permalink raw reply
* Re: [PATCH dpdk v4] net/tap: use offsets provided by rte_net_get_ptype
From: Stephen Hemminger @ 2026-05-18 3:14 UTC (permalink / raw)
To: Robin Jarry; +Cc: dev
In-Reply-To: <20260512151611.186577-2-rjarry@redhat.com>
On Tue, 12 May 2026 17:16:12 +0200
Robin Jarry <rjarry@redhat.com> wrote:
> Instead of guessing what are the proper header lengths, pass
> a rte_net_hdr_lens struct to rte_net_get_ptype and use it to get the
> proper header lengths/offsets in tap_verify_csum.
>
> This allows supporting stacked VLAN/QinQ tags and IPv6 extensions.
>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---
AI patch review
Short summary suitable for replying to the patch:
The v4 patch claims to add IPv6 extension header support, but the L4
checksum path is broken for that case. The patch matches both IPV6
and IPV6_EXT in the L3 block and then falls through to
rte_ipv6_udptcp_cksum_verify(), which is documented as not supporting
extension headers. Concretely, that helper uses ipv6_hdr->proto for
the pseudo-header (which is the first extension-header type, not the
L4 protocol) and uses ipv6_hdr->payload_len as the raw-cksum length
(which over-reads past the L4 data by the size of the extensions).
Result: valid TCP/UDP over IPv6+ext packets get tagged
RX_L4_CKSUM_BAD, contradicting the commit message.
Suggested fix: keep IPV6_EXT in the L3 sanity-check block, but skip
L4 verification when l3 == RTE_PTYPE_L3_IPV6_EXT until a helper that
handles extension headers exists.
^ permalink raw reply
* Re: [PATCH v3 0/7] net/netvsc: fix VF hotplug and service reset handling
From: Stephen Hemminger @ 2026-05-18 2:59 UTC (permalink / raw)
To: Long Li; +Cc: dev, Wei Hu
In-Reply-To: <20260515192843.552762-1-longli@microsoft.com>
On Fri, 15 May 2026 12:28:34 -0700
Long Li <longli@microsoft.com> wrote:
> This series fixes several issues in the netvsc PMD's VF hot-plug retry
> logic and adds support for MANA service reset (suspend/resume) recovery.
>
> Patches 1-5 fix the VF hot-add retry path to handle Azure-specific
> timing issues: slow MANA driver probe (>100s), udev interface renames,
> asynchronous mana_ib registration, and multi-NIC staggered VF
> appearance.
>
> Patch 6 fixes per-queue stats forwarding from VF to netvsc.
>
> Patch 7 adds recovery event handling for MANA service resets, where
> the kernel suspends/resumes the VF without PCI remove.
>
> v3:
> - Patch 1: wrapped rte_eal_alarm_set lines to fix checkpatch
> line-length warning
> - Patch 4: changed "retry loop exiting" log from NOTICE to DEBUG
> to avoid noise on every successful VF re-attach
> - Patch 6: removed dead -ENOTSUP fallback to rte_eth_stats_get,
> replaced with direct -ENOTSUP return; documented caller contract
> for zeroed buffers
> - Patch 7: deferred all recovery callbacks via rte_eal_alarm_set
> consistent with INTR_RMV pattern; dropped unlocked vf_attached
> guard in recovery_failed; cancel new alarms in hn_vf_close
>
> v2:
> - Patch 1: added comment explaining why indefinite retry is safe
> - Patch 2: changed SIOCGIFHWADDR retry log to DEBUG
> - Patch 3: restored ERR log for non-ENODEV/EEXIST failures
> - Patch 4: changed per-iteration logs from NOTICE to DEBUG;
> used RTE_ETHER_ADDR_PRT_FMT macros
> - Patch 5: fixed commit message (limit 120 not 30); changed
> mac_retry log to DEBUG; explicit NULL comparisons
> - Patch 6: added comment for direct dev_ops call; added -ENOTSUP
> fallback
> - Patch 7: added dev_started check in recovery_success; added
> vf_attached guard in recovery_failed
>
> Long Li (7):
> net/netvsc: retry VF hotplug indefinitely until PCI device disappears
> net/netvsc: retry on SIOCGIFHWADDR failure during VF hotplug
> net/netvsc: retry full probe when IB device not ready during hotplug
> net/netvsc: add debug logging for VF hotplug retry
> net/netvsc: retry when no matching MAC found in net directory
> net/netvsc: forward per-queue stats from VF device
> net/netvsc: handle VF recovery events for service reset
>
> drivers/net/netvsc/hn_ethdev.c | 142 +++++++++++++++++++++----
> drivers/net/netvsc/hn_var.h | 1 +
> drivers/net/netvsc/hn_vf.c | 182 ++++++++++++++++++++++++++++++++-
> 3 files changed, 302 insertions(+), 23 deletions(-)
>
Applied to next-net
^ permalink raw reply
* Re: [PATCH] ethdev: fix pointer check in GENEVE and RAW flow copy
From: Stephen Hemminger @ 2026-05-18 2:45 UTC (permalink / raw)
To: Denis Lyulin
Cc: Ori Kam, Thomas Monjalon, Andrew Rybchenko, Ferruh Yigit,
Michael Baum, dev, stable, adrien.mazarguil
In-Reply-To: <20260507112012.119140-1-lyulin.2003@mail.ru>
On Thu, 7 May 2026 14:20:11 +0300
Denis Lyulin <lyulin.2003@mail.ru> wrote:
> When rte_flow_conv_item_spec() is called from rte_flow_conv_pattern(),
> the spec, last and mask pointers are checked separately. If the API
> is used incorrectly, the spec pointer may be NULL while last and mask
> may be valid pointers.
>
> In rte_flow_conv_item_spec() for GENVE_OPT and RAW item types the spec
> pointer is used even if the function is called to copy last or mask.
> It may cause a NULL pointer (spec) dereference.
>
> This commit adds extra check of item->spec and if it is NULL, does not
> copy further data relying on it
>
> Fixes: 841a0445442d ("ethdev: fix GENEVE option item conversion")
> Cc: michaelba@nvidia.com
> Cc: adrien.mazarguil@6wind.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Denis Lyulin <lyulin.2003@mail.ru>
> ---
AI spotted a couple issues here, please address.
Warnings
========
* lib/ethdev/rte_flow.c: RAW case guard is overly broad and regresses
the LAST-without-spec path.
The new wrapper `if (spec.raw && last.raw) { ... } else { tmp = 0; }`
is too coarse. The original code only dereferences spec.raw on the
SPEC and MASK paths:
if (type == RTE_FLOW_CONV_ITEM_SPEC || // uses spec, mask
(type == RTE_FLOW_CONV_ITEM_MASK && // uses spec, last, mask
((spec.raw->length & mask.raw->length) >=
(last.raw->length & mask.raw->length))))
tmp = spec.raw->length & mask.raw->length;
else
tmp = last.raw->length & mask.raw->length; // uses last, mask only
For type == RTE_FLOW_CONV_ITEM_LAST the else branch is taken and
spec.raw is never touched. With the patch, when an item has
item->last set but item->spec == NULL, the wrapper short-circuits
to tmp = 0 and the `last` pattern is silently dropped (dst.raw->
pattern stays NULL after the struct-initializer memcpy). The
original code copied it correctly.
rte_flow_conv_pattern() calls this function with type=LAST whenever
src->last is non-NULL, regardless of src->spec, so this path is
reachable through the public conv API.
Note also that mask.raw is guaranteed non-NULL by the
`item->mask ? item->mask : &rte_flow_item_raw_mask` fallback two
lines above, so it does not need to participate in the guard.
A more surgical guard preserves the LAST behaviour, e.g.:
if (type == RTE_FLOW_CONV_ITEM_SPEC && spec.raw)
tmp = spec.raw->length & mask.raw->length;
else if (type == RTE_FLOW_CONV_ITEM_MASK && spec.raw && last.raw &&
((spec.raw->length & mask.raw->length) >=
(last.raw->length & mask.raw->length)))
tmp = spec.raw->length & mask.raw->length;
else if (last.raw)
tmp = last.raw->length & mask.raw->length;
else
tmp = 0;
Info
====
* The commit message attributes the bug only to calls from
rte_flow_conv_pattern(), but the more directly reachable path is
rte_flow_conv() with RTE_FLOW_CONV_OP_ITEM_MASK (rte_flow.c around
line 1144), which validates only item->mask and leaves item->spec
potentially NULL before invoking rte_flow_conv_item_spec() with
type=MASK. Worth mentioning so reviewers can locate the trigger.
* The GENEVE_OPT hunk uses spec.geneve_opt->option_len to size the
copy of src.geneve_opt->data, even when src is the mask (type=MASK)
or the last (type=LAST). This pre-existing assumption (that
spec.option_len describes the layout for all three) is preserved by
the patch and is out of scope here, but worth a sanity check that
the spec.option_len fallback to 0 (no copy) is the desired
behaviour when only a mask is supplied via OP_ITEM_MASK.
^ permalink raw reply
* Re: [PATCH v3 1/3] net/zxdh: optimize queue structure to improve performance
From: Stephen Hemminger @ 2026-05-18 2:20 UTC (permalink / raw)
To: Junlong Wang; +Cc: dev
In-Reply-To: <20260509062930.3254766-2-wang.junlong1@zte.com.cn>
On Sat, 9 May 2026 14:29:27 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:
> Reorganize structure fields for better cache locality.
> Remove RX software ring (sw_ring) to reduce memory allocation and
> copy.
>
> Signed-off-by: Junlong Wang <wang.junlong1@zte.com.cn>
> ---
Looks good.
Some AI comments:
[PATCH v3 1/3] net/zxdh: optimize queue structure to improve performance
Warning: silent bug fix
This patch quietly fixes a real bug in zxdh_queue_enable_intr(). Upstream has:
static inline void
zxdh_queue_enable_intr(struct zxdh_virtqueue *vq)
{
if (vq->vq_packed.event_flags_shadow == ZXDH_RING_EVENT_FLAGS_DISABLE) {
vq->vq_packed.event_flags_shadow = ZXDH_RING_EVENT_FLAGS_DISABLE;
vq->vq_packed.ring.driver->desc_event_flags = vq->vq_packed.event_flags_shadow;
}
}
The function checks for DISABLE and then sets to DISABLE — interrupts are never enabled. The patch corrects both occurrences to ENABLE. That fix is not mentioned in the commit message, and it has no Fixes: tag or Cc: stable@dpdk.org. Please split it into its own patch ahead of the structure reorganization so it can be backported.
The commit message also omits other non-trivial changes: removal of zxdh_mb(), and the inlining of zxdh_queue_notify() so it no longer dispatches through ZXDH_VTPCI_OPS()->notify_queue. Worth a sentence each.
^ permalink raw reply
* Re: [PATCH v3 3/3] net/zxdh: optimize Tx xmit pkts performance
From: Stephen Hemminger @ 2026-05-18 2:22 UTC (permalink / raw)
To: Junlong Wang; +Cc: dev
In-Reply-To: <20260509062930.3254766-4-wang.junlong1@zte.com.cn>
On Sat, 9 May 2026 14:29:29 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:
> Add simple Tx xmit functions (zxdh_xmit_pkts_simple)
> for single-segment packet xmit.
>
> Signed-off-by: Junlong Wang <wang.junlong1@zte.com.cn>
> ---
Some AI review feedback.
PATCH v3 3/3] net/zxdh: optimize Tx xmit pkts performance
Error: NULL pointer dereference in pkt_padding()
hdr = (struct zxdh_net_hdr_dl *)rte_pktmbuf_prepend(cookie, hdr_len);
rte_memcpy(hdr, net_hdr_dl, hdr_len);
rte_pktmbuf_prepend() returns NULL when headroom is insufficient. The existing zxdh_xmit_pkts_packed() path guards its push fast-path with txm->data_off >= ZXDH_DL_NET_HDR_SIZE and falls back to the indirect path when that fails. The simple Tx path has no such guard; any mbuf submitted with headroom < hw->dl_net_hdr_len will crash here.
Add a NULL check, or screen mbufs with insufficient headroom in zxdh_xmit_pkts_simple() before calling submit_to_backend_simple().
Error: out-of-bounds read in zxdh_xmit_pkts_simple() stats loop
if ((vq->vq_avail_idx + nb_pkts) >= vq->vq_nentries) {
nb_tx = vq->vq_nentries - vq->vq_avail_idx;
nb_tx_left = nb_pkts - nb_tx;
submit_to_backend_simple(vq, tx_pkts, nb_tx);
...
tx_pkts += nb_tx;
}
if (nb_tx_left) {
submit_to_backend_simple(vq, tx_pkts, nb_tx_left);
...
}
zxdh_queue_notify(vq);
txvq->stats.packets += nb_pkts;
for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++)
zxdh_update_packet_stats(&txvq->stats, tx_pkts[nb_tx]);
When the ring wraps within a burst, tx_pkts is advanced past the first chunk. The stats loop then walks nb_pkts entries from the advanced pointer, reading past the end of the caller's mbuf array. The first chunk's per-packet stats are also skipped.
Use a separate cursor for the submit calls and iterate the stats loop over the original tx_pkts, or accumulate per-packet stats inside each submit step.
^ permalink raw reply
* Re: [PATCH v15 00/11] net/sxe2: fix logic errors and address feedback
From: Stephen Hemminger @ 2026-05-18 0:02 UTC (permalink / raw)
To: liujie5; +Cc: dev
In-Reply-To: <20260516074618.2343883-1-liujie5@linkdatatechnology.com>
On Sat, 16 May 2026 15:46:07 +0800
liujie5@linkdatatechnology.com wrote:
> From: Jie Liu <liujie5@linkdatatechnology.com>
>
> This patch set addresses the feedback received on the v10 submission
> for the sxe2 PMD. The primary focus is on fixing vector path selection,
> ensuring memory safety during mbuf initialization, and cleaning up
> redundant logic in the configuration functions.
Driver is in much better shape now.
AI is still finding a few things.
Review of [PATCH v15 00/11] sxe2 PMD
====================================
Overall comments
----------------
Most of the issues called out in the v13 review have been
addressed in v15:
- Inverted "if (!ret)" error checks in sxe2_dev_pci_map_init()
are now "if (ret)".
- Non-ASCII characters in sxe2_net_map_addr_info_pf[] removed.
- rte_ticketlock_t around blocking ioctl() replaced with
pthread_mutex_t.
- Driver-private u8/s8/u16/... typedefs, __le*/__be* aliases,
sxe2_errno.h, STATIC macro, and the reinvented
LIST_FOR_EACH_ENTRY / COMPILER_BARRIER / sxe2_*_lock /
udelay/mdelay/msleep / __iomem / IS_ERR helpers are gone.
- "MTU update = Y" claim in the features matrix dropped;
"Free Tx mbuf on demand = Y" is now backed by the new
.tx_done_cleanup callback added in patch 11.
- Redundant queue_idx bounds checks at the top of
rx_queue_setup / tx_queue_setup removed.
- "reseted" log-message typo fixed.
What remains:
Patch 11/11: net/sxe2: implement Tx done cleanup
------------------------------------------------
Error: NULL pointer dereference of tx_queue parameter.
sxe2_tx_done_cleanup() dereferences txq before checking it for
NULL:
int32_t sxe2_tx_done_cleanup(void *tx_queue, uint32_t free_cnt)
{
struct sxe2_tx_queue *txq = (struct sxe2_tx_queue *)tx_queue;
struct sxe2_adapter *adapter = txq->vsi->adapter; /* segfault if txq == NULL */
int32_t ret;
if (txq == NULL) { /* too late */
ret = 0;
goto l_end;
}
...
The NULL check must run before any field access:
struct sxe2_tx_queue *txq = tx_queue;
struct sxe2_adapter *adapter;
if (txq == NULL)
return 0;
adapter = txq->vsi->adapter;
...
Patch 03/10 -> 09/10: vector mode probe gated on RTE_PROC_PRIMARY
-----------------------------------------------------------------
Warning: sxe2_rx_mode_func_set() runs the vector capability probe
only in the primary process:
if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
ret = sxe2_rx_vec_support_check(dev, &vec_flags);
...
}
but sxe2_dev_init() calls sxe2_rx_mode_func_set(dev) and
sxe2_tx_mode_func_set(dev) from the secondary-process branch as
well. In a secondary, the vector probe is skipped, rx_mode_flags
stays 0, and dev->rx_pkt_burst lands on
sxe2_rx_pkts_scattered{,_split}. Since dev->rx_pkt_burst is
per-rte_eth_dev and re-written by whichever process attaches
last, the effective Rx burst mode depends on attach ordering.
The same pattern was flagged in v13 and is unchanged in v15.
Either run the same probe in secondaries, or share the primary's
decision through rte_eth_dev_data so secondaries inherit it.
Patch 03/10: common/sxe2: add sxe2 basic structures
---------------------------------------------------
Warning: macro identifier BITS_TO_uint32_t in sxe2_osal.h. This
is the visible result of a wholesale "u32 -> uint32_t" rename
applied to identifier text without manual review. The macro is
unused in the rest of the driver (the only mentions are the
definition itself and a later rename of the right-hand side),
so the cleanest fix is to delete it. If a "bits to 32-bit-word
count" macro is wanted later, name it BITS_TO_U32 or use
RTE_DIM/RTE_ALIGN_MUL_CEIL idioms.
Patch 09/11: drivers: add data path for Rx and Tx
-------------------------------------------------
Info: in sxe2_tx_pkts() the exit chain remains
l_exit_logic:
if (tx_num == 0)
goto l_end;
goto l_end_of_tx;
l_end_of_tx:
SXE2_PCI_REG_WRITE_WC(...);
The unconditional "goto l_end_of_tx;" immediately before the
"l_end_of_tx:" label is dead — fall through. The same pattern
appears in sxe2_rx_mode_func_set() at the end:
dev->rx_pkt_burst = sxe2_rx_pkts_scattered;
goto l_end;
l_end:
PMD_LOG_DEBUG(...);
Both can simply be deleted.
Series hygiene
--------------
Info: several identifiers are introduced with typos in one patch
and corrected in a later patch within the same series:
- sxe2_commoin_inited (patch 03) -> sxe2_common_inited (patch 05)
- sxe2_drv_dev_handshark (patch 03) -> sxe2_drv_dev_handshke
(patch 04). The renamed form is still a typo (missing 'a'
between 'h' and 'k'). Log strings around it were corrected
to "handshake" in patch 05; the function name was not.
- Dead "if (ret != 0)" after two void-returning
sxe2_{rx,tx}_mode_func_set() calls is added in patch 08 and
removed again in patch 09.
Each commit needs to be independently correct so that "git
bisect" lands on meaningful states. Renaming an exported
internal symbol (RTE_EXPORT_INTERNAL_SYMBOL(sxe2_drv_dev_handshke))
mid-series also churns the auto-generated version map. Please
collapse these into the patches that introduce the names, and
fix sxe2_drv_dev_handshke -> sxe2_drv_dev_handshake.
^ permalink raw reply
* Re: [PATCH v4 05/20] net/txgbe: fix inaccuracy in Tx rate limiting
From: Stephen Hemminger @ 2026-05-17 23:40 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-6-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:47 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> Amber-lite NIC's TX rate limiting has large deviations for small
> packets. To address this, the following changes are made:
>
> 1. Set TDM_RL_ADJ (0x1820c) to 21B (includes 7B Ethernet preamble,
> 1B SFD, 1B EFD, and 12B IPG).
> 2) Remove the rate offset in the driver (e.g., 105 / 100, a rough
> compensation value from Linux kernel driver tests).
>
> After these changes, accuracy deviation for 64B packets is within
> ~5%, while large packets show lower deviation.
>
> Fixes: a309ab43acf3 ("net/txgbe: support Tx queue rate limiting for Amber-Lite")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
AI feedback on this one. Looks ok as is but worth looking at.
# DPDK Patch Review: net/txgbe: fix inaccuracy in Tx rate limiting
## Summary
This patch improves TX rate limiting accuracy for Amber-lite NIC by:
1. Setting TDM_RL_ADJ register to 21B (Ethernet framing overhead)
2. Removing the 105/100 compensation factor
## Review Findings
### **Errors**
None found.
### **Warnings**
**1. TDM_RL_ADJ write placement causes per-queue reconfiguration**
The `TXGBE_TDM_RL_ADJ` register write is inside the per-queue rate limit configuration loop, causing the same global register to be written every time any queue's rate limit is set. This register appears to be a global hardware adjustment parameter (affects all queues), not a per-queue setting.
**Suggested fix:** Move the register write outside the queue configuration context, ideally to device initialization or as a one-time setup when rate limiting is first enabled:
```c
/* Set once during device init or first rate limit config */
static bool rl_adj_initialized = false;
if (!rl_adj_initialized) {
wr32(hw, TXGBE_TDM_RL_ADJ, 21);
rl_adj_initialized = true;
}
wr32(hw, TXGBE_TDM_RL_QUEUE_IDX, queue_idx);
```
Or better yet, move it to a device initialization function if this is a hardware parameter that should be set once at device start.
**2. Magic number 21 lacks explanation**
The value 21 is documented in the commit message but not in the code. Future maintainers will not see the breakdown (7B preamble + 1B SFD + 1B EFD + 12B IPG).
**Suggested fix:** Add a comment or define:
```c
/* Ethernet framing overhead: 7B preamble + 1B SFD + 1B EFD + 12B IPG */
#define TXGBE_TDM_RL_ADJ_OVERHEAD 21
wr32(hw, TXGBE_TDM_RL_ADJ, TXGBE_TDM_RL_ADJ_OVERHEAD);
```
**3. Missing release notes update**
This is a fix that changes hardware behavior and improves accuracy. Users may observe different TX rates after upgrading. The change should be documented in release notes to inform users of the accuracy improvement.
**Suggested fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or current release):
```rst
* **net/txgbe: Improved TX rate limiting accuracy for Amber-lite NIC.**
Fixed TX rate limiting accuracy for 64-byte packets from ~20% deviation
to within ~5% by correcting hardware framing overhead configuration.
```
### **Info**
**1. Consider validating tx_rate is non-zero**
The code computes `factor_int = link_speed / tx_rate`. If `tx_rate` is somehow zero (due to caller error or invalid input), this would cause division by zero. The existing code may have validation elsewhere, but defensive programming would check this.
```c
if (tx_rate == 0) {
PMD_DRV_LOG(ERR, "TX rate cannot be zero");
return -EINVAL;
}
```
**2. Variable scope: factor_fra could be declared at initialization**
The variable `factor_fra` is declared but not initialized, then assigned in both branches of the conditional. C99 style would allow:
```c
u32 factor_fra = (frac * 16384) / 10000;
if (factor_fra >= 8192)
factor_fra = 0;
```
This is stylistic only; the existing code is correct.
^ permalink raw reply
* Re: [PATCH v4 17/20] net/txgbe: fix get module info operation
From: Stephen Hemminger @ 2026-05-17 23:53 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-18-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:59 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The original I2C access flow in the module information retrieval
> process was flawed. Correct the implementation to properly fetch
> module info.
>
> Fixes: abf042d32b39 ("net/txgbe: add Amber-Lite 25G/40G NICs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
More errors spotted in AI review
=== Patch 17/20 ===
# DPDK Patch Review: net/txgbe: fix get module info operation
## Summary
This patch refactors the module information retrieval code for the txgbe driver. The changes add proper identifier detection for different SFP/QSFP module types and improve the I2C access flow.
---
## ERRORS
### 1. Resource leak on error path after lock acquisition
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Lines 5368-5382
The code acquires a software semaphore (`TXGBE_MNGSEM_SWPHY`) at line 5368 but returns directly with `-EIO` at line 5382 without releasing the lock.
```c
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
if (status)
return -EBUSY;
if (hw->mac.type == txgbe_mac_aml40) {
status = hw->phy.read_i2c_sff8636(hw, 0,
TXGBE_SFF_IDENTIFIER,
&identifier);
} else {
status = hw->phy.read_i2c_eeprom(hw,
TXGBE_SFF_IDENTIFIER,
&identifier);
}
if (status != 0)
goto ERROR_IO; /* GOOD - releases lock */
```
However, the early returns at lines 5358-5365 before the lock is acquired would bypass the lock entirely if the pre-checks fail. While these don't leak the lock, they create an inconsistent state-checking flow.
**Suggested fix:** Move the presence checks after the lock acquisition to ensure consistent locking semantics:
```c
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
if (status)
return -EBUSY;
/* Check module presence after acquiring lock */
if (hw->mac.type == txgbe_mac_aml40) {
value = rd32(hw, TXGBE_GPIOEXT);
if (value & TXGBE_SFP1_MOD_PRST_LS) {
status = -EIO;
goto ERROR_IO;
}
}
if (hw->mac.type == txgbe_mac_aml) {
value = rd32(hw, TXGBE_GPIOEXT);
if (value & TXGBE_SFP1_MOD_ABS_LS) {
status = -EIO;
goto ERROR_IO;
}
}
```
---
## WARNINGS
### 1. Implicit comparison with zero
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Multiple occurrences (lines 5383, 5389, 5399, 5424)
The code uses `if (status != 0)` consistently, which is correct. However, there is one instance that needs verification:
```c
if (status)
return -EBUSY;
```
**Suggested fix:** For consistency with the rest of the function:
```c
if (status != 0)
return -EBUSY;
```
### 2. Mixed type usage: `u8` vs `uint8_t`
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Lines 5350-5351, others use `uint8_t`
```c
u8 identifier = 0;
u8 sff8636_rev = 0;
```
While `u32 value;` appears at line 5353, the rest of the function uses `uint8_t` and `uint32_t`. DPDK coding standards prefer explicit-width types with the `_t` suffix.
**Suggested fix:**
```c
uint8_t identifier = 0;
uint8_t sff8636_rev = 0;
uint32_t value;
```
### 3. Unnecessary variable initialization
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Lines 5350-5351
```c
u8 identifier = 0;
u8 sff8636_rev = 0;
```
Both variables are assigned before use via the `read_i2c_*` functions. Initializing to 0 defeats uninitialized-variable warnings.
**Suggested fix:**
```c
uint8_t identifier;
uint8_t sff8636_rev;
```
### 4. Release notes required
This patch fixes a bug (I2C access flow) and should be documented in the current release notes under "Fixed Issues" for the txgbe driver, per the contribution guidelines for bug fixes.
---
## INFO
### 1. Code organization improvement
The new code structure with a switch statement based on `identifier` is clearer than the original logic. This is a good refactoring that makes the code easier to maintain.
### 2. Error message clarity
The error messages "I2C IO ERROR" and "SFF Module Type not recognized" are informative. Consider whether these should include additional context (e.g., the identifier value for unrecognized types) to aid debugging.
---
## POSITIVE OBSERVATIONS
1. **Proper lock cleanup:** The `ERROR_IO` label correctly releases the semaphore before returning, which is the right pattern for error handling with acquired resources.
2. **Consistent error propagation:** The function properly returns `-EIO`, `-EINVAL`, and `-EBUSY` based on the failure type.
3. **Cc: stable@dpdk.org included:** Appropriate for a bug fix that should be backported.
---
## SUMMARY OF REQUIRED CHANGES
**Must fix (Errors):**
1. Move module presence checks after lock acquisition to ensure lock is held during all hardware access and properly released on all error paths
**Should fix (Warnings):**
1. Use explicit comparison `status != 0` instead of implicit `if (status)`
2. Use `uint8_t` and `uint32_t` instead of `u8` and `u32`
3. Remove unnecessary initialization of `identifier` and `sff8636_rev`
4. Add release notes entry for this bug fix
============================================================
^ permalink raw reply
* Re: [PATCH v4 18/20] net/txgbe: fix get EEPROM operation
From: Stephen Hemminger @ 2026-05-17 23:54 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-19-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:36:00 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The original I2C access flow in the module information retrieval
> process was flawed. Correct the implementation to properly fetch
> module info.
>
> Fixes: abf042d32b39 ("net/txgbe: add Amber-Lite 25G/40G NICs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
I agree with AI that u8, u16 should not be used except in base/ code.
# Review of Patch 18/20: net/txgbe: fix get EEPROM operation
## Errors
### 1. Wrong integer types used for kernel/hardware types
The code uses kernel-style type names (`u32`, `u8`, `u16`) instead of standard C types or DPDK types. This is inconsistent with DPDK coding standards which require standard C types.
**Location:**
```c
u32 value;
u8 identifier = 0;
u16 offset;
u8 page = 0;
```
**Fix:** Use standard C types:
```c
uint32_t value;
uint8_t identifier = 0;
uint16_t offset;
uint8_t page = 0;
```
### 2. Resource leak on error path
The function acquires a semaphore with `hw->mac.acquire_swfw_sync()` but fails to release it if `info->length == 0` check fails.
**Location:**
```c
if (info->length == 0)
return -EINVAL;
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
```
**Fix:** The `info->length == 0` check should be moved before the semaphore acquisition, OR the error path should release the semaphore:
```c
if (info->length == 0)
return -EINVAL;
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
if (status)
return -EBUSY;
```
This is actually correct as-is since the check happens before acquisition. **NOT AN ERROR.**
### 3. Implicit comparison for boolean
```c
if (identifier == TXGBE_SFF_IDENTIFIER_SFP)
is_sfp = true;
```
This uses an integer comparison to set a boolean, which is fine. However, the boolean `is_sfp` is used directly in conditionals later, which is acceptable for `bool` types. **NOT AN ERROR.**
## Warnings
### 1. Missing bounds check on page calculation
The `page` variable is incremented in a loop without any upper bound check. While the loop is bounded by `info->length`, there's no verification that `page` doesn't exceed valid page numbers.
**Location:**
```c
while (offset >= RTE_ETH_MODULE_SFF_8436_LEN) {
offset -= RTE_ETH_MODULE_SFF_8436_LEN / 2;
page++;
}
```
**Suggestion:** Add bounds check on page number before use, or verify that `info->length` cannot cause page overflow:
```c
while (offset >= RTE_ETH_MODULE_SFF_8436_LEN) {
offset -= RTE_ETH_MODULE_SFF_8436_LEN / 2;
page++;
}
if (page > MAX_VALID_PAGE) {
status = -EINVAL;
goto ERROR_IO;
}
```
### 2. Buffer initialization redundancy
`memset(data, 0, info->length)` clears the buffer, but then every byte is written in the loop. The memset may be needed for partial reads on error, but the error path returns without partial data, making this initialization unnecessary overhead.
**Location:**
```c
memset(data, 0, info->length);
for (i = info->offset; i < info->offset + info->length; i++) {
...
data[i - info->offset] = databyte;
}
```
**Consideration:** If partial reads are acceptable (keeping what was read before error), the memset is appropriate. Otherwise, it's unnecessary.
### 3. Inconsistent coding style with bool keyword
The patch uses C99 `bool` type (`bool is_sfp = false`), which is good practice per guidelines. However, verify this is consistently available (requires `<stdbool.h>` which should be included).
### 4. Error path could leak partial data
On error, the function returns without clearing the potentially partially-filled buffer. If the caller expects zeroed data on error, this could be an information leak (though likely harmless in this context).
## Info
### 1. Magic number in condition
```c
if (page == 0 || !(data[0x2] & 0x4))
```
The `0x2` offset and `0x4` bitmask are magic numbers that should be defined as named constants for clarity.
**Suggestion:**
```c
#define SFF8636_STATUS_OFFSET 0x2
#define SFF8636_FLAT_MEM_BIT 0x4
if (page == 0 || !(data[SFF8636_STATUS_OFFSET] & SFF8636_FLAT_MEM_BIT))
```
### 2. Potential integer overflow in loop bound
```c
for (i = info->offset; i < info->offset + info->length; i++)
```
If `info->offset + info->length` overflows (though unlikely with typical EEPROM sizes), the loop condition becomes incorrect. Consider bounds validation before the loop.
---
## Summary
**Critical issues:** 1 (wrong type names - must use C standard types)
**Important warnings:** 2 (missing page bounds check, potential partial data exposure)
**Style/clarity issues:** 2 (magic numbers, redundant memset)
The most important fix is changing the type declarations from `u8`/`u16`/`u32` to `uint8_t`/`uint16_t`/`uint32_t`.
^ permalink raw reply
* Re: [PATCH v4 07/20] net/txgbe: fix Tx desc free logic
From: Stephen Hemminger @ 2026-05-17 23:44 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-8-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:49 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> On some server environments, this driver caused TDM non-fatal errors
> or PCIe request errors during Tx operation
>
> In Amber-Lite NIC's Tx head write-back mode, the hardware periodically
> writes back a head index pointing to the next descriptor it is adout
> to process in Tx ring. All descriptors before the head are considered
> processed by hardware and can be safely freed by the driver.
>
> The root cause is that the driver can safely free a batch of descriptors
> only when the hardware's write-back head pointer has advanced beyond all
> descriptors in that batch, meaning they have all been processed by the
> hardware. If the driver frees a descriptor before the hardware has
> finished processing it, invalid memory access may occur, leading to the
> observed bug.
>
> To fix the issue, correct the boundary check in all three Tx cleanup
> functions, each of which was missing the proper condition to prevent
> freeing unprocessed descriptors.
>
> Fixes: 8ada71d0bb7f ("net/txgbe: add Tx head write-back mode for Amber-Lite")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Lots of AI feedback on this one.
# Review of DPDK Patch: net/txgbe: fix Tx desc free logic
## Errors
### 1. Shared variable access without atomics (3 locations)
**Lines affected:**
- `drivers/net/txgbe/txgbe_rxtx.c:101` (txgbe_tx_free_bufs)
- `drivers/net/txgbe/txgbe_rxtx.c:655` (txgbe_xmit_cleanup)
- `drivers/net/txgbe/txgbe_rxtx_vec_common.h:98` (txgbe_tx_free_bufs)
```c
volatile uint16_t head = (uint16_t)*txq->headwb_mem;
```
**Problem:** The code reads a hardware-updated value (`*txq->headwb_mem`) using a `volatile` qualifier. The `volatile` keyword does NOT provide atomicity or memory ordering guarantees between the hardware and CPU. On some architectures, reading a `uint16_t` from DMA-accessible memory without proper barriers or atomic operations can result in torn reads, stale cached values, or reordering issues.
**Fix:** Use atomic load with appropriate memory ordering:
```c
uint16_t head = rte_atomic_load_explicit(
(uint16_t *)txq->headwb_mem,
rte_memory_order_acquire);
```
The `acquire` ordering ensures that all descriptor writes by the hardware are visible to the CPU after reading the head pointer. This is critical for correctness in hardware write-back scenarios.
Note: The `headwb_mem` field type may need adjustment to be an `rte_atomic` type or cast appropriately.
---
### 2. Whitespace/indentation error in vec_common.h
**Line:** `drivers/net/txgbe/txgbe_rxtx_vec_common.h:97-98`
```c
tx_last_dd -= txq->nb_tx_desc;
volatile uint16_t head = (uint16_t)*txq->headwb_mem;
```
**Problem:** Inconsistent indentation. The `volatile uint16_t head` declaration is indented with tabs-only (correct), but it visually appears to be part of a removed block based on the diff context showing the previous line had different indentation.
**Fix:** Verify the indentation matches the surrounding code block. Based on the diff, this appears correct after the change, but confirm the tabs align with the function's indentation level.
---
## Warnings
### 1. Logic change correctness verification needed
The patch adds three new early-return conditions checking if `head == tx_next_dd`. While this appears correct (hardware hasn't advanced past the point we want to free to), the logic for wrap-around cases should be verified:
```c
if (txq->tx_next_dd == head)
return 0;
else if (txq->tx_next_dd > head && head > tx_last_dd)
return 0;
else if (tx_last_dd > txq->tx_next_dd &&
(head > tx_last_dd || head < txq->tx_next_dd))
return 0;
```
**Concern:** The wrap-around logic assumes the ring wraps at `nb_tx_desc`. The condition `tx_last_dd > txq->tx_next_dd` handles the wrap case, but verify that:
- `tx_last_dd` calculation correctly wraps (it does: `tx_last_dd -= txq->nb_tx_desc`)
- The inequality checks are strict `>` not `>=` consistently
The existing code appears correct, but this is complex boundary logic that warrants testing, especially the second `else if` condition in wrap-around scenarios.
---
### 2. Missing const qualifier on read-only atomic load result
**All three locations:** The `head` variable is read from hardware but never modified:
```c
volatile uint16_t head = (uint16_t)*txq->headwb_mem;
```
**Suggestion:** Once converted to atomic operations, declare as `const`:
```c
const uint16_t head = rte_atomic_load_explicit(...);
```
This documents that `head` is never modified after the initial read and prevents accidental writes.
---
## Info
### 1. Consider helper function for repeated logic
The same boundary-check logic appears in three separate functions with only minor variations. Consider extracting it to a static inline helper:
```c
static inline bool
txgbe_hw_head_in_range(uint16_t hw_head, uint16_t start,
uint16_t end, uint16_t ring_size)
{
if (start == hw_head)
return false;
if (start > hw_head)
return hw_head <= end;
else /* wrap case */
return hw_head > end || hw_head < start;
}
```
This would reduce code duplication and make the logic easier to test and maintain.
---
### 2. Comment explaining write-back head semantics
The commit message mentions "Tx head write-back mode" but the code lacks comments explaining the hardware behavior. Consider adding:
```c
/*
* In Amber-Lite NIC Tx head write-back mode, hardware periodically
* updates *headwb_mem with the index of the next descriptor to process.
* Descriptors before this head pointer are safe to free.
*/
const uint16_t head = rte_atomic_load_explicit(...);
```
---
## Summary
**Must fix (Errors):**
1. Replace `volatile` read with `rte_atomic_load_explicit()` with `rte_memory_order_acquire` ordering in all three locations
**Should fix (Warnings):**
1. Add `const` qualifier to the loaded `head` value after converting to atomics
2. Verify wrap-around boundary logic with testing (appears correct but is complex)
**Consider (Info):**
1. Extract repeated boundary-check logic to a helper function
2. Add comments documenting hardware write-back semantics
The core fix (adding the `head == tx_next_dd` check) appears correct and addresses the described bug. The primary issue is the use of `volatile` instead of proper atomic operations for accessing hardware-updated shared memory.
^ permalink raw reply
* Re: [PATCH v4 09/20] net/txgbe: fix link flow control config for Sapphire
From: Stephen Hemminger @ 2026-05-17 23:46 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu, Ferruh Yigit
In-Reply-To: <20260511103604.19724-10-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:51 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> SP chips have a hardware bug preventing XON flow control support,
> so the driver disables it.
>
> Fixes: 69ce8c8a4ce3 ("net/txgbe: support flow control")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Comment vs code mismatch
# Review of Patch 9/20: net/txgbe: fix link flow control config for Sapphire
## Errors
None.
## Warnings
### W1: Conditional logic inverted (line 1042-1043)
The comment states "SP doesn't support xon" but the condition enables XON for `txgbe_mac_aml` and `txgbe_mac_aml40`, which are **not** SP chips based on typical wangxun naming. The logic appears inverted.
**Why this matters**: If SP chips are `txgbe_mac_sp` or similar (not aml/aml40), then XON is being enabled on the wrong hardware, causing the bug the patch claims to fix.
**Suggested fix**: Verify the hardware type constants. If SP chips are a different enum value, the condition should be:
```c
/* SP doesn't support xon */
if (hw->mac.type != txgbe_mac_sp && hw->mac.type != txgbe_mac_sp40)
fcrtl |= TXGBE_FCWTRLO_XON;
```
Or if the current condition is correct, the comment should read "Only AML supports xon" to match the code.
### W2: Inconsistent capitalization in comment (line 1042)
Comment uses lowercase "xon" while the code uses uppercase `XON` in the macro name `TXGBE_FCWTRLO_XON`.
**Suggested fix**:
```c
/* SP doesn't support XON */
```
## Info
None.
---
**Summary**: The primary concern is the apparent mismatch between the comment and the conditional logic. The patch claims to disable XON for SP chips, but the condition enables it for aml/aml40. This requires verification that either the comment is wrong or the condition should be negated/use different constants.
^ permalink raw reply
* Re: [PATCH v4 12/20] net/txgbe: fix link stability for 25G NIC
From: Stephen Hemminger @ 2026-05-17 23:49 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-13-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:54 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The link was previously configured via firmware, but this approach
> resulted in unstable link behavior. To resolve the issue, re-add the
> PHY configuration flow directly into the driver.
>
> Fixes: ead3616f630d ("net/txgbe: support PHY configuration via SW-FW mailbox")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Lots of AI review feedback here:
# Review of Patch 12/20: net/txgbe: fix link stability for 25G NIC
## Errors
### 1. Use-after-free potential in txgbe_e56_rxs_calib_adapt_seq (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines ~1600-1650
The function calls `txgbe_e56_rxs_osc_init_for_temp_track_range(hw, speed)` which can return `TXGBE_ERR_TIMEOUT`. When the timeout path at line ~1115 is taken, it writes to `E56PHY_PMD_CFG_0_ADDR` to disable RX and then returns `-1`. However, the caller `txgbe_e56_rxs_calib_adapt_seq` continues execution and accesses PHY registers without verifying the hardware state is valid. If the initialization timed out, subsequent register accesses may be unreliable.
**Suggested fix:**
```c
status = txgbe_e56_rxs_osc_init_for_temp_track_range(hw, speed);
if (status != 0)
return status; /* Propagate timeout or error immediately */
```
### 2. Resource leak: lock not released on error path (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_aml.c`
**Location:** Lines 241-247
In `txgbe_setup_phy_link_aml`, `rte_spinlock_lock(&hw->phy_lock)` is acquired at line 241. If `txgbe_set_link_to_amlite` returns `TXGBE_ERR_PHY_INIT_NOT_DONE` at line 246, the code jumps to `out:` at line 264 without releasing the spinlock. The lock is only released on the success path or `TXGBE_ERR_TIMEOUT`.
**Suggested fix:**
```c
rte_spinlock_lock(&hw->phy_lock);
ret_status = txgbe_set_link_to_amlite(hw, speed);
rte_spinlock_unlock(&hw->phy_lock); /* Always release before checking status */
if (ret_status == TXGBE_ERR_PHY_INIT_NOT_DONE)
goto out;
```
### 3. Missing error propagation to caller (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_aml.c`
**Location:** Lines 280-283
In `txgbe_setup_phy_link_aml`, the function returns `status` which is initialized to 0 and never assigned a failure value, even when `txgbe_set_link_to_amlite` fails or link is not established. The function always returns success regardless of actual outcome.
**Suggested fix:**
```c
if (!link_up) {
*need_reset = true;
return TXGBE_ERR_TIMEOUT; /* Return error when link fails */
}
```
### 4. Double error return value overwrite (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1090-1095, 1157-1162, 1197-1202, 1242-1247
In `txgbe_e56_rxs_osc_init_for_temp_track_range`, the pattern:
```c
if (timer++ > PHYINIT_TIMEOUT) {
DEBUGOUT("ERROR: ...");
break;
return -1; /* Unreachable: break exits the loop */
}
```
The `return -1` is unreachable. After `break`, execution continues to the next loop iteration check and then proceeds past the loop. The error is not propagated.
**Suggested fix:**
```c
if (timer++ > PHYINIT_TIMEOUT) {
DEBUGOUT("ERROR: Wait timeout");
return TXGBE_ERR_TIMEOUT; /* Remove break, return immediately */
}
```
### 5. Error code dropped without propagation (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1828-1830
In `txgbe_temp_track_seq`:
```c
status = txgbe_e56_get_temp(hw, &temperature);
if (status)
return 0; /* Returns success when get_temp failed */
```
Temperature reading failure is silently converted to success. The caller cannot distinguish between a successful temperature track and a failed temperature read.
**Suggested fix:**
```c
status = txgbe_e56_get_temp(hw, &temperature);
if (status)
return status; /* Propagate the error */
```
### 6. Integer overflow in bit shift (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 19-31 in `set_fields_e56()`
The function performs `(1 << bit_low)` where `bit_low` can be up to 31 (since bit fields in the code span 0-31). The literal `1` is `int` (signed 32-bit). When `bit_low >= 31`, `1 << 31` invokes undefined behavior (signed overflow).
**Suggested fix:**
```c
if (bit_high == bit_low) {
if (set_value == 0)
*src_data &= ~(1U << bit_low); /* Use 1U for unsigned */
else
*src_data |= (1U << bit_low);
} else {
for (i = bit_low; i <= bit_high; i++)
*src_data &= ~(1U << i);
*src_data |= (set_value << bit_low);
}
```
### 7. Missing bounds check on array size calculation (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Line 1498 in `txgbe_e56_rx_rd_second_code()`
The code calls `qsort(RXS_BBCDR_SECOND_ORDER_ST, array_size, sizeof(int), ...)` where `array_size = ARRAY_SIZE(RXS_BBCDR_SECOND_ORDER_ST)`. The array has 5 elements, but the calculation `array_size = ARRAY_SIZE(...)` trusts the macro. If N changes in the loop initialization (line 1489: `N = 5`), the array could overflow.
**Suggested fix:**
```c
/* Ensure loop count matches array size */
BUILD_ASSERT(N <= ARRAY_SIZE(RXS_BBCDR_SECOND_ORDER_ST));
```
## Warnings
### 1. Variables declared but may be read before initialization (STYLE)
**File:** `drivers/net/txgbe/base/txgbe_aml.c`
**Location:** Lines 325-327, 355-357
Variables `need_reset` are declared immediately before use, which is acceptable C99 style. However, they are declared in the middle of a conditional block scope, which can reduce clarity when the same variable name is reused in different branches.
**Suggested improvement:**
Declare `need_reset` once at function scope for better readability, though the current usage is technically correct.
### 2. Missing documentation for complex algorithm (STYLE)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 902-1264 (txgbe_e56_rxs_osc_init_for_temp_track_range)
This 360-line function implements a complex PHY oscillator calibration sequence with numbered steps in comments (1, 2, 3...). The algorithm is critical for 25G link stability but lacks a high-level overview comment explaining the purpose, temperature dependency, and why specific register sequences are needed.
**Suggested improvement:**
Add a function-level Doxygen comment explaining the calibration procedure and referencing the hardware specification section.
### 3. Magic numbers without named constants (STYLE)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Multiple locations (e.g., lines 505, 732, 1000)
Numerous magic numbers appear in register writes (e.g., `0x68c1`, `0x3321`, `0x973e`, `0xccde` at lines 2281-2284) without explanation. These appear to be RS-FEC configuration magic values but are not documented.
**Suggested improvement:**
Define named constants or add inline comments explaining what these values configure:
```c
#define RS_FEC_CONFIG_VAL1 0x68c1 /* RS-FEC configuration register value */
```
## Info
### 1. Deep nesting in calibration sequence
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1550-1750 (txgbe_e56_rxs_calib_adapt_seq)
The ADC calibration loop (16 iterations) has 5 levels of nesting. Consider extracting the calibration step into a helper function to improve readability.
### 2. Possible candidate for helper function
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1950-2050 (repeated CTLE bypass pattern)
The code to override CTLE bypass for multiple lanes (RXS1, RXS2, RXS3) at lines 1999-2030 is repetitive. This pattern could be extracted into a helper function.
---
## Summary
**Critical Issues:** 7 Errors identified, primarily correctness bugs affecting resource management and error handling.
**Style Issues:** 3 Warnings - documentation and code organization improvements.
**Observations:** 2 Info items - code structure suggestions.
The most serious issues are:
1. Lock leak on error path (Error #2)
2. Missing error propagation causing silent failures (Errors #3, #5)
3. Undefined behavior in bit operations (Error #6)
4. Unreachable error returns masking timeouts (Error #4)
============================================================
^ 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