Download raw body.
vio: Reduce code duplication in control queue handling
Hi,
On Wed, 4 Sep 2024, Alexander Bluhm wrote:
> Comments inline.
Some answers inline. New diff below.
> >
> > - if ((r = vio_wait_ctrl(sc)) != 0)
> > - return r;
> > + while (sc->sc_ctrl_inuse != FREE) {
> > + if (sc->sc_ctrl_inuse == RESET || vio_needs_reset(sc))
> > + return ENXIO;
> > + r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO, "viowait", INFSLP);
>
> The old code aborted if sleep() returned an error.
In the absence of PCATCH or a timeout, tsleep should not return an error.
But I added a return anyway.
> > vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_status,
> > sizeof(*sc->sc_ctrl_status), 0);
> > +
> > virtio_enqueue_commit(vsc, vq, slot, 1);
> >
> > - if ((r = vio_wait_ctrl_done(sc)) != 0)
> > - goto out;
> > + while (sc->sc_ctrl_inuse != DONE) {
> > + if (sc->sc_ctrl_inuse == RESET || vio_needs_reset(sc))
> > + return ENXIO;
> > + r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO, "viodone",
> > + VIRTIO_NET_CTRL_TIMEOUT);
> > + if (r == EWOULDBLOCK) {
> > + printf("%s: ctrl queue timeout\n",
> > + sc->sc_dev.dv_xname);
> > + vio_ctrl_wakeup(sc, RESET);
> > + return ENXIO;
> > + }
>
> Do some error handling if sleep failed. The old code did goto out.
Also fixed, though tsleep cannot currently return a different error.
> > +/* issue a VIRTIO_NET_CTRL_RX class command and wait for completion */
> > +int
> > +vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
> > +{
> > + struct virtio_softc *vsc = sc->sc_virtio;
> > + struct virtqueue *vq = sc->sc_ctl_vq;
> > + int r, slot;
> > +
> > + r = vio_ctrl_start(sc, VIRTIO_NET_CTRL_RX, cmd, 1, &slot);
> > + if (r != 0)
> > + return r;
> > +
> > + sc->sc_ctrl_rx->onoff = onoff;
> > +
> > + vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_rx,
> > + sizeof(*sc->sc_ctrl_rx), 1);
> > +
> > + r = vio_ctrl_submit(sc, slot);
> > + VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_rx,
> > + sizeof(*sc->sc_ctrl_rx), BUS_DMASYNC_POSTWRITE);
> > + if (r != 0)
> > printf("%s: ctrl cmd %d failed\n", sc->sc_dev.dv_xname, cmd);
> > - r = EIO;
> > - }
>
> When vio_ctrl_submit() fails it, sometimes calls wakeup.
> But in any case we call vio_ctrl_finish() to wakeup again.
> The cases when we abort in a case of an error and when we just
> continue are hard to follow.
I have added some comments to clarify this. If vio_ctrl_submit fails the
caller should always call vio_ctrl_finish() and vio_ctrl_finish() will
deal with the different failure modes. Otherwise we would need to have the
handling of different error conditions in every caller of
vio_ctrl_submit(). However, I think if vio_ctrl_submit() fails the caller
should still do his dmasync to avoid bounce buffer and main memory being
out of sync. But this makes the code more difficult to follow. Do you have
better ideas for this? In general, in case of ENXIO, we cannot know if the
device did DMA or not.
Cheers,
Stefan
vio: Reduce code duplication in control queue handling
Pull the common parts of all the control queue operations into separate
functions.
While there, avoid setting sc_ctrl_inuse FREE if it was RESET, except in
vio_stop. Doing so could lead to more race conditions.
diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
index 7a37400584b..fc7f330c0ee 100644
--- a/sys/dev/pv/if_vio.c
+++ b/sys/dev/pv/if_vio.c
@@ -317,8 +317,9 @@ void vio_iff(struct vio_softc *);
int vio_media_change(struct ifnet *);
void vio_media_status(struct ifnet *, struct ifmediareq *);
int vio_ctrleof(struct virtqueue *);
-int vio_wait_ctrl(struct vio_softc *sc);
-int vio_wait_ctrl_done(struct vio_softc *sc);
+int vio_ctrl_start(struct vio_softc *, uint8_t, uint8_t, int, int *);
+int vio_ctrl_submit(struct vio_softc *, int);
+void vio_ctrl_finish(struct vio_softc *);
void vio_ctrl_wakeup(struct vio_softc *, enum vio_ctrl_state);
int vio_alloc_mem(struct vio_softc *);
int vio_alloc_dmamem(struct vio_softc *);
@@ -1483,59 +1484,137 @@ vio_tx_drain(struct vio_softc *sc)
/*
* Control vq
*/
-/* issue a VIRTIO_NET_CTRL_RX class command and wait for completion */
+
+/*
+ * Lock the control queue and the sc_ctrl_* structs and prepare a request.
+ *
+ * If this function succeeds, the caller must also call either
+ * vio_ctrl_submit() or virtio_enqueue_abort(), in both cases followed by
+ * vio_ctrl_finish().
+ */
int
-vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
+vio_ctrl_start(struct vio_softc *sc, uint8_t class, uint8_t cmd, int nslots,
+ int *slotp)
{
struct virtio_softc *vsc = sc->sc_virtio;
struct virtqueue *vq = sc->sc_ctl_vq;
- int r, slot;
+ int r;
splassert(IPL_NET);
- if ((r = vio_wait_ctrl(sc)) != 0)
- return r;
+ while (sc->sc_ctrl_inuse != FREE) {
+ if (sc->sc_ctrl_inuse == RESET || vio_needs_reset(sc))
+ return ENXIO;
+ r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO, "viowait", INFSLP);
+ if (r != 0)
+ return r;
+ }
+ sc->sc_ctrl_inuse = INUSE;
- sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_RX;
+ sc->sc_ctrl_cmd->class = class;
sc->sc_ctrl_cmd->command = cmd;
- sc->sc_ctrl_rx->onoff = onoff;
- r = virtio_enqueue_prep(vq, &slot);
+ r = virtio_enqueue_prep(vq, slotp);
if (r != 0)
panic("%s: %s virtio_enqueue_prep: control vq busy",
sc->sc_dev.dv_xname, __func__);
- r = virtio_enqueue_reserve(vq, slot, 3);
+ r = virtio_enqueue_reserve(vq, *slotp, nslots + 2);
if (r != 0)
panic("%s: %s virtio_enqueue_reserve: control vq busy",
sc->sc_dev.dv_xname, __func__);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_cmd,
+
+ vio_dmamem_enqueue(vsc, sc, vq, *slotp, sc->sc_ctrl_cmd,
sizeof(*sc->sc_ctrl_cmd), 1);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_rx,
- sizeof(*sc->sc_ctrl_rx), 1);
+
+ return 0;
+}
+
+/*
+ * Submit a control queue request and wait for the result.
+ *
+ * vio_ctrl_start() must have been called successfully.
+ * After vio_ctrl_submit(), the caller may inspect the
+ * data returned from the hypervisor. Afterwards, the caller
+ * must always call vio_ctrl_finish().
+ */
+int
+vio_ctrl_submit(struct vio_softc *sc, int slot)
+{
+ struct virtio_softc *vsc = sc->sc_virtio;
+ struct virtqueue *vq = sc->sc_ctl_vq;
+ int r;
+
vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_status,
sizeof(*sc->sc_ctrl_status), 0);
+
virtio_enqueue_commit(vsc, vq, slot, 1);
- if ((r = vio_wait_ctrl_done(sc)) != 0)
- goto out;
+ while (sc->sc_ctrl_inuse != DONE) {
+ if (sc->sc_ctrl_inuse == RESET || vio_needs_reset(sc))
+ return ENXIO;
+ r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO, "viodone",
+ VIRTIO_NET_CTRL_TIMEOUT);
+ if (r != 0) {
+ if (r == EWOULDBLOCK)
+ printf("%s: ctrl queue timeout\n",
+ sc->sc_dev.dv_xname);
+ vio_ctrl_wakeup(sc, RESET);
+ return ENXIO;
+ }
+ }
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
- VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_rx,
- sizeof(*sc->sc_ctrl_rx), BUS_DMASYNC_POSTWRITE);
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_status,
sizeof(*sc->sc_ctrl_status), BUS_DMASYNC_POSTREAD);
- if (sc->sc_ctrl_status->ack == VIRTIO_NET_OK) {
- r = 0;
- } else {
+ if (sc->sc_ctrl_status->ack != VIRTIO_NET_OK)
+ return EIO;
+
+ return 0;
+}
+
+/*
+ * Unlock the control queue and the sc_ctrl_* structs.
+ *
+ * It is ok to call this function if the control queue is marked dead
+ * due to a fatal error.
+ */
+void
+vio_ctrl_finish(struct vio_softc *sc)
+{
+ if (sc->sc_ctrl_inuse == RESET)
+ return;
+
+ vio_ctrl_wakeup(sc, FREE);
+}
+
+/* issue a VIRTIO_NET_CTRL_RX class command and wait for completion */
+int
+vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
+{
+ struct virtio_softc *vsc = sc->sc_virtio;
+ struct virtqueue *vq = sc->sc_ctl_vq;
+ int r, slot;
+
+ r = vio_ctrl_start(sc, VIRTIO_NET_CTRL_RX, cmd, 1, &slot);
+ if (r != 0)
+ return r;
+
+ sc->sc_ctrl_rx->onoff = onoff;
+
+ vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_rx,
+ sizeof(*sc->sc_ctrl_rx), 1);
+
+ r = vio_ctrl_submit(sc, slot);
+ VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_rx,
+ sizeof(*sc->sc_ctrl_rx), BUS_DMASYNC_POSTWRITE);
+ if (r != 0)
printf("%s: ctrl cmd %d failed\n", sc->sc_dev.dv_xname, cmd);
- r = EIO;
- }
DPRINTF("%s: cmd %d %d: %d\n", __func__, cmd, onoff, r);
-out:
- vio_ctrl_wakeup(sc, FREE);
+
+ vio_ctrl_finish(sc);
return r;
}
@@ -1546,87 +1625,29 @@ vio_ctrl_guest_offloads(struct vio_softc *sc, uint64_t features)
struct virtqueue *vq = sc->sc_ctl_vq;
int r, slot;
- splassert(IPL_NET);
-
- if ((r = vio_wait_ctrl(sc)) != 0)
+ r = vio_ctrl_start(sc, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
+ VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, 1, &slot);
+ if (r != 0)
return r;
- sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_GUEST_OFFLOADS;
- sc->sc_ctrl_cmd->command = VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET;
sc->sc_ctrl_guest_offloads->offloads = features;
- r = virtio_enqueue_prep(vq, &slot);
- if (r != 0)
- panic("%s: %s virtio_enqueue_prep: control vq busy",
- sc->sc_dev.dv_xname, __func__);
- r = virtio_enqueue_reserve(vq, slot, 3);
- if (r != 0)
- panic("%s: %s virtio_enqueue_reserve: control vq busy",
- sc->sc_dev.dv_xname, __func__);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_cmd,
- sizeof(*sc->sc_ctrl_cmd), 1);
vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_guest_offloads,
sizeof(*sc->sc_ctrl_guest_offloads), 1);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_status,
- sizeof(*sc->sc_ctrl_status), 0);
- virtio_enqueue_commit(vsc, vq, slot, 1);
- if ((r = vio_wait_ctrl_done(sc)) != 0)
- goto out;
+ r = vio_ctrl_submit(sc, slot);
- VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
- sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_guest_offloads,
sizeof(*sc->sc_ctrl_guest_offloads), BUS_DMASYNC_POSTWRITE);
- VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_status,
- sizeof(*sc->sc_ctrl_status), BUS_DMASYNC_POSTREAD);
- if (sc->sc_ctrl_status->ack == VIRTIO_NET_OK) {
- r = 0;
- } else {
+ if (r != 0) {
printf("%s: offload features 0x%llx failed\n",
sc->sc_dev.dv_xname, features);
- r = EIO;
- }
-
- DPRINTF("%s: features 0x%llx: %d\n", __func__, features, r);
- out:
- vio_ctrl_wakeup(sc, FREE);
- return r;
-}
-
-int
-vio_wait_ctrl(struct vio_softc *sc)
-{
- int r = 0;
-
- while (sc->sc_ctrl_inuse != FREE) {
- if (sc->sc_ctrl_inuse == RESET || vio_needs_reset(sc))
- return ENXIO;
- r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO, "viowait", INFSLP);
}
- sc->sc_ctrl_inuse = INUSE;
- return r;
-}
-
-int
-vio_wait_ctrl_done(struct vio_softc *sc)
-{
- int r = 0;
+ DPRINTF("%s: offload features 0x%llx: %d\n", __func__, features, r);
- while (sc->sc_ctrl_inuse != DONE) {
- if (sc->sc_ctrl_inuse == RESET || vio_needs_reset(sc))
- return ENXIO;
- r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO, "viodone",
- VIRTIO_NET_CTRL_TIMEOUT);
- if (r == EWOULDBLOCK) {
- printf("%s: ctrl queue timeout\n",
- sc->sc_dev.dv_xname);
- vio_ctrl_wakeup(sc, RESET);
- return ENXIO;
- }
- }
+ vio_ctrl_finish(sc);
return r;
}
@@ -1665,55 +1686,35 @@ vio_set_rx_filter(struct vio_softc *sc)
struct virtio_softc *vsc = sc->sc_virtio;
struct virtqueue *vq = sc->sc_ctl_vq;
int r, slot;
+ size_t len_uc, len_mc;
- splassert(IPL_NET);
-
- if ((r = vio_wait_ctrl(sc)) != 0)
- return r;
-
- sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_MAC;
- sc->sc_ctrl_cmd->command = VIRTIO_NET_CTRL_MAC_TABLE_SET;
- r = virtio_enqueue_prep(vq, &slot);
+ r = vio_ctrl_start(sc, VIRTIO_NET_CTRL_MAC,
+ VIRTIO_NET_CTRL_MAC_TABLE_SET, 2, &slot);
if (r != 0)
- panic("%s: %s virtio_enqueue_prep: control vq busy",
- sc->sc_dev.dv_xname, __func__);
- r = virtio_enqueue_reserve(vq, slot, 4);
- if (r != 0)
- panic("%s: %s virtio_enqueue_reserve: control vq busy",
- sc->sc_dev.dv_xname, __func__);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_cmd,
- sizeof(*sc->sc_ctrl_cmd), 1);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_mac_tbl_uc,
- sizeof(*sc->sc_ctrl_mac_tbl_uc) +
- sc->sc_ctrl_mac_tbl_uc->nentries * ETHER_ADDR_LEN, 1);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_mac_tbl_mc,
- sizeof(*sc->sc_ctrl_mac_tbl_mc) +
- sc->sc_ctrl_mac_tbl_mc->nentries * ETHER_ADDR_LEN, 1);
- vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_status,
- sizeof(*sc->sc_ctrl_status), 0);
- virtio_enqueue_commit(vsc, vq, slot, 1);
-
- if ((r = vio_wait_ctrl_done(sc)) != 0)
- goto out;
-
- VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
- sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
- VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_mac_info,
- VIO_CTRL_MAC_INFO_SIZE, BUS_DMASYNC_POSTWRITE);
- VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_status,
- sizeof(*sc->sc_ctrl_status), BUS_DMASYNC_POSTREAD);
+ return r;
- if (sc->sc_ctrl_status->ack == VIRTIO_NET_OK) {
- r = 0;
- } else {
+ len_uc = sizeof(*sc->sc_ctrl_mac_tbl_uc) +
+ sc->sc_ctrl_mac_tbl_uc->nentries * ETHER_ADDR_LEN;
+ len_mc = sizeof(*sc->sc_ctrl_mac_tbl_mc) +
+ sc->sc_ctrl_mac_tbl_mc->nentries * ETHER_ADDR_LEN;
+ vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_mac_tbl_uc, len_uc,
+ 1);
+ vio_dmamem_enqueue(vsc, sc, vq, slot, sc->sc_ctrl_mac_tbl_mc, len_mc,
+ 1);
+
+ r = vio_ctrl_submit(sc, slot);
+ VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_mac_tbl_uc, len_uc,
+ BUS_DMASYNC_POSTWRITE);
+ VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_mac_tbl_mc, len_mc,
+ BUS_DMASYNC_POSTWRITE);
+
+ if (r != 0) {
/* The host's filter table is not large enough */
printf("%s: failed setting rx filter\n", sc->sc_dev.dv_xname);
- r = EIO;
}
-out:
- vio_ctrl_wakeup(sc, FREE);
+ vio_ctrl_finish(sc);
return r;
}
vio: Reduce code duplication in control queue handling