feat: add FreeBSD support - #2
Conversation
|
This brings us halfway to drop From SPDK function _configure_freebsd() {
local freebsd_bufsz=${FREEBSD_BUFSZ:-256}
if ! check_for_driver_freebsd; then
echo "DPDK drivers (contigmem and/or nic_uio) are missing, aborting" >&2
return 1
fi
configure_freebsd_pci "$@"
if [[ $SKIP_HUGE == yes ]]; then
# Do nothing as requested
return 0
fi
# If contigmem is already loaded but the HUGEMEM specified doesn't match the
# previous value, unload contigmem so that we can reload with the new value.
if kldstat -q -m contigmem; then
# contigmem may be loaded, but the kernel environment doesn't have to
# be necessarily set at this point. If it isn't, kenv will fail to
# pick up the hw. options. Handle it.
if ! contigmem_num_buffers=$(kenv hw.contigmem.num_buffers); then
contigmem_num_buffers=-1
fi 2> /dev/null
if ((contigmem_num_buffers != HUGEMEM / freebsd_bufsz)); then
kldunload contigmem.ko
fi
fi
if ! kldstat -q -m contigmem; then
kenv hw.contigmem.num_buffers=$((HUGEMEM / freebsd_bufsz))
kenv hw.contigmem.buffer_size=$((freebsd_bufsz * 1024 * 1024))
kldload contigmem.ko
fi
}@safl Do you think we should put the |
karlowich
left a comment
There was a problem hiding this comment.
Great.
I propose adding the test_linux.py in a later commit, to avoid changing the test in 3 consecutive commits.
Also see the few inline comments.
| handles: list = field(default_factory=list) | ||
|
|
||
|
|
||
| class Backend(abc.ABC): |
There was a problem hiding this comment.
I think this should be called platform instead. That puts it more in line with the doc-string: "Platform-specific PCI device-driver binding operations"
There was a problem hiding this comment.
Renamed to Platform, and get_backend() became get_platform() to match.
| DRIVERS: set = set() | ||
|
|
||
| def driver_names(self) -> set: | ||
| """Return the set of driver-names this backend can bind to""" |
There was a problem hiding this comment.
Let's rename backend to platform here too.
There was a problem hiding this comment.
Done. backend is now platform throughout the docstrings and comments too.
|
|
||
| class System: | ||
| DRIVERS = {"nvme", "vfio-pci", "vfio-noiommu", "uio_pci_generic"} | ||
| class LinuxBackend(Backend): |
There was a problem hiding this comment.
if we rename from backend to platform, then this class could just be named Linux
|
|
||
| def unbind(device: Device): | ||
| log.info(f"Unbinding({device.bdf}) from '{device.driver}'") | ||
| def get_backend() -> Backend: |
There was a problem hiding this comment.
Renamed to get_platform().
Yeah, separate tool. Since, the main thing about the suite of tools: |
0.3.10 taught --device to bypass the class filter. The check reads
props.get("bdf"), but the scan loop stores the address under "slot",
the key lspci prints. The rename to bdf happens later, in
Device.from_dict. The lookup always came back empty. --device matched
nothing at all, not even NVMe devices that used to match. The command
still exited 0, so the failure was silent.
Read the slot key. A regression test for the bypass lands with the
Linux test module in the platform split.
Signed-off-by: Jaeyoon Choi <j_yoon.choi@samsung.com>
device_scan, bind, and unbind read the argparse namespace directly. That coupled them to the CLI. unbind never used args at all. bind took it only to pass it along. Tests could not call the helpers without building a fake args object. Take classcode, bdf, and driver_name as plain parameters instead. main() passes the parsed values at the call sites. This keeps the upcoming platform interface free of argparse. Behavior is unchanged. Signed-off-by: Jaeyoon Choi <j_yoon.choi@samsung.com>
24c1e7e to
5e8f0dd
Compare
Thanks for the review! Both points are addressed:
|
I will integrate contigmem into hugepages. |
|
Karl, Could you please add Simon to reviewer? |
|
@jaeyoon-choi I don't have the rights on this repo to do that, but I will let him know. |
|
@jaeyoon-choi Looks like the tests are failing on formatting. You need to run |
Prepare for adding other platforms. The module-level scan, bind, unbind, and probe helpers move onto a Linux class behind a small abstract Platform interface. get_platform() picks the implementation for the running platform. System.drivers/limits become instance state filled in from the platform. The memlock remediation text becomes a platform hint, assembled into the same warning. The parse-time driver vocabulary splits off as KNOWN_DRIVERS, and Device.MANDATORY_KEYS becomes _LSPCI_KEYS next to the lspci parser. The tool and sysfs notes from the file header move onto the Linux class as its docstring. Output and behavior on Linux are unchanged. scan_devices() keeps the --device bypass of 0.3.10. Add the Linux unit tests: lspci parsing and filtering, the --device bypass, the driver_override -> bind -> setpci sequence, and platform selection. Signed-off-by: Jaeyoon Choi <j_yoon.choi@samsung.com>
510b10b to
351486b
Compare
safl
left a comment
There was a problem hiding this comment.
Reviewed the whole branch at 351486b against main. The nic_uio bind reads correctly now: registering the device in hw.nic_uio.bdfs, dropping any pinned driver, reloading the module, and verifying the attach through pciconf -l before writing the command register is the right shape, and the new tests cover it. On Linux here, --list output matches main and the --device fix in 3db4842 is confirmed: main prints no props for a live BDF, this branch prints one. ruff format --check and ruff check pass with the pinned 0.11.4, and the suite passes on 3.13.
Two things worth a look, both inline. Separately, the description above still describes the design as Backend/LinuxBackend, which the code no longer uses, and its table has the same staleness as the README one.
I could not exercise any FreeBSD path. The kenv plus kldunload/kldload sequence, devctl clear driver -f, and the CAM mapping rest on the VM testing you describe.
| system = System() | ||
| system.probe_drivers() | ||
| system.probe_limits() | ||
| system.drivers = platform.probe_drivers() | ||
| system.probe_limits(platform.memlock_remediation_hint()) | ||
|
|
||
| if args.list: | ||
| system.pp() | ||
|
|
||
| devices = list(device_scan(args)) | ||
| devices = list(platform.scan_devices(args.classcode, args.device)) | ||
|
|
||
| try: |
There was a problem hiding this comment.
probe_drivers() and list(scan_devices(...)) sit outside the try below, so the two calls most likely to fail on a machine without PCI sysfs still end in a traceback rather than the one-line error 01c31f9 introduces.
Reproduced on Linux by making Path.resolve raise FileNotFoundError for /sys/bus/pci/drivers:
File "src/devbind/devbind.py", line 165, in probe_drivers
(path.name for path in Path("/sys/bus/pci/drivers").resolve(strict=True).glob("*"))
FileNotFoundError: [Errno 2] No such file or directory: '/sys/bus/pci/drivers'
Moving both calls inside the existing try covers it.
| | enumerate / inspect | `lspci`, sysfs | `pciconf -l`, `camcontrol` | | ||
| | user space framework | `vfio-pci`, `uio_pci_generic` | `nic_uio` | | ||
| | unbind | sysfs `driver/unbind` | `devctl detach` | | ||
| | bind | sysfs `drivers/<drv>/bind` | `devctl set driver` | |
There was a problem hiding this comment.
This is now wrong for the case the change is actually about. nic_uio binds through hw.nic_uio.bdfs plus kldload; only kernel drivers go through devctl set driver. The commit message for 351486b explains it correctly, so it is just the table that is stale.
An unsupported platform, a --bind name the platform does not have, and a failing probe, scan, bind, or unbind all ended in a Python traceback. The driver-name case gets worse once more platforms exist. A name owned by another platform would reach the device before failing. Catch the platform errors in main() and exit with a one-line error. The probe and the scan run inside that same block, so a machine without PCI sysfs gets the one-line error too. Validate --bind against the active platform's driver_names() before touching the device. Signed-off-by: Jaeyoon Choi <j_yoon.choi@samsung.com>
Implement the Platform interface with FreeBSD's native tools: - pciconf -l enumerates devices. Both output formats are parsed: the packed chip=/card= fields of 14.3 and earlier, and the split vendor=/device= fields of 15.x and 14.4. - devctl detach / devctl set driver implement unbind and bind for kernel drivers. nic_uio cannot be attached that way: it only probes devices listed in the hw.nic_uio.bdfs tunable. Binding to nic_uio registers the device there, drops any driver pinned by an earlier set driver, and (re)loads the module, which detaches the previous driver and claims the device by itself. Binding back to a kernel driver removes the bdfs entry again. - camcontrol devlist maps an nvmeX controller to its CAM disk (ndaY), so the fstat in-use check also sees users of the disk device. - kldstat reports whether nic_uio is loaded. - scan_devices() honors the --device class-filter bypass, matching the Linux platform. - pciconf -w enables bus-mastering once the nic_uio attach is verified through pciconf -l. A failed write is reported instead of ignored. Helpers convert between the pciX:B:S:F selector and the domain:bus:device.function bdf form. nic_uio is added to KNOWN_DRIVERS, completion, and the docs. User-facing text drops its Linux assumptions: the --bind help says driver file instead of .ko file, and the memlock warning says DMA mapping instead of VFIO_IOMMU_MAP_DMA. Unit tests cover both pciconf formats, the fstat heuristic, kldstat probing, camcontrol parsing, the nic_uio bdfs flow, and the devctl bind for kernel drivers. Signed-off-by: Jaeyoon Choi <j_yoon.choi@samsung.com>
351486b to
3179a33
Compare
Thank you for the review! |
Summary
Add FreeBSD support to devbind. A platform implementation picked at runtime
maps the same CLI onto each platform's native tooling, so one command
prepares an NVMe device for DPDK/SPDK and xNVMe on both Linux and FreeBSD.
Verified end-to-end on FreeBSD 15.1 and 14.4 VMs with an emulated NVMe
device, including a real nic_uio bind with the dpdk kmod installed, and
re-verified on Linux the same way. The branch is structured for per-commit
review; every commit passes the test suite on its own.
DPDK/SPDK and xNVMe support FreeBSD, but preparing a device there — loading
nic_uio, runningdevctl detach, enabling bus-mastering by hand — wasmanual. devbind covered only Linux, with the sysfs paths hardcoded in the
tool.
The Linux code now sits behind a small
Platforminterface, and a FreeBSDimplementation joins it:
lspci, sysfspciconf -l,camcontroldriver/unbinddevctl detachdrivers/<drv>/binddevctl set driverdriver_override+bindhw.nic_uio.bdfs+kldloadlsoffstat+camcontrol(nvmeX → ndaY)vfio-pci,uio_pci_genericnic_uiosetpcipciconf -wFreeBSD has no VFIO, so
nic_uio(theuio_pci_genericanalogue) is theuser space path there, and
iommugroupreportsNone. Naming a driver therunning platform does not have is rejected before the device is touched.
The five commits are meant to be read one at a time. A fix for the 0.3.10
--device lookup comes first: it read an unmapped key and matched nothing.
Two refactors follow. One decouples scanning and binding from argparse. The
other moves the Linux code onto a
Linuxclass behind thePlatforminterface — a pure move, easiest to read with
--color-moved, and output onLinux is unchanged. A small fix then turns platform errors into one-line
messages instead of tracebacks. The feat commit at the top adds the FreeBSD
platform, unit tests for both platforms, and the docs.
Two notes for review:
pciconf -loutput differs by release. 14.3 and earlier print packedchip=/card=fields. 15.x and 14.4 print splitvendor=/device=fields. Both are parsed, and the unit fixtures cover both.
nic_uiocannot be attached withdevctl set driver. It only probesdevices listed in
hw.nic_uio.bdfs, so binding registers the devicethere, drops any pinned driver, and reloads the module, which claims the
device by itself. The attach is verified through
pciconf -lbefore thecommand register is written.