kern/52103: vioif(4) writes to VIRTIO_NET_S_LINK_UP that is read-only bit

[s-yamaguchi%iij.ad.jp@localhost]

>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:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 23 02:45:00 +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