From: Jan Klemkow <jan@openbsd.org>
Subject: Re: virtio: Refactor attach logic some more
To: Stefan Fritsch <sf@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 19 Dec 2024 14:11:50 +0100

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
>