vmd hackers and users seem to be able to trigger the assertion
below every time they do down and then up (with a dhclient for
instance):
panic: kernel diagnostic assertion "m != NULL" failed: file
"/usr/src/sys/dev/pci/if_vio.c", line 1008
Starting stack trace...
panic() at panic+0x10b
__assert() at __assert+0x25
vio_rxeof() at vio_rxeof+0x1db
vio_rx_intr() at vio_rx_intr+0x28
virtio_check_vqs() at virtio_check_vqs+0x8c
virtio_pci_legacy_intr() at virtio_pci_legacy_intr+0x71
intr_handler() at intr_handler+0x67
Xintr_legacy7() at Xintr_legacy7+0xdd
--- interrupt ---
The culprit is that vio_rx_drain and vio_tx_drain functions
are not properly implemented: they m_freem mbufs attached to
ring slots but do nothing about unconfiguring the backend
virtqueue. This results in us thinking that the ring is
empty when we UP the interface the next time, but then a
completion event arrives for the slot that doesn't have an
associated mbuf anymore.
To fix this properly a virtqueue draining code has to be
implemented, possibly based on what FreeBSD has. However,
there's nothing really wrong with leaving the rings the
way they are and "resuming" on UP, vio doesn't destroy
them anyway.
The other part of the diff is removing virtio_reinit_start
and virtio_negotiate_features. I'm not sure why does it
make sense to re-negotiate features on stop, but reinit
functions mess around the receive ring and if I just leave
them there, the interface will not be operational on UP.
So far this has been tested under vmd by rzalamena@ and
reyk@ but it would be nice to get it tested under the KVM.
diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
index 97af2a2..96b5fdf 100644
--- sys/dev/pci/if_vio.c
+++ sys/dev/pci/if_vio.c
@@ -596,10 +596,13 @@ vio_attach(struct device *parent, struct device *self,
void *aux)
ifmedia_set(&sc->sc_media, IFM_ETHER | IFM_AUTO);
vsc->sc_config_change = vio_config_change;
timeout_set(&sc->sc_txtick, vio_txtick, &sc->sc_vq[VQTX]);
timeout_set(&sc->sc_rxtick, vio_rxtick, &sc->sc_vq[VQRX]);
+ if_rxr_init(&sc->sc_rx_ring, 2 * ((ifp->if_hardmtu / MCLBYTES) + 1),
+ sc->sc_vq[VQRX].vq_num);
+
if_attach(ifp);
ether_ifattach(ifp);
return;
@@ -664,12 +667,10 @@ int
vio_init(struct ifnet *ifp)
{
struct vio_softc *sc = ifp->if_softc;
vio_stop(ifp, 0);
- if_rxr_init(&sc->sc_rx_ring, 2 * ((ifp->if_hardmtu / MCLBYTES) + 1),
- sc->sc_vq[VQRX].vq_num);
vio_populate_rx_mbufs(sc);
ifp->if_flags |= IFF_RUNNING;
ifq_clr_oactive(&ifp->if_snd);
vio_iff(sc);
vio_link_state(ifp);
@@ -689,21 +690,15 @@ vio_stop(struct ifnet *ifp, int disable)
/* only way to stop I/O and DMA is resetting... */
virtio_reset(vsc);
vio_rxeof(sc);
if (vsc->sc_nvqs >= 3)
vio_ctrleof(&sc->sc_vq[VQCTL]);
- vio_tx_drain(sc);
- if (disable)
- vio_rx_drain(sc);
- virtio_reinit_start(vsc);
- virtio_negotiate_features(vsc, vsc->sc_features, NULL);
virtio_start_vq_intr(vsc, &sc->sc_vq[VQRX]);
virtio_stop_vq_intr(vsc, &sc->sc_vq[VQTX]);
if (vsc->sc_nvqs >= 3)
virtio_start_vq_intr(vsc, &sc->sc_vq[VQCTL]);
- virtio_reinit_end(vsc);
if (vsc->sc_nvqs >= 3) {
if (sc->sc_ctrl_inuse != FREE)
sc->sc_ctrl_inuse = RESET;
wakeup(&sc->sc_ctrl_inuse);
}
