[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 3/7] plat/drivers: Introduce the virtio bus
Hey Sharan, this patch looks good. A have a few minor comments. Thanks, Simon On 19.10.18 15:06, Sharan Santhanam wrote: This patch introduces the virtio bus to bridge the virtio device with the bus. We introduce the functions to add driver to control the virtio device. Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx> --- plat/drivers/include/virtio/virtio.h | 130 +++++++++++++++++++++++++++++++++++ plat/drivers/virtio/virtio_bus.c | 109 +++++++++++++++++++++++++++++ plat/kvm/Config.uk | 11 +++ plat/kvm/Makefile.uk | 12 ++++ 4 files changed, 262 insertions(+) create mode 100644 plat/drivers/include/virtio/virtio.h create mode 100644 plat/drivers/virtio/virtio_bus.c diff --git a/plat/drivers/include/virtio/virtio.h b/plat/drivers/include/virtio/virtio.h Hum, wouldn't it to make sense to call also the header file virtio_bus.h? I think it would separate the purpose of the headers more clearly. new file mode 100644 index 0000000..d471cab --- /dev/null +++ b/plat/drivers/include/virtio/virtio.h @@ -0,0 +1,130 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* + * Authors: Sharan Santhanam <sharan.santhanam@xxxxxxxxx> + * + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the copyright holder nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY. + */ + +#ifndef __PLAT_DRV_VIRTIO_H +#define __PLAT_DRV_VIRTIO_H + +#include <uk/config.h> +#include <errno.h> +#include <uk/errptr.h> +#include <uk/arch/types.h> +#include <uk/arch/lcpu.h> +#include <uk/alloc.h> +#include <virtio/virtio_config.h> + +#ifdef __cplusplus +extern "C" { +#endif /* __cplusplus */ + +#define VIRTIO_FEATURES_UPDATE(features, bpos) \ + (features |= (1ULL << bpos)) + +struct virtio_dev; +typedef int (*virtio_driver_init_func_t)(struct uk_alloc *); +typedef int (*virtio_driver_add_func_t)(struct virtio_dev *); + +typedef enum virtio_dev_status_t { + /** Device reset */ + VIRTIO_DEV_RESET, + /** Device Acknowledge and ready to be configured */ + VIRTIO_DEV_INITIALIZED, + /** Device feature negotiated and ready to the started */ + VIRTIO_DEV_CONFIGURED, + /** Device Running */ + VIRTIO_DEV_RUNNING, + /** Device Stopped */ + VIRTIO_DEV_STOPPED, +} virtio_dev_status_t; Do we really need this as a typedef? I usually prefer keeping"enum virtio_dev_status" in the argument lists. For me this is more clear what the argument about and less hiding details. + +/** + * The structure define a virtio device. + */ +struct virtio_dev_id { + /** Device identifier of the virtio device */ + __u16 virtio_device_id; +}; Do you think we need this as a struct? What is the idea behind? + +/** + * The structure define the virtio driver. + */ +struct virtio_driver { + /** Next entry of the driver list */ + UK_TAILQ_ENTRY(struct virtio_driver) next; + /** The id map for the virtio device */ + const struct virtio_dev_id *dev_ids; + /** The init function for the driver */ + virtio_driver_init_func_t init; + /** Adding the virtio device */ + virtio_driver_add_func_t add_dev; +}; + +/** + * The structure defines the virtio device. + */ +struct virtio_dev { + /* Feature bit describing the virtio device */ + __u64 features; + /* Private data of the driver */ + void *priv; + /* Virtio device identifier */ + struct virtio_dev_id id; + /* Reference to the virtio driver for the device */ + struct virtio_driver *vdrv; + /* Status of the device */ + virtio_dev_status_t status; +}; + +/** + * Operation exported by the virtio device. + */ +void _virtio_register_driver(struct virtio_driver *vdrv); + +#define VIRTIO_REGISTER_DRIVER(b) \ + _VIRTIO_REGISTER_DRIVER(__LIBNAME__, b) VIRTIOBUS_REGISTER_DRIVER() or VIRTIO_BUS_REGISTER_DRIVER()? + +#define _VIRTIO_REGFNAME(x, y) x##y + +#define _VIRTIO_REGISTER_DRIVER(libname, b) \ + static void __constructor_prio(104) \ + _VIRTIO_REGFNAME(libname, _virtio_register_driver)(void) \ + { \ + _virtio_register_driver((b)); \ + } + + +#ifdef __cplusplus +} +#endif /* __cplusplus */ + +#endif /* __PLAT_DRV_VIRTIO_H */ diff --git a/plat/drivers/virtio/virtio_bus.c b/plat/drivers/virtio/virtio_bus.c new file mode 100644 index 0000000..88cb99a --- /dev/null +++ b/plat/drivers/virtio/virtio_bus.c @@ -0,0 +1,109 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* + * Authors: Sharan Santhanam <sharan.santhanam@xxxxxxxxx> + * + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the copyright holder nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY. + */ + +#include <uk/config.h> +#include <uk/arch/types.h> +#include <uk/list.h> +#include <uk/alloc.h> +#include <uk/bus.h> +#include <virtio/virtio_ids.h> +#include <virtio/virtio_config.h> +#include <virtio/virtio.h> + +UK_TAILQ_HEAD(virtio_driver_list, struct virtio_driver); +/** + * Module local data structure(s). + */ +static struct virtio_driver_list virtio_drvs = + UK_TAILQ_HEAD_INITIALIZER(virtio_drvs); +static struct uk_alloc *a; + +/** + * Driver module local function(s). + */ +static int virtio_bus_init(struct uk_alloc *mem_alloc); +static int virtio_bus_probe(void); + +/** + * Probe for the virtio device. + */ +static int virtio_bus_probe(void) +{ + return 0; +} + +/** + * Initialize the virtio bus driver(s). + * @param mem_alloc + * Reference to the mem_allocator. + * @return + * (int) On successful initialization return the count of device + * initialized. + * On error return -1. + */ +static int virtio_bus_init(struct uk_alloc *mem_alloc) +{ + struct virtio_driver *drv = NULL, *ndrv = NULL; + int ret = 0, dev_count = 0; + + a = mem_alloc; + UK_TAILQ_FOREACH_SAFE(drv, &virtio_drvs, next, ndrv) { + if (drv->init) { + ret = drv->init(a); + if (unlikely(ret)) { + uk_pr_err("Failed to initialize virtio driver %p: %d\n", + drv, ret); + UK_TAILQ_REMOVE(&virtio_drvs, drv, next); + } else + dev_count++; + } + } + return (dev_count > 0) ? dev_count : -1; +} + +/** + * Register the virtio driver(s). + * @param vdrv + * Reference to the virtio_driver + */ +void _virtio_register_driver(struct virtio_driver *vdrv) _virtio_bus_register_driver(struct virtio_driver *vdrv)? +{ + UK_TAILQ_INSERT_TAIL(&virtio_drvs, vdrv, next); +} + +static struct uk_bus virtio_bus = { + .init = virtio_bus_init, + .probe = virtio_bus_probe, +}; +UK_BUS_REGISTER(&virtio_bus); diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk index 0f10b4d..f8a8aaf 100644 --- a/plat/kvm/Config.uk +++ b/plat/kvm/Config.uk @@ -50,4 +50,15 @@ config KVM_PCI select LIBUKBUS help PCI bus driver for probing and operating PCI devices + +config VIRTIO_BUS + bool "Virtio bus driver" + default y + depends on (ARCH_X86_64) + depends on LIBUKBUS + select LIBUKALLOC + help + Virtio bus driver for probing and operating virtio device and + transport layer. + endif diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk index 06d8410..80a0c05 100644 --- a/plat/kvm/Makefile.uk +++ b/plat/kvm/Makefile.uk @@ -8,6 +8,7 @@ $(eval $(call addplat_s,kvm,$(CONFIG_PLAT_KVM))) ## $(eval $(call addplatlib,kvm,libkvmplat)) $(eval $(call addplatlib_s,kvm,libkvmpci,$(CONFIG_KVM_PCI))) +$(eval $(call addplatlib_s,kvm,libkvmvirtio,$(CONFIG_VIRTIO)))#### Platform library definitions @@ -80,3 +81,14 @@ LIBKVMPCI_ASINCLUDES-$(CONFIG_ARCH_X86_64) += -I$(UK_PLAT_COMMON_BASE)/include LIBKVMPCI_CINCLUDES-$(CONFIG_ARCH_X86_64) += -I$(UK_PLAT_COMMON_BASE)/include LIBKVMPCI_SRCS-$(CONFIG_ARCH_X86_64) += $(UK_PLAT_COMMON_BASE)/pci_bus.c|common+##+## Virtio library definitions +## +LIBKVMVIRTIO_ASINCLUDES-y += -I$(LIBKVMPLAT_BASE)/include +LIBKVMVIRTIO_CINCLUDES-y += -I$(LIBKVMPLAT_BASE)/include +LIBKVMVIRTIO_ASINCLUDES-y += -I$(UK_PLAT_COMMON_BASE)/include +LIBKVMVIRTIO_CINCLUDES-y += -I$(UK_PLAT_COMMON_BASE)/include +LIBKVMVIRTIO_ASINCLUDES-y += -I$(UK_PLAT_DRIVERS_BASE)/include +LIBKVMVIRTIO_CINCLUDES-y += -I$(UK_PLAT_DRIVERS_BASE)/include +LIBKVMVIRTIO_SRCS-$(CONFIG_VIRTIO_BUS) +=\ + $(UK_PLAT_DRIVERS_BASE)/virtio/virtio_bus.c _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |