Re: [DO_NOT_APPLY v3 1/3] add xdp-forwarder

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 5629 bytes --]

Yureka Lilian <yureka@cyberchaos.dev> writes:

> Signed-off-by: Yureka Lilian <yureka@cyberchaos.dev>

When you submit this for real, make sure to format the commit message
like other Spectrum commits (e.g. "tools: add xdp-forwarder"), and maybe
add a little background info, since it's not documented yet.

> ---
>  pkgs/default.nix                              |   5 +
>  tools/default.nix                             |  15 +-
>  tools/meson.build                             |   5 +
>  tools/meson_options.txt                       |   4 +
>  tools/xdp-forwarder/include/parsing_helpers.h | 273 ++++++++++++++++++
>  tools/xdp-forwarder/include/rewrite_helpers.h | 145 ++++++++++
>  tools/xdp-forwarder/prog_physical.c           |  37 +++
>  tools/xdp-forwarder/prog_router.c             |  43 +++
>  tools/xdp-forwarder/set_router_iface.c        |  32 ++

Shouldn't there be a tools/xdp-forwarder/meson.build?

>  9 files changed, 556 insertions(+), 3 deletions(-)
>  create mode 100644 tools/xdp-forwarder/include/parsing_helpers.h
>  create mode 100644 tools/xdp-forwarder/include/rewrite_helpers.h
>  create mode 100644 tools/xdp-forwarder/prog_physical.c
>  create mode 100644 tools/xdp-forwarder/prog_router.c
>  create mode 100644 tools/xdp-forwarder/set_router_iface.c
>
> diff --git a/tools/default.nix b/tools/default.nix
> index 95d76a1..e664f47 100644
> --- a/tools/default.nix
> +++ b/tools/default.nix
> @@ -1,13 +1,16 @@
>  # SPDX-License-Identifier: MIT
>  # SPDX-FileCopyrightText: 2022-2025 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Yureka Lilian <yureka@cyberchaos.dev>
>  
>  import ../lib/call-package.nix (
>  { src, lib, stdenv, fetchCrate, fetchurl, runCommand, buildPackages
>  , meson, ninja, pkg-config, rustc
>  , clang-tools, clippy
>  , dbus
> +, clang, libbpf
>  , guestSupport ? true
>  , hostSupport ? false
> +, driverSupport ? false
>  }:
>  
>  let
> @@ -70,15 +73,18 @@ stdenv.mkDerivation (finalAttrs: {
>        ./lsvm
>        ./start-vmm
>        ./subprojects
> +    ] ++ lib.optionals driverSupport [
> +      ./xdp-forwarder
>      ]));
>    };
>    sourceRoot = "source/tools";
>  
>    depsBuildBuild = lib.optionals hostSupport [ buildPackages.stdenv.cc ];
>    nativeBuildInputs = [ meson ninja ]
> -    ++ lib.optionals guestSupport [ pkg-config ]
> -    ++ lib.optionals hostSupport [ rustc ];
> -  buildInputs = lib.optionals guestSupport [ dbus ];
> +    ++ lib.optionals (guestSupport || driverSupport) [ pkg-config ]
> +    ++ lib.optionals hostSupport [ rustc ]
> +    ++ lib.optionals driverSupport [ clang ];
> +  buildInputs = lib.optionals guestSupport [ dbus ] ++ lib.optionals driverSupport [ libbpf ];
>  
>    postPatch = lib.optionals hostSupport (lib.concatMapStringsSep "\n" (crate: ''
>      mkdir -p subprojects/packagecache
> @@ -88,12 +94,15 @@ stdenv.mkDerivation (finalAttrs: {
>    mesonFlags = [
>      (lib.mesonBool "guest" guestSupport)
>      (lib.mesonBool "host" hostSupport)
> +    (lib.mesonBool "driver" driverSupport)
>      "-Dhostfsrootdir=/run/virtiofs/virtiofs0"
>      "-Dtests=false"
>      "-Dunwind=false"
>      "-Dwerror=true"
>    ];
>  
> +  hardeningDisable = lib.optionals driverSupport [ "zerocallusedregs" ];
> +

Should we use the unwrapped compiler?  The wrapper doesn't give us
anything useful for other targets, and makes us need things like this.

>    passthru.tests = {
>      clang-tidy = finalAttrs.finalPackage.overrideAttrs (
>        { name, src, nativeBuildInputs ? [], ... }:
> diff --git a/tools/meson_options.txt b/tools/meson_options.txt
> index 4af0031..887e388 100644
> --- a/tools/meson_options.txt
> +++ b/tools/meson_options.txt
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: EUPL-1.2+
>  # SPDX-FileCopyrightText: 2022-2024 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Yureka Lilian <yureka@cyberchaos.dev>
>  
>  option('host', type : 'boolean', value : false,
>    description : 'Build tools for the Spectrum host')
> @@ -7,6 +8,9 @@ option('host', type : 'boolean', value : false,
>  option('guest', type : 'boolean',
>    description : 'Build tools for Spectrum guests')
>  
> +option('driver', type : 'boolean',
> +  description : 'Build tools for Spectrum driver VMs')
> +

Should be grouped with host and guest (no blank line between).

> diff --git a/tools/xdp-forwarder/set_router_iface.c b/tools/xdp-forwarder/set_router_iface.c
> new file mode 100644
> index 0000000..f1a2bac
> --- /dev/null
> +++ b/tools/xdp-forwarder/set_router_iface.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: EUPL-1.2+
> +// SPDX-FileCopyrightText: 2025 Yureka Lilian <yureka@cyberchaos.dev>
> +
> +#include <stdio.h>
> +#include <net/if.h>
> +#include <bpf/bpf.h>
> +
> +int main(int argc, char **argv)
> +{
> +	if (argc < 2) {
> +		fprintf(stderr, "missing interface name\n");
> +		return 1;
> +	}
> +
> +	int router_idx = if_nametoindex(argv[1]);
> +	if (router_idx <= 0) {
> +		perror("error getting router interface");
> +		return 1;

Can we use err(EXIT_FAILURE, "error getting router interface") like we
do elsewhere in Spectrum?

> +	}
> +
> +	int map_fd = bpf_obj_get("/sys/fs/bpf/router_iface");

Do we want to namespace this at all?  Is there a convention?

> +	if (map_fd < 0) {
> +		perror("failed to open bpf map");
> +		return 1;
> +	}
> +
> +	int id = 0;
> +	if (bpf_map_update_elem(map_fd, &id, &router_idx, 0) < 0) {
> +		perror("failed to update bpf map");
> +		return 1;
> +	}
> +}
> -- 
> 2.50.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]