[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 4/7] lib/ukconsdev: Configure console device
On 19.06.19 15:35, Birlea Costin wrote:
This patch introduces the API for configuring a Unikraft console device.
The configuration is done in the following order:
(1) Configure main aspects of device (placeholder for future work)
(2) Configure both rx and tx (tx is also a placeholder)
Configure API initializes the main data structures for rx/tx.
Signed-off-by: Birlea Costin <costin.birlea@xxxxxxxxx>
---
lib/ukconsdev/Config.uk | 14 ++
lib/ukconsdev/consdev.c | 213 ++++++++++++++++++++++++++++++
lib/ukconsdev/exportsyms.uk | 4 +
lib/ukconsdev/include/uk/consdev.h | 66 +++++++++
lib/ukconsdev/include/uk/consdev_core.h | 90 +++++++++++++
lib/ukconsdev/include/uk/consdev_driver.h | 24 ++++
6 files changed, 411 insertions(+)
diff --git a/lib/ukconsdev/Config.uk b/lib/ukconsdev/Config.uk
index bf231b66..c71247a1 100644
--- a/lib/ukconsdev/Config.uk
+++ b/lib/ukconsdev/Config.uk
@@ -3,4 +3,18 @@ menuconfig LIBUKCONSDEV
default n
select LIBUKALLOC
select LIBNOLIBC if !HAVE_LIBC
+
+if LIBUKCONSDEV
+ config LIBUKCONSDEV_DISPATCHERTHREADS
+ bool "Dispatcher threads for event callbacks"
+ select LIBUKSCHED
+ select LIBUKLOCK
+ select LIBUKLOCK_SEMAPHORE
+ default n
+ help
+ Event callbacks are dispatched in a bottom half
+ thread context instead of the device interrupt context.
+ When this option is enabled a dispatcher thread is
+ allocated for each configured receive queue.
Two comments:
(1) Either adopt the description with the "ring" naming if you want to
keep it (otherwise call everything queue).
(2) We do not support multiple ring/queue pairs for a single device,
right? So it is fine to write something like "[...] one dispatcher
thread is allocated for each device to handle receive events".
+ libuksched is required for this option.
endif
diff --git a/lib/ukconsdev/consdev.c b/lib/ukconsdev/consdev.c
index d61d42a8..8542b0e8 100644
--- a/lib/ukconsdev/consdev.c
+++ b/lib/ukconsdev/consdev.c
@@ -84,6 +84,215 @@ int uk_consdev_tx_info_get(struct uk_consdev *dev,
return dev->ops->tx_info_get(dev, ring_info);
}
+int uk_consdev_configure(struct uk_consdev *dev,
+ const struct uk_consdev_conf *dev_conf)
+{
+ int rc = 0;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->ops);
+ UK_ASSERT(dev->ops->configure);
+ UK_ASSERT(dev_conf);
+
+ if (dev->_data->state != UK_CONSDEV_UNCONFIGURED)
+ return -EINVAL;
+
+ rc = dev->ops->configure(dev, dev_conf);
+ if (rc < 0) {
+ uk_pr_err("consdev%"PRIu16": Failed to configure device: %d\n",
+ dev->_data->id, rc);
+ goto out;
+ }
+
+ dev->_data->state = UK_CONSDEV_CONFIGURED;
+
+out:
+ return rc;
+}
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+static void _dispatcher(void *arg)
+{
+ struct uk_consdev_event_handler *handler =
+ (struct uk_consdev_event_handler *) arg;
+
+ UK_ASSERT(handler);
+ UK_ASSERT(handler->callback);
+
+ for (;;) {
+ uk_semaphore_down(&handler->events);
+ handler->callback(handler->dev, handler->cb_cookie);
+ }
+}
+#endif
+
+static int _create_event_handler(uk_consdev_event_t callback,
+ void *cb_cookie,
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ struct uk_consdev *dev,
+ const char *queue_type_str,
+ struct uk_sched *s,
+#endif
+ struct uk_consdev_event_handler *h)
+{
+ UK_ASSERT(h);
+ UK_ASSERT(callback || (!callback && !cb_cookie));
+
+ h->callback = callback;
+ h->cb_cookie = cb_cookie;
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ UK_ASSERT(!h->dispatcher);
+
+ /* If we do not have a callback, we do not need a thread */
+ if (!callback)
+ return 0;
+
+ h->dev = dev;
+ uk_semaphore_init(&h->events, 0);
+ h->dispatcher_s = s;
+
+
+ /* Create a name for the dispatcher thread.
+ * In case of errors, we just continue without a name
+ */
+ if (asprintf(&h->dispatcher_name,
+ "consdev%"PRIu16"-%s",
+ dev->_data->id, queue_type_str) < 0) {
+ h->dispatcher_name = NULL;
+ }
+
+ h->dispatcher = uk_thread_create(h->dispatcher_s,
+ h->dispatcher_name, NULL, _dispatcher, h);
+ if (!h->dispatcher) {
+ if (h->dispatcher_name) {
+ free(h->dispatcher_name);
+ h->dispatcher_name = NULL;
+ }
+ return -ENOMEM;
+ }
+#endif
+
+ return 0;
+}
+
+static void _destroy_event_handler(struct uk_consdev_event_handler *h
+ __maybe_unused)
+{
+ UK_ASSERT(h);
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ if (h->dispatcher) {
+ UK_ASSERT(h->dispatcher_s);
+ uk_thread_kill(h->dispatcher);
+ uk_thread_wait(h->dispatcher);
+ h->dispatcher = NULL;
+ }
+
+ if (h->dispatcher_name) {
+ free(h->dispatcher_name);
+ h->dispatcher_name = NULL;
+ }
+#endif
+}
+
+int uk_consdev_rx_configure(struct uk_consdev *dev,
+ uint16_t nb_desc,
+ const struct uk_consdev_rx_conf *rx_conf)
_rxr_configure or _rxq_configure?
+{
+ int rc = 0;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->ops);
+ UK_ASSERT(dev->ops->rx_configure);
+ UK_ASSERT(rx_conf);
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ UK_ASSERT((rx_conf->callback && rx_conf->s)
+ || !rx_conf->callback);
+#endif
+
+ if (dev->_data->state != UK_CONSDEV_CONFIGURED)
+ return -EINVAL;
+
+ rc = _create_event_handler(rx_conf->callback, rx_conf->cb_cookie,
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ dev, "rx", rx_conf->s,
+#endif
+ &dev->_data->rx_handler);
+ if (rc < 0)
+ goto out;
+
+ rc = dev->ops->rx_configure(dev, nb_desc, rx_conf);
+ if (rc < 0) {
+ uk_pr_err("consdev%"PRIu16": Failed to configure rx: %d\n",
+ dev->_data->id, rc);
Please also adopt the messages by saying
"[...] Failed to configure rx ring: [...]" or with "queue"...
The same for tx ring messages.
+ goto err_destroy_handler;
+ }
+
+out:
+ return rc;
+
+err_destroy_handler:
+ _destroy_event_handler(&dev->_data->rx_handler);
+ goto out;
+}
+
+int uk_consdev_tx_configure(struct uk_consdev *dev,
+ uint16_t nb_desc,
+ const struct uk_consdev_tx_conf *tx_conf)
_txr_configure or _txq_configure?
+{
+ int rc = 0;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->ops);
+ UK_ASSERT(dev->ops->tx_configure);
+ UK_ASSERT(tx_conf);
+
+ if (dev->_data->state != UK_CONSDEV_CONFIGURED)
+ return -EINVAL;
+
+ rc = dev->ops->tx_configure(dev, nb_desc, tx_conf);
+ if (rc < 0) {
+ uk_pr_err("consdev%"PRIu16": Failed to configure tx: %d\n",
+ dev->_data->id, rc);
+ goto out;
+ }
+
+out:
+ return rc;
+}
+
+void uk_consdev_release(struct uk_consdev *dev)
+{
I think `uk_consdev_release()` should be called
`uk_consdev_unconfigure()` because it transists the device from
CONFIGURED to UNCONFIGURED state.
+ int rc = 0;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->ops);
+ UK_ASSERT(dev->ops->release);
+ UK_ASSERT(dev->_data->state != UK_CONSDEV_RUNNING);
+
+#if CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS
+ if (dev->_data->rx_handler.callback)
+ _destroy_event_handler(&dev->_data->rx_handler);
+#endif
+
+ rc = dev->ops->release(dev);
+ if (rc < 0) {
+ uk_pr_err("Failed to release consdev%"PRIu16" device %d\n",
+ dev->_data->id, rc);
+ return;
+ }
+
+ dev->_data->state = UK_CONSDEV_UNCONFIGURED;
+ uk_pr_info("Released consdev%"PRIu16" device\n",
+ dev->_data->id);
+}
+
unsigned int uk_consdev_count(void)
{
return (unsigned int) consdev_count;
@@ -154,6 +363,10 @@ int uk_consdev_drv_register(struct uk_consdev *dev, struct
uk_alloc *a,
UK_ASSERT(dev->ops->info_get);
UK_ASSERT(dev->ops->rx_info_get);
UK_ASSERT(dev->ops->tx_info_get);
+ UK_ASSERT(dev->ops->configure);
+ UK_ASSERT(dev->ops->rx_configure);
+ UK_ASSERT(dev->ops->tx_configure);
+ UK_ASSERT(dev->ops->release);
UK_ASSERT(dev->ops->close);
dev->_data = _alloc_data(a, consdev_count, drv_name);
diff --git a/lib/ukconsdev/exportsyms.uk b/lib/ukconsdev/exportsyms.uk
index f456b117..6fc1b3f3 100644
--- a/lib/ukconsdev/exportsyms.uk
+++ b/lib/ukconsdev/exportsyms.uk
@@ -1,6 +1,10 @@
uk_consdev_info_get
uk_consdev_rx_info_get
uk_consdev_tx_info_get
+uk_consdev_configure
+uk_consdev_rx_configure
+uk_consdev_tx_configure
+uk_consdev_release
uk_consdev_count
uk_consdev_get
uk_consdev_id_get
diff --git a/lib/ukconsdev/include/uk/consdev.h
b/lib/ukconsdev/include/uk/consdev.h
index 3d98e5cc..04b4c0dc 100644
--- a/lib/ukconsdev/include/uk/consdev.h
+++ b/lib/ukconsdev/include/uk/consdev.h
@@ -124,6 +124,72 @@ int uk_consdev_tx_info_get(struct uk_consdev *dev,
struct uk_consdev_ring_info *ring_info);
/**
+ * Configure an Unikraft console device.
+ * This function must be invoked first before any other function in the
+ * Unikraft Console API. This function can also be re-invoked when a device is
+ * in the stopped state.
+ *
+ * @param dev
+ * The Unikraft Console Device in configured state.
+ * @param dev_conf
+ * The pointer to the configuration data to be used for the Unikraft
+ * console device.
+ * @return
+ * - (0): Success, device configured.
+ * - (<0): Error code returned by the driver configuration function.
+ */
+int uk_consdev_configure(struct uk_consdev *dev,
+ const struct uk_consdev_conf *dev_conf);
+
+/**
+ * Sets up receive for an Unikraft console device.
+ *
+ * @param dev
+ * The Unikraft Console Device in unconfigured state.
+ * @param nb_desc
+ * Number of descriptors for the queue. Inspect uk_consdev_rx_info_get() to
+ * retrieve limitations. If nb_desc is set to 0, the driver chooses a default
+ * value.
I think nb_desc is probably misleading. Read my comment below (type
definition `uk_consdev_rx_configure_t`). You may want to update the API
description, too.
+ * @param rx_conf
+ * The pointer to the configuration data to be used for receive.
+ * Its memory can be released after invoking this function.
rxr_conf or rxq_conf?
+ * @return
+ * - (0): Success, receive correctly set up.
+ * - (<0): Unable to allocate the receive ring descriptors.
+ */
+int uk_consdev_rx_configure(struct uk_consdev *dev,
+ uint16_t nb_desc,
+ const struct uk_consdev_rx_conf *rx_conf);
+
+/**
+ * Sets up receive for an Unikraft console device.
+ *
+ * @param dev
+ * The Unikraft Console Device in unconfigured state.
+ * @param nb_desc
+ * Number of descriptors for the queue. Inspect uk_consdev_tx_info_get() to
+ * retrieve limitations. If nb_desc is set to 0, the driver chooses a default
+ * value.
Same here.
+ * @param tx_conf
+ * The pointer to the configuration data to be used for transmit.
+ * Its memory can be released after invoking this function.
+ * @return
+ * - (0): Success, receive correctly set up.
+ * - (<0): Unable to allocate the transmit ring descriptors.
+ */
+int uk_consdev_tx_configure(struct uk_consdev *dev,
+ uint16_t nb_desc,
+ const struct uk_consdev_tx_conf *tx_conf);
+
+/**
+ * Free rx and tx rings for an Unikraft console device.
+ *
+ * @param dev
+ * The Unikraft Console Device in configured state.
+ */
+void uk_consdev_release(struct uk_consdev *dev);
+
+/**
* Get the number of available Unikraft Console devices.
*
* @return
diff --git a/lib/ukconsdev/include/uk/consdev_core.h
b/lib/ukconsdev/include/uk/consdev_core.h
index 5edc159a..1295a913 100644
--- a/lib/ukconsdev/include/uk/consdev_core.h
+++ b/lib/ukconsdev/include/uk/consdev_core.h
@@ -38,6 +38,10 @@
#include <uk/list.h>
#include <uk/config.h>
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+#include <uk/sched.h>
+#include <uk/semaphore.h>
+#endif
/**
* Unikraft console API common declarations.
@@ -92,6 +96,45 @@ struct uk_consdev_ring_info {
int nb_is_power_of_two;
};
+/**
+ * Structure used to configure a console device.
+ */
+struct uk_consdev_conf {
+
+};
+
+/**
+ * Function type used for event callbacks.
+ *
+ * @param dev
+ * The Unikraft Console Device.
+ * @param argp
+ * Extra argument that can be defined on callback registration.
+ */
+typedef void (*uk_consdev_event_t)(struct uk_consdev *dev, void *argp);
+
+/**
+ * Structure used to configure an Unikraft console device RX queue.
+ */
+struct uk_consdev_rx_conf {
+ /* Event callback function. */
+ uk_consdev_event_t callback;
+ /* Argument pointer for callback. */
+ void *cb_cookie;
Call it just "cookie" or "callback_cookie" in order to avoid confusion.
It seems we choose the later in uknetdev.
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ /* Scheduler for dispatcher. */
+ struct uk_sched *s;
+#endif
+};
+
+/**
+ * Structure used to configure an Unikraft console device TX queue.
+ */
+struct uk_consdev_tx_conf {
+
+};
+
/** Driver callback type to read device/driver capabilities,
* used for configuring the device
*/
@@ -110,6 +153,23 @@ typedef int (*uk_consdev_rx_info_t) (struct uk_consdev
*dev,
typedef int (*uk_consdev_tx_info_t) (struct uk_consdev *dev,
struct uk_consdev_ring_info *ring_info);
+/** Driver callback type to configure a console device. */
+typedef int (*uk_consdev_configure_t)(struct uk_consdev *dev,
+ const struct uk_consdev_conf *dev_conf);
+
+/** Driver callback type to set up a RX ring of an Unikraft console device. */
+typedef int (*uk_consdev_rx_configure_t)(struct uk_consdev *dev,
+ uint16_t nb_desc,
+ const struct uk_consdev_rx_conf *rx_conf);
I think nb_desc may be not the right name for console devices. It is the
ring size (or queue size) right? The number of characters a ring can
hold, right? Would it be better to call it
`uint16_t rlen` or `uint16_t qlen`?
If this makes sense you should replace the name in all corresponding
interfaces, too (e.g., uk_consdev_rxr_configure(),
uk_consdev_txr_configure())
+
+/** Driver callback type to set up a TX ring of an Unikraft console device. */
+typedef int (*uk_consdev_tx_configure_t)(struct uk_consdev *dev,
+ uint16_t nb_desc,
+ const struct uk_consdev_tx_conf *tx_conf);
+
+/** Driver callback type to release a configured Unikraft console device */
+typedef int (*uk_consdev_release_t)(struct uk_consdev *dev);
+
/** Driver callback type to close an Unikraft console device. */
typedef void (*uk_consdev_close_t)(struct uk_consdev *dev);
@@ -117,11 +177,39 @@ struct uk_consdev_ops {
uk_consdev_info_t info_get;
uk_consdev_rx_info_t rx_info_get;
uk_consdev_tx_info_t tx_info_get;
+ uk_consdev_configure_t configure;
+ uk_consdev_release_t release;
+ uk_consdev_rx_configure_t rx_configure;
`uk_consdev_rxr_configure_t rxr_configure;` or
`uk_consdev_rxq_configure_t rxq_configure;` ?
+ uk_consdev_tx_configure_t tx_configure;
Same here.
uk_consdev_close_t close;
};
/**
* @internal
+ * Event handler configuration (internal to libukconsdev)
+ */
+struct uk_consdev_event_handler {
+ /* Callback */
+ uk_consdev_event_t callback;
+ /* Parameter for callback */
+ void *cb_cookie;
You can call the field just `cookie` like we did in uknetdev.
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ /* Semaphore to trigger events. */
+ struct uk_semaphore events;
+ /* Reference to console device */
+ struct uk_consdev *dev;
+ /* Dispatcher thread. */
+ struct uk_thread *dispatcher;
+ /* reference to thread name. */
+ char *dispatcher_name;
+ /* Scheduler for dispatcher. */
+ struct uk_sched *dispatcher_s;
+#endif
+};
+
+/**
+ * @internal
* libukconsdev internal data associated with each console device.
*/
struct uk_consdev_data {
@@ -129,6 +217,8 @@ struct uk_consdev_data {
const uint16_t id;
/* Device state */
enum uk_consdev_state state;
+ /* Event handler for rx */
+ struct uk_consdev_event_handler rx_handler;
/* Name of device*/
const char *drv_name;
/* Device allocator */
diff --git a/lib/ukconsdev/include/uk/consdev_driver.h
b/lib/ukconsdev/include/uk/consdev_driver.h
index 75f12b43..a90778c3 100644
--- a/lib/ukconsdev/include/uk/consdev_driver.h
+++ b/lib/ukconsdev/include/uk/consdev_driver.h
@@ -51,6 +51,30 @@ extern "C" {
#endif
/**
+ * Forwards an rx event to the API user
+ * Can (and should) be called from device interrupt context
+ *
+ * @param dev
+ * Unikraft console device to which the event relates to
+ */
+static inline void uk_consdev_drv_rx_event(struct uk_consdev *dev)
+{
+ struct uk_consdev_event_handler *rx_handler;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+
+ rx_handler = &dev->_data->rx_handler;
+
+#ifdef CONFIG_LIBUKCONSDEV_DISPATCHERTHREADS
+ uk_semaphore_up(&rx_handler->events);
+#else
+ if (rx_handler->callback)
+ rx_handler->callback(dev, rx_handler->cb_cookie);
+#endif
+}
+
+/**
* Adds a Unikraft console device to the device list.
* This should be called whenever a driver adds a new found device.
*
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
