On Tue, 22 Nov 2016, Mike Belopuhov wrote:
> 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.
No, that's wrong. vio_*_drain don't need to do anything with the rings
because the device is reset before they are called. (the virtio_reset()
call). It sounds like vmd does not implement the reset properly. When the
device is reset, it
* must stop all dma activities to the rings
* must stop interrupts
* should probably forget about the configured virtqueues.
It must do that before the io-port write to the status register finishes
and returns control to the guest.
As far as I can see, vio(4) does even the right thing if an interrupt
occurs between splnet() and virtio_reset(). It will call vio_rxeof() after
the reset, which will remove all used descriptors from the ring. Even if
vio_rx_intr() would be called after splx(), there cannot be any used
descriptors in the ring, the call to vio_rxeof() will return 0, and
vio_rx_intr() will do nothing. But this could be made more explicit (and
robust?) by putting a check for IF_RUNNING into vio_rx_intr().
> 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.
vio must reset the device because otherwise it could not remove
descriptors from the available ring before the device has used them. And
the device won't use them unless packets arrive.
> 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.
The reinit is necessary after reset.
Cheers,
Stefan
>
> 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);
> }
>
