Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/netbsd-10]: src/sys/dev Pull up following revision(s) (requested by yama...



details:   https://anonhg.NetBSD.org/src/rev/46ee90e15577
branches:  netbsd-10
changeset: 376158:46ee90e15577
user:      martin <martin%NetBSD.org@localhost>
date:      Sat Jun 03 14:40:25 2023 +0000

description:
Pull up following revision(s) (requested by yamaguchi in ticket #186):

        sys/dev/pci/virtio_pci.c: revision 1.41
        sys/dev/pci/virtio_pci.c: revision 1.42
        sys/dev/virtio/virtio_mmio.c: revision 1.10
        sys/dev/pci/virtiovar.h: revision 1.29
        sys/dev/pci/virtio.c: revision 1.75
        sys/dev/pci/virtio.c: revision 1.76
        sys/dev/pci/virtio.c: revision 1.77
        sys/dev/pci/virtio.c: revision 1.78

virtio@pci: Fix assertion on detach.

If the child never attached in the first place, it's OK for it to not
have detached.

XXX This should not be a set of flags; this should be a state
enumeration, because some flags make no sense, like FINISHED|FAILED.

XXX This should not be asserted separately in each bus; there should
be a single place in virtio.c to assert this, uniformly in all buses.

PR kern/57357


Use enumeration for state of a child driver instead of flags
and check its detaching by using sc->sc_child in virtio_softc
pointed out by riastradh, thanks.
fixes PR/57357

Fix not to allocate unnecessary descriptor
fixes PR/57358

virtio(4): change variable name, nfc

virtio(4): change members of struct vring_desc_extra before free a slot

This prevents the following race condition.
1. Thread-A: calls virtio_dequeue_commit() and
             puts a slot into free descriptor chain in vq_free_slot()
2. Thread-B: calls virtio_enqueue_prep() and get the slot stored by Thread-A
3. Thread-B: calls virtio_enqueue_reserve() and
             changes desc_base and desc_free_idx for the slot
4. Thread-A: changes the same members updated by Thread-B
reported by hannken, thanks.

diffstat:

 sys/dev/pci/virtio.c         |  75 ++++++++++++++++++++++++-------------------
 sys/dev/pci/virtio_pci.c     |   8 ++--
 sys/dev/pci/virtiovar.h      |  11 +++--
 sys/dev/virtio/virtio_mmio.c |   6 +-
 4 files changed, 55 insertions(+), 45 deletions(-)

diffs (273 lines):

diff -r c6b6da0cae44 -r 46ee90e15577 sys/dev/pci/virtio.c
--- a/sys/dev/pci/virtio.c      Sat Jun 03 14:33:55 2023 +0000
+++ b/sys/dev/pci/virtio.c      Sat Jun 03 14:40:25 2023 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: virtio.c,v 1.63.2.4 2023/05/13 10:56:10 martin Exp $   */
+/*     $NetBSD: virtio.c,v 1.63.2.5 2023/06/03 14:40:25 martin Exp $   */
 
 /*
  * Copyright (c) 2020 The NetBSD Foundation, Inc.
@@ -28,7 +28,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: virtio.c,v 1.63.2.4 2023/05/13 10:56:10 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: virtio.c,v 1.63.2.5 2023/06/03 14:40:25 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -944,12 +944,12 @@ vq_alloc_slot_locked(struct virtio_softc
     size_t nslots)
 {
        struct vring_desc *vd;
-       uint16_t rv, tail;
+       uint16_t head, tail;
        size_t i;
 
        KASSERT(mutex_owned(&vq->vq_freedesc_lock));
 
-       tail = virtio_rw16(sc, vq->vq_free_idx);
+       head = tail = virtio_rw16(sc, vq->vq_free_idx);
        for (i = 0; i < nslots - 1; i++) {
                if (tail == VRING_DESC_CHAIN_END)
                        return VRING_DESC_CHAIN_END;
@@ -962,13 +962,11 @@ vq_alloc_slot_locked(struct virtio_softc
        if (tail == VRING_DESC_CHAIN_END)
                return VRING_DESC_CHAIN_END;
 
-       rv = virtio_rw16(sc, vq->vq_free_idx);
-
        vd = &vq->vq_desc[tail];
        vd->flags = virtio_rw16(sc, 0);
        vq->vq_free_idx = vd->next;
 
-       return rv;
+       return head;
 }
 static uint16_t
 vq_alloc_slot(struct virtio_softc *sc, struct virtqueue *vq, size_t nslots)
@@ -1096,17 +1094,18 @@ virtio_enqueue_reserve(struct virtio_sof
                }
                vd[i].flags  = virtio_rw16(sc, 0);
        } else {
-               uint16_t s;
+               if (nsegs > 1) {
+                       uint16_t s;
 
-               s = vq_alloc_slot(sc, vq, nsegs - 1);
-               if (s == VRING_DESC_CHAIN_END) {
-                       vq_free_slot(sc, vq, slot);
-                       return EAGAIN;
+                       s = vq_alloc_slot(sc, vq, nsegs - 1);
+                       if (s == VRING_DESC_CHAIN_END) {
+                               vq_free_slot(sc, vq, slot);
+                               return EAGAIN;
+                       }
+                       vd->next = virtio_rw16(sc, s);
+                       vd->flags = virtio_rw16(sc, VRING_DESC_F_NEXT);
                }
 
-               vd->next = virtio_rw16(sc, s);
-               vd->flags = virtio_rw16(sc, VRING_DESC_F_NEXT);
-
                vdx->desc_base = &vq->vq_desc[0];
                vdx->desc_free_idx = slot;
        }
@@ -1259,12 +1258,12 @@ virtio_enqueue_abort(struct virtio_softc
 {
        struct vring_desc_extra *vdx;
 
-       vq_free_slot(sc, vq, slot);
-
        vdx = &vq->vq_descx[slot];
        vdx->desc_free_idx = VRING_DESC_CHAIN_END;
        vdx->desc_base = NULL;
 
+       vq_free_slot(sc, vq, slot);
+
        return 0;
 }
 
@@ -1309,12 +1308,12 @@ virtio_dequeue_commit(struct virtio_soft
 {
        struct vring_desc_extra *vdx;
 
-       vq_free_slot(sc, vq, slot);
-
        vdx = &vq->vq_descx[slot];
        vdx->desc_base = NULL;
        vdx->desc_free_idx = VRING_DESC_CHAIN_END;
 
+       vq_free_slot(sc, vq, slot);
+
        return 0;
 }
 
@@ -1328,7 +1327,7 @@ virtio_child_attach_start(struct virtio_
        char buf[1024];
 
        KASSERT(sc->sc_child == NULL);
-       KASSERT(!ISSET(sc->sc_child_flags, VIRTIO_CHILD_DETACHED));
+       KASSERT(sc->sc_child_state == VIRTIO_NO_CHILD);
 
        sc->sc_child = child;
        sc->sc_ipl = ipl;
@@ -1404,7 +1403,7 @@ virtio_child_attach_finish(struct virtio
                }
        }
 
-       SET(sc->sc_child_flags, VIRTIO_CHILD_ATTACH_FINISHED);
+       sc->sc_child_state = VIRTIO_CHILD_ATTACH_FINISHED;
        virtio_set_status(sc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
        return 0;
 
@@ -1425,10 +1424,9 @@ virtio_child_detach(struct virtio_softc 
 {
 
        /* already detached */
-       if (ISSET(sc->sc_child_flags, VIRTIO_CHILD_DETACHED))
+       if (sc->sc_child == NULL)
                return;
 
-       sc->sc_vqs = NULL;
 
        virtio_device_reset(sc);
 
@@ -1439,7 +1437,8 @@ virtio_child_detach(struct virtio_softc 
                sc->sc_soft_ih = NULL;
        }
 
-       SET(sc->sc_child_flags, VIRTIO_CHILD_DETACHED);
+       sc->sc_vqs = NULL;
+       sc->sc_child = NULL;
 }
 
 void
@@ -1449,7 +1448,7 @@ virtio_child_attach_failed(struct virtio
 
        virtio_set_status(sc, VIRTIO_CONFIG_DEVICE_STATUS_FAILED);
 
-       SET(sc->sc_child_flags, VIRTIO_CHILD_ATTACH_FAILED);
+       sc->sc_child_state = VIRTIO_CHILD_ATTACH_FAILED;
 }
 
 bus_dma_tag_t
@@ -1485,19 +1484,29 @@ virtio_attach_failed(struct virtio_softc
        if (sc->sc_childdevid == 0)
                return 1;
 
-       if (ISSET(sc->sc_child_flags, VIRTIO_CHILD_ATTACH_FAILED)) {
-               aprint_error_dev(self, "virtio configuration failed\n");
-               return 1;
-       }
+       if (sc->sc_child == NULL) {
+               switch (sc->sc_child_state) {
+               case VIRTIO_CHILD_ATTACH_FAILED:
+                       aprint_error_dev(self,
+                           "virtio configuration failed\n");
+                       break;
+               case VIRTIO_NO_CHILD:
+                       aprint_error_dev(self,
+                           "no matching child driver; not configured\n");
+                       break;
+               default:
+                       /* sanity check */
+                       aprint_error_dev(self,
+                           "virtio internal error, "
+                           "child driver is not configured\n");
+                       break;
+               }
 
-       if (sc->sc_child == NULL) {
-               aprint_error_dev(self,
-                   "no matching child driver; not configured\n");
                return 1;
        }
 
        /* sanity check */
-       if (!ISSET(sc->sc_child_flags, VIRTIO_CHILD_ATTACH_FINISHED)) {
+       if (sc->sc_child_state != VIRTIO_CHILD_ATTACH_FINISHED) {
                aprint_error_dev(self, "virtio internal error, child driver "
                    "signaled OK but didn't initialize interrupts\n");
                return 1;
diff -r c6b6da0cae44 -r 46ee90e15577 sys/dev/pci/virtio_pci.c
--- a/sys/dev/pci/virtio_pci.c  Sat Jun 03 14:33:55 2023 +0000
+++ b/sys/dev/pci/virtio_pci.c  Sat Jun 03 14:40:25 2023 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: virtio_pci.c,v 1.38.4.1 2023/05/13 10:56:10 martin Exp $ */
+/* $NetBSD: virtio_pci.c,v 1.38.4.2 2023/06/03 14:40:25 martin Exp $ */
 
 /*
  * Copyright (c) 2020 The NetBSD Foundation, Inc.
@@ -28,7 +28,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: virtio_pci.c,v 1.38.4.1 2023/05/13 10:56:10 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: virtio_pci.c,v 1.38.4.2 2023/06/03 14:40:25 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -333,8 +333,8 @@ virtio_pci_detach(device_t self, int fla
        if (r != 0)
                return r;
 
-       /* Check that child detached properly */
-       KASSERT(ISSET(sc->sc_child_flags, VIRTIO_CHILD_DETACHED));
+       /* Check that child never attached, or detached properly */
+       KASSERT(sc->sc_child == NULL);
        KASSERT(sc->sc_vqs == NULL);
        KASSERT(psc->sc_ihs_num == 0);
 
diff -r c6b6da0cae44 -r 46ee90e15577 sys/dev/pci/virtiovar.h
--- a/sys/dev/pci/virtiovar.h   Sat Jun 03 14:33:55 2023 +0000
+++ b/sys/dev/pci/virtiovar.h   Sat Jun 03 14:40:25 2023 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: virtiovar.h,v 1.24.4.1 2023/05/13 10:56:10 martin Exp $        */
+/*     $NetBSD: virtiovar.h,v 1.24.4.2 2023/06/03 14:40:25 martin Exp $        */
 
 /*
  * Copyright (c) 2010 Minoura Makoto.
@@ -164,10 +164,11 @@ struct virtio_softc {
 
        int                     sc_childdevid;
        device_t                sc_child;               /* set by child */
-       uint32_t                sc_child_flags;
-#define VIRTIO_CHILD_ATTACH_FINISHED   __BIT(0)
-#define VIRTIO_CHILD_ATTACH_FAILED     __BIT(1)
-#define VIRTIO_CHILD_DETACHED          __BIT(2)
+       enum {
+               VIRTIO_NO_CHILD = 0,
+               VIRTIO_CHILD_ATTACH_FINISHED,
+               VIRTIO_CHILD_ATTACH_FAILED
+       }                       sc_child_state;
 
        virtio_callback         sc_config_change;       /* set by child */
        virtio_callback         sc_intrhand;
diff -r c6b6da0cae44 -r 46ee90e15577 sys/dev/virtio/virtio_mmio.c
--- a/sys/dev/virtio/virtio_mmio.c      Sat Jun 03 14:33:55 2023 +0000
+++ b/sys/dev/virtio/virtio_mmio.c      Sat Jun 03 14:40:25 2023 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: virtio_mmio.c,v 1.7.4.1 2023/05/13 10:56:10 martin Exp $       */
+/*     $NetBSD: virtio_mmio.c,v 1.7.4.2 2023/06/03 14:40:25 martin Exp $       */
 /*     $OpenBSD: virtio_mmio.c,v 1.2 2017/02/24 17:12:31 patrick Exp $ */
 
 /*
@@ -29,7 +29,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: virtio_mmio.c,v 1.7.4.1 2023/05/13 10:56:10 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: virtio_mmio.c,v 1.7.4.2 2023/06/03 14:40:25 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -217,7 +217,7 @@ virtio_mmio_common_detach(struct virtio_
        if (r != 0)
                return r;
 
-       KASSERT(ISSET(vsc->sc_child_flags, VIRTIO_CHILD_DETACHED));
+       KASSERT(vsc->sc_child == NULL);
        KASSERT(vsc->sc_vqs == NULL);
        KASSERT(sc->sc_ih == NULL);
 



Home | Main Index | Thread Index | Old Index