On Sun, May 23, 2021 at 10:25:38PM -0400, Dave Voutila wrote:
> The following diff adds in virtio 1.1's VIRTIO_NET_F_MTU feature support
> to vmd(8)'s virtio networking device. This allows for communicating an MTU
> to the guest driver and then enforcing it in the emulated device.
>
> When the feature is offered, per Virtio v1.1, 5.1.4.1 [1]:
>
> "The device MUST NOT pass received packets that exceed mtu (plus low
> level ethernet header length) size with gso_type NONE or ECN after
> VIRTIO_NET_F_MTU has been successfully negotiated."
>
> (GSO is not supported or negotiated, so it's always NONE. This is
> primarly because the vmd vionet device also doesn't support or negotiate
> checksum offloading.)
>
> The prior logic in place simply checked the packet was of a allowable
> size, which meant the largest IP packet (65535) plus an ethernet header.
>
> If testing the diff, you can change the VIONET_MTU definition to
> something other than 1500 and check that a non-OpenBSD guest defaults to
> using the value and forbids setting it higher. This is easy in an Alpine
> or Debian Linux guest using:
>
> a) to view the mtu: ip link
> b) to set the mtu: sudo ip link set dev <devname> mtu <mtu value>
>
> For example:
>
> dave@debian:~$ sudo ip link set dev enp0s2 mtu 1501
> Error: mtu greater than device maximum.
>
> Since the diff lacks context of the goto, it jumps to section that
> advances to the next ring
>
> Currently, vio(4) does not negotiate this feature and won't obey it. I'm
> working on that separately.
>
> OK? Feedback?
>
> [1]
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-2000004
>
> Index: virtio.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 virtio.c
> --- virtio.c 18 May 2021 11:06:43 -0000 1.87
> +++ virtio.c 24 May 2021 01:31:22 -0000
> @@ -60,6 +60,7 @@ int nr_vioblk;
>
> #define MAXPHYS (64 * 1024) /* max raw I/O transfer size */
>
> +#define VIRTIO_NET_F_MTU (1<<3)
> #define VIRTIO_NET_F_MAC (1<<5)
>
> #define VMMCI_F_TIMESYNC (1<<0)
> @@ -1046,6 +1047,26 @@ virtio_net_io(int dir, uint16_t reg, uin
> *data = dev->mac[reg -
> VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI];
> break;
> + case VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 10:
> + if (sz == 2) {
> + *data = VIONET_MTU;
> + } else if (sz == 1) {
> + *data &= 0xFF00;
> + *data |= (uint32_t)(VIONET_MTU) & 0xFF;
> + } else {
> + log_warnx("%s: illegal read of vionet_mtu",
> + __progname);
> + }
> + break;
> + case VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 11:
> + if (sz == 1) {
> + *data &= 0xFF00;
> + *data = (uint32_t)(VIONET_MTU >> 8) & 0xFF;
> + } else {
> + log_warnx("%s: illegal read of vionet_mtu",
> + __progname);
> + }
> + break;
Is it possible to get proper defines for these two options?
This VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 11 is ugly.
> case VIRTIO_CONFIG_DEVICE_FEATURES:
> *data = dev->cfg.device_feature;
> break;
> @@ -1437,7 +1458,7 @@ vionet_notify_tx(struct vionet_dev *dev)
> size_t pktsz, chunk_size = 0;
> ssize_t dhcpsz;
> int ret, num_enq, ofs, spc;
> - char *vr, *pkt, *dhcppkt;
> + char *vr, *pkt = NULL, *dhcppkt;
> struct vring_desc *desc, *pkt_desc, *hdr_desc;
> struct vring_avail *avail;
> struct vring_used *used;
> @@ -1505,12 +1526,13 @@ vionet_notify_tx(struct vionet_dev *dev)
> /* Remove virtio header descriptor len */
> pktsz -= hdr_desc->len;
>
> - /* Only allow buffer len < max IP packet + Ethernet header */
> - if (pktsz > IP_MAXPACKET + ETHER_HDR_LEN) {
> + /* Drop frames larger than our MTU + ethernet header */
> + if (pktsz > VIONET_MTU + ETHER_HDR_LEN) {
> log_warnx("%s: invalid packet size %lu", __func__,
> pktsz);
> - goto out;
> + goto drop_packet;
> }
> +
> pkt = malloc(pktsz);
> if (pkt == NULL) {
> log_warn("malloc error alloc packet buf");
> @@ -1590,6 +1612,7 @@ vionet_notify_tx(struct vionet_dev *dev)
> goto out;
> }
>
> + drop_packet:
> ret = 1;
> dev->cfg.isr_status = 1;
> used->ring[used->idx & VIONET_QUEUE_MASK].id = hdr_desc_idx;
> @@ -1965,6 +1988,9 @@ virtio_init(struct vmd_vm *vm, int child
> vionet[i].pxeboot = 1;
> vionet[i].idx = i;
> vionet[i].pci_id = id;
> +
> + /* Allow drivers to identify the MTU */
> + vionet[i].cfg.device_feature |= VIRTIO_NET_F_MTU;
>
> log_debug("%s: vm \"%s\" vio%u lladdr %s%s%s%s",
> __func__, vcp->vcp_name, i,
> Index: virtio.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.h,v
> retrieving revision 1.38
> diff -u -p -r1.38 virtio.h
> --- virtio.h 21 Apr 2021 18:27:36 -0000 1.38
> +++ virtio.h 24 May 2021 01:31:22 -0000
> @@ -35,6 +35,7 @@
>
> #define VIONET_QUEUE_SIZE 256
> #define VIONET_QUEUE_MASK (VIONET_QUEUE_SIZE - 1)
> +#define VIONET_MTU 1500
>
> /* VMM Control Interface shutdown timeout (in seconds) */
> #define VMMCI_TIMEOUT 3
>
>From a network perspective enforcing a MTU 1500 is not ideal in modern
days. In general devices should support jumbo frames (around 9000 bytes) or
at list mini-jumbo frames (something around 1536 bytes). With a hard limit
on 1500 bytes additional encapsulations like vlan(4) wont work properly.
Now since these network interfaces depend on tap(4) an option would be to
use TUNMRU (16384) as the max limit.
--
:wq Claudio
