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

[Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>]

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