NetBSD Problem Report #52103
From www@NetBSD.org Thu Mar 23 02:44:40 2017
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
by mollari.NetBSD.org (Postfix) with ESMTPS id 4CE737A27B
for <gnats-bugs@gnats.NetBSD.org>; Thu, 23 Mar 2017 02:44:40 +0000 (UTC)
Message-Id: <20170323024438.EDCA17A2B2@mollari.NetBSD.org>
Date: Thu, 23 Mar 2017 02:44:38 +0000 (UTC)
From: s-yamaguchi@iij.ad.jp
Reply-To: s-yamaguchi@iij.ad.jp
To: gnats-bugs@NetBSD.org
Subject: vioif(4) writes to VIRTIO_NET_S_LINK_UP that is read-only bit
X-Send-Pr-Version: www-1.0
>Number: 52103
>Category: kern
>Synopsis: vioif(4) writes to VIRTIO_NET_S_LINK_UP that is read-only bit
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: kern-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Thu Mar 23 02:45:00 +0000 2017
>Closed-Date: Tue Mar 28 04:28:14 +0000 2017
>Last-Modified: Tue Mar 28 04:28:14 +0000 2017
>Originator: Shoichi Yamaguchi
>Release: -current
>Organization:
Internet Initiative Japan Inc.
>Environment:
>Description:
According to Virtio PCI Card Specification v0.9.5
(http://ozlabs.org/~rusty/virtio-spec/.), VIRTIO_NET_S_LINK_UP bit is
defined as read-only.But, vioif(4) writes values in there to control
link status when the interface up and down. By this problem, the hung
up is happened on Google Compute Engine if MSI-X is disable.
>How-To-Repeat:
Up or down the interface.
- ifconfig vioif0 up
- ifconfig vioif0 down
>Fix:
patch 1: Use virtio_reset() for a pre-init state to realize link
down instead of write 0 to VIRTIO_NET_S_LINK_UP
patch 2: Add the handler for config change interrupt to synchronize
with hypervisors link state
I referred the FreeBSD implementation for the two patches
===== patch 1 =====
diff --git a/sys/dev/pci/if_vioif.c b/sys/dev/pci/if_vioif.c
index e7fdbf2..68b9923 100644
--- a/sys/dev/pci/if_vioif.c
+++ b/sys/dev/pci/if_vioif.c
@@ -267,7 +267,6 @@ static int vioif_tx_vq_done_locked(struct virtqueue *);
static void vioif_tx_drain(struct vioif_softc *);
/* other control */
-static int vioif_updown(struct vioif_softc *, bool);
static int vioif_ctrl_rx(struct vioif_softc *, int, bool);
static int vioif_set_promisc(struct vioif_softc *, bool);
static int vioif_set_allmulti(struct vioif_softc *, bool);
@@ -719,12 +718,19 @@ static int
vioif_init(struct ifnet *ifp)
{
struct vioif_softc *sc = ifp->if_softc;
+ struct virtio_softc *vsc = sc->sc_virtio;
vioif_stop(ifp, 0);
- if (!sc->sc_deferred_init_done) {
- struct virtio_softc *vsc = sc->sc_virtio;
+ virtio_reinit_start(vsc);
+ virtio_negotiate_features(vsc, vsc->sc_features);
+ virtio_start_vq_intr(vsc, &sc->sc_vq[VQ_RX]);
+ virtio_stop_vq_intr(vsc, &sc->sc_vq[VQ_TX]);
+ if (vsc->sc_nvqs >= 3)
+ virtio_start_vq_intr(vsc, &sc->sc_vq[VQ_CTRL]);
+ virtio_reinit_end(vsc);
+ if (!sc->sc_deferred_init_done) {
sc->sc_deferred_init_done = 1;
if (vsc->sc_nvqs == 3)
vioif_deferred_init(sc->sc_dev);
@@ -735,7 +741,6 @@ vioif_init(struct ifnet *ifp)
vioif_populate_rx_mbufs(sc);
- vioif_updown(sc, true);
ifp->if_flags |= IFF_RUNNING;
ifp->if_flags &= ~IFF_OACTIVE;
vioif_rx_filter(sc);
@@ -756,6 +761,12 @@ vioif_stop(struct ifnet *ifp, int disable)
VIOIF_RX_UNLOCK(sc);
VIOIF_TX_UNLOCK(sc);
+ /* disable interrupts */
+ virtio_stop_vq_intr(vsc, &sc->sc_vq[VQ_RX]);
+ virtio_stop_vq_intr(vsc, &sc->sc_vq[VQ_TX]);
+ if (vsc->sc_nvqs >= 3)
+ virtio_stop_vq_intr(vsc, &sc->sc_vq[VQ_CTRL]);
+
/* only way to stop I/O and DMA is resetting... */
virtio_reset(vsc);
vioif_rx_deq(sc);
@@ -764,15 +775,6 @@ vioif_stop(struct ifnet *ifp, int disable)
if (disable)
vioif_rx_drain(sc);
-
- virtio_reinit_start(vsc);
- virtio_negotiate_features(vsc, vsc->sc_features);
- virtio_start_vq_intr(vsc, &sc->sc_vq[VQ_RX]);
- virtio_stop_vq_intr(vsc, &sc->sc_vq[VQ_TX]);
- if (vsc->sc_nvqs >= 3)
- virtio_start_vq_intr(vsc, &sc->sc_vq[VQ_CTRL]);
- virtio_reinit_end(vsc);
- vioif_updown(sc, false);
}
static void
@@ -1502,20 +1504,6 @@ set:
return r;
}
-/* change link status */
-static int
-vioif_updown(struct vioif_softc *sc, bool isup)
-{
- struct virtio_softc *vsc = sc->sc_virtio;
-
- if (!(vsc->sc_features & VIRTIO_NET_F_STATUS))
- return ENODEV;
- virtio_write_device_config_1(vsc,
- VIRTIO_NET_CONFIG_STATUS,
- isup?VIRTIO_NET_S_LINK_UP:0);
- return 0;
-}
-
MODULE(MODULE_CLASS_DRIVER, if_vioif, "virtio");
#ifdef _MODULE
===== patch 1 =====
===== patch 2 =====
diff --git a/sys/dev/pci/if_vioif.c b/sys/dev/pci/if_vioif.c
index 68b9923..f9a80e1 100644
--- a/sys/dev/pci/if_vioif.c
+++ b/sys/dev/pci/if_vioif.c
@@ -190,6 +190,7 @@ struct vioif_softc {
uint8_t sc_mac[ETHER_ADDR_LEN];
struct ethercom sc_ethercom;
short sc_deferred_init_done;
+ bool sc_link_active;
/* bus_dmamem */
bus_dma_segment_t sc_hdr_segs[1];
@@ -218,6 +219,7 @@ struct vioif_softc {
bus_dmamap_t sc_ctrl_tbl_mc_dmamap;
void *sc_rx_softint;
+ void *sc_ctl_softint;
enum {
FREE, INUSE, DONE
@@ -267,12 +269,16 @@ static int vioif_tx_vq_done_locked(struct virtqueue *);
static void vioif_tx_drain(struct vioif_softc *);
/* other control */
+static bool vioif_is_link_up(struct vioif_softc *);
+static void vioif_update_link_status(struct vioif_softc *);
static int vioif_ctrl_rx(struct vioif_softc *, int, bool);
static int vioif_set_promisc(struct vioif_softc *, bool);
static int vioif_set_allmulti(struct vioif_softc *, bool);
static int vioif_set_rx_filter(struct vioif_softc *);
static int vioif_rx_filter(struct vioif_softc *);
static int vioif_ctrl_vq_done(struct virtqueue *);
+static int vioif_config_change(struct virtio_softc *);
+static void vioif_ctl_softint(void *);
CFATTACH_DECL_NEW(vioif, sizeof(struct vioif_softc),
vioif_match, vioif_attach, NULL, NULL);
@@ -523,11 +529,12 @@ vioif_attach(device_t parent, device_t self, void *aux)
sc->sc_dev = self;
sc->sc_virtio = vsc;
+ sc->sc_link_active = false;
vsc->sc_child = self;
vsc->sc_ipl = IPL_NET;
vsc->sc_vqs = &sc->sc_vq[0];
- vsc->sc_config_change = NULL;
+ vsc->sc_config_change = vioif_config_change;
vsc->sc_intrhand = virtio_vq_intr;
vsc->sc_flags = 0;
@@ -651,7 +658,13 @@ skip:
#endif
sc->sc_rx_softint = softint_establish(flags, vioif_rx_softint, sc);
if (sc->sc_rx_softint == NULL) {
- aprint_error_dev(self, "cannot establish softint\n");
+ aprint_error_dev(self, "cannot establish rx softint\n");
+ goto err;
+ }
+
+ sc->sc_ctl_softint = softint_establish(flags, vioif_ctl_softint, sc);
+ if (sc->sc_ctl_softint == NULL) {
+ aprint_error_dev(self, "cannot establish ctl softint\n");
goto err;
}
@@ -741,6 +754,7 @@ vioif_init(struct ifnet *ifp)
vioif_populate_rx_mbufs(sc);
+ vioif_update_link_status(sc);
ifp->if_flags |= IFF_RUNNING;
ifp->if_flags &= ~IFF_OACTIVE;
vioif_rx_filter(sc);
@@ -772,6 +786,7 @@ vioif_stop(struct ifnet *ifp, int disable)
vioif_rx_deq(sc);
vioif_tx_drain(sc);
ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
+ sc->sc_link_active = false;
if (disable)
vioif_rx_drain(sc);
@@ -788,7 +803,8 @@ vioif_start(struct ifnet *ifp)
VIOIF_TX_LOCK(sc);
- if ((ifp->if_flags & (IFF_RUNNING|IFF_OACTIVE)) != IFF_RUNNING)
+ if ((ifp->if_flags & (IFF_RUNNING|IFF_OACTIVE)) != IFF_RUNNING ||
+ !sc->sc_link_active)
goto out;
if (sc->sc_stopping)
@@ -1504,6 +1520,82 @@ set:
return r;
}
+static bool
+vioif_is_link_up(struct vioif_softc *sc)
+{
+ struct virtio_softc *vsc = sc->sc_virtio;
+ uint16_t status;
+
+ if (vsc->sc_features & VIRTIO_NET_F_STATUS)
+ status = virtio_read_device_config_2(vsc,
+ VIRTIO_NET_CONFIG_STATUS);
+ else
+ status = VIRTIO_NET_S_LINK_UP;
+
+ return ((status & VIRTIO_NET_S_LINK_UP) != 0);
+}
+
+/* change link status */
+static void
+vioif_update_link_status(struct vioif_softc *sc)
+{
+ struct ifnet *ifp = &sc->sc_ethercom.ec_if;
+ bool active, changed;
+ int link;
+
+ active = vioif_is_link_up(sc);
+ changed = false;
+
+ VIOIF_TX_LOCK(sc);
+ if (active) {
+ if (!sc->sc_link_active)
+ changed = true;
+
+ link = LINK_STATE_UP;
+ sc->sc_link_active = true;
+ } else {
+ if (sc->sc_link_active)
+ changed = true;
+
+ link = LINK_STATE_DOWN;
+ sc->sc_link_active = false;
+ }
+ VIOIF_TX_UNLOCK(sc);
+
+ if (changed)
+ if_link_state_change(ifp, link);
+}
+
+static int
+vioif_config_change(struct virtio_softc *vsc)
+{
+ struct vioif_softc *sc = device_private(vsc->sc_child);
+
+#ifdef VIOIF_SOFTINT_INTR
+ struct ifnet *ifp = &sc->sc_ethercom.ec_if;
+#endif
+
+#ifdef VIOIF_SOFTINT_INTR
+ KASSERT(!cpu_intr_p());
+ vioif_update_link_status(sc);
+ vioif_start(ifp);
+#else
+ softint_schedule(sc->sc_ctl_softint);
+#endif
+
+ return 0;
+}
+
+static void
+vioif_ctl_softint(void *arg)
+{
+ struct vioif_softc *sc = arg;
+ struct ifnet *ifp = &sc->sc_ethercom.ec_if;
+
+ vioif_update_link_status(sc);
+ vioif_start(ifp);
+}
+
MODULE(MODULE_CLASS_DRIVER, if_vioif, "virtio");
#ifdef _MODULE
diff --git a/sys/dev/pci/virtio.c b/sys/dev/pci/virtio.c
index 8efda6b..063b3e0 100644
--- a/sys/dev/pci/virtio.c
+++ b/sys/dev/pci/virtio.c
@@ -653,10 +653,11 @@ static int
virtio_msix_config_intr(void *arg)
{
struct virtio_softc *sc = arg;
+ int r = 0;
- /* TODO: handle events */
- aprint_debug_dev(sc->sc_dev, "%s\n", __func__);
- return 1;
+ if (sc->sc_config_change != NULL)
+ r = (sc->sc_config_change)(sc);
+ return r;
}
static void
>Release-Note:
>Audit-Trail:
From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52103 CVS commit: src/sys/dev/pci
Date: Tue, 28 Mar 2017 04:09:53 +0000
Module Name: src
Committed By: ozaki-r
Date: Tue Mar 28 04:09:52 UTC 2017
Modified Files:
src/sys/dev/pci: if_vioif.c
Log Message:
Don't write to read-only VIRTIO_NET_S_LINK_UP bit
The bit is defined as read-only in the Virtio PCI Card Specification.
The fix is inspired by FreeBSD.
PR kern/52103 by s-yamaguchi@IIJ
To generate a diff of this commit:
cvs rdiff -u -r1.32 -r1.33 src/sys/dev/pci/if_vioif.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52103 CVS commit: src/sys/dev/pci
Date: Tue, 28 Mar 2017 04:10:33 +0000
Module Name: src
Committed By: ozaki-r
Date: Tue Mar 28 04:10:33 UTC 2017
Modified Files:
src/sys/dev/pci: if_vioif.c virtio.c
Log Message:
Handle config change interrupts to inhibit sending packets while link down
PR kern/52103 by s-yamaguchi@IIJ
To generate a diff of this commit:
cvs rdiff -u -r1.33 -r1.34 src/sys/dev/pci/if_vioif.c
cvs rdiff -u -r1.26 -r1.27 src/sys/dev/pci/virtio.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->closed
State-Changed-By: ozaki-r@NetBSD.org
State-Changed-When: Tue, 28 Mar 2017 04:28:14 +0000
State-Changed-Why:
The patches have been committed. Thanks.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.