Download raw body.
virtio: Refactor attach logic some more
On Mon, Nov 25, 2024 at 09:00:24PM GMT, Stefan Fritsch wrote:
> here is the next reviewable part from the large virtio multiqueue diff.
>
> virtio 1.x requires that all queue setup, including the queue interrupt
> vector, is done before setting the queue_enable register to 1. This
> conflicts with how we do things right now:
>
> * We implicitly make queue setup in virtio_alloc_vq(), which is called
> from the child driver attach functions. This also sets queue_enable=1.
>
> * Later, we allocate the interrupts and set the queue interrupt vectors
> in the second half of the virtio transport attach functions.
>
> This is a violation of a MUST from the standard and causes problems with
> some hypervisors, in particular those that support only virtio 1.x. As 0.9
> has no such ordering requirements, hypervisors with 0.9 support tend to be
> more tolerant.
>
> To fix this:
>
> * Move the interrupt allocation to a new virtio_attach_finish() function.
> This does all queue setup, including the interrupt vectors.
>
> * Don't call virtio_setup_queue() in virtio_alloc_vq() anymore.
>
> * We can also move the setting of the DRIVER_OK flag into this function.
> virtio_attach_finish() must be called before using any virtqueue or
> writing any virtio config register.
>
> While there,
>
> * also streamline the attach error handling in all drivers.
>
> * skip initially setting sc_config_change to NULL, the softc is
> initialized to 0.
>
> Comments? OK?
Diff looks fine to me. I also tested it on OpenBSD/VMM (amd64) and
Linux/KVM (amd64 and arm64).
ok jan@
> diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
> index 5d7a6db6465..5e0fc4d8e01 100644
> --- a/sys/dev/fdt/virtio_mmio.c
> +++ b/sys/dev/fdt/virtio_mmio.c
> @@ -98,6 +98,7 @@ void virtio_mmio_write_device_config_8(struct virtio_softc *, int, uint64_t);
> uint16_t virtio_mmio_read_queue_size(struct virtio_softc *, uint16_t);
> void virtio_mmio_setup_queue(struct virtio_softc *, struct virtqueue *, uint64_t);
> void virtio_mmio_setup_intrs(struct virtio_softc *);
> +int virtio_mmio_attach_finish(struct virtio_softc *, struct virtio_attach_args *);
> int virtio_mmio_get_status(struct virtio_softc *);
> void virtio_mmio_set_status(struct virtio_softc *, int);
> int virtio_mmio_negotiate_features(struct virtio_softc *,
> @@ -119,6 +120,11 @@ struct virtio_mmio_softc {
> uint32_t sc_version;
> };
>
> +struct virtio_mmio_attach_args {
> + struct virtio_attach_args vma_va;
> + struct fdt_attach_args *vma_fa;
> +};
> +
> const struct cfattach virtio_mmio_ca = {
> sizeof(struct virtio_mmio_softc),
> virtio_mmio_match,
> @@ -151,6 +157,7 @@ const struct virtio_ops virtio_mmio_ops = {
> virtio_mmio_get_status,
> virtio_mmio_set_status,
> virtio_mmio_negotiate_features,
> + virtio_mmio_attach_finish,
> virtio_mmio_intr,
> virtio_mmio_intr_barrier,
> };
> @@ -250,7 +257,7 @@ virtio_mmio_attach(struct device *parent, struct device *self, void *aux)
> struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)self;
> struct virtio_softc *vsc = &sc->sc_sc;
> uint32_t id, magic;
> - struct virtio_attach_args va = { 0 };
> + struct virtio_mmio_attach_args vma = { { 0 }, faa };
>
> if (faa->fa_nreg < 1) {
> printf(": no register data\n");
> @@ -299,36 +306,43 @@ virtio_mmio_attach(struct device *parent, struct device *self, void *aux)
> virtio_mmio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_ACK);
> virtio_mmio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER);
>
> - va.va_devid = id;
> - va.va_nintr = 1;
> + vma.vma_va.va_devid = id;
> + vma.vma_va.va_nintr = 1;
> vsc->sc_child = NULL;
> - config_found(self, &va, NULL);
> + config_found(self, &vma, NULL);
> if (vsc->sc_child == NULL) {
> printf("%s: no matching child driver; not configured\n",
> vsc->sc_dev.dv_xname);
> - goto fail_1;
> + goto fail;
> }
> if (vsc->sc_child == VIRTIO_CHILD_ERROR) {
> printf("%s: virtio configuration failed\n",
> vsc->sc_dev.dv_xname);
> - goto fail_1;
> + goto fail;
> }
>
> - sc->sc_ih = fdt_intr_establish(faa->fa_node, vsc->sc_ipl,
> + return;
> +
> +fail:
> + virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_FAILED);
> +}
> +
> +int
> +virtio_mmio_attach_finish(struct virtio_softc *vsc,
> + struct virtio_attach_args *va)
> +{
> + struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)vsc;
> + struct virtio_mmio_attach_args *vma =
> + (struct virtio_mmio_attach_args *)va;
> +
> + sc->sc_ih = fdt_intr_establish(vma->vma_fa->fa_node, vsc->sc_ipl,
> virtio_mmio_intr, sc, vsc->sc_dev.dv_xname);
> if (sc->sc_ih == NULL) {
> printf("%s: couldn't establish interrupt\n",
> vsc->sc_dev.dv_xname);
> - goto fail_2;
> + return -EIO;
> }
> -
> - return;
> -
> -fail_2:
> - config_detach(vsc->sc_child, 0);
> -fail_1:
> - /* no mmio_mapreg_unmap() or mmio_intr_unmap() */
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_FAILED);
> + return 0;
> }
>
> int
> diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
> index ad9ce8a1254..9810e834a63 100644
> --- a/sys/dev/pci/virtio_pci.c
> +++ b/sys/dev/pci/virtio_pci.c
> @@ -74,6 +74,7 @@ void virtio_pci_write_device_config_8(struct virtio_softc *, int, uint64_t);
> uint16_t virtio_pci_read_queue_size(struct virtio_softc *, uint16_t);
> void virtio_pci_setup_queue(struct virtio_softc *, struct virtqueue *, uint64_t);
> void virtio_pci_setup_intrs(struct virtio_softc *);
> +int virtio_pci_attach_finish(struct virtio_softc *, struct virtio_attach_args *);
> int virtio_pci_get_status(struct virtio_softc *);
> void virtio_pci_set_status(struct virtio_softc *, int);
> int virtio_pci_negotiate_features(struct virtio_softc *, const struct virtio_feature_name *);
> @@ -175,6 +176,7 @@ const struct virtio_ops virtio_pci_ops = {
> virtio_pci_get_status,
> virtio_pci_set_status,
> virtio_pci_negotiate_features,
> + virtio_pci_attach_finish,
> virtio_pci_poll_intr,
> virtio_pci_intr_barrier,
> };
> @@ -592,8 +594,6 @@ virtio_pci_attach(struct device *parent, struct device *self, void *aux)
> pcitag_t tag = pa->pa_tag;
> int revision, ret = ENODEV;
> pcireg_t id;
> - char const *intrstr;
> - pci_intr_handle_t ih;
> struct virtio_pci_attach_args vpa = { { 0 }, pa };
>
> revision = PCI_REVISION(pa->pa_class);
> @@ -645,13 +645,13 @@ virtio_pci_attach(struct device *parent, struct device *self, void *aux)
> }
> if (ret != 0) {
> printf(": Cannot attach (%d)\n", ret);
> - goto fail_0;
> + goto free;
> }
>
> sc->sc_devcfg_offset = VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI;
> sc->sc_irq_type = IRQ_NO_MSIX;
> if (virtio_pci_adjust_config_region(sc) != 0)
> - goto fail_0;
> + goto err;
>
> virtio_device_reset(vsc);
> virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_ACK);
> @@ -664,25 +664,47 @@ virtio_pci_attach(struct device *parent, struct device *self, void *aux)
> if (vsc->sc_child == NULL) {
> printf("%s: no matching child driver; not configured\n",
> vsc->sc_dev.dv_xname);
> - goto fail_1;
> + goto err;
> }
> if (vsc->sc_child == VIRTIO_CHILD_ERROR) {
> printf("%s: virtio configuration failed\n",
> vsc->sc_dev.dv_xname);
> - goto fail_1;
> + goto err;
> }
>
> - if (virtio_pci_setup_msix(sc, &vpa, 0) == 0) {
> + return;
> +
> +err:
> + /* no pci_mapreg_unmap() or pci_intr_unmap() */
> + virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_FAILED);
> +free:
> + free(sc->sc_intr, M_DEVBUF, sc->sc_nintr * sizeof(*sc->sc_intr));
> +}
> +
> +int
> +virtio_pci_attach_finish(struct virtio_softc *vsc,
> + struct virtio_attach_args *va)
> +{
> + struct virtio_pci_softc *sc = (struct virtio_pci_softc *)vsc;
> + struct virtio_pci_attach_args *vpa =
> + (struct virtio_pci_attach_args *)va;
> + pci_intr_handle_t ih;
> + pci_chipset_tag_t pc = vpa->vpa_pa->pa_pc;
> + char const *intrstr;
> +
> + if (virtio_pci_setup_msix(sc, vpa, 0) == 0) {
> sc->sc_irq_type = IRQ_MSIX_PER_VQ;
> intrstr = "msix per-VQ";
> - } else if (virtio_pci_setup_msix(sc, &vpa, 1) == 0) {
> + } else if (virtio_pci_setup_msix(sc, vpa, 1) == 0) {
> sc->sc_irq_type = IRQ_MSIX_SHARED;
> intrstr = "msix shared";
> } else {
> int (*ih_func)(void *) = virtio_pci_legacy_intr;
> - if (pci_intr_map_msi(pa, &ih) != 0 && pci_intr_map(pa, &ih) != 0) {
> - printf("%s: couldn't map interrupt\n", vsc->sc_dev.dv_xname);
> - goto fail_2;
> + if (pci_intr_map_msi(vpa->vpa_pa, &ih) != 0 &&
> + pci_intr_map(vpa->vpa_pa, &ih) != 0) {
> + printf("%s: couldn't map interrupt\n",
> + vsc->sc_dev.dv_xname);
> + return -EIO;
> }
> intrstr = pci_intr_string(pc, ih);
> /*
> @@ -696,25 +718,17 @@ virtio_pci_attach(struct device *parent, struct device *self, void *aux)
> vsc->sc_ipl | IPL_MPSAFE, ih_func, sc,
> vsc->sc_child->dv_xname);
> if (sc->sc_intr[0].ih == NULL) {
> - printf("%s: couldn't establish interrupt", vsc->sc_dev.dv_xname);
> + printf("%s: couldn't establish interrupt",
> + vsc->sc_dev.dv_xname);
> if (intrstr != NULL)
> printf(" at %s", intrstr);
> printf("\n");
> - goto fail_2;
> + return -EIO;
> }
> }
> - virtio_pci_setup_intrs(vsc);
> - printf("%s: %s\n", vsc->sc_dev.dv_xname, intrstr);
>
> - return;
> -
> -fail_2:
> - config_detach(vsc->sc_child, 0);
> -fail_1:
> - /* no pci_mapreg_unmap() or pci_intr_unmap() */
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_FAILED);
> -fail_0:
> - free(sc->sc_intr, M_DEVBUF, sc->sc_nintr * sizeof(*sc->sc_intr));
> + printf("%s: %s\n", vsc->sc_dev.dv_xname, intrstr);
> + return 0;
> }
>
> int
> diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
> index 3d4d728e02d..9ca8f27eac6 100644
> --- a/sys/dev/pv/if_vio.c
> +++ b/sys/dev/pv/if_vio.c
> @@ -597,6 +597,7 @@ vio_attach(struct device *parent, struct device *self, void *aux)
> {
> struct vio_softc *sc = (struct vio_softc *)self;
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> + struct virtio_attach_args *va = aux;
> int i, tx_max_segments;
> struct ifnet *ifp = &sc->sc_ac.ac_if;
>
> @@ -610,7 +611,6 @@ vio_attach(struct device *parent, struct device *self, void *aux)
>
> vsc->sc_child = self;
> vsc->sc_ipl = IPL_NET | IPL_MPSAFE;
> - vsc->sc_config_change = NULL;
> vsc->sc_driver_features = VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS |
> VIRTIO_NET_F_CTRL_VQ | VIRTIO_NET_F_CTRL_RX |
> VIRTIO_NET_F_MRG_RXBUF | VIRTIO_NET_F_CSUM |
> @@ -623,7 +623,8 @@ vio_attach(struct device *parent, struct device *self, void *aux)
> vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_TSO4;
> vsc->sc_driver_features |= VIRTIO_NET_F_GUEST_TSO6;
>
> - virtio_negotiate_features(vsc, virtio_net_feature_names);
> + if (virtio_negotiate_features(vsc, virtio_net_feature_names) != 0)
> + goto err;
>
> sc->sc_nqueues = 1;
> vsc->sc_nvqs = 2 * sc->sc_nqueues;
> @@ -756,7 +757,9 @@ vio_attach(struct device *parent, struct device *self, void *aux)
> timeout_set(&sc->sc_txtick, vio_txtick, sc->sc_q[0].viq_txvq);
> timeout_set(&sc->sc_rxtick, vio_rxtick, sc->sc_q[0].viq_rxvq);
>
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err;
> +
> if_attach(ifp);
> ether_ifattach(ifp);
> vio_link_state(ifp);
> diff --git a/sys/dev/pv/vioblk.c b/sys/dev/pv/vioblk.c
> index 153a2762170..0bf8fdd1ac3 100644
> --- a/sys/dev/pv/vioblk.c
> +++ b/sys/dev/pv/vioblk.c
> @@ -170,12 +170,12 @@ vioblk_attach(struct device *parent, struct device *self, void *aux)
> {
> struct vioblk_softc *sc = (struct vioblk_softc *)self;
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> + struct virtio_attach_args *va = aux;
> struct scsibus_attach_args saa;
> int qsize;
>
> vsc->sc_vqs = &sc->sc_vq[0];
> vsc->sc_nvqs = 1;
> - vsc->sc_config_change = NULL;
> if (vsc->sc_child)
> panic("already attached to something else");
> vsc->sc_child = self;
> @@ -184,7 +184,8 @@ vioblk_attach(struct device *parent, struct device *self, void *aux)
> vsc->sc_driver_features = VIRTIO_BLK_F_RO | VIRTIO_F_NOTIFY_ON_EMPTY |
> VIRTIO_BLK_F_SIZE_MAX | VIRTIO_BLK_F_SEG_MAX | VIRTIO_BLK_F_FLUSH;
>
> - virtio_negotiate_features(vsc, vioblk_feature_names);
> + if (virtio_negotiate_features(vsc, vioblk_feature_names) != 0)
> + goto err;
>
> if (virtio_has_feature(vsc, VIRTIO_BLK_F_SIZE_MAX)) {
> uint32_t size_max = virtio_read_device_config_4(vsc,
> @@ -252,10 +253,11 @@ vioblk_attach(struct device *parent, struct device *self, void *aux)
> saa.saa_quirks = 0;
> saa.saa_wwpn = saa.saa_wwnn = 0;
>
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err;
> config_found(self, &saa, scsiprint);
> -
> return;
> +
> err:
> vsc->sc_child = VIRTIO_CHILD_ERROR;
> return;
> diff --git a/sys/dev/pv/viocon.c b/sys/dev/pv/viocon.c
> index 681a842cd93..6f097a3309b 100644
> --- a/sys/dev/pv/viocon.c
> +++ b/sys/dev/pv/viocon.c
> @@ -179,7 +179,6 @@ viocon_attach(struct device *parent, struct device *self, void *aux)
> panic("already attached to something else");
> vsc->sc_child = self;
> vsc->sc_ipl = IPL_TTY;
> - vsc->sc_config_change = NULL;
> sc->sc_virtio = vsc;
> sc->sc_max_ports = maxports;
>
> @@ -193,7 +192,8 @@ viocon_attach(struct device *parent, struct device *self, void *aux)
> }
>
> vsc->sc_driver_features = VIRTIO_CONSOLE_F_SIZE;
> - virtio_negotiate_features(vsc, viocon_feature_names);
> + if (virtio_negotiate_features(vsc, viocon_feature_names) != 0)
> + goto err;
>
> printf("\n");
> DPRINTF("%s: softc: %p\n", __func__, sc);
> @@ -201,10 +201,11 @@ viocon_attach(struct device *parent, struct device *self, void *aux)
> printf("\n%s: viocon_port_create failed\n", __func__);
> goto err;
> }
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err;
> viocon_rx_fill(sc->sc_ports[0]);
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> -
> return;
> +
> err:
> vsc->sc_child = VIRTIO_CHILD_ERROR;
> free(vsc->sc_vqs, M_DEVBUF, 2 * (maxports + 1) * sizeof(struct virtqueue));
> diff --git a/sys/dev/pv/viogpu.c b/sys/dev/pv/viogpu.c
> index cce31d8780e..1bf45b7caa7 100644
> --- a/sys/dev/pv/viogpu.c
> +++ b/sys/dev/pv/viogpu.c
> @@ -150,6 +150,7 @@ viogpu_attach(struct device *parent, struct device *self, void *aux)
> {
> struct viogpu_softc *sc = (struct viogpu_softc *)self;
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> + struct virtio_attach_args *va = aux;
> struct wsemuldisplaydev_attach_args waa;
> struct rasops_info *ri = &sc->sc_ri;
> uint32_t defattr;
> @@ -161,10 +162,11 @@ viogpu_attach(struct device *parent, struct device *self, void *aux)
> }
> vsc->sc_child = self;
>
> - virtio_negotiate_features(vsc, viogpu_feature_names);
> + if (virtio_negotiate_features(vsc, viogpu_feature_names) != 0)
> + goto err;
> if (!vsc->sc_version_1) {
> printf(": requires virtio version 1\n");
> - return;
> + goto err;
> }
>
> vsc->sc_ipl = IPL_TTY;
> @@ -175,13 +177,13 @@ viogpu_attach(struct device *parent, struct device *self, void *aux)
> vsc->sc_vqs = sc->sc_vqs;
> if (virtio_alloc_vq(vsc, &sc->sc_vqs[VQCTRL], VQCTRL, 1, "control")) {
> printf(": alloc_vq failed\n");
> - return;
> + goto err;
> }
> sc->sc_vqs[VQCTRL].vq_done = viogpu_vq_done;
>
> if (virtio_alloc_vq(vsc, &sc->sc_vqs[VQCURS], VQCURS, 1, "cursor")) {
> printf(": alloc_vq failed\n");
> - return;
> + goto err;
> }
> vsc->sc_nvqs = nitems(sc->sc_vqs);
>
> @@ -191,7 +193,7 @@ viogpu_attach(struct device *parent, struct device *self, void *aux)
> sc->sc_dma_size, 0, BUS_DMA_NOWAIT | BUS_DMA_ALLOCNOW,
> &sc->sc_dma_map) != 0) {
> printf(": create failed");
> - goto err;
> + goto errdma;
> }
> if (bus_dmamem_alloc(vsc->sc_dmat, sc->sc_dma_size, 16, 0,
> &sc->sc_dma_seg, 1, &nsegs, BUS_DMA_NOWAIT | BUS_DMA_ZERO) != 0) {
> @@ -209,7 +211,8 @@ viogpu_attach(struct device *parent, struct device *self, void *aux)
> goto unmap;
> }
>
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto unmap;
>
> if (viogpu_get_display_info(sc) != 0)
> goto unmap;
> @@ -302,8 +305,10 @@ free:
> bus_dmamem_free(vsc->sc_dmat, &sc->sc_dma_seg, 1);
> destroy:
> bus_dmamap_destroy(vsc->sc_dmat, sc->sc_dma_map);
> -err:
> +errdma:
> printf(": DMA setup failed\n");
> +err:
> + vsc->sc_child = VIRTIO_CHILD_ERROR;
> return;
> }
>
> diff --git a/sys/dev/pv/viomb.c b/sys/dev/pv/viomb.c
> index e8d16c58102..52eddc30a76 100644
> --- a/sys/dev/pv/viomb.c
> +++ b/sys/dev/pv/viomb.c
> @@ -135,6 +135,7 @@ viomb_attach(struct device *parent, struct device *self, void *aux)
> {
> struct viomb_softc *sc = (struct viomb_softc *)self;
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> + struct virtio_attach_args *va = aux;
> int i;
>
> if (vsc->sc_child != NULL) {
> @@ -219,8 +220,10 @@ viomb_attach(struct device *parent, struct device *self, void *aux)
> sensordev_install(&sc->sc_sensdev);
>
> printf("\n");
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err_dmamap;
> return;
> +
> err_dmamap:
> bus_dmamap_destroy(vsc->sc_dmat, sc->sc_req.bl_dmamap);
> err:
> diff --git a/sys/dev/pv/viornd.c b/sys/dev/pv/viornd.c
> index 8139b4a6a27..f349cc63257 100644
> --- a/sys/dev/pv/viornd.c
> +++ b/sys/dev/pv/viornd.c
> @@ -83,18 +83,19 @@ viornd_attach(struct device *parent, struct device *self, void *aux)
> {
> struct viornd_softc *sc = (struct viornd_softc *)self;
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> + struct virtio_attach_args *va = aux;
> unsigned int shift;
>
> vsc->sc_vqs = &sc->sc_vq;
> vsc->sc_nvqs = 1;
> - vsc->sc_config_change = NULL;
> if (vsc->sc_child != NULL)
> panic("already attached to something else");
> vsc->sc_child = self;
> vsc->sc_ipl = IPL_NET;
> sc->sc_virtio = vsc;
>
> - virtio_negotiate_features(vsc, NULL);
> + if (virtio_negotiate_features(vsc, NULL) != 0)
> + goto err;
>
> if (sc->sc_dev.dv_cfdata->cf_flags & VIORND_ONESHOT) {
> sc->sc_interval = 0;
> @@ -136,7 +137,8 @@ viornd_attach(struct device *parent, struct device *self, void *aux)
> timeout_add(&sc->sc_tick, 1);
>
> printf("\n");
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err2;
> return;
> err2:
> bus_dmamap_destroy(vsc->sc_dmat, sc->sc_dmamap);
> diff --git a/sys/dev/pv/vioscsi.c b/sys/dev/pv/vioscsi.c
> index 41dbc113d0d..0e8f9b41a78 100644
> --- a/sys/dev/pv/vioscsi.c
> +++ b/sys/dev/pv/vioscsi.c
> @@ -105,6 +105,7 @@ vioscsi_attach(struct device *parent, struct device *self, void *aux)
> {
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> struct vioscsi_softc *sc = (struct vioscsi_softc *)self;
> + struct virtio_attach_args *va = aux;
> struct scsibus_attach_args saa;
> int i, rv;
>
> @@ -120,7 +121,8 @@ vioscsi_attach(struct device *parent, struct device *self, void *aux)
> vsc->sc_vqs = sc->sc_vqs;
> vsc->sc_nvqs = nitems(sc->sc_vqs);
>
> - virtio_negotiate_features(vsc, NULL);
> + if (virtio_negotiate_features(vsc, NULL) != 0)
> + goto err;
> uint32_t cmd_per_lun = virtio_read_device_config_4(vsc,
> VIRTIO_SCSI_CONFIG_CMD_PER_LUN);
> uint32_t seg_max = virtio_read_device_config_4(vsc,
> @@ -166,7 +168,8 @@ vioscsi_attach(struct device *parent, struct device *self, void *aux)
> saa.saa_quirks = saa.saa_flags = 0;
> saa.saa_wwpn = saa.saa_wwnn = 0;
>
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err;
> config_found(self, &saa, scsiprint);
> return;
>
> diff --git a/sys/dev/pv/virtio.c b/sys/dev/pv/virtio.c
> index 53e75db9a23..bc3dca19a0b 100644
> --- a/sys/dev/pv/virtio.c
> +++ b/sys/dev/pv/virtio.c
> @@ -154,6 +154,25 @@ virtio_reset(struct virtio_softc *sc)
> sc->sc_active_features = 0;
> }
>
> +int
> +virtio_attach_finish(struct virtio_softc *sc, struct virtio_attach_args *va)
> +{
> + int i, ret;
> +
> + ret = sc->sc_ops->attach_finish(sc, va);
> + if (ret != 0)
> + return ret;
> +
> + sc->sc_ops->setup_intrs(sc);
> + for (i = 0; i < sc->sc_nvqs; i++) {
> + struct virtqueue *vq = &sc->sc_vqs[i];
> +
> + virtio_setup_queue(sc, vq, vq->vq_dmamap->dm_segs[0].ds_addr);
> + }
> + virtio_set_status(sc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + return 0;
> +}
> +
> void
> virtio_reinit_start(struct virtio_softc *sc)
> {
> @@ -162,6 +181,7 @@ virtio_reinit_start(struct virtio_softc *sc)
> virtio_set_status(sc, VIRTIO_CONFIG_DEVICE_STATUS_ACK);
> virtio_set_status(sc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER);
> virtio_negotiate_features(sc, NULL);
> + sc->sc_ops->setup_intrs(sc);
> for (i = 0; i < sc->sc_nvqs; i++) {
> int n;
> struct virtqueue *vq = &sc->sc_vqs[i];
> @@ -175,7 +195,6 @@ virtio_reinit_start(struct virtio_softc *sc)
> virtio_init_vq(sc, vq);
> virtio_setup_queue(sc, vq, vq->vq_dmamap->dm_segs[0].ds_addr);
> }
> - sc->sc_ops->setup_intrs(sc);
> }
>
> void
> @@ -421,7 +440,6 @@ virtio_alloc_vq(struct virtio_softc *sc, struct virtqueue *vq, int index,
> }
>
> virtio_init_vq(sc, vq);
> - virtio_setup_queue(sc, vq, vq->vq_dmamap->dm_segs[0].ds_addr);
>
> #if VIRTIO_DEBUG
> printf("\nallocated %u byte for virtqueue %d for %s, size %d\n",
> diff --git a/sys/dev/pv/virtiovar.h b/sys/dev/pv/virtiovar.h
> index eb15dace89a..c8c09d3d75b 100644
> --- a/sys/dev/pv/virtiovar.h
> +++ b/sys/dev/pv/virtiovar.h
> @@ -161,6 +161,7 @@ struct virtio_ops {
> int (*get_status)(struct virtio_softc *);
> void (*set_status)(struct virtio_softc *, int);
> int (*neg_features)(struct virtio_softc *, const struct virtio_feature_name *);
> + int (*attach_finish)(struct virtio_softc *, struct virtio_attach_args *);
> int (*poll_intr)(void *);
> void (*intr_barrier)(struct virtio_softc *);
> };
> @@ -220,6 +221,7 @@ virtio_has_feature(struct virtio_softc *sc, uint64_t fbit)
> int virtio_alloc_vq(struct virtio_softc*, struct virtqueue*, int, int,
> const char*);
> int virtio_free_vq(struct virtio_softc*, struct virtqueue*);
> +int virtio_attach_finish(struct virtio_softc *, struct virtio_attach_args *);
> void virtio_reset(struct virtio_softc *);
> void virtio_reinit_start(struct virtio_softc *);
> void virtio_reinit_end(struct virtio_softc *);
> diff --git a/sys/dev/pv/vmmci.c b/sys/dev/pv/vmmci.c
> index 580228bdb4c..e91e9f7e53e 100644
> --- a/sys/dev/pv/vmmci.c
> +++ b/sys/dev/pv/vmmci.c
> @@ -89,6 +89,7 @@ vmmci_attach(struct device *parent, struct device *self, void *aux)
> {
> struct vmmci_softc *sc = (struct vmmci_softc *)self;
> struct virtio_softc *vsc = (struct virtio_softc *)parent;
> + struct virtio_attach_args *va = aux;
>
> if (vsc->sc_child != NULL)
> panic("already attached to something else");
> @@ -101,7 +102,8 @@ vmmci_attach(struct device *parent, struct device *self, void *aux)
>
> vsc->sc_driver_features = VMMCI_F_TIMESYNC | VMMCI_F_ACK |
> VMMCI_F_SYNCRTC;
> - virtio_negotiate_features(vsc, NULL);
> + if (virtio_negotiate_features(vsc, NULL) != 0)
> + goto err;
>
> if (virtio_has_feature(vsc, VMMCI_F_TIMESYNC)) {
> strlcpy(sc->sc_sensordev.xname, sc->sc_dev.dv_xname,
> @@ -115,7 +117,12 @@ vmmci_attach(struct device *parent, struct device *self, void *aux)
> }
>
> printf("\n");
> - virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
> + if (virtio_attach_finish(vsc, va) != 0)
> + goto err;
> + return;
> +
> +err:
> + vsc->sc_child = VIRTIO_CHILD_ERROR;
> }
>
> int
>
virtio: Refactor attach logic some more