On Mon, May 24, 2021 at 08:25:04AM +0200, Claudio Jeker wrote:
> 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.
>
We could fix the + 11 part, but about the best we could do would be something
like the following:
VIRTIO_CONFIG_NET_MTU
VIRTIO_CONFIG_NET_MTU + 1
VIRTIO_CONFIG_NET_MTU + 2
VIRTIO_CONFIG_NET_MTU + 3
Since this is a pci config space access and I've seen Linux use 1, 2, and 4 byte
accesses. But, yes, we could improve the actual name.
Once dv@ gets this in I'll go back and redo the other devices (since we do a
similar thing for those as well).
-ml
> > 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
>
