[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



Hello Simon,

On 10/22/2018 04:45 PM, Simon Kuenzer wrote:
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.


I will change it in the next patch.
+
+/**
+ * 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 idea behind this was to treat the virtio_id as a black box data structure.
+
+/**
+ * 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()?


I prefer VIRTIO_BUS and will update it in the next version.
+
+#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



Thanks & Regards
Sharan

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.