> Hi Jose, > Apologies for the late reply, I haven't been feeling well for the past > few days.
No worries! >> > This patch adds the vmtest-tool subdirectory under contrib which tests >> > BPF programs under a live kernel using a QEMU VM. It automatically >> > builds the specified kernel version with eBPF support enabled >> > and stores it under "~/.vmtest-tool", which is reused for future >> > invocations. >> >> I wonder, would it be a good idea to have "bpf" as part of the name of >> the directory. Something like bpf-vmtest-tool? > > I think adding the "bpf" prefix is a good idea. Should we also rename > the directory under contrib to include the bpf prefix for consistency? Yes I would say so. >> > +To run a BPF source file in the VM: >> > + >> > + python main.py --kernel-image 6.15 --bpf-src fail.c >> > + >> >> Wouldn't --kernel-image expect the path to a kernel image? Typo? > > Thanks for catching that. I’ll fix it in the revision. Thanks. >> > +DEVELOPMENT >> > +=========== >> > + >> > +This tool uses `uv` (https://github.com/astral-sh/uv) for virtual >> > environment >> > +and dependency management. >> > + >> > +To install development dependencies: >> > + >> > + uv sync >> > + >> > +To run the test suite: >> > + >> > + uv run pytest >> > + >> > +A `.pre-commit-config.yaml` is provided to assist with development. >> > +Pre-commit hooks will auto-generate `requirements-dev.txt` and lint python >> > +files. >> > + >> > +To enable pre-commit hooks: >> > + >> > + uv run pre-commit install >> >> Having uv installed would only be necessary for vmtest-tool development >> purposes, right? Not to run the script. I see that all the >> dependencies in vmtest-tool/pyproject.toml are related to testing and >> python linting. > > Yes, uv is only needed for the development setup. The main script only > uses the standard library. Since all metadata is in the pyproject.toml, > any standard Python package manager (like pip, poetry, etc.) can be > used; there's no strict requirement to use uv. Only the pre-commit > script depends on uv I think it would be good to stick to simplicitly whenever possible here. The less dependencies the better. >> Is the 3.10 minimum Python version requirement associated with any of >> these dependencies? If so, we could maybe relax the minimum version >> requirement for users of the script? My Debian-like system has python >> 3.9.2, for example. > > I chose Python 3.10 as the minimum because 3.9 was nearing EOL in a few > months. The script only uses the standard library, so it works with 3.9 > by making the following change, since the | type union isn't supported > in 3.9. I'll update the next revision to support 3.9 Thanks. That will help many users. > diff --git a/contrib/vmtest-tool/bpf.py b/contrib/vmtest-tool/bpf.py > index 291e251e64c..91bfc0c5ae1 100644 > --- a/contrib/vmtest-tool/bpf.py > +++ b/contrib/vmtest-tool/bpf.py > @@ -3,6 +3,7 @@ import subprocess > import logging > from pathlib import Path > import tempfile > +from typing import Optional > import utils > import config > > @@ -25,8 +26,8 @@ class BPFProgram: > > def __init__( > self, > - source_path: Path | None = None, > - bpf_bytecode_path: Path | None = None, > + source_path: Optional[Path] = None, > + bpf_bytecode_path: Optional[Path] = None, > use_temp_dir: bool = False, > ): > path = source_path or bpf_bytecode_path > >> > + >> > + def _compile_bpf(self): >> > + """Compile the eBPF program using gcc""" >> > + logger.info(f"Compiling eBPF source: {self.bpf_src}") >> > + cmd = [ >> > + "bpf-unknown-none-gcc", >> > + "-g", >> > + "-O2", >> > + "-std=gnu17", >> > + f"-D__TARGET_ARCH_{config.ARCH}", >> > + "-gbtf", >> > + "-Wno-error=attributes", >> > + "-Wno-error=address-of-packed-member", >> > + "-Wno-compare-distinct-pointer-types", >> > + *BPF_INCLUDES, >> > + "-c", >> > + str(self.bpf_src), >> > + "-o", >> > + str(self.bpf_obj), >> > + ] >> >> It shall definitely be possible to specify the set of compilation flags >> using some environment variables like BPF_CFLAGS and BPF_CPPFLAGS, or >> arguments to the script. The GCC testsuite will want to do torture-like >> testing of the bpf programs by compiling and running them using >> different set of optimization options (-O0, -Os, -O2, other options...) >> any of which may break verifiability. > > Thanks for the suggestion. I'll add support for this in the next > revision. Thanks. > One can also pass a precompiled BPF object with the desired > optimization options to the script to check it with the verifier. Yes, but AFAIK building objects that can be actually loaded in the kernel and verified requires in practice including kernel headers, which is what this tool makes easier. So it seems to me that we will be likely using the script to build sources most (if not all) of the time? > >> > + >> > + // STEP 3: Load the program (this will trigger verifier log >> > output) >> > + err = {self.name}__load(skel); >> > + fprintf( >> > + stderr, >> > + "--- Verifier log start ---\\n" >> > + "%s\\n" >> > + "--- Verifier log end ---\\n", >> > + log_buf >> >> Eventually, the output of the loader when the verifier rejects a program >> ought to be suitable for our dejagnu glue code to interpret it as a pass >> or a fail. > > Yes, for now this is meant to print to stdout for the user. This will > change with DejaGNU integration. Ok, sweet. Looking forward to the next version. Thanks!
