Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 09:10:44PM +0100, Stefan Fritsch wrote:
> On Wed, 23 Nov 2016, Rafael Zalamena wrote:
>
> > > Maybe something like this is enough already (untested):
> >
> > I tried your diff without Mike's if_vio diff and it doesn't panic anymore,
> > however it doesn't work.
> >
> > vioX can send packets to host, host receives them and reply, but vioX
> > doesn't see any packets back. I don't even need to touch the interface
> > up/down status to see this happening. Also when the interface comes
> > up after being shutdown it sends a bunch of packets to host.
>
> Sorry, device_status is a bitmask, not a plain value.
>
> Try the patch below. The first hunk is to fix the 'sends a bunch of
> packets'. If it causes any problems, leave it out.
>
OK reyk@
> diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c
> index 93def73..6436e6a 100644
> --- usr.sbin/vmd/virtio.c
> +++ usr.sbin/vmd/virtio.c
> @@ -703,6 +703,13 @@ virtio_net_io(int dir, uint16_t reg, uint32_t *data,
> uint8_t *intr,
> break;
> case VIRTIO_CONFIG_DEVICE_STATUS:
> dev->cfg.device_status = *data;
> + if (*data == 0) {
> + dev->vq[0].last_avail = 0;
> + dev->vq[0].notified_avail = 0;
> + dev->vq[1].last_avail = 0;
> + dev->vq[1].notified_avail = 0;
> + /* XXX do proper reset */
> + }
> break;
> default:
> break;
> @@ -796,6 +803,9 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, ssize_t
> sz, int *spc)
>
> ret = 0;
>
> + if (!(dev->cfg.device_status & VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK))
> + return ret;
> +
> vr_sz = vring_size(VIONET_QUEUE_SIZE);
> q_gpa = dev->vq[0].qa;
> q_gpa = q_gpa * VIRTIO_PAGE_SIZE;
>
--
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 21:10 +0100, Stefan Fritsch wrote:
> On Wed, 23 Nov 2016, Rafael Zalamena wrote:
>
> > > Maybe something like this is enough already (untested):
> >
> > I tried your diff without Mike's if_vio diff and it doesn't panic anymore,
> > however it doesn't work.
> >
> > vioX can send packets to host, host receives them and reply, but vioX
> > doesn't see any packets back. I don't even need to touch the interface
> > up/down status to see this happening. Also when the interface comes
> > up after being shutdown it sends a bunch of packets to host.
>
> Sorry, device_status is a bitmask, not a plain value.
>
> Try the patch below. The first hunk is to fix the 'sends a bunch of
> packets'. If it causes any problems, leave it out.
>
> diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c
> index 93def73..6436e6a 100644
> --- usr.sbin/vmd/virtio.c
> +++ usr.sbin/vmd/virtio.c
> @@ -703,6 +703,13 @@ virtio_net_io(int dir, uint16_t reg, uint32_t *data,
> uint8_t *intr,
> break;
> case VIRTIO_CONFIG_DEVICE_STATUS:
> dev->cfg.device_status = *data;
> + if (*data == 0) {
> + dev->vq[0].last_avail = 0;
> + dev->vq[0].notified_avail = 0;
> + dev->vq[1].last_avail = 0;
> + dev->vq[1].notified_avail = 0;
> + /* XXX do proper reset */
> + }
> break;
> default:
> break;
> @@ -796,6 +803,9 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, ssize_t
> sz, int *spc)
>
> ret = 0;
>
> + if (!(dev->cfg.device_status & VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK))
> + return ret;
> +
> vr_sz = vring_size(VIONET_QUEUE_SIZE);
> q_gpa = dev->vq[0].qa;
> q_gpa = q_gpa * VIRTIO_PAGE_SIZE;
>
This diff works fine, huge thanks for stepping in!
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 09:10:44PM +0100, Stefan Fritsch wrote:
> On Wed, 23 Nov 2016, Rafael Zalamena wrote:
>
> > > Maybe something like this is enough already (untested):
> >
> > I tried your diff without Mike's if_vio diff and it doesn't panic anymore,
> > however it doesn't work.
> >
> > vioX can send packets to host, host receives them and reply, but vioX
> > doesn't see any packets back. I don't even need to touch the interface
> > up/down status to see this happening. Also when the interface comes
> > up after being shutdown it sends a bunch of packets to host.
>
> Sorry, device_status is a bitmask, not a plain value.
>
> Try the patch below. The first hunk is to fix the 'sends a bunch of
> packets'. If it causes any problems, leave it out.
This diff fixes the panic and makes the interface work again after a 'down'.
Thank you and Mike for fixing this. I'm going to play with this
more today and I can give feedbacks later if anything bad happens.
>
> diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c
> index 93def73..6436e6a 100644
> --- usr.sbin/vmd/virtio.c
> +++ usr.sbin/vmd/virtio.c
> @@ -703,6 +703,13 @@ virtio_net_io(int dir, uint16_t reg, uint32_t *data,
> uint8_t *intr,
> break;
> case VIRTIO_CONFIG_DEVICE_STATUS:
> dev->cfg.device_status = *data;
> + if (*data == 0) {
> + dev->vq[0].last_avail = 0;
> + dev->vq[0].notified_avail = 0;
> + dev->vq[1].last_avail = 0;
> + dev->vq[1].notified_avail = 0;
> + /* XXX do proper reset */
> + }
> break;
> default:
> break;
> @@ -796,6 +803,9 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, ssize_t
> sz, int *spc)
>
> ret = 0;
>
> + if (!(dev->cfg.device_status & VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK))
> + return ret;
> +
> vr_sz = vring_size(VIONET_QUEUE_SIZE);
> q_gpa = dev->vq[0].qa;
> q_gpa = q_gpa * VIRTIO_PAGE_SIZE;
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 09:10:44PM +0100, Stefan Fritsch wrote:
> On Wed, 23 Nov 2016, Rafael Zalamena wrote:
>
> > > Maybe something like this is enough already (untested):
> >
> > I tried your diff without Mike's if_vio diff and it doesn't panic anymore,
> > however it doesn't work.
> >
> > vioX can send packets to host, host receives them and reply, but vioX
> > doesn't see any packets back. I don't even need to touch the interface
> > up/down status to see this happening. Also when the interface comes
> > up after being shutdown it sends a bunch of packets to host.
>
> Sorry, device_status is a bitmask, not a plain value.
>
> Try the patch below. The first hunk is to fix the 'sends a bunch of
> packets'. If it causes any problems, leave it out.
>
While I have not tested any of these diffs, I certainly do appreciate help from
everyone to fix up vmd while I work on the kernel parts of vmm.
Thanks!
-ml
> diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c
> index 93def73..6436e6a 100644
> --- usr.sbin/vmd/virtio.c
> +++ usr.sbin/vmd/virtio.c
> @@ -703,6 +703,13 @@ virtio_net_io(int dir, uint16_t reg, uint32_t *data,
> uint8_t *intr,
> break;
> case VIRTIO_CONFIG_DEVICE_STATUS:
> dev->cfg.device_status = *data;
> + if (*data == 0) {
> + dev->vq[0].last_avail = 0;
> + dev->vq[0].notified_avail = 0;
> + dev->vq[1].last_avail = 0;
> + dev->vq[1].notified_avail = 0;
> + /* XXX do proper reset */
> + }
> break;
> default:
> break;
> @@ -796,6 +803,9 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, ssize_t
> sz, int *spc)
>
> ret = 0;
>
> + if (!(dev->cfg.device_status & VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK))
> + return ret;
> +
> vr_sz = vring_size(VIONET_QUEUE_SIZE);
> q_gpa = dev->vq[0].qa;
> q_gpa = q_gpa * VIRTIO_PAGE_SIZE;
>
Re: vio(4): fixup crash on up/down
On Wed, 23 Nov 2016, Rafael Zalamena wrote:
> > Maybe something like this is enough already (untested):
>
> I tried your diff without Mike's if_vio diff and it doesn't panic anymore,
> however it doesn't work.
>
> vioX can send packets to host, host receives them and reply, but vioX
> doesn't see any packets back. I don't even need to touch the interface
> up/down status to see this happening. Also when the interface comes
> up after being shutdown it sends a bunch of packets to host.
Sorry, device_status is a bitmask, not a plain value.
Try the patch below. The first hunk is to fix the 'sends a bunch of
packets'. If it causes any problems, leave it out.
diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c
index 93def73..6436e6a 100644
--- usr.sbin/vmd/virtio.c
+++ usr.sbin/vmd/virtio.c
@@ -703,6 +703,13 @@ virtio_net_io(int dir, uint16_t reg, uint32_t *data,
uint8_t *intr,
break;
case VIRTIO_CONFIG_DEVICE_STATUS:
dev->cfg.device_status = *data;
+ if (*data == 0) {
+ dev->vq[0].last_avail = 0;
+ dev->vq[0].notified_avail = 0;
+ dev->vq[1].last_avail = 0;
+ dev->vq[1].notified_avail = 0;
+ /* XXX do proper reset */
+ }
break;
default:
break;
@@ -796,6 +803,9 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, ssize_t
sz, int *spc)
ret = 0;
+ if (!(dev->cfg.device_status & VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK))
+ return ret;
+
vr_sz = vring_size(VIONET_QUEUE_SIZE);
q_gpa = dev->vq[0].qa;
q_gpa = q_gpa * VIRTIO_PAGE_SIZE;
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 09:03:46AM +0100, Stefan Fritsch wrote: > On Wed, 23 Nov 2016, Mike Belopuhov wrote: > > > I guess we could do that. But then we cannot free the mbufs on DOWN > > > until the device has used them. > > > > Diff to this effect is below. Works on vmd and qemu (original > > one didn't because I kept the virtio_reset). > > > > > That sounds like an unnecessary waste of memory to me. > > > > > > > This is not so much memory we lose and then if you up it again > > you're going to have it all back. We can revert to the present > > behavior once vmd matures, in the meantime people won't have to > > juggle diffs around in their trees :) > > I am not convinced. Doing a reset allows to recover from all kinds of > problems with DOWN/UP. That was useful when we had bugs in the event_idx > implementation. > > Also, I don't like to change code that is known to work with at least 4 > independent device implementations to work around problems in one > incomplete implementation that we can easily change. > > Maybe something like this is enough already (untested): I tried your diff without Mike's if_vio diff and it doesn't panic anymore, however it doesn't work. vioX can send packets to host, host receives them and reply, but vioX doesn't see any packets back. I don't even need to touch the interface up/down status to see this happening. Also when the interface comes up after being shutdown it sends a bunch of packets to host. > > --- usr.sbin/vmd/virtio.c 2016-10-20 05:05:49.049943724 +0200 > +++ usr.sbin/vmd/virtio.c 2016-11-23 08:55:38.829501275 +0100 > @@ -796,6 +796,9 @@ > > ret = 0; > > + if (dev->cfg.device_status != VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK) > + return ret; > + > vr_sz = vring_size(VIONET_QUEUE_SIZE); > q_gpa = dev->vq[0].qa; > q_gpa = q_gpa * VIRTIO_PAGE_SIZE; >
Re: vio(4): fixup crash on up/down
On Wed, 23 Nov 2016, Mike Belopuhov wrote: > > I am not convinced. Doing a reset allows to recover from all kinds of > > problems with DOWN/UP. That was useful when we had bugs in the event_idx > > implementation. > > > > I don't think this justifies it since bugs need to be fixed regardless. And vmd bugs should be fixed in vmd.
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 09:03 +0100, Stefan Fritsch wrote: > On Wed, 23 Nov 2016, Mike Belopuhov wrote: > > > I guess we could do that. But then we cannot free the mbufs on DOWN > > > until the device has used them. > > > > Diff to this effect is below. Works on vmd and qemu (original > > one didn't because I kept the virtio_reset). > > > > > That sounds like an unnecessary waste of memory to me. > > > > > > > This is not so much memory we lose and then if you up it again > > you're going to have it all back. We can revert to the present > > behavior once vmd matures, in the meantime people won't have to > > juggle diffs around in their trees :) > > I am not convinced. Doing a reset allows to recover from all kinds of > problems with DOWN/UP. That was useful when we had bugs in the event_idx > implementation. > I don't think this justifies it since bugs need to be fixed regardless. > Also, I don't like to change code that is known to work with at least 4 > independent device implementations to work around problems in one > incomplete implementation that we can easily change. > > Maybe something like this is enough already (untested): > The diff didn't help.
Re: vio(4): fixup crash on up/down
On Wed, 23 Nov 2016, Mike Belopuhov wrote: > > I guess we could do that. But then we cannot free the mbufs on DOWN > > until the device has used them. > > Diff to this effect is below. Works on vmd and qemu (original > one didn't because I kept the virtio_reset). > > > That sounds like an unnecessary waste of memory to me. > > > > This is not so much memory we lose and then if you up it again > you're going to have it all back. We can revert to the present > behavior once vmd matures, in the meantime people won't have to > juggle diffs around in their trees :) I am not convinced. Doing a reset allows to recover from all kinds of problems with DOWN/UP. That was useful when we had bugs in the event_idx implementation. Also, I don't like to change code that is known to work with at least 4 independent device implementations to work around problems in one incomplete implementation that we can easily change. Maybe something like this is enough already (untested): --- usr.sbin/vmd/virtio.c 2016-10-20 05:05:49.049943724 +0200 +++ usr.sbin/vmd/virtio.c 2016-11-23 08:55:38.829501275 +0100 @@ -796,6 +796,9 @@ ret = 0; + if (dev->cfg.device_status != VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK) + return ret; + vr_sz = vring_size(VIONET_QUEUE_SIZE); q_gpa = dev->vq[0].qa; q_gpa = q_gpa * VIRTIO_PAGE_SIZE;
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 00:42 +0100, Stefan Fritsch wrote:
> On Wednesday, 23 November 2016 00:34:42 CET Mike Belopuhov wrote:
> > 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.
>
> I guess we could do that. But then we cannot free the mbufs on DOWN
> until the device has used them.
Diff to this effect is below. Works on vmd and qemu (original
one didn't because I kept the virtio_reset).
> That sounds like an unnecessary waste of memory to me.
>
This is not so much memory we lose and then if you up it again
you're going to have it all back. We can revert to the present
behavior once vmd matures, in the meantime people won't have to
juggle diffs around in their trees :)
The diff is meant to be applied on top of the previous two.
Please note I've also added a virtio_stop_vq_intr to vio_stop.
diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
index 68e6636..156c16f 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;
@@ -665,19 +668,14 @@ vio_init(struct ifnet *ifp)
{
struct vio_softc *sc = ifp->if_softc;
struct virtio_softc *vsc = sc->sc_virtio;
vio_stop(ifp, 0);
- if_rxr_init(&sc->sc_rx_ring, 2 * ((ifp->if_hardmtu / MCLBYTES) + 1),
- sc->sc_vq[VQRX].vq_num);
- 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);
vio_populate_rx_mbufs(sc);
ifp->if_flags |= IFF_RUNNING;
ifq_clr_oactive(&ifp->if_snd);
vio_iff(sc);
vio_link_state(ifp);
@@ -692,18 +690,17 @@ vio_stop(struct ifnet *ifp, int disable)
timeout_del(&sc->sc_txtick);
timeout_del(&sc->sc_rxtick);
ifp->if_flags &= ~IFF_RUNNING;
ifq_clr_oactive(&ifp->if_snd);
- /* 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_stop_vq_intr(vsc, &sc->sc_vq[VQRX]);
+ if (vsc->sc_nvqs >= 3)
+ virtio_stop_vq_intr(vsc, &sc->sc_vq[VQCTL]);
if (vsc->sc_nvqs >= 3) {
if (sc->sc_ctrl_inuse != FREE)
sc->sc_ctrl_inuse = RESET;
wakeup(&sc->sc_ctrl_inuse);
@@ -1029,22 +1026,26 @@ vio_rxeof(struct vio_softc *sc)
mlast = m;
bufs_left--;
}
if (bufs_left == 0) {
- ml_enqueue(&ml, m0);
+ if (ifp->if_flags & IFF_RUNNING)
+ ml_enqueue(&ml, m0);
+ else
+ m_freem(m0);
m0 = NULL;
}
}
if (m0 != NULL) {
DBGPRINT("expected %d buffers, got %d", (int)hdr->num_buffers,
(int)hdr->num_buffers - bufs_left);
ifp->if_ierrors++;
m_freem(m0);
}
- if_input(ifp, &ml);
+ if (ifp->if_flags & IFF_RUNNING)
+ if_input(ifp, &ml);
return r;
}
int
vio_rx_intr(struct virtqueue *vq)
Re: vio(4): fixup crash on up/down
On Wednesday, 23 November 2016 00:34:42 CET Mike Belopuhov wrote: > 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. I guess we could do that. But then we cannot free the mbufs on DOWN until the device has used them. That sounds like an unnecessary waste of memory to me.
Re: vio(4): fixup crash on up/down
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.
Re: vio(4): fixup crash on up/down
On Wed, Nov 23, 2016 at 12:09:57AM +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 It doesn't. At least not yet.
Re: vio(4): fixup crash on up/down
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);
> }
>
Re: vio(4): fixup crash on up/down
On Tue, Nov 22, 2016 at 09:45:30PM +0100, 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.
>
> 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.
>
This has been on my to-do list to look at for a while, thanks a ton
mikeb for tackling this.
-ml
>
>
> 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);
> }
>
vio(4): fixup crash on up/down
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);
}
