On Wed, Nov 23, 2016 at 00:09 +0100, s...@openbsd.org wrote: > 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 > >
Yes, after looking closer at virtio code I agree with you. However, stop/init is purely OpenBSD specific action. There are no provisions or requirements from the hardware really. Thus we can treat UP/DOWN as purely software state and don't stop/reinit the PCI device.
