[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH v2 4/6] lib/ukblkdev: Request interface
Hey Roxana,
thanks for the changes! I will take this patch and apply 3 minor
modifications while upstreaming. They are basically about adding some
more details within your comments. I think by having them, the API is
becoming even more clear.
Thanks,
Simon
Reviewed-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
On 01.07.19 12:04, Roxana Nicolescu wrote:
This patch introduces the interface for sending a request and receiving
a response. This is designed to support asynchronous operations.
The interface permits sector-wide access only.
Receiving the response from backend can be done with interrupts or
with polling.
Signed-off-by: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>
---
lib/ukblkdev/blkdev.c | 34 ++++++
lib/ukblkdev/exportsyms.uk | 10 ++
lib/ukblkdev/include/uk/blkdev.h | 199 ++++++++++++++++++++++++++++++++
lib/ukblkdev/include/uk/blkdev_core.h | 62 ++++++++++
lib/ukblkdev/include/uk/blkdev_driver.h | 28 +++++
lib/ukblkdev/include/uk/blkreq.h | 112 ++++++++++++++++++
6 files changed, 445 insertions(+)
create mode 100644 lib/ukblkdev/include/uk/blkreq.h
diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
index c7098274..a92b29f1 100644
--- a/lib/ukblkdev/blkdev.c
+++ b/lib/ukblkdev/blkdev.c
@@ -84,6 +84,12 @@ int uk_blkdev_drv_register(struct uk_blkdev *dev, struct
uk_alloc *a,
UK_ASSERT(dev->dev_ops->queue_setup);
UK_ASSERT(dev->dev_ops->get_info);
UK_ASSERT(dev->dev_ops->queue_get_info);
+ UK_ASSERT(dev->submit_one);
+ UK_ASSERT(dev->finish_reqs);
+ UK_ASSERT((dev->dev_ops->queue_intr_enable &&
+ dev->dev_ops->queue_intr_disable)
+ || (!dev->dev_ops->queue_intr_enable
+ && !dev->dev_ops->queue_intr_disable));
dev->_data = _alloc_data(a, blkdev_count, drv_name);
if (!dev->_data)
@@ -374,3 +380,31 @@ int uk_blkdev_start(struct uk_blkdev *dev)
return rc;
}
+
+int uk_blkdev_queue_submit_one(struct uk_blkdev *dev,
+ uint16_t queue_id,
+ struct uk_blkreq *req)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->submit_one);
+ UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+ UK_ASSERT(dev->_data->state == UK_BLKDEV_RUNNING);
+ UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+ UK_ASSERT(req != NULL);
+
+ return dev->submit_one(dev, dev->_queue[queue_id], req);
+}
+
+int uk_blkdev_queue_finish_reqs(struct uk_blkdev *dev,
+ uint16_t queue_id)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->finish_reqs);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+ UK_ASSERT(dev->_data->state == UK_BLKDEV_RUNNING);
+ UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+
+ return dev->finish_reqs(dev, dev->_queue[queue_id]);
+}
diff --git a/lib/ukblkdev/exportsyms.uk b/lib/ukblkdev/exportsyms.uk
index 077994f0..33402f78 100644
--- a/lib/ukblkdev/exportsyms.uk
+++ b/lib/ukblkdev/exportsyms.uk
@@ -9,3 +9,13 @@ uk_blkdev_configure
uk_blkdev_queue_get_info
uk_blkdev_queue_configure
uk_blkdev_start
+uk_blkdev_queue_intr_enable
+uk_blkdev_queue_intr_disable
+uk_blkdev_capabilities
+uk_blkdev_ssize
+uk_blkdev_max_sec_per_req
+uk_blkdev_mode
+uk_blkdev_sectors
+uk_blkdev_ioalign
I did not notice last time that you do not need to add inline functions
and macros to the `exportsyms.uk` file. Those definitions aren't
creating any symbol in the library object. But on the other hand it does
not hurt to have them listed here. It is just a statement without
effect. If you want to remove it, you can do this with a follow-up
patch. I will accept this one in order to move on with reviewing your
block drivers.
+uk_blkdev_queue_submit_one
+uk_blkdev_queue_finish_reqs
diff --git a/lib/ukblkdev/include/uk/blkdev.h b/lib/ukblkdev/include/uk/blkdev.h
index 375407ed..b941911c 100644
--- a/lib/ukblkdev/include/uk/blkdev.h
+++ b/lib/ukblkdev/include/uk/blkdev.h
@@ -222,6 +222,205 @@ int uk_blkdev_queue_configure(struct uk_blkdev *dev,
uint16_t queue_id,
*/
int uk_blkdev_start(struct uk_blkdev *dev);
+/**
+ * Get the capabilities info which stores info about the device,
+ * like nb_of_sectors, sector_size etc
+ * The device state has to be UK_BLKDEV_RUNNING.
+ *
+ * @param dev
+ * The Unikraft Block Device.
+ *
+ * @return
+ * A pointer to a structure of type *uk_blkdev_capabilities*.
+ **/
+static inline const struct uk_blkdev_cap *uk_blkdev_capabilities(
+ struct uk_blkdev *blkdev)
+{
+ UK_ASSERT(blkdev);
+ UK_ASSERT(blkdev->_data->state >= UK_BLKDEV_RUNNING);
+
+ return &blkdev->capabilities;
+}
+
+#define uk_blkdev_ssize(blkdev) \
+ (uk_blkdev_capabilities(blkdev)->ssize)
+
+#define uk_blkdev_max_sec_per_req(blkdev) \
+ (uk_blkdev_capabilities(blkdev)->max_sectors_per_req)
+
+#define uk_blkdev_mode(blkdev) \
+ (uk_blkdev_capabilities(blkdev)->mode)
+
+#define uk_blkdev_sectors(blkdev) \
+ (uk_blkdev_capabilities(blkdev)->sectors)
+
+#define uk_blkdev_ioalign(blkdev) \
+ (uk_blkdev_capabilities(blkdev)->ioalign)
+/**
+ * Enable interrupts for a queue.
+ *
+ * @param dev
+ * The Unikraft Block Device in running state.
+ * @param queue_id
+ * The index of the queue to set up.
+ * The value must be in the range [0, nb_queue - 1] previously supplied
+ * to uk_blkdev_configure().
+ * @return
+ * - (0): Success, interrupts enabled.
+ * - (-ENOTSUP): Driver does not support interrupts.
+ */
+static inline int uk_blkdev_queue_intr_enable(struct uk_blkdev *dev,
+ uint16_t queue_id)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->dev_ops);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+ UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+
+ if (unlikely(!dev->dev_ops->queue_intr_enable))
+ return -ENOTSUP;
+
+ return dev->dev_ops->queue_intr_enable(dev, dev->_queue[queue_id]);
+}
+
+/**
+ * Disable interrupts for a queue.
+ *
+ * @param dev
+ * The Unikraft Block Device in running state.
+ * @param queue_id
+ * The index of the queue to set up.
+ * The value must be in the range [0, nb_queue - 1] previously supplied
+ * to uk_blkdev_configure().
+ * @return
+ * - (0): Success, interrupts disabled.
+ * - (-ENOTSUP): Driver does not support interrupts.
+ */
+static inline int uk_blkdev_queue_intr_disable(struct uk_blkdev *dev,
+ uint16_t queue_id)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->dev_ops);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+ UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+
+ if (unlikely(!dev->dev_ops->queue_intr_disable))
+ return -ENOTSUP;
+
+ return dev->dev_ops->queue_intr_disable(dev, dev->_queue[queue_id]);
+}
+
+/**
+ * Make an aio request to the device
+ *
+ * @param dev
+ * The Unikraft Block Device
+ * @param queue_id
+ * The index of the receive queue to receive from.
+ * The value must be in the range [0, nb_queue - 1] previously supplied
+ * to uk_blkdev_configure().
+ * @param req
+ * Request structure
+ * @return
+ * - (>=0): Positive value with status flags
+ * - UK_BLKDEV_STATUS_SUCCESS: `req` was successfully put to the
+ * queue.
+ * - UK_BLKDEV_STATUS_MORE: Indicates there is still at least
+ * one descriptor available for a subsequent transmission.
+ * If the flag is unset means that the queue is full.
+ * This may only be set together with UK_BLKDEV_STATUS_SUCCESS.
+ * - (<0): Negative value with error code from driver, no request was sent.
+ */
+int uk_blkdev_queue_submit_one(struct uk_blkdev *dev, uint16_t queue_id,
+ struct uk_blkreq *req);
+
+/**
+ * Tests for status flags returned by `uk_blkdev_submit_one`
+ * When the function returned an error code or one of the selected flags is
+ * unset, this macro returns False.
+ *
+ * @param status
+ * Return status (int)
+ * @param flag
+ * Flag(s) to test
+ * @return
+ * - (True): All flags are set and status is not negative
+ * - (False): At least one flag is not set or status is negative
+ */
+#define uk_blkdev_status_test_set(status, flag) \
+ (((int)(status) & ((int)(flag) | INT_MIN)) == (flag))
+
+/**
+ * Tests for unset status flags returned by `uk_blkdev_submit_one`
+ * When the function returned an error code or one of the
+ * selected flags is set, this macro returns False.
+ *
+ * @param status
+ * Return status (int)
+ * @param flag
+ * Flag(s) to test
+ * @return
+ * - (True): Flags are not set and status is not negative
+ * - (False): At least one flag is set or status is negative
+ */
+#define uk_blkdev_status_test_unset(status, flag) \
+ (((int)(status) & ((int)(flag) | INT_MIN)) == (0x0))
+
+/**
+ * Tests if the return status of `uk_blkdev_submit_one`
+ * indicates a successful operation.
+ *
+ * @param status
+ * Return status (int)
+ * @return
+ * - (True): Operation was successful
+ * - (False): Operation was unsuccessful or error happened
+ */
+#define uk_blkdev_status_successful(status) \
+ uk_blkdev_status_test_set((status), UK_BLKDEV_STATUS_SUCCESS)
+
+/**
+ * Tests if the return status of `uk_blkdev_submit_one`
+ * indicates that the operation should be retried/
+ *
+ * @param status
+ * Return status (int)
+ * @return
+ * - (True): Operation should be retried
+ * - (False): Operation was successful or error happened
+ */
+#define uk_blkdev_status_notready(status) \
+ uk_blkdev_status_test_unset((status), UK_BLKDEV_STATUS_SUCCESS)
+
+/**
+ * Tests if the return status of `uk_blkdev_submit_one`
+ * indicates that the last operation can be successfully repeated again.
+ *
+ * @param status
+ * Return status (int)
+ * @return
+ * - (True): Flag UK_BLKDEV_STATUS_MORE is set
+ * - (False): Operation was successful or error happened
+ */
+#define uk_blkdev_status_more(status) \
+ uk_blkdev_status_test_set((status), (UK_BLKDEV_STATUS_SUCCESS \
+ | UK_BLKDEV_STATUS_MORE))
+
+/**
+ * Get responses from the queue
+ *
+ * @param dev
+ * The Unikraft Block Device
+ * @param queue_id
+ * queue id
+ * @return
+ * - 0: Success
+ * - (<0): on error returned by driver
+ */
+int uk_blkdev_queue_finish_reqs(struct uk_blkdev *dev, uint16_t queue_id);
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/ukblkdev/include/uk/blkdev_core.h
b/lib/ukblkdev/include/uk/blkdev_core.h
index c4c458ab..13cd44ba 100644
--- a/lib/ukblkdev/include/uk/blkdev_core.h
+++ b/lib/ukblkdev/include/uk/blkdev_core.h
@@ -38,6 +38,7 @@
#include <uk/list.h>
#include <uk/config.h>
+#include <uk/blkreq.h>
#if defined(CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS)
#include <uk/sched.h>
#include <uk/semaphore.h>
@@ -121,6 +122,9 @@ struct uk_blkdev_queue;
* The queue on the Unikraft block device on which the event happened.
* @param argp
* Extra argument that can be defined on callback registration.
+ *
+ * Note: This should call dev->finish_reqs function in order to process the
+ * received responses.
*/
typedef void (*uk_blkdev_queue_event_t)(struct uk_blkdev *dev,
uint16_t queue_id, void *argp);
@@ -163,12 +167,64 @@ typedef struct uk_blkdev_queue *
(*uk_blkdev_queue_configure_t)(
/** Driver callback type to start a configured Unikraft block device. */
typedef int (*uk_blkdev_start_t)(struct uk_blkdev *dev);
+/**
+ * Driver callback type to enable interrupts
+ * for a queue on Unikraft block device.
+ **/
+typedef int (*uk_blkdev_queue_intr_enable_t)(struct uk_blkdev *dev,
+ struct uk_blkdev_queue *queue);
+
+/**
+ * Driver callback type to disable interrupts
+ * for a queue on Unikraft block device.
+ **/
+typedef int (*uk_blkdev_queue_intr_disable_t)(struct uk_blkdev *dev,
+ struct uk_blkdev_queue *queue);
+/**
+ * Status code flags returned queue_submit_one function
+ */
+/** Successful operation. */
+#define UK_BLKDEV_STATUS_SUCCESS (0x1)
+/**
+ * More room available for operation (e.g., still space on queue for sending
+ * a request.
+ */
+#define UK_BLKDEV_STATUS_MORE (0x2)
+
+/** Driver callback type to submit a request to Unikraft block device. */
+typedef int (*uk_blkdev_queue_submit_one_t)(struct uk_blkdev *dev,
+ struct uk_blkdev_queue *queue, struct uk_blkreq *req);
+/**
+ * Driver callback type to finish
+ * a bunch of requests to Unikraft block device.
+ **/
+typedef int (*uk_blkdev_queue_finish_reqs_t)(struct uk_blkdev *dev,
+ struct uk_blkdev_queue *queue);
+
struct uk_blkdev_ops {
uk_blkdev_get_info_t get_info;
uk_blkdev_configure_t dev_configure;
uk_blkdev_queue_get_info_t queue_get_info;
uk_blkdev_queue_configure_t queue_setup;
uk_blkdev_start_t dev_start;
+ uk_blkdev_queue_intr_enable_t queue_intr_enable;
+ uk_blkdev_queue_intr_disable_t queue_intr_disable;
+};
+
+/**
+ * Device info
+ */
+struct uk_blkdev_cap {
+ /* Number of sectors */
+ __sector sectors;
+ /* Sector size */
+ size_t ssize;
+ /* Access mode */
I will `#include <fcntl.h>` to this header and change the comment to
/* Access mode (O_RDONLY, O_RDWR, O_WRONLY) */
while upstreaming. Then it should be clear what this field is about.
+ int mode;
+ /* Max nb of supported sectors for an op */
+ __sector max_sectors_per_req;
+ /* Alignment (number of bytes) for data used in future requests */
+ uint16_t ioalign;
};
/**
@@ -216,8 +272,14 @@ struct uk_blkdev_data {
};
struct uk_blkdev {
+ /* Pointer to submit request function */
+ uk_blkdev_queue_submit_one_t submit_one;
+ /* Pointer to handle_responses function */
+ uk_blkdev_queue_finish_reqs_t finish_reqs;
/* Pointer to API-internal state data. */
struct uk_blkdev_data *_data;
+ /* Capabilities. */
+ struct uk_blkdev_cap capabilities;
/* Functions callbacks by driver. */
const struct uk_blkdev_ops *dev_ops;
/* Pointers to queues (API-private) */
diff --git a/lib/ukblkdev/include/uk/blkdev_driver.h
b/lib/ukblkdev/include/uk/blkdev_driver.h
index 5dc9657d..534542a1 100644
--- a/lib/ukblkdev/include/uk/blkdev_driver.h
+++ b/lib/ukblkdev/include/uk/blkdev_driver.h
@@ -69,6 +69,34 @@ extern "C" {
int uk_blkdev_drv_register(struct uk_blkdev *dev, struct uk_alloc *a,
const char *drv_name);
+/**
+ * Forwards a queue event to the API user
+ * Can (and should) be called from device interrupt context
+ *
+ * @param dev
+ * Unikraft block device to which the event relates to
+ * @param queue_id
+ * receive queue ID to which the event relates to
+ */
+static inline void uk_blkdev_drv_queue_event(struct uk_blkdev *dev,
+ uint16_t queue_id)
+{
+ struct uk_blkdev_event_handler *queue_handler;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+
+ queue_handler = &dev->_data->queue_handler[queue_id];
+
+#if CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS
+ uk_semaphore_up(&queue_handler->events);
+#else
+ if (queue_handler->callback)
+ queue_handler->callback(dev, queue_id, queue_handler->cookie);
+#endif
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/ukblkdev/include/uk/blkreq.h b/lib/ukblkdev/include/uk/blkreq.h
new file mode 100644
index 00000000..aa2cf9c4
--- /dev/null
+++ b/lib/ukblkdev/include/uk/blkreq.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>
+ *
+ * Copyright (c) 2019, University Politehnica of Bucharest
+ * 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_BLKREQ_H_
+#define UK_BLKREQ_H_
+
+#include <uk/refcount.h>
+
+/**
+ * Unikraft block API request declaration.
+ *
+ * This header contains all the API data types used for requests operation.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define __sector size_t
+struct uk_blkreq;
+
+/**
+ * Operation status
+ */
+enum uk_blkreq_state {
+ UK_BLKDEV_REQ_FINISHED = 0,
+ UK_BLKDEV_REQ_UNFINISHED
+};
+
+/**
+ * Supported operations
+ */
+enum uk_blkreq_op {
+ /* Read operation */
+ UK_BLKDEV_READ = 0,
+ /* Write operation */
+ UK_BLKDEV_WRITE,
+ /* Flush the volatile write cache */
+ UK_BLKDEV_FFLUSH = 4
+};
+
+/**
+ * Function type used for request callback after a response is processed.
+ *
+ * @param req
+ * The request object on which the event is triggered
+ * @param cookie_callback
+ * Optional parameter set by user at request submit.
+ */
+typedef void (*uk_blkreq_event_t)(struct uk_blkreq *req, void *cb_cookie);
+
+/**
+ * Used for sending a request to the device.
+ */
+struct uk_blkreq {
+ /* Input members */
+ /* Operation type */
+ enum uk_blkreq_op operation;
+ /* Start Sector from where the op begin */
+ __sector start_sector;
+ /* Size in number of sectors */
+ __sector nb_sectors;
+ /* Request callback and its parameters */
+ uk_blkreq_event_t cb;
+ void *cb_cookie;
+ /* Pointer to data */
+ void *aio_buf;
Now, you moved it too far ;-). In the end it is only regarding
readability. While upstreaming, I will move aio_buf before cookie to
group I/O paremeters together again.
+
+ /* Output members */
+ /* State of request: finished/unfinished*/
+ __atomic state;
+ /* Result status of operation */
I will also change this comment to
/* Result status of operation (< 0 on errors) */
...while upstreaming.
+ int result;
+
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* UK_BLKREQ_H_ */
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
