[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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |