[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH 03/15] plat/driver: Add tap device to the uk_netdev



Hi Sharan,


See inline.


Roxana

On 07.10.2019 12:46, Sharan Santhanam wrote:
The patch provides the implementation to parse the user provided tap
device information. The tap device detected are registered to the
uk_netdev.

Signed-off-by: Sharan Santhanam<sharan.santhanam@xxxxxxxxx>
---
  plat/drivers/tap/tap.c           | 126 +++++++++++++++++++++++++++++++++++++++
  plat/linuxu/Config.uk            |   7 +++
  plat/linuxu/Makefile.uk          |   4 ++
  plat/linuxu/include/linuxu/tap.h |  45 ++++++++++++++
  4 files changed, 182 insertions(+)
  create mode 100644 plat/linuxu/include/linuxu/tap.h

diff --git a/plat/drivers/tap/tap.c b/plat/drivers/tap/tap.c
index ffd21be..2f49788 100644
--- a/plat/drivers/tap/tap.c
+++ b/plat/drivers/tap/tap.c
@@ -40,8 +40,14 @@
  #include <uk/netdev_driver.h>
  #include <uk/netbuf.h>
  #include <uk/errptr.h>
+#include <uk/libparam.h>
  #include <uk/bus.h>
+#ifdef CONFIG_PLAT_LINUXU
+#include <linuxu/tap.h>
+#else
+#error "The driver does not support this Unikraft platform"
+#endif /* CONFIG_PLAT_LINUXU */
I don't understand the purpose of this if macro. If `PLAT_LINUXU` is not selected, the driver will not be selected according to the menu configuration.
  #define TAPDEV_MAX_CNT                 7
  #define TAPDEV_IFNAME_BRS       255
@@ -58,6 +64,31 @@
#define ETH_PKT_PAYLOAD_LEN 1500 +#define to_tapnetdev(dev) \
+               __containerof(dev, struct tap_net_dev, ndev)
+
+struct tap_net_dev {
+       /* Net device structure */
+       struct uk_netdev    ndev;
+       /* max number of queues */
+       __u16 max_qpairs;
+       /* Number of rxq configured */
+       /* The list of the tap device */
+       UK_TAILQ_ENTRY(struct tap_net_dev) next;
+       /* UK Netdevice identifier */
+       __u16 tid;
+       /* UK Netdevice identifier */
+       __u16 id;
Why are two variable with the same purpose/description?
+       /* Name of the character device */
+       char name[IFNAMSIZ];
+       /* MTU of the device */
+       __u16  mtu;
+       /* RX promiscuous mode */
+       __u8 promisc : 1;
+       /* State of the net device */
+       __u8 state;
+};
+
  struct tap_net_drv {
        struct uk_alloc *a;
        UK_TAILQ_HEAD(tdev_list, struct tap_net_dev) tap_dev_list;
@@ -69,6 +100,15 @@ struct tap_net_drv {
   * Module level variables
   */
  static struct tap_net_drv tap_drv = {0};
+static const char *drv_name = DRIVER_NAME;
+static int tap_dev_cnt;
+static char *bridgenames;
+
+/**
+ * Module Parameter.
+ */
+UK_LIB_PARAM(tap_dev_cnt, __u32);
+UK_LIB_PARAM_STR(bridgenames);
I think it would improve a lot the visibility to provide an example for this parameters.
  /**
   * Module functions
@@ -246,6 +286,53 @@ static const struct uk_netdev_ops tap_netdev_ops = {
        .txq_info_get = tap_netdev_txq_info_get,
        .rxq_info_get = tap_netdev_rxq_info_get,
  };
+
+/**
+ * Registering the network device.
+ */
+static int tap_dev_init(int id)
+{
+       struct tap_net_dev *tdev;
+       int rc = 0;
+
+       tdev = uk_zalloc(tap_drv.a, sizeof(*tdev));
+       if (!tdev) {
+               uk_pr_err(DRIVER_NAME": Failed to allocate tap_device\n");
+               rc = -ENOMEM;
+               goto exit;
+       }
+       tdev->ndev.rx_one = tap_netdev_recv;
+       tdev->ndev.tx_one = tap_netdev_xmit;
+       tdev->ndev.ops = &tap_netdev_ops;
+       tdev->tid = id;
Now I understand the difference between `id` and `tid`.
+       /**
+        * TODO:
+        * As an initial implementation we have limit on the number of queues.
+        */
+       tdev->max_qpairs = 1;
+
+       /* Registering the tap device with libuknet*/
+       rc = uk_netdev_drv_register(&tdev->ndev, tap_drv.a, drv_name);
+       if (rc < 0) {
+               uk_pr_err(DRIVER_NAME": Failed to register the network 
device\n");
+               goto free_tdev;
+       }
+       tdev->id = rc;
+       rc = 0;
+       tdev->mtu = ETH_PKT_PAYLOAD_LEN;
+       tdev->promisc = 0;
+       uk_pr_info(DRIVER_NAME": device(%d) registered with the libuknet\n",
+                  tdev->id);
+
+       /* Adding the list of devices maintained by this driver */
+       UK_TAILQ_INSERT_TAIL(&tap_drv.tap_dev_list, tdev, next);
+exit:
+       return rc;
+free_tdev:
+       uk_free(tap_drv.a, tdev);
+       goto exit;
+}
+
  /**
   * Register a tap driver as bus. Currently in Unikraft, the uk_bus interface
   * provides the necessary to provide callbacks for bring a pseudo device. In 
the
@@ -253,6 +340,45 @@ static const struct uk_netdev_ops tap_netdev_ops = {
   */
  static int tap_drv_probe(void)
  {
+       int i;
+       int rc = 0;
+       char *idx = NULL, *prev_idx;
+
+       if (tap_dev_cnt > 0) {
+               tap_drv.bridge_ifs = uk_calloc(tap_drv.a, sizeof(char *),
+                                            i  tap_dev_cnt);
+               if (!tap_drv.bridge_ifs) {
+                       uk_pr_err(DRIVER_NAME": Failed to allocate 
brigde_ifs\n");
+                       return -ENOMEM;
+               }
+       }
+
+       idx = bridgenames;
+       for (i = 0; i < tap_dev_cnt; i++) {
+               if (idx) {
+                       prev_idx = idx;
+                       idx = strchr(idx, ' ');
+                       if (idx) {
+                               *idx = '\0';
+                               idx++;
+                       }
+                       tap_drv.bridgThis should depend on `TAP_NET`, 
right?e_ifs[i] = prev_idx;
+                       uk_pr_debug(DRIVER_NAME": Adding bridge %s\n",
+                                   prev_idx);
+               } else {
+                       uk_pr_warn(DRIVER_NAME": Adding tap device %d without 
bridge\n",
+                                  i);
+                       tap_drv.bridge_ifs[i] = NULL;
+               }
+
+               rc = tap_dev_init(i);
+               if (rc < 0) {
+                       uk_pr_err(DRIVER_NAME": Failed to initialize the tap dev id: 
%d\n",
+                                 i);
+                       return rc;
+               }
+               tap_drv.tap_dev_cnt++;
+       }
        return 0;
  }
  tap_drv.a = _a;
   388         UK_TAILQ_INIT(&tap_drv.tap_dev_list);
   389         return 0;
diff --git a/plat/linuxu/Config.uk b/plat/linuxu/Config.uk
index d85b6fd..a06f872 100644
--- a/plat/linuxu/Config.uk
+++ b/plat/linuxu/Config.uk
@@ -23,8 +23,15 @@ if (PLAT_LINUXU)
        default n
        depends on LIBUKNETDEV
        select LIBUKBUS
+       imply LIBUKLIBPARAM
        help
                Enable drivers to support tap device on the linuxu platform. The
                driver implements the uknetdev interface and provides an 
interface
                for the network stack to send/receive network packets.
+
+       config TAP_DEV_DEBUG
This should depend on `TAP_NET`, right?
+       bool "Tap Device Debug"
+       default n
+       help
+               Enable debug messages from the tap device.
  endif
diff --git a/plat/linuxu/Makefile.uk b/plat/linuxu/Makefile.uk
index 1ace47c..561909e 100644
--- a/plat/linuxu/Makefile.uk
+++ b/plat/linuxu/Makefile.uk
@@ -11,6 +11,8 @@ $(eval $(call addplatlib,linuxu,liblinuxutapnet))
  tap_drv.a = _a;
   388         UK_TAILQ_INIT(&tap_drv.tap_dev_list);
   389         return 0;
  ## Adding libparam for the linuxu platform
  $(eval $(call addlib_paramprefix,liblinuxuplat,linuxu))
+$(eval $(call addlib_paramprefix,liblinuxutapnet,tap))
+
  ##
  ## Platform library definitions
  ##
@@ -53,4 +55,6 @@ LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_ARM_32) += \
  LIBLINUXUTAPNET_ASINCLUED-y         += -I$(LIBLINUXUPLAT_BASE)/include
  LIBLINUXUTAPNET_CINCLUDES-y         += -I$(LIBLINUXUPLAT_BASE)/include
+LIBLINUXUTAPNET_CFLAGS-$(CONFIG_TAP_DEV_DEBUG) += -DUK_DEBUG
+
  LIBLINUXUTAPNET_SRCS-y                  += $(UK_PLAT_DRIVERS_BASE)/tap/tap.c
diff --git a/plat/linuxu/include/linuxu/tap.h b/plat/linuxu/include/linuxu/tap.h
new file mode 100644
index 0000000..83116f5
--- /dev/null
+++ b/plat/linuxu/include/linuxu/tap.h
@@ -0,0 +1,45 @@
+/* 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 formThis should depend on `TAP_NET`, right? 
must reproduce the above copyright
+ *    notice, this list of conditions and thie 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 __TAP_H__
+#define __TAP_H__
+
+#include <uk/arch/types.h>
+
+/**
+ * Using the musl as reference for the data structure definition
+ * Commit-id: 39ef612aa193
+ */
+#define IFNAMSIZ        16
+
+#endif /* LINUXU_TAP_H */
I don't understand why this header is not in `drivers/tap/include`.

_______________________________________________
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®.