Xen project Mailing List

Hey,

I've made the requested changes and will follow up with virtio-net and lwip, based on v3.

În lun., 23 iul. 2018 la 15:26, Simon Kuenzer <simon.kuenzer@xxxxxxxxx> a scris:

Hi Razvan,

thanks for the updated version, see my comments inline.

On 20.07.2018 09:22, Razvan Cojocaru wrote:
> Introduces the Unikraft Network API, that acts as a generalised
> interface between network drivers and network stack implementations
> (or low level networking applications).
>
> Using the function definitions in netdev_core.h, the driver should
> implement the functions in uk_netdev_opts and fill in the fields
> from uk_netdev and uk_netdev_data.
> The user-facing part of the API is in netdev.h. The network stack
> glue code can use these functions to configure network devices, as
> well as send/receive packets.
>
> Inspired from from DPDK RTE Ethernet API.
> IP utility functions taken from LWIP.
>
> Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
> ---
> lib/Config.uk | 1 +
> lib/Makefile.uk | 1 +
> lib/uknetdev/Config.uk | 12 +
> lib/uknetdev/Makefile.uk | 6 +
> lib/uknetdev/include/uk/netdev.h | 427 ++++++++++++++++++++++++++++++++++
> lib/uknetdev/include/uk/netdev_core.h | 299 ++++++++++++++++++++++++
> lib/uknetdev/netdev.c | 243 +++++++++++++++++++
> 7 files changed, 989 insertions(+)
> create mode 100644 lib/uknetdev/Config.uk
> create mode 100644 lib/uknetdev/Makefile.uk
> create mode 100644 lib/uknetdev/include/uk/netdev.h
> create mode 100644 lib/uknetdev/include/uk/netdev_core.h
> create mode 100644 lib/uknetdev/netdev.c
>
> diff --git a/lib/Config.uk b/lib/Config.uk
> index e438603..003bd4f 100644
> --- a/lib/Config.uk
> +++ b/lib/Config.uk
> @@ -37,3 +37,4 @@ source "lib/uklock/Config.uk"
> source "lib/ukmpi/Config.uk"
> source "lib/ukswrand/Config.uk"
> source "lib/ukbus/Config.uk"
> +source "lib/uknetdev/Config.uk"
> diff --git a/lib/Makefile.uk b/lib/Makefile.uk
> index 40c65d0..bcf22f3 100644
> --- a/lib/Makefile.uk
> +++ b/lib/Makefile.uk
> @@ -19,3 +19,4 @@ $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/vfscore))
> $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uklock))
> $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/ukmpi))
> $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/ukbus))
> +$(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uknetdev))
> diff --git a/lib/uknetdev/Config.uk b/lib/uknetdev/Config.uk
> new file mode 100644
> index 0000000..caec4a2
> --- /dev/null
> +++ b/lib/uknetdev/Config.uk
> @@ -0,0 +1,12 @@
> +menuconfig LIBUKNETDEV
> + bool "uknetdev: Network driver interface"
> + default n
> + select LIBUKALLOC
> +
> +if LIBUKNETDEV
> +config LIBUKNETDEV_NAME
> + bool "Network device names"
> + default y

Can we make this off on default? ;-) In principle, default configuration
should be minimal.

Done

> + help
> + Support driver-defined names for network devices.
> +endif
> diff --git a/lib/uknetdev/Makefile.uk b/lib/uknetdev/Makefile.uk
> new file mode 100644
> index 0000000..4b845a4
> --- /dev/null
> +++ b/lib/uknetdev/Makefile.uk
> @@ -0,0 +1,6 @@
> +$(eval $(call addlib_s,libuknetdev,$(CONFIG_LIBUKNETDEV)))
> +
> +CINCLUDES-$(CONFIG_LIBUKNETDEV) += -I$(LIBUKNETDEV_BASE)/include
> +CXXINCLUDES-$(CONFIG_LIBUKNETDEV) += -I$(LIBUKNETDEV_BASE)/include
> +
> +LIBUKBUS_SRCS-y += $(LIBUKNETDEV_BASE)/netdev.c
> diff --git a/lib/uknetdev/include/uk/netdev.h b/lib/uknetdev/include/uk/netdev.h
> new file mode 100644
> index 0000000..52e3eba
> --- /dev/null
> +++ b/lib/uknetdev/include/uk/netdev.h
> @@ -0,0 +1,427 @@
> +/* 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.
> + */
> +/* Taken and adapted from DPDK rte_ethdev.h */
> +
> +#ifndef __UK_NETDEV__
> +#define __UK_NETDEV__
> +
> +/**
> + * Unikraft Network API
> + *
> + * The Unikraft NET API provides a generalized interface between Unikraft
> + * drivers and network stack implementations or low-level network applications.
> + *
> + * Most NET API functions take as parameter a reference to the corresponding
> + * Unikraft Network Device (struct uk_netdev) which can be obtained with a call
> + * to uk_netdev_get(). The network application should store this reference and
> + * use it for all subsequent API calls.
> + *
> + * The functions exported by the Unikraft NET API to setup a device
> + * designated by its ID must be invoked in the following order:
> + * - uk_netdev_configure()
> + * - uk_netdev_tx_queue_setup()
> + * - uk_netdev_rx_queue_setup()
> + * - uk_netdev_start()
> + * If the network application wants to change configurations (call queue_setup
> + * or configure again), it must call uk_netdev_stop() first to stop the
> + * device and then do the reconfiguration before calling uk_netdev_start()
> + * again. The transmit and receive functions should not be invoked when the
> + * device is stopped.
> + * In order to clean up all of the information stored in the configuration
> + * phase, uk_netdev_close() can be called, but only on a stopped device.
> + *
> + * There are 3 states in which a network device can be found:
> + * - UK_NETDEV_UNCONFIGURED
> + * - UK_NETDEV_CONFIGURED
> + * - UK_NETDEV_RUNNING
> + */
> +
> +
> +#include <stddef.h>

Btw, why is stddef.h included? Wouldn't sys/types.h be good engough for
this purpose.

> +#include <stdint.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <uk/list.h>
> +#include <uk/alloc.h>
> +#include <uk/print.h>
> +#include <uk/assert.h>

Are assertions and mallocs are used in this file?

Moved them from netdev.h to netdev.c

Replaced stddef.h with sys/types.h

> +#include "netdev_core.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * Get the number of available Unikraft Network devices.
> + *
> + * @return
> + * - (unsigned int): number of network devices.
> + */
> +unsigned int uk_netdev_count(void);
> +
> +/**
> + * Get a reference to a Unikraft Network Device, based on its ID.
> + * This reference should be saved by the application and used for subsequent
> + * API calls.
> + *
> + * @param id
> + * The identifier of the Unikraft network device to configure.
> + * @return
> + * - NULL: device not found in list
> + * - (struct uk_netdev *): reference to be passed to API calls
> + */
> +struct uk_netdev *uk_netdev_get(unsigned int id);
> +
> +/**
> + * Configure an Unikraft network device.
> + * This function must be invoked first before any other function in the
> + * Unikraft NET API. This function can also be re-invoked when a device is
> + * in the stopped state.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param eth_conf

The param is called conf.

Replaced eth_conf with conf.

> + * The pointer to the configuration data to be used for the Unikraft
> + * network device.
> + *
> + * Embedding all configuration information in a single data structure
> + * is the more flexible method that allows the addition of new features
> + * without changing the syntax of the API.
> + * @return
> + * - 0: Success, device configured.
> + * - <0: Error code returned by the driver configuration function.
> + */
> +int uk_netdev_configure(struct uk_netdev *dev,
> + const struct uk_netdev_conf *conf);
> +
> +/**
> + * Start a Network device.
> + *
> + * The device start step is the last one and consists of setting the configured
> + * offload features and in starting the transmit and the receive units of the
> + * device.
> + * On success, all basic functions exported by the Unikraft NET API (link
> + * status, receive/transmit, and so on) can be invoked.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - 0: Success, Unikraft network device started.
> + * - <0: Error code of the driver device start function.
> + */
> +int uk_netdev_start(struct uk_netdev *dev);
> +
> +/**
> + * Stop an Unikraft network device, and bring it to the UK_NETDEV_CONFIGURED
> + * state.
> + * The device can be restarted with a call to uk_netdev_start().
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + */
> +void uk_netdev_stop(struct uk_netdev *dev);
> +
> +/**
> + * Close a stopped Unikraft network device.
> + * The function frees all resources except for needed by the
> + * UK_NETDEV_UNCONFIGURED state.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + */
> +void uk_netdev_close(struct uk_netdev *dev);
> +
> +/**
> + * Set the MAC address.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param mac_addr
> + * New MAC address.
> + * @return
> + * - (0) if successful.
> + * - (-ENOTSUP) if hardware doesn't support.
> + * - (-EINVAL) if MAC address is invalid.
> + */
> +int uk_netdev_mac_addr_set(struct uk_netdev *dev, struct ether_addr *mac_addr);

Can the struct be const? It is read-only, right (please check also all
other references in other API functions if they could potentially become
const)?

int uk_netdev_mac_addr_set(struct uk_netdev *dev, const struct
ether_addr *mac_addr);

Makes sense. Setting them to const.

> +
> +/**
> + * Returns the MAC address of the Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - (NULL) no MAC address available
> + * - MAC address
> + */
> +struct ether_addr *uk_netdev_mac_addr_get(struct uk_netdev *dev);
> +

Hum..., please return 'const struct ether_addr *'. We do not want that
anyone is modifying the driver internal struct. We are returning just
the reference to it and do not crate a copy, right?

Same

> +/**
> + * Enable receipt in promiscuous mode for an Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - (0) if successful.
> + * - (-ENOTSUP) if driver doesn't support promiscuous mode.
> + */
> +int uk_netdev_promiscuous_enable(struct uk_netdev *dev);
> +
> +/**
> + * Disable receipt in promiscuous mode for an Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - (0) if successful.
> + * - (-ENOTSUP) if driver doesn't support promiscuous mode.
> + */
> +int uk_netdev_promiscuous_disable(struct uk_netdev *dev);
> +
> +/**
> + * Return the value of promiscuous mode for an Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - (1) if promiscuous is enabled
> + * - (0) if promiscuous is disabled.
> + * - (-1) on error
> + */
> +int uk_netdev_promiscuous_get(struct uk_netdev *dev);
> +
> +/**
> + * Extra configuration query interface.
> + * The user can query the driver for any additional information, using a
> + * number of pre-defined configuration types.
> + *
> + * If the driver doesn't support the provided data type, it must return NULL.
> + *
> + * This allows the driver to provide configuration data without the need of
> + * parsing it in a pre-determined way, eliminating the need for utility
> + * functions in the API, or parsing the data multiple times both by driver
> + * and user.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param econf
> + * Extra configuration data type.
> + * @return
> + * - (NULL) if configuration unavailable or data type unsupported
> + * - configuration in format specified by *econf*
> + */
> +static inline const void *uk_netdev_extra_conf_get(struct uk_netdev *dev,
> + enum uk_netdev_extra_conf_type econf)
> +{
> + if (!dev->dev_ops->econf_get)
> + return NULL; /* driver does not provide
> + any extra configuration */
> + return dev->dev_ops->econf_get(dev, econf);
> +}
> +
> +/**
> + * Change the MTU of an Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param mtu
> + * A uint16_t for the MTU to be applied.
> + * @return
> + * - (0) if successful.
> + * - (-ENOTSUP) if operation is not supported.
> + * - (-EIO) if device is removed.
> + * - (-EINVAL) if *mtu* invalid.
> + * - (-EBUSY) if operation is not allowed when the device is running
> + */
> +int uk_netdev_mtu_set(struct uk_netdev *dev, uint16_t mtu);
> +
> +/**
> + * Returns the MTU of an Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - (>0) MTU of the uk_netdev
> + * - (-ENOTSUP) driver did not set a MTU.
> + */
> +int uk_netdev_mtu_get(struct uk_netdev *dev);
> +
> +/**
> + * Returns the name of the Unikraft network device.
> + * If name is not defined by driver, returns NULL
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - NULL if no name defined or names unsupported.
> + * - String if name is available.
> + */
> +const char *uk_netdev_name_get(struct uk_netdev *dev);
> +
> +/**
> + * Set the name of the Unikraft network device.
> + * Should only be called by the driver.
> + * The name is copied into a pre-allocated buffer in the uk_netdev
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param name
> + * Null-terminated string containing the name.
> + * @param len
> + * Length of the string.
> + * @return
> + * - (0): success.
> + * - (-ENOTSUP): names not supported.
> + * - (-EINVAL): name too long or invalid string.
> + */
> +int uk_netdev_name_set(struct uk_netdev *dev, char *name, uint16_t len);

Usually, I like the length parameter on string interfaces. However, your
documentation says the string is anyways null-terminated. Len would be
then only helpful to get a substring. Is this intended. It is fine if
yes. Could you document this briefly as part of the string.

*name can be const, too, right?:
len should be size_t. This a more common data type for string length
(avoids typecasts) - or do we want to limit it to 64K characters?

int uk_netdev_name_set(struct uk_netdev *dev, const char *name, size_t len);

I added it with the purpose of not doing strlen on user-provided strings.

I'll change the documentation to provide info about substrings, and do the cropping or

terminator add in the implementation, since it might prove useful.

> +
> +/**
> + * Allocate and set up a receive queue for an Unikraft network device.
> + *
> + * The function handles setup of receive callback for interrupt-based modes.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param rx_queue_id
> + * The index of the receive queue to set up.
> + * The value must be in the range [0, nb_rx_queue - 1] previously supplied
> + * to uk_netdev_eth_dev_configure().
> + * @param rx_conf
> + * The pointer to the configuration data to be used for the receive queue.
> + * NULL value is allowed, in which case default RX configuration
> + * will be used.
> + * The *rx_conf* structure contains an *rx_thresh* structure with the values
> + * of the Prefetch, Host, and Write-Back threshold registers of the receive
> + * ring.
> + * In addition it contains the hardware offloads features to activate using
> + * the DEV_RX_OFFLOAD_* flags.
> + * @return
> + * - 0: Success, receive queue correctly set up.
> + * - -EIO: if device is removed.
> + */
> +int uk_netdev_rx_queue_setup(struct uk_netdev *dev, uint16_t rx_queue_id,
> + const struct uk_netdev_rxqueue_conf *rx_conf);
> +
> +/**
> + * Allocate and set up a transmit queue for an Unikraft network device.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param tx_queue_id
> + * The index of the transmit queue to set up.
> + * The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + * to uk_netdev_configure().
> + * @param tx_conf
> + * The pointer to the configuration data to be used for the transmit queue.
> + * NULL value is allowed, in which case default TX configuration
> + * will be used.
> + * @return
> + * - 0: Success, the transmit queue is correctly set up.
> + * - -ENOMEM: Unable to allocate the transmit ring descriptors.
> + */
> +int uk_netdev_tx_queue_setup(struct uk_netdev *dev, uint16_t tx_queue_id,
> + const struct uk_netdev_txqueue_conf *tx_conf);
> +
> +/**
> + * Enable interrupts for an RX queue.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - 0: Success, interrupts enabled.
> + * - (-ENOTSUP): Driver does not support interrupt enable.
> + */
> +int uk_netdev_rx_enable_intr(struct uk_netdev *dev,
> + uint16_t rx_queue_id);
> +
> +/**
> + * Disable interrupts for an RX queue.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @return
> + * - 0: Success, interrupts enabled.
> + * - (-ENOTSUP): Driver does not support interrupt disable.
> + */
> +int uk_netdev_rx_disable_intr(struct uk_netdev *dev,
> + uint16_t rx_queue_id);
> +
> +/**
> + * Basic RX function.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param pkt
> + * The buffer in which the received packet will be placed.
> + * @param queue_id
> + * The index of the receive queue from which to retrieve input packets.
> + * @return
> + * - 0: No new packets
> + * - >0: Length of the received packet
> + */
> +int uk_netdev_rx(struct uk_netdev *dev, uint16_t queue_id,
> + struct uk_mbuf *pkt); > +
> +/**
> + * Basic TX function.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + * @param queue_id
> + * The index of the transmit queue through which output packets must be
> + * sent.
> + * @param pkt
> + * The buffer containing the packet to be sent.
> + * @return
> + */
> +int uk_netdev_tx(struct uk_netdev *dev, uint16_t queue_id,
> + struct uk_mbuf *pkt);
> +
> +/**
> + * Add a new Unikraft network device in the device list.
> + * Should be called by the driver in the configuration step.
> + *
> + * @param dev
> + * The Unikraft Network Device.
> + */
> +void uk_netdev_register(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..990b528
> --- /dev/null
> +++ b/lib/uknetdev/include/uk/netdev_core.h
> @@ -0,0 +1,299 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: 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.
> + */
> +/* Taken and adapted from DPDK rte_ethdev_core.h */
> +
> +#ifndef __UK_NETDEV_CORE__
> +#define __UK_NETDEV_CORE__
> +
> +/**
> + * Unikraft Network Device internal header.
> + *
> + * This header contains internal data types. But they are still part of the
> + * public API because they are used by inline functions in the published 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.
> + */
> +
> +
> +#define NETDEV_NAME_MAX_LEN 64
> +
> +#define ETHER_ADDR_LEN 6 /**< Length of Ethernet address. */
> +
> +
> +struct ether_addr {
> + uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
> +} __packed;
> +
We probably should call it 'struct uk_ether_addr' or 'struct uk_hwaddr'.
It is for avoiding any namespace clushes of datatype definitions.

Changed to uk_hwaddr everywhere.

> +/**
> + * A structure used to hold a single packet.
> + */
> +struct uk_mbuf {
> + void *payload; /**< Address of packet buffer. */
> + uint32_t len; /**< Total packet length. */
> +};
> +
> +/**
> + * A set of values to describe the possible states of an eth device.
> + */
> +enum uk_netdev_state {
> + UK_NETDEV_UNCONFIGURED = 0,
> + UK_NETDEV_CONFIGURED,
> + UK_NETDEV_RUNNING,
> +};
> +
> +/**
> + * A structure used to configure an Unikraft network device.
> + */
> +struct uk_netdev_conf {
> +};
> +
> +/**
> + * Generic type enum used by the extra configuration query interface.
> + *
> + * The purpose of this generic type is to allow drivers to define extra
> + * configurations such as IP information, without the need to parse this data.
> + * This prevents the need to introduce any additional parsing logic inside
> + * uknetdev API.
> + *
> + * This list is extensible in the future without needing the drivers to adopt
> + * any or all of the data types.
> + */
> +enum uk_netdev_extra_conf_type {
> + IPv4ADDR_INT, /**< IPv4 address as network-order raw int (4 bytes) */
> + IPv4ADDR_STR, /**< IPv4 address as null-terminated string */
> + IPv4MASK_INT, /**< IPv4 mask as network-order raw int (4 bytes) */
> + IPv4MASK_STR, /**< IPv4 mask as null-terminated string */
> + IPv4GW_INT, /**< IPv4 gateway as network-order raw int (4 bytes) */
> + IPv4GW_STR, /**< IPv4 gateway as null-terminated string */
> + IPv4DNS0_INT, /**< IPv4 DNS as network-order raw int (4 bytes) */
> + IPv4DNS0_STR, /**< IPv4 DNS as null-terminated string */
> +};
> +
> +UK_TAILQ_HEAD(uk_netdev_list, struct uk_netdev);
> +
> +#define UK_NETDEV_LIST_FOREACH(b) \
> + UK_TAILQ_FOREACH(b, &uk_netdev_list, next)
> +
> +/**
> + * Function type used for RX packet processing packet callbacks.
> + *
> + * The callback function is called on RX with a packet that has been received
> + * on the given device and queue.
> + *
> + * @param id
> + * The identifier of the device on which RX is being performed.
> + * @param queue
> + * The queue on the Unikraft network device which is being used to receive
> + * the packets.
> + * @param pkt
> + * Packet that should be processed by the callback function.
> + */
> +typedef void (*rx_callback_fn)(uint16_t id, uint16_t queue,
> + struct uk_mbuf *pkt);

Didn't we say that we do not forward the pkt with the callback and that
the developer of the callback is going to call the rx function? This
makes implementing select/poll with uk_netdev devices much easier later.
I suggest also to namespace the new type with prefixing uk_netdev_.
Instead of the id, handover the netdev struct. What about:

typedef void (*uk_netdev_queue_event_t)(struct uk_netdev *dev,
uint16_t queue_id);

This callback function signature should be independent of rx and tx, so
we could use the same definition for both queue types (although we only
implement rx for now).

Changed it to remove pkt param.

This will add some overhead to the current implementation, since the drivers

are not zerocopy in their current form, but I guess it's better to have the API

more generic.

> +
> +/**
> + * A structure used to configure an Unikraft network device RX queue.
> + */
> +struct uk_netdev_rxqueue_conf {
> + rx_callback_fn rx_cb;
> +};
> +
> +/**
> + * A structure used to configure an Unikraft network device TX queue.
> + */
> +struct uk_netdev_txqueue_conf {
> +};
> +
> +
> +typedef int (*uk_netdev_configure_t)(struct uk_netdev *dev,
> + const struct uk_netdev_conf *conf);
> +/**< @internal Unikraft network device configuration. */
> +
> +typedef int (*uk_netdev_start_t)(struct uk_netdev *dev);
> +/**< @internal Function used to start a configured Unikraft network device. */
> +
> +typedef void (*uk_netdev_stop_t)(struct uk_netdev *dev);
> +/**< @internal Function used to stop a configured Unikraft network device. */
> +
> +typedef void (*uk_netdev_close_t)(struct uk_netdev *dev);
> +/**< @internal Function used to close a configured Unikraft network device. */
> +
> +typedef int (*uk_netdev_mac_addr_set_t)(struct uk_netdev *dev,
> + struct ether_addr *mac_addr);
> +/**< @internal Set the MAC address */
> +
> +typedef int (*uk_netdev_mtu_set_t)(struct uk_netdev *dev, uint16_t mtu);
> +/**< @internal Set MTU. */
> +
> +typedef int (*uk_netdev_mtu_get_t)(struct uk_netdev *dev);
> +/**< @internal Get MTU. */
> +
> +typedef int (*uk_netdev_promiscuous_enable_t)(struct uk_netdev *dev);
> +/**< @internal Function used to enable the RX promiscuous mode of an
> + * Unikraft network device.
> + */
> +
> +typedef int (*uk_netdev_promiscuous_disable_t)(struct uk_netdev *dev);
> +/**< @internal Function used to disable the RX promiscuous mode of an
> + * Unikraft network device.
> + */
> +
> +typedef const void *(*uk_netdev_econf_get_t)(struct uk_netdev *dev,
> + enum uk_netdev_extra_conf_type econf);
> +/**< @internal Read any extra configuration provided by the driver */
> +
> +typedef int (*uk_netdev_rx_queue_setup_t)(struct uk_netdev *dev,
> + uint16_t rx_queue_id, const struct uk_netdev_rxqueue_conf *rx_conf);
> +/**< @internal Set up a receive queue of an Unikraft network device. */
> +
> +typedef int (*uk_netdev_tx_queue_setup_t)(struct uk_netdev *dev,
> + uint16_t tx_queue_id, const struct uk_netdev_txqueue_conf *tx_conf);
> +/**< @internal Setup a transmit queue of an Unikraft network device. */
> +
> +typedef void (*uk_netdev_queue_release_t)(void *queue);
> +/**< @internal Release memory resources allocated by given RX/TX queue. */
> +
> +typedef int (*uk_netdev_rx_enable_intr_t)(struct uk_netdev *dev,
> + uint16_t rx_queue_id);
> +/**< @internal Enable interrupt of a receive queue of an
> + * Unikraft network device.
> + */
> +
> +typedef int (*uk_netdev_rx_disable_intr_t)(struct uk_netdev *dev,
> + uint16_t rx_queue_id);
> +/**< @internal Disable interrupt of a receive queue of an
> + * Unikraft network device.
> + */
> +
> +typedef int (*uk_netdev_rx_t)(struct uk_netdev *dev, uint16_t queue_id,
> + struct uk_mbuf *pkt);
> +/**< @internal Retrieve one input packet from an Unikraft network device. */
> +
> +typedef int (*uk_netdev_tx_t)(struct uk_netdev *dev, uint16_t queue_id,
> + struct uk_mbuf *pkt);
> +/**< @internal Send one output packet to an Unikraft network device. */
> +
> +
> +/**
> + * @internal A structure containing the functions exported by a driver.
> + */
> +struct uk_netdev_ops {
> + uk_netdev_configure_t dev_configure; /**< Configure device. */
> + uk_netdev_start_t dev_start; /**< Start device. */
> + uk_netdev_stop_t dev_stop; /**< Stop device. */
> + uk_netdev_close_t dev_close; /**< Close device. */
> +
> + uk_netdev_mac_addr_set_t mac_addr_set; /**< Set a MAC address. */
> + uk_netdev_mtu_set_t mtu_set; /**< Set MTU. */
> + uk_netdev_econf_get_t econf_get; /**< Return additional config. */
> +
> + /** Promiscuous ON. */
> + uk_netdev_promiscuous_enable_t promiscuous_enable;
> + /** Promiscuous OFF. */
> + uk_netdev_promiscuous_disable_t promiscuous_disable;
> +
> + /** Set up device RX queue. */
> + uk_netdev_rx_queue_setup_t rx_queue_setup;
> + /** Release RX queue. */
> + uk_netdev_queue_release_t rx_queue_release;
> +
> + /** Set up device TX queue. */
> + uk_netdev_tx_queue_setup_t tx_queue_setup;
> + /** Release TX queue. */
> + uk_netdev_queue_release_t tx_queue_release;
> +
> + uk_netdev_rx_enable_intr_t rx_enable_intr; /**< Enable RX interrupts*/
> + uk_netdev_rx_disable_intr_t rx_disable_intr;/**< Disable RX interrupts*/
> +};
> +
> +/**
> + * @internal
> + * The data part, with no function pointers, associated with each
> + * network device.
> + *
> + * This structure is safe to place in shared memory to be common among different
> + * processes in a multi-process configuration.
> + */
> +struct uk_netdev_data {
> +#ifdef CONFIG_LIBUKNETDEV_NAME
> + char name[NETDEV_NAME_MAX_LEN]; /**< Network device name */
> +#else
> + const char *name; /**< Network device name */
> +#endif
> + uint16_t id; /**< Device [external] port identifier. */
> +
> + struct ether_addr mac_addr; /**< Device Ethernet Link address. */
> + uint16_t mtu; /**< Maximum Transmission Unit. */
> +
> + uint8_t promiscuous : 1; /**< RX promiscuous mode ON(1) / OFF(0). */
> +
> + enum uk_netdev_state state; /**< Flag indicating the device state */
> + uint8_t rx_queue_state;
> + /**< Queues state: STARTED(1) / STOPPED(0) */

Do we want an enum also for the queue states (enum uk_netdev_queue_state)?

I don't really see how this would be useful. By keeping the status binary, we could

update the states to bit-flags as performance optimizations.

Started/stopped was taken from DPDK.

> + uint8_t tx_queue_state;
> + /**< Queues state: STARTED(1) / STOPPED(0) */
> +};
> +
> +/**
> + * @internal
> + * The generic data structure associated with each network device.
> + *
> + * Pointers to all the function callbacks registered by the driver, along
> + * with the pointer to where all the data elements for the particular device
> + * are stored in shared memory. This split allows the function pointer and
> + * driver data to be per-process, while the actual configuration data for
> + * the device is shared.
> + *
> + * Packet RX/TX functions are added directly to this structure for performance
> + * reasons, in order to prevent another indirection layer to dev_ops.
> + */
> +struct uk_netdev {
> + UK_TAILQ_ENTRY(struct uk_netdev) next;
> +
> + uk_netdev_rx_t rx_pkt; /**< Pointer to receive function. */
> + uk_netdev_tx_t tx_pkt; /**< Pointer to transmit function. */ > +
> + /**< Pointer to device data */
> + struct uk_netdev_data *data;
> + /**< Functions exported by driver */
> + const struct uk_netdev_ops *dev_ops;
> +
> + /** User-supplied function called from driver on new packet RX */
> + rx_callback_fn rx_cb;
> +};
> +
> +#endif //__UK_NETDEV_CORE__
> diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
> new file mode 100644
> index 0000000..25c9c81
> --- /dev/null
> +++ b/lib/uknetdev/netdev.c
> @@ -0,0 +1,243 @@
> +/* 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 <uk/netdev.h>
> +#include <string.h>
> +#include <uk/assert.h>
> +#include <uk/config.h>
> +#include <uk/print.h>
> +#include <uk/plat/ctors.h>
> +
> +#define UK_NETDEV_CTOR_PRIO (102U)
> +
> +struct uk_netdev_list uk_netdev_list;
> +static uint16_t netdev_count;
> +
> +/* This library does not have any dependency to another library for
> + * initialization, except a libc -> We use priority 1
> + */
> +static void _uk_netdev_ctor(void) __constructor_prio(UK_NETDEV_CTOR_PRIO);
> +
> +void uk_netdev_register(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev != NULL);
> +
> + uk_printd(DLVL_INFO, "Register netdev%u: %p\n",
> + netdev_count, dev);
> + dev->data->id = netdev_count;
> + UK_TAILQ_INSERT_TAIL(&uk_netdev_list, dev, next);
> +
> + ++netdev_count;
> +}
> +
> +unsigned int uk_netdev_count(void)
> +{
> + return netdev_count;
> +}
> +
> +struct uk_netdev *uk_netdev_get(unsigned int id)
> +{
> + struct uk_netdev *n;
> +
> + UK_NETDEV_LIST_FOREACH(n) {
> + if (n->data->id == id)
> + return n;
> + }
> + return NULL;
> +}
> +
> +int uk_netdev_configure(struct uk_netdev *dev,
> + const struct uk_netdev_conf *eth_conf)
> +{
> + UK_ASSERT(dev);
> + uk_printd(DLVL_INFO, "Configure device 0x%p\n", dev);
> +
> + return dev->dev_ops->dev_configure(dev, eth_conf);
> +}
> +
> +int uk_netdev_rx_queue_setup(struct uk_netdev *dev, uint16_t rx_queue_id,
> + const struct uk_netdev_rxqueue_conf *rx_conf)
> +{
> + UK_ASSERT(dev);

It is feasable to say rx_conf is a mandatory argument. Otherwise this
function is meaning-less. So, add another UK_ASSERT(rx_conf); instead of
the following if statement.

> +
> + if ((rx_conf != NULL) && (rx_conf->rx_cb != NULL)) {

What if we want to delete a previously configured callback function? I
think you should always just set the handed over callback function, even
if it is NULL.

> + uk_printd(DLVL_INFO,
> + "Setting up receive callback\n");

If you want to print a message, then tell also the new callback pointer:

uk_printd(DLVL_INFO, "Configure device 0x%p: Setting up callback for rx
queue %"__PRIu16": %p\n", dev, rx_queue_id, rx_conf->rx_cb);

Done

> + dev->rx_cb = rx_conf->rx_cb;
> + }
> +
> + return dev->dev_ops->rx_queue_setup(dev, rx_queue_id, rx_conf);
> +}
> +
> +int uk_netdev_tx_queue_setup(struct uk_netdev *dev, uint16_t tx_queue_id,
> + const struct uk_netdev_txqueue_conf *tx_conf)
> +{
> + UK_ASSERT(dev);
UK_ASSERT(tx_conf);
> + return dev->dev_ops->tx_queue_setup(dev, tx_queue_id, tx_conf);
> +}
> +
> +int uk_netdev_start(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> + return dev->dev_ops->dev_start(dev);
> +}
> +
> +void uk_netdev_stop(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> + dev->dev_ops->dev_stop(dev);
> +}
> +
> +int uk_netdev_mac_addr_set(struct uk_netdev *dev, struct ether_addr *mac_addr)
int uk_netdev_mac_addr_set(struct uk_netdev *dev, const struct
ether_addr *mac_addr)

> +{
> + int rc;
> +
> + UK_ASSERT(dev);
> + if (dev->dev_ops->mac_addr_set == NULL)
> + return -ENOTSUP;
> + rc = dev->dev_ops->mac_addr_set(dev, mac_addr);
> + if (rc >= 0)
> + memcpy(&dev->data->mac_addr, mac_addr, sizeof(struct ether_addr));
> + return rc;
> +}
> +
> +struct ether_addr *uk_netdev_mac_addr_get(struct uk_netdev *dev)

const struct ether_addr *uk_netdev_mac_addr_get(struct uk_netdev *dev)

> +{
> + UK_ASSERT(dev);
> + return &dev->data->mac_addr;
> +} > +
> +const char *uk_netdev_name_get(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> +#ifdef CONFIG_LIBUKNETDEV_NAME
> + return dev->data->name;
> +#else
> + return NULL;
> +#endif
> +}
> +
> +int uk_netdev_name_set(struct uk_netdev *dev, char *name, uint16_t len) {
int uk_netdev_name_set(struct uk_netdev *dev, const char *name, size_t
len) {

> + UK_ASSERT(dev);
UK_ASSERT(name);

> +#ifdef CONFIG_LIBUKNETDEV_NAME
> + if (len > NETDEV_NAME_MAX_LEN)
> + return -EINVAL;
> + memcpy(dev->data->name, name, len);

You should use strncpy instead since it does zero padding if the string
is shorter than len. Also, ensure null-termination by executing the
following command before returning:

dev->data->name[NETDEV_NAME_MAX_LEN - 1] = '\0';

Ok, replacing memcpy with strncpy.

Added const to mac/name

> + return 0;
> +#else
> + return -ENOTSUP;
> +#endif
> +}
> +
> +int uk_netdev_mtu_set(struct uk_netdev *dev, uint16_t mtu)
> +{
> + int rc;
> +
> + UK_ASSERT(dev);
> + if (dev->dev_ops->mtu_set == NULL)
> + return -ENOTSUP;
> + rc = dev->dev_ops->mtu_set(dev, mtu);
> + if (rc >= 0)
> + dev->data->mtu = mtu;
> + return rc;
> +}
> +
> +int uk_netdev_mtu_get(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> + return dev->data->mtu;
> +}
> +

If you want, you could only call the promiscuous enable callback when
dev->data->promiscuous is saying it is currently disabled. Equivalent
behavior could be done for uk_netdev_promiscuous_disable. Similarly you
could add such a check to set_mtu. However, I am accepting it also
without the check. It is just a minor detail.

Since we're not really supporting promiscuous, I left it out in case the

actual representation is changed when it's implemented. I wouldn't put

too much effort in something that can't be tested at this point.

> +int uk_netdev_promiscuous_enable(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> + if (!dev->dev_ops->promiscuous_enable)
> + return -ENOTSUP;
> + else
> + return dev->dev_ops->promiscuous_enable(dev);
> +}
> +
> +int uk_netdev_promiscuous_disable(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> + if (!dev->dev_ops->promiscuous_disable)
> + return -ENOTSUP;
> + else
> + return dev->dev_ops->promiscuous_disable(dev);
> +}
> +
> +int uk_netdev_promiscuous_get(struct uk_netdev *dev)
> +{
> + UK_ASSERT(dev);
> + return dev->data->promiscuous;
> +}
> +
> +int uk_netdev_rx_enable_intr(struct uk_netdev *dev,
> + uint16_t rx_queue_id)
> +{
> + UK_ASSERT(dev);
> + if (!dev->dev_ops->rx_enable_intr)
> + return -ENOTSUP;
> + return dev->dev_ops->rx_enable_intr(dev, rx_queue_id);
> +}
> +
> +int uk_netdev_rx_disable_intr(struct uk_netdev *dev,
> + uint16_t rx_queue_id)
> +{
> + UK_ASSERT(dev);
> + if (!dev->dev_ops->rx_disable_intr)
> + return -ENOTSUP;
> + return dev->dev_ops->rx_disable_intr(dev, rx_queue_id);
> +}
> +
> +int uk_netdev_rx(struct uk_netdev *dev, uint16_t queue_id,
> + struct uk_mbuf *pkt)
> +{
> + UK_ASSERT(dev);
UK_ASSERT(pkt);

> + return dev->rx_pkt(dev, queue_id, pkt);
> +}
> +
> +int uk_netdev_tx(struct uk_netdev *dev, uint16_t queue_id,
> + struct uk_mbuf *pkt)
> +{
> + UK_ASSERT(dev);
UK_ASSERT(pkt);

Added the asserts.

> + return dev->tx_pkt(dev, queue_id, pkt);
> +}
> +
> +static void _uk_netdev_ctor(void)
> +{
> + UK_TAILQ_INIT(&uk_netdev_list);
> + netdev_count = 0;
> +}
>

Btw, could you release your virtio driver for this interface with the v3
of this patch?

Thanks,

Simon

Re: [Minios-devel] [UNIKRAFT PATCH v2] lib/uknetdev: Unikraft Network API