Date: Tue, 19 Jan 2021 05:08:36 GMT From: Bryan Venteicher <bryanv@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: e6cc42f181ce - main - virtio: Handle possible failure of virtio_finalize_features() Message-ID: <202101190508.10J58aZw086221@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by bryanv: URL: https://cgit.FreeBSD.org/src/commit/?id=e6cc42f181ce346a294ae7aa1d1c321fdc90f241 commit e6cc42f181ce346a294ae7aa1d1c321fdc90f241 Author: Bryan Venteicher <bryanv@FreeBSD.org> AuthorDate: 2021-01-19 04:55:26 +0000 Commit: Bryan Venteicher <bryanv@FreeBSD.org> CommitDate: 2021-01-19 04:55:26 +0000 virtio: Handle possible failure of virtio_finalize_features() Try to standardize how drivers negotiate feature and the function names Reviewed by: grehan (mentor) Differential Revision: https://reviews.freebsd.org/D27930 --- sys/dev/virtio/balloon/virtio_balloon.c | 33 +++++++++++++++------ sys/dev/virtio/block/virtio_blk.c | 28 ++++++++++++------ sys/dev/virtio/console/virtio_console.c | 25 ++++++++++------ sys/dev/virtio/network/if_vtnet.c | 25 ++++++++++------ sys/dev/virtio/pci/virtio_pci_modern.c | 2 -- sys/dev/virtio/random/virtio_random.c | 51 +++++++++++++++++++++++---------- sys/dev/virtio/scsi/virtio_scsi.c | 50 ++++++++++++++++++++++---------- 7 files changed, 149 insertions(+), 65 deletions(-) diff --git a/sys/dev/virtio/balloon/virtio_balloon.c b/sys/dev/virtio/balloon/virtio_balloon.c index 0973528887c5..848dd4e9a7f5 100644 --- a/sys/dev/virtio/balloon/virtio_balloon.c +++ b/sys/dev/virtio/balloon/virtio_balloon.c @@ -90,7 +90,8 @@ static int vtballoon_attach(device_t); static int vtballoon_detach(device_t); static int vtballoon_config_change(device_t); -static void vtballoon_negotiate_features(struct vtballoon_softc *); +static int vtballoon_negotiate_features(struct vtballoon_softc *); +static int vtballoon_setup_features(struct vtballoon_softc *); static int vtballoon_alloc_virtqueues(struct vtballoon_softc *); static void vtballoon_vq_intr(void *); @@ -110,7 +111,7 @@ static void vtballoon_free_page(struct vtballoon_softc *, vm_page_t); static int vtballoon_sleep(struct vtballoon_softc *); static void vtballoon_thread(void *); -static void vtballoon_add_sysctl(struct vtballoon_softc *); +static void vtballoon_setup_sysctl(struct vtballoon_softc *); #define vtballoon_modern(_sc) \ (((_sc)->vtballoon_features & VIRTIO_F_VERSION_1) != 0) @@ -183,14 +184,18 @@ vtballoon_attach(device_t dev) sc = device_get_softc(dev); sc->vtballoon_dev = dev; + virtio_set_feature_desc(dev, vtballoon_feature_desc); VTBALLOON_LOCK_INIT(sc, device_get_nameunit(dev)); TAILQ_INIT(&sc->vtballoon_pages); - vtballoon_add_sysctl(sc); + vtballoon_setup_sysctl(sc); - virtio_set_feature_desc(dev, vtballoon_feature_desc); - vtballoon_negotiate_features(sc); + error = vtballoon_setup_features(sc); + if (error) { + device_printf(dev, "cannot setup features\n"); + goto fail; + } sc->vtballoon_page_frames = malloc(VTBALLOON_PAGES_PER_REQUEST * sizeof(uint32_t), M_DEVBUF, M_NOWAIT | M_ZERO); @@ -276,7 +281,7 @@ vtballoon_config_change(device_t dev) return (1); } -static void +static int vtballoon_negotiate_features(struct vtballoon_softc *sc) { device_t dev; @@ -286,7 +291,19 @@ vtballoon_negotiate_features(struct vtballoon_softc *sc) features = VTBALLOON_FEATURES; sc->vtballoon_features = virtio_negotiate_features(dev, features); - virtio_finalize_features(dev); + return (virtio_finalize_features(dev)); +} + +static int +vtballoon_setup_features(struct vtballoon_softc *sc) +{ + int error; + + error = vtballoon_negotiate_features(sc); + if (error) + return (error); + + return (0); } static int @@ -559,7 +576,7 @@ vtballoon_thread(void *xsc) } static void -vtballoon_add_sysctl(struct vtballoon_softc *sc) +vtballoon_setup_sysctl(struct vtballoon_softc *sc) { device_t dev; struct sysctl_ctx_list *ctx; diff --git a/sys/dev/virtio/block/virtio_blk.c b/sys/dev/virtio/block/virtio_blk.c index 08df77d6de5b..6056771e3735 100644 --- a/sys/dev/virtio/block/virtio_blk.c +++ b/sys/dev/virtio/block/virtio_blk.c @@ -135,8 +135,8 @@ static int vtblk_ioctl(struct disk *, u_long, void *, int, static int vtblk_dump(void *, void *, vm_offset_t, off_t, size_t); static void vtblk_strategy(struct bio *); -static void vtblk_negotiate_features(struct vtblk_softc *); -static void vtblk_setup_features(struct vtblk_softc *); +static int vtblk_negotiate_features(struct vtblk_softc *); +static int vtblk_setup_features(struct vtblk_softc *); static int vtblk_maximum_segments(struct vtblk_softc *, struct virtio_blk_config *); static int vtblk_alloc_virtqueue(struct vtblk_softc *); @@ -312,10 +312,10 @@ vtblk_attach(device_t dev) struct virtio_blk_config blkcfg; int error; - virtio_set_feature_desc(dev, vtblk_feature_desc); - sc = device_get_softc(dev); sc->vtblk_dev = dev; + virtio_set_feature_desc(dev, vtblk_feature_desc); + VTBLK_LOCK_INIT(sc, device_get_nameunit(dev)); bioq_init(&sc->vtblk_bioq); TAILQ_INIT(&sc->vtblk_dump_queue); @@ -323,7 +323,12 @@ vtblk_attach(device_t dev) TAILQ_INIT(&sc->vtblk_req_ready); vtblk_setup_sysctl(sc); - vtblk_setup_features(sc); + + error = vtblk_setup_features(sc); + if (error) { + device_printf(dev, "cannot setup features\n"); + goto fail; + } vtblk_read_config(sc, &blkcfg); @@ -572,7 +577,7 @@ vtblk_strategy(struct bio *bp) VTBLK_UNLOCK(sc); } -static void +static int vtblk_negotiate_features(struct vtblk_softc *sc) { device_t dev; @@ -583,17 +588,20 @@ vtblk_negotiate_features(struct vtblk_softc *sc) VTBLK_LEGACY_FEATURES; sc->vtblk_features = virtio_negotiate_features(dev, features); - virtio_finalize_features(dev); + return (virtio_finalize_features(dev)); } -static void +static int vtblk_setup_features(struct vtblk_softc *sc) { device_t dev; + int error; dev = sc->vtblk_dev; - vtblk_negotiate_features(sc); + error = vtblk_negotiate_features(sc); + if (error) + return (error); if (virtio_with_feature(dev, VIRTIO_RING_F_INDIRECT_DESC)) sc->vtblk_flags |= VTBLK_FLAG_INDIRECT; @@ -603,6 +611,8 @@ vtblk_setup_features(struct vtblk_softc *sc) /* Legacy. */ if (virtio_with_feature(dev, VIRTIO_BLK_F_BARRIER)) sc->vtblk_flags |= VTBLK_FLAG_BARRIER; + + return (0); } static int diff --git a/sys/dev/virtio/console/virtio_console.c b/sys/dev/virtio/console/virtio_console.c index 0bd7c982e3f5..315eb59716b4 100644 --- a/sys/dev/virtio/console/virtio_console.c +++ b/sys/dev/virtio/console/virtio_console.c @@ -157,8 +157,8 @@ static int vtcon_attach(device_t); static int vtcon_detach(device_t); static int vtcon_config_change(device_t); -static void vtcon_setup_features(struct vtcon_softc *); -static void vtcon_negotiate_features(struct vtcon_softc *); +static int vtcon_setup_features(struct vtcon_softc *); +static int vtcon_negotiate_features(struct vtcon_softc *); static int vtcon_alloc_scports(struct vtcon_softc *); static int vtcon_alloc_virtqueues(struct vtcon_softc *); static void vtcon_read_config(struct vtcon_softc *, @@ -331,12 +331,16 @@ vtcon_attach(device_t dev) sc = device_get_softc(dev); sc->vtcon_dev = dev; + virtio_set_feature_desc(dev, vtcon_feature_desc); mtx_init(&sc->vtcon_mtx, "vtconmtx", NULL, MTX_DEF); mtx_init(&sc->vtcon_ctrl_tx_mtx, "vtconctrlmtx", NULL, MTX_DEF); - virtio_set_feature_desc(dev, vtcon_feature_desc); - vtcon_setup_features(sc); + error = vtcon_setup_features(sc); + if (error) { + device_printf(dev, "cannot setup features\n"); + goto fail; + } vtcon_read_config(sc, &concfg); vtcon_determine_max_ports(sc, &concfg); @@ -428,7 +432,7 @@ vtcon_config_change(device_t dev) return (0); } -static void +static int vtcon_negotiate_features(struct vtcon_softc *sc) { device_t dev; @@ -438,22 +442,27 @@ vtcon_negotiate_features(struct vtcon_softc *sc) features = VTCON_FEATURES; sc->vtcon_features = virtio_negotiate_features(dev, features); - virtio_finalize_features(dev); + return (virtio_finalize_features(dev)); } -static void +static int vtcon_setup_features(struct vtcon_softc *sc) { device_t dev; + int error; dev = sc->vtcon_dev; - vtcon_negotiate_features(sc); + error = vtcon_negotiate_features(sc); + if (error) + return (error); if (virtio_with_feature(dev, VIRTIO_CONSOLE_F_SIZE)) sc->vtcon_flags |= VTCON_FLAG_SIZE; if (virtio_with_feature(dev, VIRTIO_CONSOLE_F_MULTIPORT)) sc->vtcon_flags |= VTCON_FLAG_MULTIPORT; + + return (0); } #define VTCON_GET_CONFIG(_dev, _feature, _field, _cfg) \ diff --git a/sys/dev/virtio/network/if_vtnet.c b/sys/dev/virtio/network/if_vtnet.c index ed3065b61283..8d0770f5ac2d 100644 --- a/sys/dev/virtio/network/if_vtnet.c +++ b/sys/dev/virtio/network/if_vtnet.c @@ -102,8 +102,8 @@ static int vtnet_shutdown(device_t); static int vtnet_attach_completed(device_t); static int vtnet_config_change(device_t); -static void vtnet_negotiate_features(struct vtnet_softc *); -static void vtnet_setup_features(struct vtnet_softc *); +static int vtnet_negotiate_features(struct vtnet_softc *); +static int vtnet_setup_features(struct vtnet_softc *); static int vtnet_init_rxq(struct vtnet_softc *, int); static int vtnet_init_txq(struct vtnet_softc *, int); static int vtnet_alloc_rxtx_queues(struct vtnet_softc *); @@ -434,7 +434,6 @@ vtnet_attach(device_t dev) sc = device_get_softc(dev); sc->vtnet_dev = dev; - virtio_set_feature_desc(dev, vtnet_feature_desc); VTNET_CORE_LOCK_INIT(sc); @@ -448,7 +447,12 @@ vtnet_attach(device_t dev) } vtnet_setup_sysctl(sc); - vtnet_setup_features(sc); + + error = vtnet_setup_features(sc); + if (error) { + device_printf(dev, "cannot setup features\n"); + goto fail; + } error = vtnet_alloc_rx_filters(sc); if (error) { @@ -619,7 +623,7 @@ vtnet_config_change(device_t dev) return (0); } -static void +static int vtnet_negotiate_features(struct vtnet_softc *sc) { device_t dev; @@ -705,17 +709,20 @@ vtnet_negotiate_features(struct vtnet_softc *sc) sc->vtnet_features = negotiated_features; sc->vtnet_negotiated_features = negotiated_features; - virtio_finalize_features(dev); + return (virtio_finalize_features(dev)); } -static void +static int vtnet_setup_features(struct vtnet_softc *sc) { device_t dev; + int error; dev = sc->vtnet_dev; - vtnet_negotiate_features(sc); + error = vtnet_negotiate_features(sc); + if (error) + return (error); if (virtio_with_feature(dev, VIRTIO_F_VERSION_1)) sc->vtnet_flags |= VTNET_FLAG_MODERN; @@ -807,6 +814,8 @@ vtnet_setup_features(struct vtnet_softc *sc) sc->vtnet_flags |= VTNET_FLAG_MQ; } } + + return (0); } static int diff --git a/sys/dev/virtio/pci/virtio_pci_modern.c b/sys/dev/virtio/pci/virtio_pci_modern.c index 09ac0a1232e7..7029d2ff76ce 100644 --- a/sys/dev/virtio/pci/virtio_pci_modern.c +++ b/sys/dev/virtio/pci/virtio_pci_modern.c @@ -462,8 +462,6 @@ vtpci_modern_finalize_features(device_t dev) /* * Must re-read the status after setting it to verify the negotiated * features were accepted by the device. - * - * BMV: TODO Drivers need to handle possible failure of this method! */ vtpci_modern_set_status(sc, VIRTIO_CONFIG_S_FEATURES_OK); diff --git a/sys/dev/virtio/random/virtio_random.c b/sys/dev/virtio/random/virtio_random.c index 8c01b1cf6ae3..ee3a24bb5513 100644 --- a/sys/dev/virtio/random/virtio_random.c +++ b/sys/dev/virtio/random/virtio_random.c @@ -36,7 +36,6 @@ __FBSDID("$FreeBSD$"); #include <sys/malloc.h> #include <sys/module.h> #include <sys/sglist.h> -#include <sys/callout.h> #include <sys/random.h> #include <sys/stdatomic.h> @@ -50,6 +49,7 @@ __FBSDID("$FreeBSD$"); #include <dev/virtio/virtqueue.h> struct vtrnd_softc { + device_t vtrnd_dev; uint64_t vtrnd_features; struct virtqueue *vtrnd_vq; }; @@ -60,8 +60,9 @@ static int vtrnd_probe(device_t); static int vtrnd_attach(device_t); static int vtrnd_detach(device_t); -static void vtrnd_negotiate_features(device_t); -static int vtrnd_alloc_virtqueue(device_t); +static int vtrnd_negotiate_features(struct vtrnd_softc *); +static int vtrnd_setup_features(struct vtrnd_softc *); +static int vtrnd_alloc_virtqueue(struct vtrnd_softc *); static int vtrnd_harvest(struct vtrnd_softc *, void *, size_t *); static unsigned vtrnd_read(void *, unsigned); @@ -142,11 +143,16 @@ vtrnd_attach(device_t dev) int error; sc = device_get_softc(dev); - + sc->vtrnd_dev = dev; virtio_set_feature_desc(dev, vtrnd_feature_desc); - vtrnd_negotiate_features(dev); - error = vtrnd_alloc_virtqueue(dev); + error = vtrnd_setup_features(sc); + if (error) { + device_printf(dev, "cannot setup features\n"); + goto fail; + } + + error = vtrnd_alloc_virtqueue(sc); if (error) { device_printf(dev, "cannot allocate virtqueue\n"); goto fail; @@ -182,23 +188,38 @@ vtrnd_detach(device_t dev) return (0); } -static void -vtrnd_negotiate_features(device_t dev) +static int +vtrnd_negotiate_features(struct vtrnd_softc *sc) { - struct vtrnd_softc *sc; + device_t dev; + uint64_t features; - sc = device_get_softc(dev); - sc->vtrnd_features = virtio_negotiate_features(dev, VTRND_FEATURES); - virtio_finalize_features(dev); + dev = sc->vtrnd_dev; + features = VTRND_FEATURES; + + sc->vtrnd_features = virtio_negotiate_features(dev, features); + return (virtio_finalize_features(dev)); } static int -vtrnd_alloc_virtqueue(device_t dev) +vtrnd_setup_features(struct vtrnd_softc *sc) { - struct vtrnd_softc *sc; + int error; + + error = vtrnd_negotiate_features(sc); + if (error) + return (error); + + return (0); +} + +static int +vtrnd_alloc_virtqueue(struct vtrnd_softc *sc) +{ + device_t dev; struct vq_alloc_info vq_info; - sc = device_get_softc(dev); + dev = sc->vtrnd_dev; VQ_ALLOC_INFO_INIT(&vq_info, 0, NULL, sc, &sc->vtrnd_vq, "%s request", device_get_nameunit(dev)); diff --git a/sys/dev/virtio/scsi/virtio_scsi.c b/sys/dev/virtio/scsi/virtio_scsi.c index f4c716af3725..737b6d0a7a42 100644 --- a/sys/dev/virtio/scsi/virtio_scsi.c +++ b/sys/dev/virtio/scsi/virtio_scsi.c @@ -76,7 +76,8 @@ static int vtscsi_detach(device_t); static int vtscsi_suspend(device_t); static int vtscsi_resume(device_t); -static void vtscsi_negotiate_features(struct vtscsi_softc *); +static int vtscsi_negotiate_features(struct vtscsi_softc *); +static int vtscsi_setup_features(struct vtscsi_softc *); static void vtscsi_read_config(struct vtscsi_softc *, struct virtio_scsi_config *); static int vtscsi_maximum_segments(struct vtscsi_softc *, int); @@ -184,7 +185,7 @@ static void vtscsi_disable_vqs_intr(struct vtscsi_softc *); static void vtscsi_enable_vqs_intr(struct vtscsi_softc *); static void vtscsi_get_tunables(struct vtscsi_softc *); -static void vtscsi_add_sysctl(struct vtscsi_softc *); +static void vtscsi_setup_sysctl(struct vtscsi_softc *); static void vtscsi_printf_req(struct vtscsi_request *, const char *, const char *, ...); @@ -285,22 +286,19 @@ vtscsi_attach(device_t dev) sc = device_get_softc(dev); sc->vtscsi_dev = dev; + virtio_set_feature_desc(dev, vtscsi_feature_desc); VTSCSI_LOCK_INIT(sc, device_get_nameunit(dev)); TAILQ_INIT(&sc->vtscsi_req_free); vtscsi_get_tunables(sc); - vtscsi_add_sysctl(sc); - - virtio_set_feature_desc(dev, vtscsi_feature_desc); - vtscsi_negotiate_features(sc); + vtscsi_setup_sysctl(sc); - if (virtio_with_feature(dev, VIRTIO_RING_F_INDIRECT_DESC)) - sc->vtscsi_flags |= VTSCSI_FLAG_INDIRECT; - if (virtio_with_feature(dev, VIRTIO_SCSI_F_INOUT)) - sc->vtscsi_flags |= VTSCSI_FLAG_BIDIRECTIONAL; - if (virtio_with_feature(dev, VIRTIO_SCSI_F_HOTPLUG)) - sc->vtscsi_flags |= VTSCSI_FLAG_HOTPLUG; + error = vtscsi_setup_features(sc); + if (error) { + device_printf(dev, "cannot setup features\n"); + goto fail; + } vtscsi_read_config(sc, &scsicfg); @@ -413,7 +411,7 @@ vtscsi_resume(device_t dev) return (0); } -static void +static int vtscsi_negotiate_features(struct vtscsi_softc *sc) { device_t dev; @@ -423,7 +421,29 @@ vtscsi_negotiate_features(struct vtscsi_softc *sc) features = VTSCSI_FEATURES; sc->vtscsi_features = virtio_negotiate_features(dev, features); - virtio_finalize_features(dev); + return (virtio_finalize_features(dev)); +} + +static int +vtscsi_setup_features(struct vtscsi_softc *sc) +{ + device_t dev; + int error; + + dev = sc->vtscsi_dev; + + error = vtscsi_negotiate_features(sc); + if (error) + return (error); + + if (virtio_with_feature(dev, VIRTIO_RING_F_INDIRECT_DESC)) + sc->vtscsi_flags |= VTSCSI_FLAG_INDIRECT; + if (virtio_with_feature(dev, VIRTIO_SCSI_F_INOUT)) + sc->vtscsi_flags |= VTSCSI_FLAG_BIDIRECTIONAL; + if (virtio_with_feature(dev, VIRTIO_SCSI_F_HOTPLUG)) + sc->vtscsi_flags |= VTSCSI_FLAG_HOTPLUG; + + return (0); } #define VTSCSI_GET_CONFIG(_dev, _field, _cfg) \ @@ -2287,7 +2307,7 @@ vtscsi_get_tunables(struct vtscsi_softc *sc) } static void -vtscsi_add_sysctl(struct vtscsi_softc *sc) +vtscsi_setup_sysctl(struct vtscsi_softc *sc) { device_t dev; struct vtscsi_statistics *stats;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202101190508.10J58aZw086221>