Support configuring MTU for the SLIRP net backend, for example:
-s 1:0,virtio-net,slirp,mtu=2048,open
Update the manual page accordingly. While here, also document MAC
address configuration.
Details
Details
- Reviewers
markj - Group Reviewers
bhyve - Commits
- rG90b9a77ebeb8: bhyve: support MTU configuration for SLIRP net backend
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 69162 Build 66045: arc lint + arc unit
Event Timeline
Comment Actions
Actually, the slirp backend has a fixed MTU. We should either update it to handle a configured size, or make it an error to specify the MTU.
Comment Actions
I have updated this revision with these changes. It works fine for me with MTU values from 1476 to 4096 (didn't test larger), but for lower values it fails with:
Assertion failed: ((size_t)n <= priv->mtu), function slirp_recv, file /usr/home/novel/code/freebsd-src/usr.sbin/bhyve/net_backend_slirp.c, line 238.
I'll debug it further, but now I'm not sure: currently I do not validate the MTU value range in the SLIRP backend assuming it's already validated on the device side (e.g. in pci_virtio_net.c). Now I wonder if the backend might have stricter requirements for MTU than the device, should the backend implement its own validation on top of the device side validation?
Comment Actions
Do you have commit 69f61cee2efb1eec0640ca7de9b2d51599569a5d commit applied to the host kernel?
| usr.sbin/bhyve/net_backend_slirp.c | ||
|---|---|---|
| 210 | Can we please allocate this buffer in the slirp_priv structure rather than calling malloc() once for every packet? | |
| 218 | We can just get rid of this assertion, it's a bit silly. | |
| usr.sbin/bhyve/slirp/slirp-helper.c | ||
| 109 | This assertion should be updated too. | |
Comment Actions
An ng_device(4) is not an interface, so it does not have a MAC address. The way I see it, it is just a wire. If you think about a tap(4), it's like an ng_eiface(4) linked to a ng_device. But the MAC address belongs to the ng_eiface end, not the ng_device one.
For the MTU it might be a bit different because the ng_device could maybe impose a limit on to the size it could transport, but in any case there is currently no code allowing the user to configure it. The ioctls are not implemented.
Comment Actions
- bhyve.8: Drop ngdN related changes
- net_backend_slirp, slirp-helper: move buf to struct slirp_priv
- net_backend_slirp: remove assert from slirp_peek_recvlen()
- slirp-helper: update assert in slirp_cb_send_packet()
Comment Actions
Thanks for clarification. I've removed this bit from the manual page, I think it's better to focus on SLIRP for now. However, bhyve still allows to set mac= and mtu=, probably it should print error for this backend.
Comment Actions
Yes, I have commit 69f61cee2efb1eec0640ca7de9b2d51599569a5d (and also commit 82d8a5029a80a77166dca098b8fedb10d84e4e38 which appears to be a follow up of the former) included in the host kernel.
In the meantime, I was giving it some more testing and noticed another issues:
While running wget https://download.freebsd.org/releases/ISO-IMAGES/15.0/FreeBSD-15.0-RELEASE-arm64-aarch64-dvd1.iso -O /dev/null, somewhere half way it fails with:
`#1 0x000000080114c934 in raise (s=s@entry=6) at /usr/home/novel/code/freebsd-src/lib/libc/gen/raise.c:48
#2 0x00000008011fe7a9 in abort () at /usr/home/novel/code/freebsd-src/lib/libc/stdlib/abort.c:61
#3 0x000000080112f521 in assert (func=<optimized out>, file=<optimized out>, line=line@entry=115, failedexpr=<optimized out>) at /usr/home/novel/code/freebsd-src/lib/libc/gen/assert.c:47
#4 0x000000000102500b in slirp_cb_send_packet (buf=<optimized out>, len=1514, param=<optimized out>) at /usr/home/novel/code/freebsd-src/usr.sbin/bhyve/slirp/slirp-helper.c:115
#5 0x000000080141ad02 in ?? () from /usr/local/lib/libslirp.so.0
#6 0x0000000801412f7d in ?? () from /usr/local/lib/libslirp.so.0
#7 0x0000000801416366 in ?? () from /usr/local/lib/libslirp.so.0
#8 0x0000000801421197 in ?? () from /usr/local/lib/libslirp.so.0
#9 0x000000080141ef1a in ?? () from /usr/local/lib/libslirp.so.0
#10 0x0000000001024edd in slirp_pollfd_loop (priv=0x7fffffffe7e0) at /usr/home/novel/code/freebsd-src/usr.sbin/bhyve/slirp/slirp-helper.c:303
#11 main (argc=<optimized out>, argv=<optimized out>) at /usr/home/novel/code/freebsd-src/usr.sbin/bhyve/slirp/slirp-helper.c:580
(gdb) fr 4
#4 0x000000000102500b in slirp_cb_send_packet (buf=<optimized out>, len=1514, param=<optimized out>) at /usr/home/novel/code/freebsd-src/usr.sbin/bhyve/slirp/slirp-helper.c:115
115 assert((size_t)n == len);
(gdb) p n
$1 = 0
(gdb) `
That's with a fairly large mtu=4092. So it's probably a bug in my code, not a kernel issue.
Comment Actions
I noticed that I can reproduce this issue without my changes, created a PR for that: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=291616
Comment Actions
Rebased on top of commit daef625cf884, works fine now.
Testing plan:
bhyve -c 2 -m 4096 -S -A -I -u -H -P -s 0:0,hostbridge -l bootrom,/usr/local/share/uefi-firmware/BHYVE_UEFI.fd -s 1:0,lpc \ -s 7:0,virtio-blk,/data/img/ubuntu2510.img -l com1,stdio -s 8:0,virtio-net,slirp,mac=94:06:31:1a:a6:f7,mtu=4092,open \ # <-- set MTU here ubuntu2510
Inside the guest:
- iperf -c <iperf_server> -u -i 1 -P 8 -t 60 -b 500M
- while wget https://<your_freebsd_mirror>/releases/ISO-IMAGES/15.0/FreeBSD-15.0-RELEASE-arm64-aarch64-dvd1.iso -O /dev/null; do : ; done
Comment Actions
Thanks, this mostly looks good.
| usr.sbin/bhyve/net_backend_slirp.c | ||
|---|---|---|
| 69 | It would be better to call this DEFAULT_MTU or similar. | |
| 75 | size_t would be a better type for this. | |
| 161 | It would be better to do this allocation before the mevent_add_disabled() call, otherwise the error path needs some extra code to unregister the event. | |
| usr.sbin/bhyve/slirp/slirp-helper.c | ||
| 41 | It would be better to rename this DEFAULT_MTU or something like that. | |
Comment Actions
- SLIRP_MTU -> DEFAULT_MTU
- unsigned long -> size_t for mtu values
- move buf allocation before mevent_add_disabled(), update "err" cleanup routines to free() it
Comment Actions
I don't like that we have DEFAULT_MTU defined twice, but it's a bit hard to work around. We shouldn't modify the config tree in net_backend_slirp.c. Is there something else we can do?
Comment Actions
Do you mean the nvlist_clone() part for setting vmname? If so, could that be passed as a part of argv to bhyve-slirp-helper?
Comment Actions
Oh, I missed that we do that. In that case, can we please unconditionally pass along the MTU to the helper process so that it doesn't need to define a default?
Comment Actions
Drop the DEFAULT_MTU constant from the SLIRP code:
- Always pass MTU to the helper so it does not need to hardcode the default value,
- Make net_backend_slirp.c use ETHERMTU to match the pci_virtio_net.c default.
Comment Actions
While testing this approach I noticed that when MTU is not set on the command line, the SLIRP code default to 2048 (DEFAULT_MTU) and pci_virtio_net.c uses 1500 (ETHERMTU). I guess that it does not make too much sense to have different values for this case, so I updated the SLIRP code to use ETHERMTU too.
Another thing I noticed is that e1000 ignores the MTU configuration completely, I wonder if it makes sense to error out for it if users try to set MTU for it?
Comment Actions
With the suggested change this looks good, thank you very much.
| usr.sbin/bhyve/net_backend_slirp.c | ||
|---|---|---|
| 133 | ||
| 140 | Let's please do it like this: mtu_value = get_config_value_node(config, "mtu");
if (mtu_value != NULL) {
if (net_parsemtu(mtu_value, &mtu)) {
EPRINTLN("Could not parse MTU");
goto err;
}
} else {
mtu = DEFAULT_MTU;
}
nvlist_add_number(config, "mtui", mtu);Then, in the helper, you don't need to use strtoul(), just use nvlist_get_number("mtui"). | |
Comment Actions
Looks ok with the indentation fixed.
| usr.sbin/bhyve/net_backend_slirp.c | ||
|---|---|---|
| 140 | Indentation is wrong here. | |
Comment Actions
Please fix the commit message though, this is not a commit to bhyve.8 and it is not just adding things to the manual