Hey Ilya, On 7/9/25 2:29 PM, Ilya Maximets wrote: [...]
Thnaks, Daniel! I have just a couple of small nits below that I missed in v3, the rest looks good and is working fine.diff --git a/net/af-xdp.c b/net/af-xdp.c index 29c5ad16cd..005117c336 100644 --- a/net/af-xdp.c +++ b/net/af-xdp.c @@ -51,6 +51,10 @@ typedef struct AFXDPState {uint32_t xdp_flags;bool inhibit; + + char *map_path; + int map_fd; + uint32_t map_start_index; } AFXDPState;#define AF_XDP_BATCH_SIZE 64@@ -260,6 +264,7 @@ static void af_xdp_send(void *opaque) static void af_xdp_cleanup(NetClientState *nc) { AFXDPState *s = DO_UPCAST(AFXDPState, nc, nc); + int idx;qemu_purge_queued_packets(nc); @@ -273,6 +278,18 @@ static void af_xdp_cleanup(NetClientState *nc)s->umem = NULL; qemu_vfree(s->buffer); s->buffer = NULL; + + if (s->map_fd >= 0) { + idx = nc->queue_index + s->map_start_index; + if (bpf_map_delete_elem(s->map_fd, &idx)) { + fprintf(stderr, "af-xdp: unable to remove AF_XDP socket from " + "map %s\n", s->map_path);nit :I'd suggest to keep the "map" on a previous line. We have some space for it.
fixed.
+ } + close(s->map_fd); + s->map_fd = -1; + } + g_free(s->map_path); + s->map_path = NULL; }static int af_xdp_umem_create(AFXDPState *s, int sock_fd, Error **errp)@@ -336,7 +353,6 @@ static int af_xdp_socket_create(AFXDPState *s, }; int queue_id, error = 0;- s->inhibit = opts->has_inhibit && opts->inhibit;if (s->inhibit) { cfg.libxdp_flags |= XSK_LIBXDP_FLAGS__INHIBIT_PROG_LOAD; } @@ -387,6 +403,35 @@ static int af_xdp_socket_create(AFXDPState *s, return 0; }+static int af_xdp_update_xsk_map(AFXDPState *s, Error **errp)+{ + int xsk_fd, idx, error = 0; + + if (!s->map_path) { + return 0; + } + + s->map_fd = bpf_obj_get(s->map_path); + if (s->map_fd < 0) { + error = errno; + } else { + xsk_fd = xsk_socket__fd(s->xsk); + idx = s->nc.queue_index + s->map_start_index; + if (bpf_map_update_elem(s->map_fd, &idx, &xsk_fd, 0)) { + error = errno; + } + } + + if (error) { + error_setg_errno(errp, error, + "failed to insert AF_XDP socket into map %s", + s->map_path); + return -1;nit: Maybe remove this line and return 'error' below?
I left this as-is given the rest of the code has similar style as here and always returns -1 on errors. error would be positive here and we'd have to return -error, but still it seems somewhat out of place imho.
+ } + + return 0; +} + /* NetClientInfo methods. */ static NetClientInfo net_af_xdp_info = { .type = NET_CLIENT_DRIVER_AF_XDP, @@ -441,6 +486,7 @@ int net_init_af_xdp(const Netdev *netdev, int64_t i, queues; Error *err = NULL; AFXDPState *s; + bool inhibit;ifindex = if_nametoindex(opts->ifname);if (!ifindex) { @@ -456,8 +502,21 @@ int net_init_af_xdp(const Netdev *netdev, return -1; }- if ((opts->has_inhibit && opts->inhibit) != !!opts->sock_fds) {- error_setg(errp, "'inhibit=on' requires 'sock-fds' and vice versa"); + inhibit = opts->has_inhibit && opts->inhibit; + if (inhibit && !opts->sock_fds && !opts->map_path) { + error_setg(errp, "'inhibit=on' requires 'sock-fds' or 'map-path'"); + return -1; + } + if (!inhibit && (opts->sock_fds || opts->map_path)) { + error_setg(errp, "'sock-fds' and 'map-path' require 'inhibit=on'"); + return -1; + } + if (opts->sock_fds && opts->map_path) { + error_setg(errp, "'sock-fds' and 'map-path' are mutually exclusive"); + return -1; + } + if (!opts->map_path && opts->has_map_start_index) { + error_setg(errp, "'map-start-index' requires 'map-path'"); return -1; }@@ -481,14 +540,23 @@ int net_init_af_xdp(const Netdev *netdev, pstrcpy(s->ifname, sizeof(s->ifname), opts->ifname);s->ifindex = ifindex; + s->inhibit = inhibit; + + s->map_path = g_strdup(opts->map_path); + s->map_fd = -1; + s->map_start_index = 0; + if (opts->has_map_start_index && opts->map_start_index > 0) {We should error out if the user specified a negative value. I'd suggest to add the check to the list of user-input validation above instead of silently ignoring the incorrect value. And then we could skip the value check here.
fixed.
+ s->map_start_index = opts->map_start_index; + }if (af_xdp_umem_create(s, sock_fds ? sock_fds[i] : -1, &err) ||- af_xdp_socket_create(s, opts, &err)) { + af_xdp_socket_create(s, opts, &err) || + af_xdp_update_xsk_map(s, &err)) { goto err; } }- if (nc0) {+ if (nc0 && !inhibit) { s = DO_UPCAST(AFXDPState, nc, nc0); if (bpf_xdp_query_id(s->ifindex, s->xdp_flags, &prog_id) || !prog_id) { error_setg_errno(errp, errno, diff --git a/qapi/net.json b/qapi/net.json index 97ea183981..3d80a9cacd 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -454,25 +454,34 @@ # (default: 0). # # @inhibit: Don't load a default XDP program, use one already loaded -# to the interface (default: false). Requires @sock-fds. +# to the interface (default: false). Requires @sock-fds or @map-path. # # @sock-fds: A colon (:) separated list of file descriptors for # already open but not bound AF_XDP sockets in the queue order. # One fd per queue. These descriptors should already be added -# into XDP socket map for corresponding queues. Requires -# @inhibit. +# into XDP socket map for corresponding queues. @sock-fds and +# @map-path are mutually exclusive. Requires @inhibit. +# +# @map-path: The path to a pinned xsk map to push file descriptors +# for bound AF_XDP sockets into. @map-path and @sock-fds are +# mutually exclusive. Requires @inhibit. (Since 10.1) +# +# @map-start-index: Use @map-path to insert xsk sockets starting from +# this index number (default: 0). Requires @map-path. (Since 10.1) # # Since: 8.2 ## { 'struct': 'NetdevAFXDPOptions', 'data': { - 'ifname': 'str', - '*mode': 'AFXDPMode', - '*force-copy': 'bool', - '*queues': 'int', - '*start-queue': 'int', - '*inhibit': 'bool', - '*sock-fds': 'str' }, + 'ifname': 'str', + '*mode': 'AFXDPMode', + '*force-copy': 'bool', + '*queues': 'int', + '*start-queue': 'int', + '*inhibit': 'bool', + '*sock-fds': 'str', + '*map-path': 'str', + '*map-start-index': 'int' }, 'if': 'CONFIG_AF_XDP' }##diff --git a/qemu-options.hx b/qemu-options.hx index 1f862b19a6..0fd4fd8d46 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2909,6 +2909,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, #ifdef CONFIG_AF_XDP "-netdev af-xdp,id=str,ifname=name[,mode=native|skb][,force-copy=on|off]\n" " [,queues=n][,start-queue=m][,inhibit=on|off][,sock-fds=x:y:...:z]\n" + " [,map-path=/path/to/socket/map][,map-start-index=i]\n" " attach to the existing network interface 'name' with AF_XDP socket\n" " use 'mode=MODE' to specify an XDP program attach mode\n" " use 'force-copy=on|off' to force XDP copy mode even if device supports zero-copy (default: off)\n" @@ -2916,6 +2917,8 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, " with inhibit=on,\n" " use 'sock-fds' to provide file descriptors for already open AF_XDP sockets\n" " added to a socket map in XDP program. One socket per queue.\n" + " use 'map-path' to provide the socket map location to populate AF_XDP sockets with\n"nit: it feels like we need some punctuation sign after the 'with', otherwise it reads as "populate AF_XDP sockets with beginning from the", which makes no sense.
ok, reworded slightly. Thanks, Daniel
