[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH v4 06/11] lib/uknetdev: Netdev registration



On 11.10.18 14:32, Sharan Santhanam wrote:

Hello Simon,

Please find the comments inline.

On 10/10/2018 02:00 PM, Simon Kuenzer wrote:

From: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>

Introduce initial netdev API supporting device registration. We
introduce three header files are introduced for describing Unikraft's
netdev API:
   uk/netdev_core.h - Core data type definitions
   uk/netdev_driver.h - API for drivers
   uk/netdev.h - API for network applications and stacks

Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
---
  lib/uknetdev/Makefile.uk                |   1 +
  lib/uknetdev/exportsyms.uk              |   6 ++
  lib/uknetdev/include/uk/netdev.h        | 117 ++++++++++++++++++++++++++++++   lib/uknetdev/include/uk/netdev_core.h   | 105 +++++++++++++++++++++++++++ 
  lib/uknetdev/include/uk/netdev_driver.h |  74 +++++++++++++++++++
  lib/uknetdev/netdev.c                   | 124 ++++++++++++++++++++++++++++++++ 
  6 files changed, 427 insertions(+)
  create mode 100644 lib/uknetdev/include/uk/netdev.h
  create mode 100644 lib/uknetdev/include/uk/netdev_core.h
  create mode 100644 lib/uknetdev/include/uk/netdev_driver.h
  create mode 100644 lib/uknetdev/netdev.c

diff --git a/lib/uknetdev/Makefile.uk b/lib/uknetdev/Makefile.uk
index 81afae4..ca08b25 100644
--- a/lib/uknetdev/Makefile.uk
+++ b/lib/uknetdev/Makefile.uk
@@ -4,3 +4,4 @@ CINCLUDES-$(CONFIG_LIBUKNETDEV)        += -I$(LIBUKNETDEV_BASE)/include 
  CXXINCLUDES-$(CONFIG_LIBUKNETDEV)    += -I$(LIBUKNETDEV_BASE)/include
  LIBUKNETDEV_SRCS-y += $(LIBUKNETDEV_BASE)/netbuf.c
+LIBUKNETDEV_SRCS-y += $(LIBUKNETDEV_BASE)/netdev.c
diff --git a/lib/uknetdev/exportsyms.uk b/lib/uknetdev/exportsyms.uk
index 591d46c..66340c5 100644
--- a/lib/uknetdev/exportsyms.uk
+++ b/lib/uknetdev/exportsyms.uk
@@ -8,3 +8,9 @@ uk_netbuf_free
  uk_netbuf_disconnect
  uk_netbuf_connect
  uk_netbuf_append
+uk_netdev_drv_register
+uk_netdev_count
+uk_netdev_get
+uk_netdev_id_get
+uk_netdev_drv_name_get
+uk_netdev_state_get
diff --git a/lib/uknetdev/include/uk/netdev.h b/lib/uknetdev/include/uk/netdev.h 
new file mode 100644
index 0000000..1fd45c8
--- /dev/null
+++ b/lib/uknetdev/include/uk/netdev.h
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *          Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
+ *
+ * Copyright (c) 2010-2017 Intel Corporation
+ * 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.
+ */


We probably need to add the commit-id for reference


Yes, you are right.


+/* Taken and adapted from DPDK rte_ethdev.h */
+#ifndef __UK_NETDEV__
+#define __UK_NETDEV__
+
+#include <uk/netdev_core.h>
+#include <uk/assert.h>
+#include <uk/errptr.h>
+
+/**
+ * Unikraft Network API
+ *
+ * The Unikraft netdev API provides a generalized interface between network 
+ * device drivers and network stack implementations or low-level network
+ * applications.
+ *
+ * Most API interfaces take as parameter a reference to the corresponding + * Unikraft Network Device (struct uk_netdev) which can be initially obtained + * by its ID by calling uk_netdev_get(). The network application should store 
+ * this reference and use it for subsequent API calls.
+ */
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Get the number of registered network devices.
+ *
+ * @return
+ *   - (unsigned int): number of network devices.
+ */
+unsigned int uk_netdev_count(void);
+
+/**
+ * Get the reference to a network device based on its ID.
+ * Valid IDs are in the range of 0 to (n-1) where n is
+ * the number of available network devices.
+ *
+ * @param id
+ *   The identifier of the network device to configure.
+ * @return
+ *   - (NULL): device not available
+ *   - (struct uk_netdev *): reference to network device
+ */
+struct uk_netdev *uk_netdev_get(unsigned int id);
+
+/**
+ * Returns the id of a network device
+ *
+ * @param dev
+ *   The Unikraft Network Device.
+ * @return
+ *   - (>=0): Device ID
+ */
+uint16_t uk_netdev_id_get(struct uk_netdev *dev);
+
+/**
+ * Returns the driver name of a network device.
+ * The name might be set to NULL
+ *
+ * @param dev
+ *   The Unikraft Network Device.
+ * @return
+ *   - (NULL): if no name is defined.
+ *   - (const char *): Reference to string if name is available.
+ */
+const char *uk_netdev_drv_name_get(struct uk_netdev *dev);
+
+/**
+ * Returns the current state of a network device.
+ *
+ * @param dev
+ *   The Unikraft Network Device.
+ * @return
+ *   - (enum uk_netdev_state): current device state
+ */
+enum uk_netdev_state uk_netdev_state_get(struct uk_netdev *dev);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __UK_NETDEV__ */
diff --git a/lib/uknetdev/include/uk/netdev_core.h b/lib/uknetdev/include/uk/netdev_core.h 
new file mode 100644
index 0000000..eec441e
--- /dev/null
+++ b/lib/uknetdev/include/uk/netdev_core.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *          Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
+ *
+ * Copyright (c) 2017 Intel Corporation
+ * 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.
+ */


We probably need to add the commit-id for reference

+/* Taken and adapted from DPDK rte_ethdev_core.h */
+#ifndef __UK_NETDEV_CORE__
+#define __UK_NETDEV_CORE__
+
+#include <sys/types.h>
+#include <stdint.h>
+#include <errno.h>
+#include <uk/config.h>
+#include <uk/netbuf.h>
+#include <uk/list.h>
+#include <uk/alloc.h>
+#include <uk/essentials.h>
+
+/**
+ * Unikraft network API common declarations.
+ *
+ * This header contains all API data types. Some of them are part of the
+ * public API and some are part of the internal API.
+ *
+ * The device data and operations are separated. This split allows the
+ * function pointer and driver data to be per-process, while the actual
+ * configuration data for the device is shared.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+struct uk_netdev;
+UK_TAILQ_HEAD(uk_netdev_list, struct uk_netdev);
+
+
+/**
+ * Enum to describe possible states of an Unikraft network device.
+ */
+enum uk_netdev_state {
+    UK_NETDEV_UNREGISTERED = 0,
+    UK_NETDEV_UNCONFIGURED,
+    UK_NETDEV_CONFIGURED,
+    UK_NETDEV_RUNNING,
+};
+
+
+/**
+ * @internal
+ * libuknetdev internal data associated with each network device.
+ */
+struct uk_netdev_data {
+    enum uk_netdev_state state;
+
+    const uint16_t       id;    /**< ID is assigned during registration */ 
+    const char           *drv_name;
+};
+
+/**
+ * NETDEV
+ * A structure used to interact with a network device.
+ */
+struct uk_netdev {
+    /** Pointer to API-internal state data. */
+    struct uk_netdev_data       *_data;
+
+    UK_TAILQ_ENTRY(struct uk_netdev) _list;
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __UK_NETDEV_CORE__ */
diff --git a/lib/uknetdev/include/uk/netdev_driver.h b/lib/uknetdev/include/uk/netdev_driver.h 
new file mode 100644
index 0000000..ac8ce61
--- /dev/null
+++ b/lib/uknetdev/include/uk/netdev_driver.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Simon Kuenzer <simon.kuenzer@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 __UK_NETDEV_DRIVER__
+#define __UK_NETDEV_DRIVER__
+
+#include <uk/netdev_core.h>
+#include <uk/assert.h>
+
+/**
+ * Unikraft network driver API.
+ *
+ * This header contains all API functions that are supposed to be called
+ * by a network device driver.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Adds a Unikraft network device to the device list.
+ * This should be called whenever a driver adds a new found device.
+ *
+ * @param dev
+ *   Struct to unikraft network device that shall be registered
+ * @param a
+ *   Allocator to be use for libuknetdev private data (dev->_data)
+ * @param drv_name
+ *   (Optional) driver name
+ *   The memory for this string has to stay available as long as the
+ *   device is registered.
+ * @return
+ *   - (-ENOMEM): Allocation of private


Should it not be >=0


Yes.


+ *   - (>0): Network device ID on success
+ */
+int uk_netdev_drv_register(struct uk_netdev *dev, struct uk_alloc *a,
+               const char *drv_name);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __UK_NETDEV_DRIVER__ */
diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
new file mode 100644
index 0000000..ab8aa4d
--- /dev/null
+++ b/lib/uknetdev/netdev.c
@@ -0,0 +1,124 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *          Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
+ *
+ * Copyright (c) 2017-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 <string.h>
+#include <stdio.h>
+#include <uk/netdev.h>
+#include <uk/print.h>
+
+struct uk_netdev_list uk_netdev_list =
+    UK_TAILQ_HEAD_INITIALIZER(uk_netdev_list);
+static uint16_t netdev_count;
+
+static struct uk_netdev_data *_alloc_data(struct uk_alloc *a,
+                      uint16_t netdev_id,
+                      const char *drv_name)
+{
+    struct uk_netdev_data *data;
+
+    data = uk_malloc(a, sizeof(*data));
+    if (!data)
+        return NULL;
+
+    data->drv_name = drv_name;
+    data->state    = UK_NETDEV_UNCONFIGURED;
+
+    /* This is the only place where we set the device ID;
+     * during the rest of the device's life time this ID is read-only
+     */
+    *(DECONST(uint16_t *, &data->id)) = netdev_id;
+
+    return data;
+}
+
+int uk_netdev_drv_register(struct uk_netdev *dev, struct uk_alloc *a,
+               const char *drv_name)
+{
+    UK_ASSERT(dev);
+    UK_ASSERT(!dev->_data);
+
+    dev->_data = _alloc_data(a, netdev_count,  drv_name);
+    if (!dev->_data)
+        return -ENOMEM;
+
+    UK_TAILQ_INSERT_TAIL(&uk_netdev_list, dev, _list);
+    uk_pr_info("Registered netdev%"PRIu16": %p (%s)\n",
+           netdev_count, dev, drv_name);
+
+    return netdev_count++;
+}
+
+unsigned int uk_netdev_count(void)
+{
+    return (unsigned int) netdev_count;
+}
+
+struct uk_netdev *uk_netdev_get(unsigned int id)
+{
+    struct uk_netdev *dev;
+
+    UK_TAILQ_FOREACH(dev, &uk_netdev_list, _list) {
+        UK_ASSERT(dev->_data);
+
+        if (dev->_data->id == id)
+            return dev;
+    }
+    return NULL;
+}
+
+uint16_t uk_netdev_id_get(struct uk_netdev *dev)
+{
+    UK_ASSERT(dev);
+    UK_ASSERT(dev->_data);
+
+    return dev->_data->id;
+}
+
+const char *uk_netdev_drv_name_get(struct uk_netdev *dev)
+{
+    UK_ASSERT(dev);
+    UK_ASSERT(dev->_data);
+
+    return dev->_data->drv_name;
+}
+
+enum uk_netdev_state uk_netdev_state_get(struct uk_netdev *dev)
+{
+    UK_ASSERT(dev);
+


This condition below seems inconsistent with the get where we
UK_ASSERT(dev->_data);
I am not sure if we would have a case where the netdev reference exist without a valid _data. 

Right, I will change it.


+    if (!dev->_data)
+        return UK_NETDEV_UNREGISTERED;
+    return dev->_data->state;
+}


Thanks & Regards
Sharan


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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

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