[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
|
