Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 19 Jan 2021 05:14:26 +0000
From:      Jessica Clarke <jrtc27@freebsd.org>
To:        Bryan Venteicher <bryanv@FreeBSD.org>
Cc:        "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org>
Subject:   Re: git: 9da9560c4dd3 - main - virtio: Add VirtIO PCI modern (V1) support
Message-ID:  <1CDD0314-F132-4E38-A614-53EBF8738209@freebsd.org>
In-Reply-To: <202101190508.10J580Tb085558@gitrepo.freebsd.org>
References:  <202101190508.10J580Tb085558@gitrepo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Hi Bryan,
I had a couple of unaddressed review comments still unresolved.

Jess

> On 19 Jan 2021, at 05:08, Bryan Venteicher <bryanv@FreeBSD.org> wrote:
>=20
> The branch main has been updated by bryanv:
>=20
> URL: =
https://cgit.FreeBSD.org/src/commit/?id=3D9da9560c4dd3556519cd391a04f0db15=
7dc3c295
>=20
> commit 9da9560c4dd3556519cd391a04f0db157dc3c295
> Author:     Bryan Venteicher <bryanv@FreeBSD.org>
> AuthorDate: 2021-01-19 04:55:23 +0000
> Commit:     Bryan Venteicher <bryanv@FreeBSD.org>
> CommitDate: 2021-01-19 04:55:23 +0000
>=20
>    virtio: Add VirtIO PCI modern (V1) support
>=20
>    Use the existing legacy PCI driver as the basis for shared code
>    between the legacy and modern PCI drivers. The existing virtio_pci
>    kernel module will contain both the legacy and modern drivers.
>=20
>    Changes to the virtqueue and each device driver (network, block, =
etc)
>    for V1 support come in later commits.
>=20
>    Update the MMIO driver to reflect the VirtIO bus method changes, =
but
>    the modern compliance can be improved on later.
>=20
>    Note that the modern PCI driver requires bus_map_resource() to be
>    implemented, which is not the case on all archs.
>=20
>    The hw.virtio.pci.transitional tunable default value is zero so
>    transitional devices will continue to be driven via the legacy
>    driver.
>=20
>    Reviewed by: grehan (mentor)
>    Differential Revision: https://reviews.freebsd.org/D27856
> ---
> sys/conf/files                             |    3 +
> sys/dev/virtio/mmio/virtio_mmio.c          |   23 +-
> sys/dev/virtio/pci/virtio_pci.c            | 1297 =
+++++++++----------------
> sys/dev/virtio/pci/virtio_pci.h            |  173 ++--
> sys/dev/virtio/pci/virtio_pci_if.m         |   71 ++
> sys/dev/virtio/pci/virtio_pci_legacy.c     |  733 ++++++++++++++
> sys/dev/virtio/pci/virtio_pci_legacy_var.h |   78 ++
> sys/dev/virtio/pci/virtio_pci_modern.c     | 1448 =
++++++++++++++++++++++++++++
> sys/dev/virtio/pci/virtio_pci_modern_var.h |  135 +++
> sys/dev/virtio/pci/virtio_pci_var.h        |   55 ++
> sys/dev/virtio/virtio.c                    |   76 +-
> sys/dev/virtio/virtio.h                    |    8 +
> sys/dev/virtio/virtio_bus_if.m             |   11 +
> sys/dev/virtio/virtio_endian.h             |  106 ++
> sys/dev/virtio/virtqueue.c                 |   10 +-
> sys/dev/virtio/virtqueue.h                 |    4 +-
> sys/modules/virtio/pci/Makefile            |    5 +-
> 17 files changed, 3292 insertions(+), 944 deletions(-)
>=20
> diff --git a/sys/conf/files b/sys/conf/files
> index e641b79d9c46..79f3b7ca54e6 100644
> --- a/sys/conf/files
> +++ b/sys/conf/files
> @@ -3457,6 +3457,9 @@ dev/virtio/virtqueue.c			optional	=
virtio
> dev/virtio/virtio_bus_if.m		optional	virtio
> dev/virtio/virtio_if.m			optional	virtio
> dev/virtio/pci/virtio_pci.c		optional	virtio_pci
> +dev/virtio/pci/virtio_pci_if.m		optional	=
virtio_pci
> +dev/virtio/pci/virtio_pci_legacy.c	optional	virtio_pci
> +dev/virtio/pci/virtio_pci_modern.c	optional	virtio_pci
> dev/virtio/mmio/virtio_mmio.c		optional	virtio_mmio
> dev/virtio/mmio/virtio_mmio_acpi.c	optional	virtio_mmio acpi
> dev/virtio/mmio/virtio_mmio_fdt.c	optional	virtio_mmio fdt
> diff --git a/sys/dev/virtio/mmio/virtio_mmio.c =
b/sys/dev/virtio/mmio/virtio_mmio.c
> index bb5d84754f09..694d2a232fdd 100644
> --- a/sys/dev/virtio/mmio/virtio_mmio.c
> +++ b/sys/dev/virtio/mmio/virtio_mmio.c
> @@ -84,7 +84,7 @@ static void	vtmmio_stop(device_t);
> static void	vtmmio_poll(device_t);
> static int	vtmmio_reinit(device_t, uint64_t);
> static void	vtmmio_reinit_complete(device_t);
> -static void	vtmmio_notify_virtqueue(device_t, uint16_t);
> +static void	vtmmio_notify_virtqueue(device_t, uint16_t, bus_size_t);
> static uint8_t	vtmmio_get_status(device_t);
> static void	vtmmio_set_status(device_t, uint8_t);
> static void	vtmmio_read_dev_config(device_t, bus_size_t, void *, =
int);
> @@ -352,6 +352,13 @@ vtmmio_read_ivar(device_t dev, device_t child, =
int index, uintptr_t *result)
> 		 */
> 		*result =3D 0;
> 		break;
> +	case VIRTIO_IVAR_MODERN:
> +		/*
> +		 * There are several modern (aka MMIO v2) spec =
compliance
> +		 * issues with this driver, but keep the status quo.
> +		 */
> +		*result =3D sc->vtmmio_version > 1;
> +		break;
> 	default:
> 		return (ENOENT);
> 	}
> @@ -388,6 +395,10 @@ vtmmio_negotiate_features(device_t dev, uint64_t =
child_features)
>=20
> 	sc =3D device_get_softc(dev);
>=20
> +	if (sc->vtmmio_version > 1) {
> +		child_features |=3D VIRTIO_F_VERSION_1;
> +	}
> +
> 	vtmmio_write_config_4(sc, VIRTIO_MMIO_HOST_FEATURES_SEL, 1);
> 	host_features =3D vtmmio_read_config_4(sc, =
VIRTIO_MMIO_HOST_FEATURES);
> 	host_features <<=3D 32;
> @@ -402,7 +413,7 @@ vtmmio_negotiate_features(device_t dev, uint64_t =
child_features)
> 	 * host all support.
> 	 */
> 	features =3D host_features & child_features;
> -	features =3D virtqueue_filter_features(features);
> +	features =3D virtio_filter_transport_features(features);
> 	sc->vtmmio_features =3D features;
>=20
> 	vtmmio_describe_features(sc, "negotiated", features);
> @@ -504,7 +515,8 @@ vtmmio_alloc_virtqueues(device_t dev, int flags, =
int nvqs,
> 		size =3D vtmmio_read_config_4(sc, =
VIRTIO_MMIO_QUEUE_NUM_MAX);
>=20
> 		error =3D virtqueue_alloc(dev, idx, size,
> -		    VIRTIO_MMIO_VRING_ALIGN, ~(vm_paddr_t)0, info, &vq);
> +		    VIRTIO_MMIO_QUEUE_NOTIFY, VIRTIO_MMIO_VRING_ALIGN,
> +		    ~(vm_paddr_t)0, info, &vq);
> 		if (error) {
> 			device_printf(dev,
> 			    "cannot allocate virtqueue %d: %d\n",
> @@ -586,13 +598,14 @@ vtmmio_reinit_complete(device_t dev)
> }
>=20
> static void
> -vtmmio_notify_virtqueue(device_t dev, uint16_t queue)
> +vtmmio_notify_virtqueue(device_t dev, uint16_t queue, bus_size_t =
offset)
> {
> 	struct vtmmio_softc *sc;
>=20
> 	sc =3D device_get_softc(dev);
> +	MPASS(offset =3D=3D VIRTIO_MMIO_QUEUE_NOTIFY);
>=20
> -	vtmmio_write_config_4(sc, VIRTIO_MMIO_QUEUE_NOTIFY, queue);
> +	vtmmio_write_config_4(sc, offset, queue);
> }
>=20
> static uint8_t
> diff --git a/sys/dev/virtio/pci/virtio_pci.c =
b/sys/dev/virtio/pci/virtio_pci.c
> index 05a632f526a8..4d6fa929ef19 100644
> --- a/sys/dev/virtio/pci/virtio_pci.c
> +++ b/sys/dev/virtio/pci/virtio_pci.c
> @@ -1,7 +1,7 @@
> /*-
>  * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
>  *
> - * Copyright (c) 2011, Bryan Venteicher <bryanv@FreeBSD.org>
> + * Copyright (c) 2017, Bryan Venteicher <bryanv@FreeBSD.org>
>  * All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
> @@ -26,8 +26,6 @@
>  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
>=20
> -/* Driver for the VirtIO PCI interface. */
> -
> #include <sys/cdefs.h>
> __FBSDID("$FreeBSD$");
>=20
> @@ -37,7 +35,6 @@ __FBSDID("$FreeBSD$");
> #include <sys/kernel.h>
> #include <sys/module.h>
> #include <sys/malloc.h>
> -#include <sys/endian.h>
>=20
> #include <machine/bus.h>
> #include <machine/resource.h>
> @@ -50,369 +47,236 @@ __FBSDID("$FreeBSD$");
> #include <dev/virtio/virtio.h>
> #include <dev/virtio/virtqueue.h>
> #include <dev/virtio/pci/virtio_pci.h>
> +#include <dev/virtio/pci/virtio_pci_var.h>
>=20
> -#include "virtio_bus_if.h"
> +#include "virtio_pci_if.h"
> #include "virtio_if.h"
>=20
> -struct vtpci_interrupt {
> -	struct resource		*vti_irq;
> -	int			 vti_rid;
> -	void			*vti_handler;
> -};
> -
> -struct vtpci_virtqueue {
> -	struct virtqueue	*vtv_vq;
> -	int			 vtv_no_intr;
> -};
> -
> -struct vtpci_softc {
> -	device_t			 vtpci_dev;
> -	struct resource			*vtpci_res;
> -	struct resource			*vtpci_msix_res;
> -	uint64_t			 vtpci_features;
> -	uint32_t			 vtpci_flags;
> -#define VTPCI_FLAG_NO_MSI		0x0001
> -#define VTPCI_FLAG_NO_MSIX		0x0002
> -#define VTPCI_FLAG_LEGACY		0x1000
> -#define VTPCI_FLAG_MSI			0x2000
> -#define VTPCI_FLAG_MSIX			0x4000
> -#define VTPCI_FLAG_SHARED_MSIX		0x8000
> -#define VTPCI_FLAG_ITYPE_MASK		0xF000
> -
> -	/* This "bus" will only ever have one child. */
> -	device_t			 vtpci_child_dev;
> -	struct virtio_feature_desc	*vtpci_child_feat_desc;
> -
> -	int				 vtpci_nvqs;
> -	struct vtpci_virtqueue		*vtpci_vqs;
> -
> -	/*
> -	 * Ideally, each virtqueue that the driver provides a callback =
for will
> -	 * receive its own MSIX vector. If there are not sufficient =
vectors
> -	 * available, then attempt to have all the VQs share one vector. =
For
> -	 * MSIX, the configuration changed notifications must be on =
their own
> -	 * vector.
> -	 *
> -	 * If MSIX is not available, we will attempt to have the whole =
device
> -	 * share one MSI vector, and then, finally, one legacy =
interrupt.
> -	 */
> -	struct vtpci_interrupt		 vtpci_device_interrupt;
> -	struct vtpci_interrupt		*vtpci_msix_vq_interrupts;
> -	int				 vtpci_nmsix_resources;
> -};
> -
> -static int	vtpci_probe(device_t);
> -static int	vtpci_attach(device_t);
> -static int	vtpci_detach(device_t);
> -static int	vtpci_suspend(device_t);
> -static int	vtpci_resume(device_t);
> -static int	vtpci_shutdown(device_t);
> -static void	vtpci_driver_added(device_t, driver_t *);
> -static void	vtpci_child_detached(device_t, device_t);
> -static int	vtpci_read_ivar(device_t, device_t, int, uintptr_t *);
> -static int	vtpci_write_ivar(device_t, device_t, int, uintptr_t);
> -
> -static uint64_t	vtpci_negotiate_features(device_t, uint64_t);
> -static int	vtpci_with_feature(device_t, uint64_t);
> -static int	vtpci_alloc_virtqueues(device_t, int, int,
> -		    struct vq_alloc_info *);
> -static int	vtpci_setup_intr(device_t, enum intr_type);
> -static void	vtpci_stop(device_t);
> -static int	vtpci_reinit(device_t, uint64_t);
> -static void	vtpci_reinit_complete(device_t);
> -static void	vtpci_notify_virtqueue(device_t, uint16_t);
> -static uint8_t	vtpci_get_status(device_t);
> -static void	vtpci_set_status(device_t, uint8_t);
> -static void	vtpci_read_dev_config(device_t, bus_size_t, void *, =
int);
> -static void	vtpci_write_dev_config(device_t, bus_size_t, void *, =
int);
> -
> -static void	vtpci_describe_features(struct vtpci_softc *, const char =
*,
> +static void	vtpci_describe_features(struct vtpci_common *, const =
char *,
> 		    uint64_t);
> -static void	vtpci_probe_and_attach_child(struct vtpci_softc *);
> -
> -static int	vtpci_alloc_msix(struct vtpci_softc *, int);
> -static int	vtpci_alloc_msi(struct vtpci_softc *);
> -static int	vtpci_alloc_intr_msix_pervq(struct vtpci_softc *);
> -static int	vtpci_alloc_intr_msix_shared(struct vtpci_softc *);
> -static int	vtpci_alloc_intr_msi(struct vtpci_softc *);
> -static int	vtpci_alloc_intr_legacy(struct vtpci_softc *);
> -static int	vtpci_alloc_interrupt(struct vtpci_softc *, int, int,
> +static int	vtpci_alloc_msix(struct vtpci_common *, int);
> +static int	vtpci_alloc_msi(struct vtpci_common *);
> +static int	vtpci_alloc_intr_msix_pervq(struct vtpci_common *);
> +static int	vtpci_alloc_intr_msix_shared(struct vtpci_common *);
> +static int	vtpci_alloc_intr_msi(struct vtpci_common *);
> +static int	vtpci_alloc_intr_intx(struct vtpci_common *);
> +static int	vtpci_alloc_interrupt(struct vtpci_common *, int, int,
> +		    struct vtpci_interrupt *);
> +static void	vtpci_free_interrupt(struct vtpci_common *,
> 		    struct vtpci_interrupt *);
> -static int	vtpci_alloc_intr_resources(struct vtpci_softc *);
>=20
> -static int	vtpci_setup_legacy_interrupt(struct vtpci_softc *,
> +static void	vtpci_free_interrupts(struct vtpci_common *);
> +static void	vtpci_free_virtqueues(struct vtpci_common *);
> +static void	vtpci_cleanup_setup_intr_attempt(struct vtpci_common *);
> +static int	vtpci_alloc_intr_resources(struct vtpci_common *);
> +static int	vtpci_setup_intx_interrupt(struct vtpci_common *,
> 		    enum intr_type);
> -static int	vtpci_setup_pervq_msix_interrupts(struct vtpci_softc *,
> +static int	vtpci_setup_pervq_msix_interrupts(struct vtpci_common *,
> 		    enum intr_type);
> -static int	vtpci_setup_msix_interrupts(struct vtpci_softc *,
> +static int	vtpci_set_host_msix_vectors(struct vtpci_common *);
> +static int	vtpci_setup_msix_interrupts(struct vtpci_common *,
> 		    enum intr_type);
> -static int	vtpci_setup_interrupts(struct vtpci_softc *, enum =
intr_type);
> -
> -static int	vtpci_register_msix_vector(struct vtpci_softc *, int,
> -		    struct vtpci_interrupt *);
> -static int	vtpci_set_host_msix_vectors(struct vtpci_softc *);
> -static int	vtpci_reinit_virtqueue(struct vtpci_softc *, int);
> -
> -static void	vtpci_free_interrupt(struct vtpci_softc *,
> -		    struct vtpci_interrupt *);
> -static void	vtpci_free_interrupts(struct vtpci_softc *);
> -static void	vtpci_free_virtqueues(struct vtpci_softc *);
> -static void	vtpci_release_child_resources(struct vtpci_softc *);
> -static void	vtpci_cleanup_setup_intr_attempt(struct vtpci_softc *);
> -static void	vtpci_reset(struct vtpci_softc *);
> -
> -static void	vtpci_select_virtqueue(struct vtpci_softc *, int);
> -
> -static void	vtpci_legacy_intr(void *);
> +static int	vtpci_setup_intrs(struct vtpci_common *, enum =
intr_type);
> +static int	vtpci_reinit_virtqueue(struct vtpci_common *, int);
> +static void	vtpci_intx_intr(void *);
> static int	vtpci_vq_shared_intr_filter(void *);
> static void	vtpci_vq_shared_intr(void *);
> static int	vtpci_vq_intr_filter(void *);
> static void	vtpci_vq_intr(void *);
> static void	vtpci_config_intr(void *);
>=20
> -#define vtpci_setup_msi_interrupt vtpci_setup_legacy_interrupt
> -
> -#define VIRTIO_PCI_CONFIG(_sc) \
> -    VIRTIO_PCI_CONFIG_OFF((((_sc)->vtpci_flags & VTPCI_FLAG_MSIX)) !=3D=
 0)
> -
> -/*
> - * I/O port read/write wrappers.
> - */
> -#define vtpci_read_config_1(sc, o)	bus_read_1((sc)->vtpci_res, (o))
> -#define vtpci_write_config_1(sc, o, v)	=
bus_write_1((sc)->vtpci_res, (o), (v))
> +#define vtpci_setup_msi_interrupt vtpci_setup_intx_interrupt
>=20
> /*
> - * Virtio-pci specifies that PCI Configuration area is guest endian. =
However,
> - * since PCI devices are inherently little-endian, on BE systems the =
bus layer
> - * transparently converts it to BE. For virtio-legacy, this =
conversion is
> - * undesired, so an extra byte swap is required to fix it.
> + * This module contains two drivers:
> + *   - virtio_pci_legacy for pre-V1 support
> + *   - virtio_pci_modern for V1 support
>  */
> -#define vtpci_read_config_2(sc, o)      =
le16toh(bus_read_2((sc)->vtpci_res, (o)))
> -#define vtpci_read_config_4(sc, o)      =
le32toh(bus_read_4((sc)->vtpci_res, (o)))
> -#define vtpci_write_config_2(sc, o, v)  bus_write_2((sc)->vtpci_res, =
(o), (htole16(v)))
> -#define vtpci_write_config_4(sc, o, v)  bus_write_4((sc)->vtpci_res, =
(o), (htole32(v)))
> -
> -/* PCI Header LE. On BE systems the bus layer takes care of byte =
swapping */
> -#define vtpci_read_header_2(sc, o)      (bus_read_2((sc)->vtpci_res, =
(o)))
> -#define vtpci_read_header_4(sc, o)      (bus_read_4((sc)->vtpci_res, =
(o)))
> -#define vtpci_write_header_2(sc, o, v)  bus_write_2((sc)->vtpci_res, =
(o), (v))
> -#define vtpci_write_header_4(sc, o, v)  bus_write_4((sc)->vtpci_res, =
(o), (v))
> -
> -/* Tunables. */
> -static int vtpci_disable_msix =3D 0;
> -TUNABLE_INT("hw.virtio.pci.disable_msix", &vtpci_disable_msix);
> -
> -static device_method_t vtpci_methods[] =3D {
> -	/* Device interface. */
> -	DEVMETHOD(device_probe,			  vtpci_probe),
> -	DEVMETHOD(device_attach,		  vtpci_attach),
> -	DEVMETHOD(device_detach,		  vtpci_detach),
> -	DEVMETHOD(device_suspend,		  vtpci_suspend),
> -	DEVMETHOD(device_resume,		  vtpci_resume),
> -	DEVMETHOD(device_shutdown,		  vtpci_shutdown),
> -
> -	/* Bus interface. */
> -	DEVMETHOD(bus_driver_added,		  vtpci_driver_added),
> -	DEVMETHOD(bus_child_detached,		  vtpci_child_detached),
> -	DEVMETHOD(bus_child_pnpinfo_str,	  =
virtio_child_pnpinfo_str),
> -	DEVMETHOD(bus_read_ivar,		  vtpci_read_ivar),
> -	DEVMETHOD(bus_write_ivar,		  vtpci_write_ivar),
> -
> -	/* VirtIO bus interface. */
> -	DEVMETHOD(virtio_bus_negotiate_features,  =
vtpci_negotiate_features),
> -	DEVMETHOD(virtio_bus_with_feature,	  vtpci_with_feature),
> -	DEVMETHOD(virtio_bus_alloc_virtqueues,	  =
vtpci_alloc_virtqueues),
> -	DEVMETHOD(virtio_bus_setup_intr,	  vtpci_setup_intr),
> -	DEVMETHOD(virtio_bus_stop,		  vtpci_stop),
> -	DEVMETHOD(virtio_bus_reinit,		  vtpci_reinit),
> -	DEVMETHOD(virtio_bus_reinit_complete,	  =
vtpci_reinit_complete),
> -	DEVMETHOD(virtio_bus_notify_vq,		  =
vtpci_notify_virtqueue),
> -	DEVMETHOD(virtio_bus_read_device_config,  =
vtpci_read_dev_config),
> -	DEVMETHOD(virtio_bus_write_device_config, =
vtpci_write_dev_config),
> -
> -	DEVMETHOD_END
> -};
> -
> -static driver_t vtpci_driver =3D {
> -	"virtio_pci",
> -	vtpci_methods,
> -	sizeof(struct vtpci_softc)
> -};
> -
> -devclass_t vtpci_devclass;
> -
> -DRIVER_MODULE(virtio_pci, pci, vtpci_driver, vtpci_devclass, 0, 0);
> MODULE_VERSION(virtio_pci, 1);
> MODULE_DEPEND(virtio_pci, pci, 1, 1, 1);
> MODULE_DEPEND(virtio_pci, virtio, 1, 1, 1);
>=20
> -static int
> -vtpci_probe(device_t dev)
> -{
> -	char desc[36];
> -	const char *name;
> +int vtpci_disable_msix =3D 0;
> +TUNABLE_INT("hw.virtio.pci.disable_msix", &vtpci_disable_msix);
>=20
> -	if (pci_get_vendor(dev) !=3D VIRTIO_PCI_VENDORID)
> -		return (ENXIO);
> +static uint8_t
> +vtpci_read_isr(struct vtpci_common *cn)
> +{
> +	return (VIRTIO_PCI_READ_ISR(cn->vtpci_dev));
> +}
>=20
> -	if (pci_get_device(dev) < VIRTIO_PCI_DEVICEID_MIN ||
> -	    pci_get_device(dev) > VIRTIO_PCI_DEVICEID_MAX)
> -		return (ENXIO);
> +static uint16_t
> +vtpci_get_vq_size(struct vtpci_common *cn, int idx)
> +{
> +	return (VIRTIO_PCI_GET_VQ_SIZE(cn->vtpci_dev, idx));
> +}
>=20
> -	if (pci_get_revid(dev) !=3D VIRTIO_PCI_ABI_VERSION)
> -		return (ENXIO);
> +static bus_size_t
> +vtpci_get_vq_notify_off(struct vtpci_common *cn, int idx)
> +{
> +	return (VIRTIO_PCI_GET_VQ_NOTIFY_OFF(cn->vtpci_dev, idx));
> +}
>=20
> -	name =3D virtio_device_name(pci_get_subdevice(dev));
> -	if (name =3D=3D NULL)
> -		name =3D "Unknown";
> +static void
> +vtpci_set_vq(struct vtpci_common *cn, struct virtqueue *vq)
> +{
> +	VIRTIO_PCI_SET_VQ(cn->vtpci_dev, vq);
> +}
>=20
> -	snprintf(desc, sizeof(desc), "VirtIO PCI %s adapter", name);
> -	device_set_desc_copy(dev, desc);
> +static void
> +vtpci_disable_vq(struct vtpci_common *cn, int idx)
> +{
> +	VIRTIO_PCI_DISABLE_VQ(cn->vtpci_dev, idx);
> +}
>=20
> -	return (BUS_PROBE_DEFAULT);
> +static int
> +vtpci_register_cfg_msix(struct vtpci_common *cn, struct =
vtpci_interrupt *intr)
> +{
> +	return (VIRTIO_PCI_REGISTER_CFG_MSIX(cn->vtpci_dev, intr));
> }
>=20
> static int
> -vtpci_attach(device_t dev)
> +vtpci_register_vq_msix(struct vtpci_common *cn, int idx,
> +    struct vtpci_interrupt *intr)
> {
> -	struct vtpci_softc *sc;
> -	device_t child;
> -	int rid;
> +	return (VIRTIO_PCI_REGISTER_VQ_MSIX(cn->vtpci_dev, idx, intr));
> +}
>=20
> -	sc =3D device_get_softc(dev);
> -	sc->vtpci_dev =3D dev;
> +void
> +vtpci_init(struct vtpci_common *cn, device_t dev, bool modern)
> +{
>=20
> -	pci_enable_busmaster(dev);
> +	cn->vtpci_dev =3D dev;
>=20
> -	rid =3D PCIR_BAR(0);
> -	sc->vtpci_res =3D bus_alloc_resource_any(dev, SYS_RES_IOPORT, =
&rid,
> -	    RF_ACTIVE);
> -	if (sc->vtpci_res =3D=3D NULL) {
> -		device_printf(dev, "cannot map I/O space\n");
> -		return (ENXIO);
> -	}
> +	pci_enable_busmaster(dev);
>=20
> +	if (modern)
> +		cn->vtpci_flags |=3D VTPCI_FLAG_MODERN;
> 	if (pci_find_cap(dev, PCIY_MSI, NULL) !=3D 0)
> -		sc->vtpci_flags |=3D VTPCI_FLAG_NO_MSI;
> -
> -	if (pci_find_cap(dev, PCIY_MSIX, NULL) =3D=3D 0) {
> -		rid =3D PCIR_BAR(1);
> -		sc->vtpci_msix_res =3D bus_alloc_resource_any(dev,
> -		    SYS_RES_MEMORY, &rid, RF_ACTIVE);
> -	}
> -
> -	if (sc->vtpci_msix_res =3D=3D NULL)
> -		sc->vtpci_flags |=3D VTPCI_FLAG_NO_MSIX;
> +		cn->vtpci_flags |=3D VTPCI_FLAG_NO_MSI;
> +	if (pci_find_cap(dev, PCIY_MSIX, NULL) !=3D 0)
> +		cn->vtpci_flags |=3D VTPCI_FLAG_NO_MSIX;
> +}
>=20
> -	vtpci_reset(sc);
> +int
> +vtpci_add_child(struct vtpci_common *cn)
> +{
> +	device_t dev, child;
>=20
> -	/* Tell the host we've noticed this device. */
> -	vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_ACK);
> +	dev =3D cn->vtpci_dev;
>=20
> -	if ((child =3D device_add_child(dev, NULL, -1)) =3D=3D NULL) {
> +	child =3D device_add_child(dev, NULL, -1);
> +	if (child =3D=3D NULL) {
> 		device_printf(dev, "cannot create child device\n");
> -		vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_FAILED);
> -		vtpci_detach(dev);
> 		return (ENOMEM);
> 	}
>=20
> -	sc->vtpci_child_dev =3D child;
> -	vtpci_probe_and_attach_child(sc);
> +	cn->vtpci_child_dev =3D child;
>=20
> 	return (0);
> }
>=20
> -static int
> -vtpci_detach(device_t dev)
> +int
> +vtpci_delete_child(struct vtpci_common *cn)
> {
> -	struct vtpci_softc *sc;
> -	device_t child;
> +	device_t dev, child;
> 	int error;
>=20
> -	sc =3D device_get_softc(dev);
> +	dev =3D cn->vtpci_dev;
>=20
> -	if ((child =3D sc->vtpci_child_dev) !=3D NULL) {
> +	child =3D cn->vtpci_child_dev;
> +	if (child !=3D NULL) {
> 		error =3D device_delete_child(dev, child);
> 		if (error)
> 			return (error);
> -		sc->vtpci_child_dev =3D NULL;
> -	}
> -
> -	vtpci_reset(sc);
> -
> -	if (sc->vtpci_msix_res !=3D NULL) {
> -		bus_release_resource(dev, SYS_RES_MEMORY, PCIR_BAR(1),
> -		    sc->vtpci_msix_res);
> -		sc->vtpci_msix_res =3D NULL;
> -	}
> -
> -	if (sc->vtpci_res !=3D NULL) {
> -		bus_release_resource(dev, SYS_RES_IOPORT, PCIR_BAR(0),
> -		    sc->vtpci_res);
> -		sc->vtpci_res =3D NULL;
> +		cn->vtpci_child_dev =3D NULL;
> 	}
>=20
> 	return (0);
> }
>=20
> -static int
> -vtpci_suspend(device_t dev)
> +void
> +vtpci_child_detached(struct vtpci_common *cn)
> {
>=20
> -	return (bus_generic_suspend(dev));
> -}
> -
> -static int
> -vtpci_resume(device_t dev)
> -{
> +	vtpci_release_child_resources(cn);
>=20
> -	return (bus_generic_resume(dev));
> +	cn->vtpci_child_feat_desc =3D NULL;
> +	cn->vtpci_features =3D 0;
> }
>=20
> -static int
> -vtpci_shutdown(device_t dev)
> +int
> +vtpci_reinit(struct vtpci_common *cn)
> {
> +	int idx, error;
>=20
> -	(void) bus_generic_shutdown(dev);
> -	/* Forcibly stop the host device. */
> -	vtpci_stop(dev);
> +	for (idx =3D 0; idx < cn->vtpci_nvqs; idx++) {
> +		error =3D vtpci_reinit_virtqueue(cn, idx);
> +		if (error)
> +			return (error);
> +	}
> +
> +	if (vtpci_is_msix_enabled(cn)) {
> +		error =3D vtpci_set_host_msix_vectors(cn);
> +		if (error)
> +			return (error);
> +	}
>=20
> 	return (0);
> }
>=20
> static void
> -vtpci_driver_added(device_t dev, driver_t *driver)
> +vtpci_describe_features(struct vtpci_common *cn, const char *msg,
> +    uint64_t features)
> {
> -	struct vtpci_softc *sc;
> +	device_t dev, child;
> +
> +	dev =3D cn->vtpci_dev;
> +	child =3D cn->vtpci_child_dev;
>=20
> -	sc =3D device_get_softc(dev);
> +	if (device_is_attached(child) || bootverbose =3D=3D 0)
> +		return;
>=20
> -	vtpci_probe_and_attach_child(sc);
> +	virtio_describe(dev, msg, features, cn->vtpci_child_feat_desc);
> }
>=20
> -static void
> -vtpci_child_detached(device_t dev, device_t child)
> +uint64_t
> +vtpci_negotiate_features(struct vtpci_common *cn,
> +    uint64_t child_features, uint64_t host_features)
> {
> -	struct vtpci_softc *sc;
> +	uint64_t features;
>=20
> -	sc =3D device_get_softc(dev);
> +	vtpci_describe_features(cn, "host", host_features);
>=20
> -	vtpci_reset(sc);
> -	vtpci_release_child_resources(sc);
> +	/*
> +	 * Limit negotiated features to what the driver, virtqueue, and
> +	 * host all support.
> +	 */
> +	features =3D host_features & child_features;
> +	features =3D virtio_filter_transport_features(features);
> +	vtpci_describe_features(cn, "negotiated", features);
> +
> +	cn->vtpci_features =3D features;
> +
> +	return (features);
> }
>=20
> -static int
> -vtpci_read_ivar(device_t dev, device_t child, int index, uintptr_t =
*result)
> +int
> +vtpci_with_feature(struct vtpci_common *cn, uint64_t feature)
> {
> -	struct vtpci_softc *sc;
> +	return ((cn->vtpci_features & feature) !=3D 0);
> +}
>=20
> -	sc =3D device_get_softc(dev);
> +int
> +vtpci_read_ivar(struct vtpci_common *cn, int index, uintptr_t =
*result)
> +{
> +	device_t dev;
> +	int error;
>=20
> -	if (sc->vtpci_child_dev !=3D child)
> -		return (ENOENT);
> +	dev =3D cn->vtpci_dev;
> +	error =3D 0;
>=20
> 	switch (index) {
> -	case VIRTIO_IVAR_DEVTYPE:
> 	case VIRTIO_IVAR_SUBDEVICE:
> 		*result =3D pci_get_subdevice(dev);
> 		break;
> @@ -425,100 +289,74 @@ vtpci_read_ivar(device_t dev, device_t child, =
int index, uintptr_t *result)
> 	case VIRTIO_IVAR_SUBVENDOR:
> 		*result =3D pci_get_subvendor(dev);
> 		break;
> +	case VIRTIO_IVAR_MODERN:
> +		*result =3D vtpci_is_modern(cn);
> +		break;
> 	default:
> -		return (ENOENT);
> +		error =3D ENOENT;
> 	}
>=20
> -	return (0);
> +	return (error);
> }
>=20
> -static int
> -vtpci_write_ivar(device_t dev, device_t child, int index, uintptr_t =
value)
> +int
> +vtpci_write_ivar(struct vtpci_common *cn, int index, uintptr_t value)
> {
> -	struct vtpci_softc *sc;
> -
> -	sc =3D device_get_softc(dev);
> +	int error;
>=20
> -	if (sc->vtpci_child_dev !=3D child)
> -		return (ENOENT);
> +	error =3D 0;
>=20
> 	switch (index) {
> 	case VIRTIO_IVAR_FEATURE_DESC:
> -		sc->vtpci_child_feat_desc =3D (void *) value;
> +		cn->vtpci_child_feat_desc =3D (void *) value;
> 		break;
> 	default:
> -		return (ENOENT);
> +		error =3D ENOENT;
> 	}
>=20
> -	return (0);
> +	return (error);
> }
>=20
> -static uint64_t
> -vtpci_negotiate_features(device_t dev, uint64_t child_features)
> +int
> +vtpci_alloc_virtqueues(struct vtpci_common *cn, int flags, int nvqs,
> +    struct vq_alloc_info *vq_info)
> {
> -	struct vtpci_softc *sc;
> -	uint64_t host_features, features;
> -
> -	sc =3D device_get_softc(dev);
> +	device_t dev;
> +	int idx, align, error;
>=20
> -	host_features =3D vtpci_read_header_4(sc, =
VIRTIO_PCI_HOST_FEATURES);
> -	vtpci_describe_features(sc, "host", host_features);
> +	dev =3D cn->vtpci_dev;
>=20
> 	/*
> -	 * Limit negotiated features to what the driver, virtqueue, and
> -	 * host all support.
> +	 * This is VIRTIO_PCI_VRING_ALIGN from legacy VirtIO. In modern =
VirtIO,
> +	 * the tables do not have to be allocated contiguously, but we =
do so
> +	 * anyways.
> 	 */
> -	features =3D host_features & child_features;
> -	features =3D virtqueue_filter_features(features);
> -	sc->vtpci_features =3D features;
> -
> -	vtpci_describe_features(sc, "negotiated", features);
> -	vtpci_write_header_4(sc, VIRTIO_PCI_GUEST_FEATURES, features);
> -
> -	return (features);
> -}
> -
> -static int
> -vtpci_with_feature(device_t dev, uint64_t feature)
> -{
> -	struct vtpci_softc *sc;
> +	align =3D 4096;
>=20
> -	sc =3D device_get_softc(dev);
> -
> -	return ((sc->vtpci_features & feature) !=3D 0);
> -}
> -
> -static int
> -vtpci_alloc_virtqueues(device_t dev, int flags, int nvqs,
> -    struct vq_alloc_info *vq_info)
> -{
> -	struct vtpci_softc *sc;
> -	struct virtqueue *vq;
> -	struct vtpci_virtqueue *vqx;
> -	struct vq_alloc_info *info;
> -	int idx, error;
> -	uint16_t size;
> -
> -	sc =3D device_get_softc(dev);
> -
> -	if (sc->vtpci_nvqs !=3D 0)
> +	if (cn->vtpci_nvqs !=3D 0)
> 		return (EALREADY);
> 	if (nvqs <=3D 0)
> 		return (EINVAL);
>=20
> -	sc->vtpci_vqs =3D malloc(nvqs * sizeof(struct vtpci_virtqueue),
> +	cn->vtpci_vqs =3D malloc(nvqs * sizeof(struct vtpci_virtqueue),
> 	    M_DEVBUF, M_NOWAIT | M_ZERO);
> -	if (sc->vtpci_vqs =3D=3D NULL)
> +	if (cn->vtpci_vqs =3D=3D NULL)
> 		return (ENOMEM);
>=20
> 	for (idx =3D 0; idx < nvqs; idx++) {
> -		vqx =3D &sc->vtpci_vqs[idx];
> +		struct vtpci_virtqueue *vqx;
> +		struct vq_alloc_info *info;
> +		struct virtqueue *vq;
> +		bus_size_t notify_offset;
> +		uint16_t size;
> +
> +		vqx =3D &cn->vtpci_vqs[idx];
> 		info =3D &vq_info[idx];
>=20
> -		vtpci_select_virtqueue(sc, idx);
> -		size =3D vtpci_read_header_2(sc, VIRTIO_PCI_QUEUE_NUM);
> +		size =3D vtpci_get_vq_size(cn, idx);
> +		notify_offset =3D vtpci_get_vq_notify_off(cn, idx);
>=20
> -		error =3D virtqueue_alloc(dev, idx, size, =
VIRTIO_PCI_VRING_ALIGN,
> +		error =3D virtqueue_alloc(dev, idx, size, notify_offset, =
align,
> 		    ~(vm_paddr_t)0, info, &vq);
> 		if (error) {
> 			device_printf(dev,
> @@ -526,270 +364,27 @@ vtpci_alloc_virtqueues(device_t dev, int flags, =
int nvqs,
> 			break;
> 		}
>=20
> -		vtpci_write_header_4(sc, VIRTIO_PCI_QUEUE_PFN,
> -		    virtqueue_paddr(vq) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT);
> +		vtpci_set_vq(cn, vq);
>=20
> 		vqx->vtv_vq =3D *info->vqai_vq =3D vq;
> 		vqx->vtv_no_intr =3D info->vqai_intr =3D=3D NULL;
>=20
> -		sc->vtpci_nvqs++;
> +		cn->vtpci_nvqs++;
> 	}
>=20
> 	if (error)
> -		vtpci_free_virtqueues(sc);
> +		vtpci_free_virtqueues(cn);
>=20
> 	return (error);
> }
>=20
> static int
> -vtpci_setup_intr(device_t dev, enum intr_type type)
> -{
> -	struct vtpci_softc *sc;
> -	int attempt, error;
> -
> -	sc =3D device_get_softc(dev);
> -
> -	for (attempt =3D 0; attempt < 5; attempt++) {
> -		/*
> -		 * Start with the most desirable interrupt configuration =
and
> -		 * fallback towards less desirable ones.
> -		 */
> -		switch (attempt) {
> -		case 0:
> -			error =3D vtpci_alloc_intr_msix_pervq(sc);
> -			break;
> -		case 1:
> -			error =3D vtpci_alloc_intr_msix_shared(sc);
> -			break;
> -		case 2:
> -			error =3D vtpci_alloc_intr_msi(sc);
> -			break;
> -		case 3:
> -			error =3D vtpci_alloc_intr_legacy(sc);
> -			break;
> -		default:
> -			device_printf(dev,
> -			    "exhausted all interrupt allocation =
attempts\n");
> -			return (ENXIO);
> -		}
> -
> -		if (error =3D=3D 0 && vtpci_setup_interrupts(sc, type) =
=3D=3D 0)
> -			break;
> -
> -		vtpci_cleanup_setup_intr_attempt(sc);
> -	}
> -
> -	if (bootverbose) {
> -		if (sc->vtpci_flags & VTPCI_FLAG_LEGACY)
> -			device_printf(dev, "using legacy interrupt\n");
> -		else if (sc->vtpci_flags & VTPCI_FLAG_MSI)
> -			device_printf(dev, "using MSI interrupt\n");
> -		else if (sc->vtpci_flags & VTPCI_FLAG_SHARED_MSIX)
> -			device_printf(dev, "using shared MSIX =
interrupts\n");
> -		else
> -			device_printf(dev, "using per VQ MSIX =
interrupts\n");
> -	}
> -
> -	return (0);
> -}
> -
> -static void
> -vtpci_stop(device_t dev)
> -{
> -
> -	vtpci_reset(device_get_softc(dev));
> -}
> -
> -static int
> -vtpci_reinit(device_t dev, uint64_t features)
> -{
> -	struct vtpci_softc *sc;
> -	int idx, error;
> -
> -	sc =3D device_get_softc(dev);
> -
> -	/*
> -	 * Redrive the device initialization. This is a bit of an abuse =
of
> -	 * the specification, but VirtualBox, QEMU/KVM, and BHyVe seem =
to
> -	 * play nice.
> -	 *
> -	 * We do not allow the host device to change from what was =
originally
> -	 * negotiated beyond what the guest driver changed. MSIX state =
should
> -	 * not change, number of virtqueues and their size remain the =
same, etc.
> -	 * This will need to be rethought when we want to support =
migration.
> -	 */
> -
> -	if (vtpci_get_status(dev) !=3D VIRTIO_CONFIG_STATUS_RESET)
> -		vtpci_stop(dev);
> -
> -	/*
> -	 * Quickly drive the status through ACK and DRIVER. The device
> -	 * does not become usable again until vtpci_reinit_complete().
> -	 */
> -	vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_ACK);
> -	vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_DRIVER);
> -
> -	vtpci_negotiate_features(dev, features);
> -
> -	for (idx =3D 0; idx < sc->vtpci_nvqs; idx++) {
> -		error =3D vtpci_reinit_virtqueue(sc, idx);
> -		if (error)
> -			return (error);
> -	}
> -
> -	if (sc->vtpci_flags & VTPCI_FLAG_MSIX) {
> -		error =3D vtpci_set_host_msix_vectors(sc);
> -		if (error)
> -			return (error);
> -	}
> -
> -	return (0);
> -}
> -
> -static void
> -vtpci_reinit_complete(device_t dev)
> -{
> -
> -	vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> -}
> -
> -static void
> -vtpci_notify_virtqueue(device_t dev, uint16_t queue)
> -{
> -	struct vtpci_softc *sc;
> -
> -	sc =3D device_get_softc(dev);
> -
> -	vtpci_write_header_2(sc, VIRTIO_PCI_QUEUE_NOTIFY, queue);
> -}
> -
> -static uint8_t
> -vtpci_get_status(device_t dev)
> -{
> -	struct vtpci_softc *sc;
> -
> -	sc =3D device_get_softc(dev);
> -
> -	return (vtpci_read_config_1(sc, VIRTIO_PCI_STATUS));
> -}
> -
> -static void
> -vtpci_set_status(device_t dev, uint8_t status)
> -{
> -	struct vtpci_softc *sc;
> -
> -	sc =3D device_get_softc(dev);
> -
> -	if (status !=3D VIRTIO_CONFIG_STATUS_RESET)
> -		status |=3D vtpci_get_status(dev);
> -
> -	vtpci_write_config_1(sc, VIRTIO_PCI_STATUS, status);
> -}
> -
> -static void
> -vtpci_read_dev_config(device_t dev, bus_size_t offset,
> *** 4018 LINES SKIPPED ***




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1CDD0314-F132-4E38-A614-53EBF8738209>