From: Mike Larkin Subject: Re: vio: Improve feature negotiation for LRO/TSO To: Stefan Fritsch Cc: tech@openbsd.org, Jan Klemkow Date: Mon, 12 Jan 2026 14:04:15 -0800 On Mon, Jan 12, 2026 at 04:18:22PM +0100, Stefan Fritsch wrote: > Hi, > > OpenBSD requires that LRO can be switched on and off for things like > bridged vlan(4), vxlan(4), bpe(4). We currently only support switching LRO > on/off if the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature was negotiated. But > this means if the hypervisor only offers VIRTIO_NET_F_GUEST_TSO4/6 but not > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, things will break. In this case we must > redo feature negotation without the GUEST_TSO4/6 features. > > Also, if the hypervisor offers GUEST_TSO4/6 but not the > VIRTIO_NET_F_MRG_RXBUF feature, we currently put rx buffers with a single > 4k mbuf into the rx queue while the standard says we SHOULD insert buffers > of at least 65562 bytes. Apple Virtualization refuses to work with this > configuration. As 65562 is larger than MAXMCLBYTES, we would need to > rework how we allocate our rx buffers to make this work. For now, we would > to like to simply disable GUEST_TSO4/6 if MRG_RXBUF is missing. > Unfortunately, Apple Virtualization still refuses to work unless > HOST_TSO4/6 is also disabled. Therefore, we disable all TSO if MRG_RXBUF > is missing. > > With lots of input from and tested by helg@ > > ok? > ok > Cheers, > Stefan > > --- > sys/dev/pv/if_vio.c | 48 +++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c > index 85849e1eb14..6eb2377009c 100644 > --- a/sys/dev/pv/if_vio.c > +++ b/sys/dev/pv/if_vio.c > @@ -624,7 +624,7 @@ vio_attach(struct device *parent, struct device *self, void *aux) > struct vio_softc *sc = (struct vio_softc *)self; > struct virtio_softc *vsc = (struct virtio_softc *)parent; > struct virtio_attach_args *va = aux; > - int i, r, tx_max_segments; > + int i, r, tx_max_segments, want_tso = 1; > struct ifnet *ifp = &sc->sc_ac.ac_if; > > if (vsc->sc_child != NULL) { > @@ -637,24 +637,48 @@ vio_attach(struct device *parent, struct device *self, void *aux) > > vsc->sc_child = self; > vsc->sc_ipl = IPL_NET | IPL_MPSAFE; > - vsc->sc_driver_features = VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | > - VIRTIO_NET_F_CTRL_VQ | VIRTIO_NET_F_CTRL_RX | > - VIRTIO_NET_F_MRG_RXBUF | VIRTIO_NET_F_CSUM | > - VIRTIO_F_RING_EVENT_IDX | VIRTIO_NET_F_GUEST_CSUM; > > +negotiate: > + vsc->sc_driver_features = VIRTIO_F_RING_EVENT_IDX; > + vsc->sc_driver_features |= VIRTIO_NET_F_MAC; > + vsc->sc_driver_features |= VIRTIO_NET_F_STATUS; > + vsc->sc_driver_features |= VIRTIO_NET_F_CTRL_VQ; > + vsc->sc_driver_features |= VIRTIO_NET_F_CTRL_RX; > + vsc->sc_driver_features |= VIRTIO_NET_F_MRG_RXBUF; > + vsc->sc_driver_features |= VIRTIO_NET_F_CSUM; > + vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_CSUM; > + if (want_tso) { > + vsc->sc_driver_features |= VIRTIO_NET_F_CTRL_GUEST_OFFLOADS; > + vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_TSO4; > + vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_TSO6; > + vsc->sc_driver_features |= VIRTIO_NET_F_HOST_TSO4; > + vsc->sc_driver_features |= VIRTIO_NET_F_HOST_TSO6; > + } > if (va->va_nintr > 3 && ncpus > 1) > vsc->sc_driver_features |= VIRTIO_NET_F_MQ; > > - vsc->sc_driver_features |= VIRTIO_NET_F_HOST_TSO4; > - vsc->sc_driver_features |= VIRTIO_NET_F_HOST_TSO6; > - > - vsc->sc_driver_features |= VIRTIO_NET_F_CTRL_GUEST_OFFLOADS; > - vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_TSO4; > - vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_TSO6; > - > if (virtio_negotiate_features(vsc, virtio_net_feature_names) != 0) > goto err; > > + if ((virtio_has_feature(vsc, VIRTIO_NET_F_GUEST_TSO4) || > + virtio_has_feature(vsc, VIRTIO_NET_F_GUEST_TSO6)) && > + (!virtio_has_feature(vsc, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) || > + !virtio_has_feature(vsc, VIRTIO_NET_F_MRG_RXBUF))) { > + /* > + * We only support VIRTIO_NET_F_GUEST_TSO4/6 if we also have > + * both VIRTIO_NET_F_CTRL_GUEST_OFFLOADS and > + * VIRTIO_NET_F_MRG_RXBUF. In order to make this known to the > + * hypervisor, we need to redo feature negotiation. > + * > + * Unfortunately, Apple Virtualisation does not work with > + * only VIRTIO_NET_F_HOST_TSO4/6. Therefore we disable all TSO > + * in the fallback case. > + */ > + want_tso = 0; > + virtio_reset(vsc); > + goto negotiate; > + } > + > if (virtio_has_feature(vsc, VIRTIO_NET_F_MQ)) { > i = virtio_read_device_config_2(vsc, > VIRTIO_NET_CONFIG_MAX_QUEUES); > -- > 2.49.0 >