Download raw body.
vio: Reduce code duplication in control queue handling
On Wed, Sep 04, 2024 at 11:26:16AM +0200, Stefan Fritsch wrote:
> Hi,
>
> the next reviewable chunk of the V5 vio multi-queue diff is to 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 race conditions.
>
> ok?
Tested sucessfully on vmm/vmd with bounce buffers.
Comments inline.
> diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
> index 3444bba35ef..8f38feb4dd3 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,109 @@ vio_tx_drain(struct vio_softc *sc)
> /*
> * Control vq
> */
> -/* issue a VIRTIO_NET_CTRL_RX class command and wait for completion */
> 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);
The old code aborted if sleep() returned an error.
> + }
> + 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;
> +}
> +
> +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 == 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.
> + }
>
> 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 {
> + r = sc->sc_ctrl_status->ack == VIRTIO_NET_OK ? 0 : EIO;
Can we do a common error check pattern, like
if (sc->sc_ctrl_status->ack != VIRTIO_NET_OK)
return EIO;
> +
> + return r;
> +}
> +
> +void
> +vio_ctrl_finish(struct vio_softc *sc)
> +{
> + if (sc->sc_ctrl_inuse != RESET)
> + 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;
> - }
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.
>
> 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 +1597,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 +1658,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;
> }
>
> --
> 2.39.2
vio: Reduce code duplication in control queue handling