|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 3/3] plat/xen: Add Xenbus support
Hi, Yuri,
Thanks a lot for this detailed review and sorry for the patch size.
Please see me replies inline.
On 07/26/2018 04:59 PM, Yuri Volchkov wrote:
> Hi Costin!
>
>
> That's quite a patch :). I am sure any reviewer would appreciate if you
> try to find a way how to split your next contribution into few smaller
> (but logical) pieces.
>
> I have my comments inline. But for the next version would it be possible
> to split a little bit this big guy? In attempt to help you, I came up
> with a very rough idea how to do it:
>
> - basic xenbus communication: event channel handler, xs_msg_write, and
> xs_thread_func. Functions process_reply and process_watch_event would
> be just advancing the rsp_cons in this case.
>
> - everything related to xenbus replies
>
> - everything related to xenbus_watch
>
> - _xenbus_register_driver, driver initialization, and related control
> structures (e.g. xenbus_driver)
>
> - xenbus_probe and related. In the earlier patch just put a stub, always
> returning 0
>
> - xenbus_switch_state and xenbus_wait_for_state_change
>
> I did not try to actually split, so some of these ideas might be wrong.
Thanks for the suggestions, actually this split make sense.
>
> Getting back to the actual patch. First things first, Unikraft would not
> build with this patch if I disable xenbus.
Fixed.
>
> The rest of the comments are inline.
>
> -- Yuri.
>
> Costin Lupu <costin.lupu@xxxxxxxxx> writes:
>
>> The current implementation is ported from Mini-OS.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>> plat/xen/Config.uk | 8 +
>> plat/xen/Makefile.uk | 10 +
>> plat/xen/include/xenbus/client.h | 139 +++++++++++
>> plat/xen/include/xenbus/xenbus.h | 158 ++++++++++++
>> plat/xen/include/xenbus/xs.h | 221 +++++++++++++++++
>> plat/xen/xenbus/client.c | 278 +++++++++++++++++++++
>> plat/xen/xenbus/xenbus.c | 260 ++++++++++++++++++++
>> plat/xen/xenbus/xs.c | 518
>> +++++++++++++++++++++++++++++++++++++++
>> plat/xen/xenbus/xs_comms.c | 484 ++++++++++++++++++++++++++++++++++++
>> plat/xen/xenbus/xs_comms.h | 75 ++++++
>> plat/xen/xenbus/xs_watch.c | 159 ++++++++++++
>> plat/xen/xenbus/xs_watch.h | 91 +++++++
>> 12 files changed, 2401 insertions(+)
>> create mode 100644 plat/xen/include/xenbus/client.h
>> create mode 100644 plat/xen/include/xenbus/xenbus.h
>> create mode 100644 plat/xen/include/xenbus/xs.h
>> create mode 100644 plat/xen/xenbus/client.c
>> create mode 100644 plat/xen/xenbus/xenbus.c
>> create mode 100644 plat/xen/xenbus/xs.c
>> create mode 100644 plat/xen/xenbus/xs_comms.c
>> create mode 100644 plat/xen/xenbus/xs_comms.h
>> create mode 100644 plat/xen/xenbus/xs_watch.c
>> create mode 100644 plat/xen/xenbus/xs_watch.h
>>
>> diff --git a/plat/xen/Config.uk b/plat/xen/Config.uk
>> index 9c398f1..d0143e9 100644
>> --- a/plat/xen/Config.uk
>> +++ b/plat/xen/Config.uk
>> @@ -20,4 +20,12 @@ if (PLAT_XEN)
>> instead of the hypervisor console. When this
>> option is enabled the hypervisor console is used
>> for kernel messages only.
>> +
>> +menuconfig XEN_XENBUS
>> + bool "Xenbus Driver"
>> + default n
>> + depends on (ARCH_X86_64)
>> + select LIBUKBUS
>> + help
>> + Register a Xenbus driver as uk_bus
>> endif
>> diff --git a/plat/xen/Makefile.uk b/plat/xen/Makefile.uk
>> index 45096cb..ff23459 100644
>> --- a/plat/xen/Makefile.uk
>> +++ b/plat/xen/Makefile.uk
>> @@ -72,3 +72,13 @@ LIBXENPLAT_SRCS-y +=
>> $(LIBXENPLAT_BASE)/console.c
>> LIBXENPLAT_SRCS-y += $(LIBXENPLAT_BASE)/shutdown.c
>> LIBXENPLAT_SRCS-y += $(LIBXENPLAT_BASE)/events.c
>> LIBXENPLAT_SRCS-y += $(LIBXENPLAT_BASE)/gnttab.c
>> +
>> +LIBXENBUS_ASFLAGS-y += $(LIBXENPLAT_ASFLAGS-y)
>> +LIBXENBUS_ASINCLUDES-y += $(LIBXENPLAT_ASINCLUDES-y)
>> +LIBXENBUS_CFLAGS-y += $(LIBXENPLAT_CFLAGS-y)
>> +LIBXENBUS_CINCLUDES-y += $(LIBXENPLAT_CINCLUDES-y)
>> +LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xenbus.c
>> +LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/client.c
>> +LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xs_comms.c
>> +LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xs_watch.c
>> +LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xs.c
>> diff --git a/plat/xen/include/xenbus/client.h
>> b/plat/xen/include/xenbus/client.h
>> new file mode 100644
>> index 0000000..112c8c9
>> --- /dev/null
>> +++ b/plat/xen/include/xenbus/client.h
>> @@ -0,0 +1,139 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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.
>> + */
>> +/*
>> + * Client interface between the device and the Xenbus driver.
>> + * Ported from Mini-OS xenbus.c
>> + */
>> +
>> +#ifndef __XENBUS_CLIENT_H__
>> +#define __XENBUS_CLIENT_H__
>> +
>> +#include <xenbus/xenbus.h>
>> +#include <xenbus/xs.h>
>> +
>> +/*
>> + * Returns the name of the state for tracing/debugging purposes.
>> + *
>> + * @param state The Xenbus state
>> + * @return A string representing the state name
>> + */
>> +const char *xenbus_state_to_str(XenbusState state);
>> +
>> +/*
>> + * Converts a device type value to name
>> + *
>> + * @param devtype The Xenbus device type
>> + * @return A string representing the device type name
>> + */
>> +const char *xenbus_devtype_to_str(enum xenbus_dev_type devtype);
>> +
>> +/*
>> + * Converts a device type name to value
>> + *
>> + * @param devtypestr The Xenbus device type name
>> + * @return The Xenbus device type
>> + */
>> +enum xenbus_dev_type xenbus_str_to_devtype(const char *devtypestr);
>> +
>> +
>> +/*
>> + * Watches
>> + */
>> +
>> +/*
>> + * Waits for a watch event associated with the event list. If no event list
>> is
>> + * provided, a global event list is used instead. Called by a client driver.
>> + *
>> + * @param evlist The watch event list
>> + */
>> +void xenbus_wait_watch_event(xenbus_watch_evlist_t *evlist);
>> +
>> +/*
>> + * Notifies a client driver waiting for watch events.
>> + *
>> + * @param evlist The watch event list
>> + * @param event The watch event
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xenbus_notify_watch_event(xenbus_watch_evlist_t *evlist,
>> + struct xenbus_watch_event *event);
>> +
>> +/*
>> + * Waits for a value in Xenstore to change by using watches. If no event
>> list is
>> + * provided, a global event list is used instead.
>> + *
>> + * @param path Xenstore path
>> + * @param value The expected value
>> + * @param evlist The watch event list
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xenbus_wait_for_value(const char *path, const char *value,
>> + xenbus_watch_evlist_t *evlist);
>> +
>> +/*
>> + * Driver states
>> + */
>> +
>> +/*
>> + * Returns the driver state found at the given Xenstore path.
>> + *
>> + * @param path Xenstore path
>> + * @return The Xenbus driver state
>> + */
>> +XenbusState xenbus_read_driver_state(const char *path);
>> +
>> +/*
>> + * Changes the state of a Xen PV driver
>> + *
>> + * @param xendev Xenbus device
>> + * @param state The new Xenbus state
>> + * @param xbt Xenbus transaction id
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xenbus_switch_state(struct xenbus_device *xendev, XenbusState state,
>> + xenbus_transaction_t xbt);
>> +
>> +/*
>> + * Waits for the driver state found at the given Xenstore path to change by
>> + * using watches.
>> + *
>> + * @param path Xenstore path
>> + * @param state The returned Xenbus state
>> + * @param evlist The watch event list
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xenbus_wait_for_state_change(const char *path, XenbusState *state,
>> + xenbus_watch_evlist_t *evlist);
>> +
>> +#endif /* __XENBUS_CLIENT_H__ */
>> diff --git a/plat/xen/include/xenbus/xenbus.h
>> b/plat/xen/include/xenbus/xenbus.h
>> new file mode 100644
>> index 0000000..2836a85
>> --- /dev/null
>> +++ b/plat/xen/include/xenbus/xenbus.h
>> @@ -0,0 +1,158 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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 __XENBUS_H__
>> +#define __XENBUS_H__
>> +
>> +#include <uk/bus.h>
>> +#include <uk/alloc.h>
>> +#include <xen/xen.h>
>> +#include <xen/io/xenbus.h>
>> +
>> +
>> +/*
>> + * Supported device types
>> + */
>> +typedef enum xenbus_dev_type {
>> + xenbus_dev_none = 0,
>> + xenbus_dev_sysctl, /* System wise control device */
> probably "system-wide" is a better word? :)
I removed the device types for v2. They should be added one by one
whenever a new driver is added.
>> + xenbus_dev_vif, /* Virtual network interface */
>> + xenbus_dev_vbd, /* Virtual block device */
>> +} xenbus_dev_type_t;
>> +
>> +struct xenbus_device;
>> +
>> +/*
>> + * Xenbus driver
>> + */
>> +
>> +typedef int (*xenbus_driver_init_func_t)(struct uk_alloc *a);
>> +typedef int (*xenbus_driver_add_func_t)(struct xenbus_device *dev);
>> +
>> +
>> +struct xenbus_driver {
>> + UK_TAILQ_ENTRY(struct xenbus_driver) next;
>> + const xenbus_dev_type_t *device_types;
>> +
>> + xenbus_driver_init_func_t init;
>> + xenbus_driver_add_func_t add_dev;
>> +};
>> +UK_TAILQ_HEAD(xenbus_driver_list, struct xenbus_driver);
>> +
>> +
>> +#define XENBUS_REGISTER_DRIVER(b) \
>> + _XENBUS_REGISTER_DRIVER(__LIBNAME__, (b))
>> +
>> +#define _XENBUS_REGFNNAME(x, y) x##y
>> +
>> +#define _XENBUS_REGISTER_DRIVER(libname, b) \
>> + static void __constructor_prio(104) \
>> + _XENBUS_REGFNNAME(libname, _xenbus_register_driver)(void) \
>> + { \
>> + _xenbus_register_driver((b)); \
>> + }
>> +
>> +/* Do not use this function directly: */
>> +void _xenbus_register_driver(struct xenbus_driver *drv);
>> +
>> +
>> +/*
>> + * Xenbus watch
>> + */
>> +
>> +/* Watch event list */
>> +struct xenbus_watch_event {
>> + struct xenbus_watch_event *next;
>> +};
>> +typedef struct xenbus_watch_event *xenbus_watch_evlist_t;
> This typedef makes things even more complicated. I already lost between
> xenbus_watch, xs_watch, xs_watch_event. It is very hard to remember
> which is which, and adding another name for the same thing does not
> help. Especially if you use both "xenbus_watch_evlist_t" and "struct
> xenbus_watch_event"
>
> Also this would be really great if the naming scheme for these structs
> were changed. From offline conversation I gathered you are planning to
> rework this part. So I can not give ideas how to improve naming, because
> I don't know what is coming.
For v2 I redesigned the entire logic for watches and got rid of the
'xenbus_watch_event' struct. However, I prefer to keep the separation
between xenbus_watch and xs_watch. The former holds information for
xenbus watches in general. To clarify a bit what I'm trying to say here:
we might have some replacement in the future for Xenstore (e.g. NoXS in
LightVM), so implicitly we would have NoXS watches. In other words, the
xenbus watch is a generalization for watches of different store types.
The Xenstore watch (xs_watch) is a "class" extending the Xenbus watch
and holds also the Xenstore related information.
>> +
>> +struct xenbus_watch {
>> + struct xenbus_watch *next;
>> + xenbus_watch_evlist_t *events;
>> +};
> Is there a reason why you did not use TAILQ here?
Simplicity and laziness may be those reasons. Replaced with TAILQ.
>> +
>> +
>> +/*
>> + * Xenbus device
>> + */
>> +
>> +struct xenbus_device {
>> + /**< in use by Xenbus handler */
>> + UK_TAILQ_ENTRY(struct xenbus_device) next;
>> + /**< Device state */
>> + XenbusState state;
>> + /**< Device type */
>> + enum xenbus_dev_type devtype;
>> + /**< Xenstore path of the device */
>> + char *nodename;
>
> Further fields are not used in this patch series. Could we add them
> later. In the patch where you will introduce some code, which makes some
> use of them it?
I don't think it hurts to leave them here. I use them in my tests, so I
assure you they will be used by all the frontend drivers.
>> + /**< Xenstore path of the device peer (e.g. backend for frontend) */
>> + char *otherend;
>> + /**< Domain id of the other end */
>> + domid_t otherend_id;
>> + /**< Watch events list */
>> + xenbus_watch_evlist_t watch_events;
>> + /**< Xenbus driver */
>> + struct xenbus_driver *drv;
>> +};
>> +UK_TAILQ_HEAD(xenbus_device_list, struct xenbus_device);
>> +
>> +
>> +/*
>> + * Xenbus handler
>> + */
>> +
>> +struct xenbus_handler {
>> + struct uk_bus b;
>> + struct uk_alloc *a;
>> + struct xenbus_driver_list drv_list; /**< List of Xenbus drivers */
>> + int drv_list_initialized;
>> + struct xenbus_device_list dev_list; /**< List of Xenbus devices */
>> +};
>> +
>> +extern struct xenbus_handler xbh;
>> +
>> +/* Helper macros for Xenbus related allocations */
>> +#define uk_xb_malloc(size) uk_malloc(xbh.a, (size))
>> +#define uk_xb_calloc(n, size) uk_calloc(xbh.a, (n), (size))
>> +#define uk_xb_free(ptr) uk_free(xbh.a, (ptr))
> Relying on the global variable in the macro is a bit error prone. Could
> we make it a function? I believe link time optimization will make it
> inline.
>
> This way we also could make xbh static.
Ack.
>> +
>> +
>> +/* Debugging */
>> +#if DEBUG_XENBUS
>> +#define DBGXB(fmt, ...) uk_printd(DLVL_EXTRA, fmt, __VA_ARGS__)
>> +#else
>> +#define DBGXB(fmt, ...)
>> +#endif
> I would like to avoid establishing a zoo of different debug_print macros
> for different parts of the Unikraft. I have sent a patch "print
> DLVL_EXTRA messages only in debug build", which should help the problem
> you are trying to solve with the additional macro.
>
> I think having one macro which you can enable on per-file basis is
> better.
Agreed. I'm looking forward for your patch to get upstreamed.
>> +
>> +#endif /* __XENBUS_H__ */
>> diff --git a/plat/xen/include/xenbus/xs.h b/plat/xen/include/xenbus/xs.h
>> new file mode 100644
>> index 0000000..d6e1f9e
>> --- /dev/null
>> +++ b/plat/xen/include/xenbus/xs.h
>> @@ -0,0 +1,221 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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.
>> + */
>> +/* Xenstore API */
>> +
>> +#ifndef __XS_H__
>> +#define __XS_H__
>> +
>> +#include <xenbus/xenbus.h>
>> +
>> +
>> +typedef unsigned long xenbus_transaction_t;
>> +#define XBT_NIL ((xenbus_transaction_t) 0)
>> +
>> +
>> +/*
>> + * Equivalent of asprintf function.
> If this is an equivalent of asprintf, let's put it in in nolibc. I would
> propose to copy it from the musl, to not to think about any corner cases
> and compatibility with the standard asprintf.
Ack. I sent a patch series for asprintf.
>> + *
>> + * @param fmt Format string
>> + * @return On success, returns a malloc'd string. On error, returns a
>> negative
>> + * error number which should be checked using PTRISERR.
>> + */
>> +char *xs_join(const char *fmt, ...) __printf(1, 2);
>> +
>> +/*
>> + * Read the value associated with a path.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @return On success, returns a malloc'd copy of the value. On error,
>> returns
>> + * a negative error number which should be checked using PTRISERR.
>> + */
>> +char *xs_read(xenbus_transaction_t xbt, const char *path);
>> +
>> +/*
>> + * Associates a value with a path.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @param value Xenstore value
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_write(xenbus_transaction_t xbt, const char *path, const char *value);
>> +
>> +/*
>> + * List the contents of a directory.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore directory path
>> + * @return On success, returns a malloc'd array of pointers to malloc'd
>> strings.
>> + * The array is NULL terminated. On error, returns a negative error number
>> which
>> + * should be checked using PTRISERR. May block.
>> + */
>> +char **xs_ls(xenbus_transaction_t xbt, const char *path);
>> +
>> +/*
>> + * Removes the value associated with a path.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_rm(xenbus_transaction_t xbt, const char *path);
>> +
>> +/*
>> + * Reads permissions associated with a path.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @return On success, returns a malloc'd copy of the value. On error,
>> returns
>> + * a negative error number which should be checked using PTRISERR.
>> + */
>> +char *xs_get_perms(xenbus_transaction_t xbt, const char *path);
>> +
>> +/*
>> + * Sets the permissions associated with a path.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @param domid The id of the domain for which permissions are set
>> + * @param perm Permissions character (e.g. 'w', 'r', 'b', 'n')
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_set_perms(xenbus_transaction_t xbt, const char *path,
>> + domid_t domid, char perm);
>> +
>> +/*
>> + * Start a xenbus transaction. Returns the transaction in xbt on
>> + * success or an error number otherwise.
>> + *
>> + * @param xbt Address for returning the Xenbus transaction id
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_transaction_start(xenbus_transaction_t *xbt);
>> +
>> +/*
>> + * End a xenbus transaction. Returns non-zero on failure.
>> + * Parameter abort says whether the transaction should be aborted.
>> + * Returns 1 in *retry iff the transaction should be retried.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param abort Non-zero if transaction should be aborted
>> + * @param retry Address for returning the retry suggestion
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_transaction_end(xenbus_transaction_t xbt, int abort, int *retry);
>> +
>> +/*
>> + * Sends a debug message to the Xenstore daemon for writing it in the debug
>> log
>> + *
>> + * @param msg The logged message
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_debug_msg(const char *msg);
>> +
>> +/*
>> + * Read path and parse it as an integer.
>> + *
>> + * @param path Xenstore path
>> + * @param value Returned int value
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_read_integer(const char *path, int *value);
>> +
>> +/*
>> + * Contraction of sprintf and xs_read(path/node).
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param fmt Path format string
>> + * @return On success, returns a malloc'd copy of the value. On error,
>> returns
>> + * a negative error number which should be checked using PTRISERR.
>> + */
>> +char *xs_readf(xenbus_transaction_t xbt,
>> + const char *fmt, ...) __printf(2, 3);
>> +
>> +/*
>> + * Contraction of sprintf and xs_write(path/node).
>> + */
>> +int xs_printf(xenbus_transaction_t xbt, const char *node, const char *path,
>> + const char *fmt, ...) __printf(4, 5);
>> +
>> +/*
>> + * Utility function to figure out our domain id
>> + *
>> + * @return Our domain id
>> + */
>> +domid_t xs_get_self_id(void);
>> +
>> +/*
>> + * Registers a Xenstore watch
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @param token Watch identification token
>> + * @param events The associated watch events list
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_watch_path_token(xenbus_transaction_t xbt,
>> + const char *path, const char *token,
>> + xenbus_watch_evlist_t *events);
>> +
>> +/*
>> + * Unregisters a Xenstore watch
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @param token Watch identification token
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_unwatch_path_token(xenbus_transaction_t xbt,
>> + const char *path, const char *token);
>> +
>> +/*
>> + * Registers a Xenstore watch using the default global token and event list.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_watch_path(xenbus_transaction_t xbt, const char *path);
>> +
>> +/*
>> + * Unregisters a Xenstore watch using the default global token.
>> + *
>> + * @param xbt Xenbus transaction id
>> + * @param path Xenstore path
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_unwatch_path(xenbus_transaction_t xbt, const char *path);
>> +
>> +#endif /* __XS_H__ */
>> diff --git a/plat/xen/xenbus/client.c b/plat/xen/xenbus/client.c
>> new file mode 100644
>> index 0000000..f50f469
>> --- /dev/null
>> +++ b/plat/xen/xenbus/client.c
>> @@ -0,0 +1,278 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Steven Smith (sos22@xxxxxxxxx)
>> + * Grzegorz Milos (gm281@xxxxxxxxx)
>> + * John D. Ramsdell
>> + * Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2006, Cambridge University
>> + * 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.
>> + */
>> +/*
>> + * Client interface between the device and the Xenbus driver.
>> + * Ported from Mini-OS xenbus.c
>> + */
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <uk/errptr.h>
>> +#include <uk/wait.h>
>> +#include <xenbus/client.h>
>> +
>> +
>> +#define XENBUS_STATE_ENTRY(name) \
>> + [XenbusState##name] = #name
>> +
>> +static const char *const xb_state_tbl[] = {
>> + XENBUS_STATE_ENTRY(Unknown),
>> + XENBUS_STATE_ENTRY(Initialising),
>> + XENBUS_STATE_ENTRY(InitWait),
>> + XENBUS_STATE_ENTRY(Initialised),
>> + XENBUS_STATE_ENTRY(Connected),
>> + XENBUS_STATE_ENTRY(Closing),
>> + XENBUS_STATE_ENTRY(Closed),
>> + XENBUS_STATE_ENTRY(Reconfiguring),
>> + XENBUS_STATE_ENTRY(Reconfigured),
>> +};
>> +
>> +const char *xenbus_state_to_str(XenbusState state)
>> +{
>> + return (state < ARRAY_SIZE(xb_state_tbl)) ?
>> + xb_state_tbl[state] : "INVALID";
>> +}
>> +
>> +#define XENBUS_DEVTYPE_ENTRY(name) \
>> + [xenbus_dev_##name] = #name
>> +
>> +static const char *const xb_devtype_tbl[] = {
>> + XENBUS_DEVTYPE_ENTRY(none),
>> + XENBUS_DEVTYPE_ENTRY(sysctl),
>> + XENBUS_DEVTYPE_ENTRY(vif),
>> + XENBUS_DEVTYPE_ENTRY(vbd),
>> +};
>> +
>> +const char *xenbus_devtype_to_str(enum xenbus_dev_type devtype)
>> +{
>> + return (devtype < ARRAY_SIZE(xb_devtype_tbl)) ?
>> + xb_devtype_tbl[devtype] : "INVALID";
>> +}
>> +
>> +enum xenbus_dev_type xenbus_str_to_devtype(const char *devtypestr)
>> +{
>> + for (int i = 0; i < (int) ARRAY_SIZE(xb_devtype_tbl); i++) {
>> + if (!strcmp(xb_devtype_tbl[i], devtypestr))
>> + return (enum xenbus_dev_type) i;
>> + }
>> +
>> + return xenbus_dev_none;
>> +}
>> +
>> +/*
>> + * Watches
>> + */
>> +
>> +static DEFINE_WAIT_QUEUE(xenbus_watch_wq);
>> +static xenbus_watch_evlist_t xenbus_watch_evlist;
>> +
>> +/*
>> + * The split between 'xenbus_wait_watch_event_return' and
>> + * 'xenbus_wait_watch_event' was taken from Mini-OS which uses this approach
>> + * to handle the events explicitly in the TPM frontend.
>> + */
> Maybe extend this comment? It is not very clear how splitting it in two
> parts helps
I guess the safest solution here would be to simply drop this function.
We'll get back to this split only if it will be needed in the future.
>> +static struct xenbus_watch_event *
>> +xenbus_wait_watch_event_return(xenbus_watch_evlist_t *evlist)
>> +{
>> + struct xenbus_watch_event *event;
>> + DEFINE_WAIT(w);
>> +
>> + if (!evlist)
>> + evlist = &xenbus_watch_evlist;
>> +
>> + while (!(event = *evlist)) {
>> + uk_waitq_add_waiter(&xenbus_watch_wq, &w);
>> + uk_sched_yield();
>> + }
> Any event on xen bus will wake us up, because there is just a single
> xenbus_watch_wq. We will pop the event from the list anyways, even if it
> actually came from a different source.
Hum, unfortunately the observation here is not entirely correct.
Although we will be woken up on every watch event (because we're using a
single waiting queue), we will pop an event only if it was added on
*our* event list (evlist).
> There is a complex machinery in the process_watch_event(). It finds the
> element of the list, responsible for the path where the even
> happened. In the current implementation it is just wasted, because
> xenbus_wait_watch_event_return waits just for any event.
>
> Would it make sense to make use this machinery, by adding a wait queue
> to every xs_watch? Instead of waiting for allocated piece of memory sent
> from the process_watch_event (and we are not using at all this allocated
> memory btw), xenbus_wait_watch_event_return will need to find the right
> xs_watch, and sleep on its waiting queue.
>
> Another option. Before waiting, client must to create a watch point by
> calling xs_watch_path. So this function could return a queue already. I
> can not tell which one I prefer without trying to implement.. In need to
> see how it lays into the code.
Yep, it does make sense to add a waiting queue to every watch. For v2,
the xenbus watch contains a waiting queue and a counter for pending
events. Also, the event allocation was dropped. We're just searching for
watches on incoming watch events and, if any, we "notify" the found watch.
>> + uk_waitq_remove_waiter(&xenbus_watch_wq, &w);
>> +
>> + /* pop the event */
>> + *evlist = event->next;
>> +
>> + return event;
>> +}
>> +
>> +void xenbus_wait_watch_event(xenbus_watch_evlist_t *evlist)
>> +{
>> + struct xenbus_watch_event *event;
>> +
>> + if (!evlist)
>> + evlist = &xenbus_watch_evlist;
>> +
>> + event = xenbus_wait_watch_event_return(evlist);
>> + UK_ASSERT(event != NULL);
>> +
>> + uk_xb_free(event);
>> +}
>> +
>> +int xenbus_notify_watch_event(xenbus_watch_evlist_t *evlist,
>> + struct xenbus_watch_event *event)
>> +{
>> + if (evlist == NULL || event == NULL)
>> + return -EINVAL;
>> +
>> + /* add the event at the beginning of the list */
>> + event->next = *evlist;
>> + *evlist = event;
>> +
>> + uk_waitq_wake_up(&xenbus_watch_wq);
>> +
>> + return 0;
>> +}
>> +
>> +int xenbus_wait_for_value(const char *path, const char *value,
>> + xenbus_watch_evlist_t *evlist)
>> +{
>> + char *res;
>> + int rc;
>> +
>> + if (!evlist)
>> + evlist = &xenbus_watch_evlist;
>> +
>> + for (;;) {
>> + res = xs_read(XBT_NIL, path);
>> + if (PTRISERR(res))
>> + return PTR2ERR(res);
>> +
>> + rc = strcmp(value, res);
>> + uk_xb_free(res);
>> +
>> + if (rc == 0)
>> + break;
>> +
>> + xenbus_wait_watch_event(evlist);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +XenbusState xenbus_read_driver_state(const char *path)
>> +{
>> + char state_path[strlen(path) + sizeof("/state")];
>> + XenbusState state = XenbusStateUnknown;
>> +
>> + sprintf(state_path, "%s/state", path);
>> + xs_read_integer(state_path, (int *) &state);
>> +
>> + return state;
>> +}
>> +
>> +int xenbus_switch_state(struct xenbus_device *xendev, XenbusState state,
>> + xenbus_transaction_t xbt)
>> +{
>> + char state_path[strlen(xendev->nodename) + sizeof("/state")];
>> + char *current_state_str;
>> + XenbusState current_state;
>> + int xbt_flag = 0; /* non-zero if transaction started */
>> + int retry = 0;
>> + int err;
>> +
>> + sprintf(state_path, "%s/state", xendev->nodename);
>> +
>> + do {
>> + if (xbt == XBT_NIL) {
>> + err = xs_transaction_start(&xbt);
>> + if (err)
>> + goto exit;
>> + xbt_flag = 1;
> I guess xbt could be used instead of this flag? If it is zero -
> transaction is not started. In case of err, the xs_transaction_start
> does not touch the xbt so it should stay 0;
xbt_flag is set iff we need to end the transaction inside the function.
As we discussed offline, I renamed the variable to a more suggestive name.
>> + }
>> +
>> + /* check if state is already set */
>> + current_state_str = xs_read(xbt, state_path);
>> + if (PTRISERR(current_state_str)) {
>> + err = PTR2ERR(current_state_str);
>> + goto exit;
>> + }
>> +
>> + /* convert to int */
>> + current_state = (XenbusState) (current_state_str[0] - '0');
> Why atoi can't do this job?
Ack, although this code would be faster.
>> + uk_xb_free(current_state_str);
>> +
>> + if (current_state == state)
>> + /* state already set */
>> + goto exit;
>> +
>> + /* set new state */
>> + err = xs_printf(xbt, xendev->nodename, "state", "%d", state);
> Seems we do not need xs_printf. The path you already have in
> state_path. And generating a string which we are going to write is just
> a matter of allocating a small buffer on stack and sprintf-ing in it.
Right, this shouldn't be here, I forgot it in the code.
>> +
>> +exit:
>> + if (xbt_flag) {
>> + int _err;
>> +
>> + _err = xs_transaction_end(xbt, 0, &retry);
>> + if (!err)
>> + err = _err;
>> + xbt = XBT_NIL;
>> + }
>> + } while (retry);
> Shouldn't we stop trying if err is not 0? I don't really know if it is
> possible that xs_transaction_end says EAGAIN, but xs_printf returned an
> error (e.g. ENOENT).
Right. There is a problem here, we should abort if we get such errors.
And I figured out that 'retry' parameter is redundant since we can check
the error for EAGAIN.
>
> I would just iterate while(err == EAGAIN). Do you think it would be
> correct?
Yep, it's correct when dealing with the abort case mentioned above.
>> +
>> + if (err)
>> + uk_printd(DLVL_ERR, "Error switching state to %s: %d\n",
>> + xenbus_state_to_str(state), err);
>> +
>> + return err;
>> +}
>> +
>> +int xenbus_wait_for_state_change(const char *path, XenbusState *state,
>> + xenbus_watch_evlist_t *evlist)
>> +{
>> + char *current_state_str;
>> + XenbusState current_state;
>> +
>> + if (!evlist)
>> + evlist = &xenbus_watch_evlist;
>> +
>> + for (;;) {
>> + current_state_str = xs_read(XBT_NIL, path);
>> + if (PTRISERR(current_state_str))
>> + return PTR2ERR(current_state_str);
>> +
>> + /* convert to int */
>> + current_state = (XenbusState) (current_state_str[0] - '0');
> Same remark about atoi.
>
> Also would it make sense to move this into xenbus_get_state() function?
Done, I added a xenbus_intstr_to_state() function (the name was chosen
to avoid the confusion for converting from string, e.g. from
"Initialising" to 1).
>> + uk_xb_free(current_state_str);
>> +
>> + if (current_state == *state)
>> + xenbus_wait_watch_event(evlist);
> Looks like the watch on this path is not registered, so Xen would not be
> sending events on changes on this path. Or I am missing something?
This function was always called after registering a watch associated
with 'evlist'. In deed, it might be confusing and not clear enough here
about what the context should have conformed to. In v2, 'evlist' was
replaced with a xenbus watch, which may be NULL and in that case a new
watch will be created locally.
>> + else {
>> + *state = current_state;
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> diff --git a/plat/xen/xenbus/xenbus.c b/plat/xen/xenbus/xenbus.c
>> new file mode 100644
>> index 0000000..cb177e1
>> --- /dev/null
>> +++ b/plat/xen/xenbus/xenbus.c
>> @@ -0,0 +1,260 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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 <stdlib.h>
>> +#include <stdio.h>
>> +#include <inttypes.h>
>> +#include <string.h>
>> +#include <uk/essentials.h>
>> +#include <uk/list.h>
>> +#include <uk/bus.h>
>> +#include <uk/print.h>
>> +#include <uk/errptr.h>
>> +#include <uk/assert.h>
>> +#include <xenbus/xenbus.h>
>> +#include <xenbus/xs.h>
>> +#include <xenbus/client.h>
>> +#include "xs_comms.h"
>> +
>> +#define XS_DEV_PATH "device"
>> +
>> +#define FOREACH_DRIVER(drv) \
>> + UK_TAILQ_FOREACH(drv, &xbh.drv_list, next)
>> +
>> +#define FOREACH_DRIVER_SAFE(drv, drv_next) \
>> + UK_TAILQ_FOREACH_SAFE(drv, &xbh.drv_list, next, drv_next)
>> +
>> +#define FOREACH_DEVICE(dev) \
>> + UK_TAILQ_FOREACH(dev, &xbh.dev_list, ph_next)
> I would rather use UK_TAILQ_FOREACH directly. This macro does more harm
> then good.
>
> 1) It does not make the code really shorter
> 2) It hides the name of the list we are iterating. When I am reading the
> code, I need to know that. So I must search for the definition.
> 3) It is harder to grep the code to find where the list got modifyed and
> accessed
> 4) FOREACH_(DRIVER|DEVICE) already exists in pci_bus.c. Which (in this
> case) is a bit confusing. Actually I would say we do not need it in
> pci_bus.c as well.
Ack. The reasons you mentioned are reiterated in the patch changing
pci_bus.c as well.
>> +
>> +
>> +static struct xenbus_driver *xenbus_find_driver(xenbus_dev_type_t devtype)
>> +{
>> + struct xenbus_driver *drv;
>> + const xenbus_dev_type_t *pdevtype;
>> +
>> + FOREACH_DRIVER(drv) {
>> + for (pdevtype = drv->device_types;
>> + *pdevtype != xenbus_dev_none; pdevtype++) {
>> + if (*pdevtype == devtype)
>> + return drv;
>> + }
>> + }
>> +
>> + return NULL; /* no driver found */
>> +}
>> +
>> +static int xenbus_probe_device(struct xenbus_driver *drv,
>> + xenbus_dev_type_t type, const char *name)
>> +{
>> + int err;
>> + struct xenbus_device *dev;
>> + char *nodename;
>> + XenbusState state;
>> +
>> + /* device/type/name */
>> + nodename = xs_join("%s/%s/%s",
>> + XS_DEV_PATH, xenbus_devtype_to_str(type), name);
>> + if (PTRISERR(nodename)) {
>> + err = PTR2ERR(nodename);
>> + goto out;
>> + }
>> +
>> + state = xenbus_read_driver_state(nodename);
>> + if (state != XenbusStateInitialising)
>> + return 0;
>> +
>> + uk_printd(DLVL_INFO, "Xenbus device: %s\n", nodename);
>> +
>> + dev = uk_xb_calloc(1, sizeof(*dev) + strlen(nodename) + 1);
> Nodename is already allocated. Why not assign that pointer to
> drv->nodename instead? Of course this needs to be reflected in freeing
> this memory, but it seems that we do not have any :)
I actually prefer it this way because it doesn't fragment the memory.
>> + if (!dev) {
>> + uk_printd(DLVL_ERR, "Failed to initialize: Out of memory!\n");
>> + err = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + dev->state = XenbusStateInitialising;
>> + dev->devtype = type;
>> + dev->nodename = (char *) (dev + 1);
>> + strcpy(dev->nodename, nodename);
>> +
>> + err = drv->add_dev(dev);
>> + if (err) {
>> + uk_printd(DLVL_ERR, "Failed to add device.\n");
>> + uk_xb_free(dev);
>> + }
>> +
>> +out:
>> + if (!PTRISERR(nodename))
>> + uk_xb_free(nodename);
>> +
>> + return err;
>> +}
>> +
>> +static int xenbus_probe_device_type(const char *devtype_str)
>> +{
>> + struct xenbus_driver *drv;
>> + xenbus_dev_type_t devtype;
>> + char dirname[sizeof(XS_DEV_PATH) + strlen(devtype_str)];
>> + char **devices = NULL;
>> + int err = 0;
>> +
>> + devtype = xenbus_str_to_devtype(devtype_str);
>> + if (!devtype) {
>> + uk_printd(DLVL_WARN,
>> + "Unsupported device type: %s\n", devtype_str);
>> + goto out;
>> + }
>> +
>> + drv = xenbus_find_driver(devtype);
>> + if (!drv) {
>> + uk_printd(DLVL_WARN,
>> + "No driver for device type: %s\n", devtype_str);
>> + goto out;
>> + }
>> +
>> + sprintf(dirname, "%s/%s", XS_DEV_PATH, devtype_str);
>> +
>> + /* Get device list */
>> + devices = xs_ls(XBT_NIL, dirname);
>> + if (PTRISERR(devices)) {
>> + err = PTR2ERR(devices);
>> + uk_printd(DLVL_ERR,
>> + "Error reading %s devices: %d\n", devtype_str, err);
>> + goto out;
>> + }
>> +
>> + for (int i = 0; devices[i] != NULL; i++) {
>> + /* Probe only if no prior error */
>> + if (err == 0)
>> + err = xenbus_probe_device(drv, devtype, devices[i]);
>> +
>> + uk_xb_free(devices[i]);
>> + }
>> +
>> +out:
>> + if (!PTRISERR(devices))
>> + uk_xb_free(devices);
>> +
>> + return err;
>> +}
>> +
>> +static int xenbus_probe(void)
>> +{
>> + char **devtypes;
>> + int err = 0;
>> +
>> + uk_printd(DLVL_INFO, "Probe Xenbus\n");
>> +
>> + /* Get device types list */
>> + devtypes = xs_ls(XBT_NIL, XS_DEV_PATH);
>> + if (PTRISERR(devtypes)) {
>> + err = PTR2ERR(devtypes);
>> + uk_printd(DLVL_ERR, "Error reading device types: %d\n", err);
>> + goto out;
>> + }
>> +
>> + for (int i = 0; devtypes[i] != NULL; i++) {
>> + /* Probe only if no previous error */
>> + if (err == 0)
>> + err = xenbus_probe_device_type(devtypes[i]);
>> +
>> + uk_xb_free(devtypes[i]);
>> + }
>> +
>> +out:
>> + if (!PTRISERR(devtypes))
>> + uk_xb_free(devtypes);
>> +
>> + return err;
>> +}
>> +
>> +static int xenbus_init(struct uk_alloc *a)
>> +{
>> + struct xenbus_driver *drv, *drv_next;
>> + int ret = 0;
>> +
>> + UK_ASSERT(a != NULL);
>> +
>> + xbh.a = a;
>> +
>> + ret = xs_comms_init();
>> + if (ret) {
>> + uk_printd(DLVL_ERR,
>> + "Error initializing Xenstore communication.");
>> + return ret;
>> + }
>> +
>> + if (!xbh.drv_list_initialized) {
>> + UK_TAILQ_INIT(&xbh.drv_list);
>> + xbh.drv_list_initialized = 1;
>> + }
> Let's use UK_TAILQ_HEAD_INITIALIZER instead (see below). So this
> xbh.drv_list_initialized is not needed.
Fixed. Maybe we should update pci_bus.c too in this case.
>> + UK_TAILQ_INIT(&xbh.dev_list);
>> +
>> + FOREACH_DRIVER_SAFE(drv, drv_next) {
>> + if (drv->init) {
>> + ret = drv->init(a);
>> + if (ret == 0)
>> + continue;
>> + uk_printd(DLVL_ERR,
>> + "Failed to initialize driver %p: %d\n",
>> + drv, ret);
>> + UK_TAILQ_REMOVE(&xbh.drv_list, drv, next);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void _xenbus_register_driver(struct xenbus_driver *drv)
>> +{
>> + UK_ASSERT(drv != NULL);
>> +
>> + if (!xbh.drv_list_initialized) {
>> + UK_TAILQ_INIT(&xbh.drv_list);
>> + xbh.drv_list_initialized = 1;
>> + }
>> +
>> + UK_TAILQ_INSERT_TAIL(&xbh.drv_list, drv, next);
>> +}
>> +
>> +/*
>> + * Register this bus driver to libukbus:
>> + */
>> +struct xenbus_handler xbh = {
>> + .b.init = xenbus_init,
>> + .b.probe = xenbus_probe
>> +};
> Let's statically initialize the lists here as well.
>
> Also it is a good idea to put comma after the last element too. Even
> though it is not required, it makes future patches prettier. If you add
> more elements into this initialization - the last line stays unchanged
> in the patch.
>
> Here is an example:
>
> struct xenbus_handler xbh = {
> .b.init = xenbus_init,
> .b.probe = xenbus_probe,
> .drv_list = UK_TAILQ_HEAD_INITIALIZER(xbh.drv_list),
> .dev_list = UK_TAILQ_HEAD_INITIALIZER(xbh.dev_list),
> };
Fixed.
>> +
>> +UK_BUS_REGISTER(&xbh.b);
>> diff --git a/plat/xen/xenbus/xs.c b/plat/xen/xenbus/xs.c
>> new file mode 100644
>> index 0000000..ca9f6a2
>> --- /dev/null
>> +++ b/plat/xen/xenbus/xs.c
>> @@ -0,0 +1,518 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Steven Smith (sos22@xxxxxxxxx)
>> + * Grzegorz Milos (gm281@xxxxxxxxx)
>> + * John D. Ramsdell
>> + * Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2006, Cambridge University
>> + * 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.
>> + */
>> +/*
>> + * Ported from Mini-OS xenbus.c
>> + */
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <stdarg.h>
>> +#include <uk/errptr.h>
>> +#include <xen/io/xs_wire.h>
>> +#include <xenbus/xs.h>
>> +#include "xs_comms.h"
>> +#include "xs_watch.h"
>> +
>> +
>> +static char *vjoin(const char *fmt, va_list ap)
>> +{
>> + char *path;
>> + unsigned int path_len;
>> + va_list aq;
>> +
>> + /* figure out the path length */
>> + va_copy(aq, ap);
>> + path_len = vsnprintf(NULL, 0, fmt, aq);
>> + va_end(aq);
>> +
>> + path = uk_xb_malloc(path_len + 1);
>> + if (!path)
>> + return ERR2PTR(ENOMEM);
>> +
>> + vsnprintf(path, path_len + 1, fmt, ap);
>> +
>> + return path;
>> +}
>> +
>> +char *xs_join(const char *fmt, ...)
>> +{
>> + char *ret;
>> + va_list ap;
>> +
>> + if (fmt == NULL)
>> + return ERR2PTR(EINVAL);
>> +
>> + va_start(ap, fmt);
>> + ret = vjoin(fmt, ap);
>> + va_end(ap);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Converts a Xenstore reply error to a positive error number.
>> + * Returns 0 if the reply is successful.
>> + */
>> +static int reply_to_errno(struct xsd_sockmsg *rep)
>> +{
>> + int err = 0;
>> + char *errstring;
>> +
>> + if (PTRISERR(rep)) {
>> + err = PTR2ERR(rep);
>> + goto out;
>> + }
>> +
>> + if (rep->type != XS_ERROR)
>> + goto out;
>> +
>> + errstring = (char *) (rep + 1);
>> +
>> + for (int i = 0; i < (int) ARRAY_SIZE(xsd_errors); i++) {
>> + if (!strcmp(errstring, xsd_errors[i].errstring)) {
>> + err = xsd_errors[i].errnum;
>> + goto out;
>> + }
>> + }
>> +
>> + uk_printd(DLVL_WARN, "Unknown Xenstore error: %s\n", errstring);
>> + err = EINVAL;
>> +
>> +out:
>> + return err;
>> +}
>> +
>> +/* Common function used for sending requests when replies aren't handled */
>> +static int xs_msg(enum xsd_sockmsg_type type, xenbus_transaction_t xbt,
>> + struct xs_req *req, int req_num)
>> +{
>> + struct xsd_sockmsg *rep;
>> + int err;
>> +
>> + rep = xs_msg_reply(type, xbt, req, req_num);
>> + err = -reply_to_errno(rep);
>> +
>> + uk_xb_free(rep);
>> +
>> + return err;
>> +}
>> +
>> +char *xs_read(xenbus_transaction_t xbt, const char *path)
>> +{
>> + struct xs_req req;
>> + struct xsd_sockmsg *rep;
>> + char *value;
>> + int err;
>> +
>> + if (path == NULL)
>> + return ERR2PTR(EINVAL);
>> +
>> + req = XS_REQ_STR_NULL(path);
>> + rep = xs_msg_reply(XS_READ, xbt, &req, 1);
>> + err = reply_to_errno(rep);
>> + if (err) {
>> + value = ERR2PTR(err);
>> + goto out;
>> + }
>> +
>> + value = uk_xb_malloc(rep->len + 1);
>> + if (!value) {
>> + value = ERR2PTR(ENOMEM);
>> + goto out;
>> + }
>> +
>> + memcpy(value, rep + 1, rep->len);
> This would be the third memcpy and the second memory allocation for the
> same data. At least we could replace it with memmove. This will be a bit
> slower on coping, but I guess the overhead of allocating and freeing is
> still bigger.
>
> Anyways, I would like to propose more radical measures here.
>
> 1) It seams that the only thing we need from struct xs_req at this level
> is length. I would extend "struct reqid_map_value" with "struct
> xs_req". So it will come preallocated. We just need to call
> memcpy_from_ring 2 times in the process_reply() function to fill the
> header and actual message.
>
> This will allow us to return pointer to the data directly from
> xs_msg_reply(). But we going to have to add another parameter to let
> the caller get the length of the message.
>
Alright. In v2 I chose to use a 'xs_iovec' and return the error number,
but the idea is the same.
> 2) We can't escape calling reply_to_errno - we have to check if XEN
> complains. I would call it from xs_msg_reply - a bit less troubles
> for the caller.
>
Done
> 3) I guess we also need to check the type of reply message. Looks like
> it has to have the same type as the request message. The xs_msg_reply
> function is a good place for this check as well
>
Done
> This will eliminate some duplicated code, one allocation and one memcpy.
>> + value[rep->len] = 0;
>> +
>> +out:
> You need to check if rep is not error (if (!PTRISERR(rep)), otherwise
> uk_xb_free will fail
In v2 I dropped this.
>> + uk_xb_free(rep);
>> +
>> + return value;
>> +}
>> +
>> +int xs_write(xenbus_transaction_t xbt,
>> + const char *path, const char *value)
>> +{
>> + struct xs_req req[2];
>> +
>> + if (path == NULL || value == NULL)
>> + return -EINVAL;
>> +
>> + req[0] = XS_REQ_STR_NULL(path);
>> + req[1] = XS_REQ_STR(value);
> Will it work if the second req will be null-terminated as well? Just a
> bit more consistent code. And it would let us to get rid of XS_REQ_STR
> at all. The same thing about xs_debug_msg.
Fixed. Yep, it works.
>> +
>> + return xs_msg(XS_WRITE, xbt, req, ARRAY_SIZE(req));
>> +}
>> +
>> +char **xs_ls(xenbus_transaction_t xbt, const char *path)
>> +{
>> + struct xs_req req;
>> + struct xsd_sockmsg *rep;
>> + int nr_elems, offs, i;
>> + char *rep_values, **res = NULL;
>> + int err;
>> +
>> + if (path == NULL)
>> + return ERR2PTR(EINVAL);
>> +
>> + req = XS_REQ_STR_NULL(path);
>> + rep = xs_msg_reply(XS_DIRECTORY, xbt, &req, 1);
>> + err = reply_to_errno(rep);
>> + if (err)
>> + goto out_err;
>> +
>> + rep_values = (char *) (rep + 1);
>> +
>> + for (offs = nr_elems = 0; offs < (int) rep->len; offs++)
>> + nr_elems += (rep_values[offs] == 0);
>> +
>> + res = uk_xb_calloc(nr_elems + 1, sizeof(res[0]));
>> + if (!res) {
>> + err = ENOMEM;
>> + goto out_err;
>> + }
> Continuing a comment for xs_read.
>
> In this case you would need to allocate an array, as you doing it right
> now. But fill it with pointers to the original piece of memory (where we
> are storing data copied from the ring buffer) instead of allocating new
> elements every time.
>
> Tricky part is freeing. Obviously user would need just to issue a free
> call to the first pointer, but that is somewhat confusing (However not
> more confusing then allocating more space for a structure to keep a
> string too). I would propose to create a function for that (for example
> xs_free_list).
Alright. I think the best solution would be to combine your suggestions
with the way Linux is doing it: allocating a big chunk of memory for
both the array and the strings. This way, the caller would only have to
call free on the array.
>> +
>> + for (offs = i = 0; i < nr_elems; i++) {
>> + char *elem = rep_values + offs;
>> + int elem_len = strlen(elem);
>> +
>> + res[i] = uk_xb_malloc(elem_len + 1);
>> + if (!res[i]) {
>> + err = ENOMEM;
>> + goto out_err;
>> + }
>> +
>> + memcpy(res[i], elem, elem_len + 1);
>> +
>> + offs += elem_len + 1;
>> + }
>> +
>> + uk_xb_free(rep);
>> +
>> + return res;
>> +
>> +out_err:
>> + if (!PTRISERR(res)) {
>> + for (i = 0; i < nr_elems; i++) {
>> + if (res[i])
>> + uk_xb_free(res[i]);
>> + }
>> + uk_xb_free(res);
>> + }
>> + uk_xb_free(rep);
>> +
>> + return ERR2PTR(err);
>> +}
>> +
>> +int xs_rm(xenbus_transaction_t xbt, const char *path)
>> +{
>> + struct xs_req req;
>> +
>> + if (path == NULL)
>> + return -EINVAL;
>> +
>> + req = XS_REQ_STR_NULL(path);
>> +
>> + return xs_msg(XS_RM, xbt, &req, 1);
>> +}
>> +
>> +char *xs_get_perms(xenbus_transaction_t xbt, const char *path)
>> +{
>> + struct xs_req req;
>> + struct xsd_sockmsg *rep;
>> + char *value;
>> + int err;
>> +
>> + if (path == NULL)
>> + return ERR2PTR(EINVAL);
>> +
>> + req = XS_REQ_STR_NULL(path);
>> + rep = xs_msg_reply(XS_GET_PERMS, xbt, &req, 1);
>> + err = reply_to_errno(rep);
>> + if (err) {
>> + value = ERR2PTR(err);
>> + goto out;
>> + }
>> +
>> + value = uk_xb_malloc(rep->len + 1);
>> + if (!value) {
>> + value = ERR2PTR(ENOMEM);
>> + goto out;
>> + }
>> +
>> + memcpy(value, rep + 1, rep->len);
>> + value[rep->len] = 0;
>> +
>> +out:
>> + uk_xb_free(rep);
>> +
>> + return value;
>> +}
>> +
>> +#define PERM_MAX_SIZE 32
>> +int xs_set_perms(xenbus_transaction_t xbt, const char *path,
>> + domid_t domid, char perm)
>> +{
>> + char value[PERM_MAX_SIZE];
>> + struct xs_req req[2];
>> +
>> + if (path == NULL)
>> + return -EINVAL;
>> +
>> + req[0] = XS_REQ_STR_NULL(path);
>> +
>> + snprintf(value, PERM_MAX_SIZE, "%c%hu", perm, domid);
>> + req[1].data = value;
>> + req[1].len = strlen(value) + 1;
>> +
>> + return xs_msg(XS_SET_PERMS, xbt, req, ARRAY_SIZE(req));
>> +}
>> +
>> +int xs_transaction_start(xenbus_transaction_t *xbt)
>> +{
>> + /*
>> + * xenstored becomes angry if you send a length 0 message,
>> + * so just shove a nul terminator on the end
>> + */
>> + struct xs_req req;
>> + struct xsd_sockmsg *rep;
>> + int err;
>> +
>> + if (xbt == NULL)
>> + return -EINVAL;
>> +
>> + req = XS_REQ_STR_NULL("");
>> + rep = xs_msg_reply(XS_TRANSACTION_START, 0, &req, 1);
>> + err = -reply_to_errno(rep);
>> + if (err)
>> + goto out;
>> +
>> + *xbt = strtoul((char *) (rep + 1), NULL, 10);
>> +
>> +out:
>> + uk_xb_free(rep);
>> +
>> + return err;
>> +}
>> +
>> +int xs_transaction_end(xenbus_transaction_t xbt, int abort, int *retry)
>> +{
>> + struct xs_req req;
>> + int err;
>> +
>> + if (retry == NULL)
>> + return -EINVAL;
>> +
>> + req.data = abort ? "F" : "T";
>> + req.len = 2;
>> +
>> + err = xs_msg(XS_TRANSACTION_END, xbt, &req, 1);
>> +
>> + *retry = (err == EAGAIN) ? 1 : 0;
> I would rather return error code as it is. The caller needs to check
> what we put in retry anyways. So it should be equal amount of work for
> him to check if err==EAGAIN or retry==1
Fixed. As mentioned somewhere above, it makes more sense to drop the
retry parameter.
>> +
>> + return err;
>> +}
>> +
>> +/* Send a debug message to xenbus. Can block. */
>> +int xs_debug_msg(const char *msg)
>> +{
>> + struct xs_req req[3];
>> + struct xsd_sockmsg *rep;
>> + int err;
>> +
>> + if (msg == NULL)
>> + return -EINVAL;
>> +
>> + req[0] = XS_REQ_STR_NULL("print");
>> + req[1] = XS_REQ_STR(msg);
>> + req[2] = XS_REQ_STR_NULL("");
>> +
>> + rep = xs_msg_reply(XS_DEBUG, XBT_NIL, req, ARRAY_SIZE(req));
>> + err = -reply_to_errno(rep);
>> + if (err)
>> + goto out;
>> +
>> + uk_printd(DLVL_EXTRA,
>> + "Got a reply, type %"__PRIu32", id %"__PRIu32", len
>> %"__PRIu32".\n",
>> + rep->type, rep->req_id, rep->len);
>> +
>> +out:
>> + uk_xb_free(rep);
>> +
>> + return err;
>> +}
>> +
>> +int xs_read_integer(const char *path, int *value)
>> +{
>> + char *value_str;
>> +
>> + if (path == NULL || value == NULL)
>> + return -EINVAL;
>> +
>> + value_str = xs_read(XBT_NIL, path);
>> + if (PTRISERR(value_str))
>> + return PTR2ERR(value_str);
>> +
>> + *value = atoi(value_str);
>> +
>> + uk_xb_free(value_str);
>> +
>> + return 0;
>> +}
>> +
>> +char *xs_readf(xenbus_transaction_t xbt, const char *fmt, ...)
>> +{
>> + char *fullpath;
>> + char *val;
>> + va_list args;
>> +
>> + if (fmt == NULL)
>> + return ERR2PTR(EINVAL);
>> +
>> + va_start(args, fmt);
>> + fullpath = vjoin(fmt, args);
>> + va_end(args);
>> +
>> + if (PTRISERR(fullpath))
>> + return fullpath;
>> +
>> + val = xs_read(xbt, fullpath);
>> +
>> + uk_xb_free(fullpath);
>> +
>> + return val;
>> +}
>> +
>> +int xs_printf(xenbus_transaction_t xbt,
>> + const char *node, const char *path, const char *fmt, ...)
>> +{
>> +#define BUFFER_SIZE 256
>> + char fullpath[BUFFER_SIZE];
>> + char val[BUFFER_SIZE];
>> + va_list args;
>> + int err;
>> +
>> + if (node == NULL || path == NULL || fmt == NULL)
>> + return -EINVAL;
>> +
>> + if (strlen(node) + strlen(path) + 1 >= BUFFER_SIZE)
>> + return -ENOMEM;
>> +
>> + sprintf(fullpath, "%s/%s", node, path);
>> +
>> + va_start(args, fmt);
>> + vsprintf(val, fmt, args);
>> + va_end(args);
>> +
>> + err = xs_write(xbt, fullpath, val);
>> +
>> + return err;
>> +}
> 1) xs_readf and xs_printf parameters are quite confusing. For the first
> the "fmt" is used for generating the path. While for the second it is
> used for generating an actual message
You're right about the confusion. I replaced 'xs_readf' with 'xs_scanf'
which works like a 'sscanf' (thanks for the patch!) on an entry in
Xenstore (just like 'xs_printf' works as 'sprintf').
> 2) I think we do not really need them. Normally it is just a matter of
> allocating a buffer on stack and sprintf-ing in it (as I said
> before). And that is exactly what you did in this patch. The only
> place where xs_printf was used is questionable (as I commented
> above). So maybe we kill these two?
The current file, xs.c, proposes an API for Xenstore access. I expect
the API will be used by Unikraft applications as well in the future
(think of ClickOS which accessed the Xenstore in its glue code by
calling the Xenstore API of MiniOS).
>> +
>> +domid_t xs_get_self_id(void)
>> +{
>> + char *domid_str;
>> + domid_t domid;
>> +
>> + domid_str = xs_read(XBT_NIL, "domid");
>> + if (PTRISERR(domid_str))
>> + UK_CRASH("Error reading domain id.");
>> +
>> + domid = (domid_t) strtoul(domid_str, NULL, 10);
>> +
>> + uk_xb_free(domid_str);
>> +
>> + return domid;
>> +}
>> +
>> +int xs_watch_path_token(xenbus_transaction_t xbt,
>> + const char *path, const char *token,
>> + xenbus_watch_evlist_t *events)
>> +{
>> + struct xs_watch *watch;
>> + struct xs_req req[2];
>> +
>> + if (path == NULL || token == NULL || events == NULL)
>> + return -EINVAL;
>> +
>> + watch = xs_watch_create(path, token, events);
>> + if (PTRISERR(watch))
>> + return PTR2ERR(watch);
>> +
>> + req[0] = XS_REQ_STR_NULL(path);
>> + req[1] = XS_REQ_STR_NULL(token);
>> +
>> + return xs_msg(XS_WATCH, xbt, req, ARRAY_SIZE(req));
>> +}
>> +
>> +int xs_unwatch_path_token(xenbus_transaction_t xbt,
>> + const char *path, const char *token)
>> +{
>> + struct xs_req req[2];
>> + int err;
>> +
>> + if (path == NULL || token == NULL)
>> + return -EINVAL;
>> +
>> + req[0] = XS_REQ_STR_NULL(path);
>> + req[1] = XS_REQ_STR_NULL(token);
>> +
>> + err = xs_msg(XS_UNWATCH, xbt, req, ARRAY_SIZE(req));
>> + if (err)
>> + goto out;
>> +
>> + err = xs_watch_destroy(path, token);
>> +
>> +out:
>> + return err;
>> +}
>> +
>> +#define GLOBAL_XS_WATCH_TOKEN "global_xenstore_watch"
>> +static xenbus_watch_evlist_t global_xs_watch_evlist;
>> +
>> +int xs_watch_path(xenbus_transaction_t xbt, const char *path)
>> +{
>> + return xs_watch_path_token(xbt, path, GLOBAL_XS_WATCH_TOKEN,
>> + &global_xs_watch_evlist);
>> +}
>> +
>> +int xs_unwatch_path(xenbus_transaction_t xbt, const char *path)
>> +{
>> + return xs_unwatch_path_token(xbt, path, GLOBAL_XS_WATCH_TOKEN);
>> +}
>> diff --git a/plat/xen/xenbus/xs_comms.c b/plat/xen/xenbus/xs_comms.c
>> new file mode 100644
>> index 0000000..80c2bdf
>> --- /dev/null
>> +++ b/plat/xen/xenbus/xs_comms.c
>> @@ -0,0 +1,484 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Steven Smith (sos22@xxxxxxxxx)
>> + * Grzegorz Milos (gm281@xxxxxxxxx)
>> + * John D. Ramsdell
>> + * Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2006, Cambridge University
>> + * 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.
>> + */
>> +/*
>> + * Communication with Xenstore
>> + * Ported from Mini-OS xenbus.c
>> + */
>> +
>> +#include <string.h>
>> +#include <uk/errptr.h>
>> +#include <uk/wait.h>
>> +#include <uk/arch/spinlock.h>
>> +#include <common/events.h>
>> +#include <xen-x86/mm.h>
>> +#include <xen-x86/setup.h>
>> +#include "xs_comms.h"
>> +#include "xs_watch.h"
>> +
>> +
>> +/*
>> + * Xenstore handler structure
>> + */
>> +struct xs_handler {
>> + /**< Non-zero if initialized */
>> + int initialized;
>> + /**< Communication: event channel */
>> + evtchn_port_t evtchn;
>> + /**< Communication: shared memory */
>> + struct xenstore_domain_interface *buf;
>> + /**< Thread processing incoming xs replies */
>> + struct uk_thread *thread;
>> + /**< Waiting queue for notifying incoming xs replies */
>> + struct uk_waitq waitq;
>> +};
>> +
>> +static struct xs_handler xsh;
>> +
>> +
>> +struct reqid_map_value {
>> + /**< Non-zero if used and waiting for reply */
>> + int in_use;
>> + /**< Waiting queue for incoming reply notification */
>> + struct uk_waitq waitq;
>> + /**< Received reply */
>> + void *reply;
>> +};
> I think the name "xs_request" reflects the purpose better. This is
> a structure which represents a request which is currently in flight.
Done.
>> +
>> +/*
>> + * Structure for mapping in-flight requests IDs to incoming replies.
>> + * Request IDs are reused, hence the limited set of ID values.
>> + */
>> +struct reqid_map {
>> + /**< Number of live requests */
>> + __u32 num_live;
>> + /**< Current available request ID */
>> + __u32 probe;
>> + /**< Lock */
>> + spinlock_t lock;
>> + /**< Waiting queue for 'not-full' notifications */
>> + struct uk_waitq waitq;
>> +
>> + /* Map size is power of 2 */
>> +#define REQID_MAP_SHIFT 5
>> +#define REQID_MAP_SIZE (1 << REQID_MAP_SHIFT)
>> + /**< Device bus */
>> + struct reqid_map_value values[REQID_MAP_SIZE];
>> +};
>> +
>> +static struct reqid_map reqid_map;
>> +
>> +static void reqid_map_init(struct reqid_map *reqidm)
>> +{
>> + reqidm->num_live = 0;
>> + reqidm->probe = 0;
>> + ukarch_spin_lock_init(&reqidm->lock);
>> + uk_waitq_init(&reqidm->waitq);
>> +}
>> +
>> +/*
>> + * Allocate an identifier for a Xenstore request.
>> + * Blocks if none are available.
>> + */
>> +static int reqid_map_get_id(void)
> Would you like naming it xs_reqid_get() and the xs_reqid_put() for its
> pair function?
Done, it's 'xs_request_{get,put}' and it gets/puts the request entry itself.
>> +{
>> + struct reqid_map_value *reqid_map_val;
>> + __u32 probe;
>> +
>> + /* wait for an available entry */
>> + while (1) {
>> + ukarch_spin_lock(&reqid_map.lock);
>> +
>> + if (reqid_map.num_live < REQID_MAP_SIZE)
>> + break;
> That is not too efficient.. What do you think about using bitmaps here
> instead. I know there are no bitmap operations in Unikraft
> currently. But let's borrow FreeBSD implementation of linux bitops. It
> is in the file sys/compat/linuxkpi/common/include/linux/bitops.h
>
> I is basically same nice functionality, re-implemented under the BSD
> license.
As we clarified offline, this comment is about the next loop (below). I
submitted a patch for bitmaps and updated this logic based on that.
> Another thing, it would be great if id's were allocated
> sequentially. This is good from performance and debug point of view.
As we discussed offline, we'll leave this for future work. I added a
TODO comment.
>> +
>> + ukarch_spin_unlock(&reqid_map.lock);
>> +
>> + uk_waitq_wait_event(&reqid_map.waitq,
>> + (reqid_map.num_live < REQID_MAP_SIZE));
>> + }
>> +
>> + /* find an available entry */
>> + probe = reqid_map.probe;
>> + while (1) {
>> + reqid_map_val = &reqid_map.values[probe];
>> +
>> + if (!reqid_map_val->in_use)
>> + break;
>> +
>> + probe = (probe + 1) & ~REQID_MAP_SIZE;
>> + /*
>> + * The request IDs set must be big enough to hold the
>> + * maximum number of in-flight Xenstore requests.
>> + */
>> + UK_ASSERT(probe != reqid_map.probe);
>> + }
>> +
>> + reqid_map_val->in_use = 1;
>> + reqid_map.num_live++;
>> + reqid_map.probe = (probe + 1) & ~REQID_MAP_SIZE;
>> +
>> + ukarch_spin_unlock(&reqid_map.lock);
>> +
>> + uk_waitq_init(&reqid_map_val->waitq);
>> +
>> + return probe;
>> +}
>> +
>
> We have to be careful with this function. This will work as expected in
> current code, but I would write a comment that the user has to make sure
> that he is done with the request id before calling
> reqid_map_put_id(). Otherwise xs_thread_func might try to access the
> request which does not exist anymore.
This should not be a problem anymore since the 'xs_request_{get,put}'
functions use the request entries (and not IDs).
>> +/* Release a request identifier */
>> +static void reqid_map_put_id(int id)
>> +{
>> + struct reqid_map_value *reqid_map_val = &reqid_map.values[id];
>> +
>> + UK_ASSERT(reqid_map_val->in_use);
>> +
>> + ukarch_spin_lock(&reqid_map.lock);
>> +
>> + reqid_map_val->in_use = 0;
>> + reqid_map_val->reply = NULL;
>> + reqid_map.num_live--;
>> + reqid_map.probe = id;
>> +
>> + if (reqid_map.num_live == 0 || reqid_map.num_live == REQID_MAP_SIZE - 1)
>> + uk_waitq_wake_up(&reqid_map.waitq);
>> +
>> + ukarch_spin_unlock(&reqid_map.lock);
>> +}
>> +
>> +static int xs_avail_space_for_read(unsigned int req_size)
>> +{
>> + return (xsh.buf->rsp_prod - xsh.buf->rsp_cons >= req_size);
>> +}
>> +
>> +static int xs_avail_space_for_write(unsigned int req_size)
>> +{
>> + return (xsh.buf->req_prod - xsh.buf->req_cons +
>> + req_size <= XENSTORE_RING_SIZE);
>> +}
>> +
>> +static void memcpy_from_ring(const char *ring, char *dest, int off, int len)
>> +{
>> + int c1, c2;
>> +
>> + c1 = MIN(len, XENSTORE_RING_SIZE - off);
>> + c2 = len - c1;
>> +
>> + memcpy(dest, ring + off, c1);
>> + memcpy(dest + c1, ring, c2);
>> +}
>> +
>
> Would it make sense to run a thread for writing as well? For better
> batching of requests. Or we do not expect a high load of requests?
>
> This is fine in current version, but a TODO comment would not hurt.
For now we don't need that level of performance. I added the TODO comment.
>> +/*
>> + * Send data to Xenstore. This can block. All of the requests are seen
>> + * by Xenstore as if sent atomically.
>> + */
>> +static void xs_msg_write(struct xsd_sockmsg *xsd_req, struct xs_req *req)
>> +{
>> + XENSTORE_RING_IDX prod;
>> + const struct xs_req *crnt_req;
>> + struct xs_req req_hdr;
>> + unsigned int req_size, req_off;
>> + unsigned int buf_off;
>> + unsigned int this_chunk;
> The name is a bit misleading. Could you rename it this_chunk_len?
Done.
>> + int rc;
>> +
>> + req_size = sizeof(*xsd_req) + xsd_req->len;
>> + UK_ASSERT(req_size <= XENSTORE_RING_SIZE);
> UK_ASSERT is not the right choice for sure, because this condition needs
> checking in production builds too.
Done, returning ENOSPC in case of unfulfilled condition.
>> +
>> + req_hdr.data = xsd_req;
>> + req_hdr.len = sizeof(*xsd_req);
> Please put a few words of comment here, saying that we are going to send
> a batch of requests under one message header. And we turn this message
> header into the first request in the batch.
Done. The comment was adapted according to the latest changes.
>> +
>> + crnt_req = &req_hdr;
>> +
>> + /*
>> + * Wait for the ring to drain to the point where
>> + * we can send the message.
>> + */
>> + if (!xs_avail_space_for_write(req_size)) {
>> + /* Wait for there to be space on the ring */
>> + DBGXB("prod %d, len %d, cons %d, size %d; waiting.\n",
>> + prod, req_size, buf->req_cons, XENSTORE_RING_SIZE);
>> +
>> + uk_waitq_wait_event(&xsh.waitq,
>> + xs_avail_space_for_write(req_size));
>> + DBGXB("Back from wait.\n");
>> + }
>> +
>> + /*
>> + * We're now guaranteed to be able to send the message
>> + * without overflowing the ring. Do so.
>> + */
> Barrier?
Fixed.
>> +
>> + prod = xsh.buf->req_prod;
>> + req_off = 0;
>> + buf_off = 0;
>> + while (req_off < req_size) {
>> + this_chunk = MIN(crnt_req->len - buf_off,
>> + XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod));
>> +
>> + memcpy(
>> + (char *) xsh.buf->req + MASK_XENSTORE_IDX(prod),
>> + (char *) crnt_req->data + buf_off,
>> + this_chunk
>> + );
>> +
>> + prod += this_chunk;
>> + req_off += this_chunk;
>> + buf_off += this_chunk;
>> +
>> + if (buf_off == crnt_req->len) {
>> + buf_off = 0;
>> + if (crnt_req == &req_hdr)
>> + crnt_req = req;
>> + else
>> + crnt_req++;
>> + }
>> + }
>> +
>> + DBGXB("Complete main loop of xb_write.\n");
>> + UK_ASSERT(buf_off == 0);
>> + UK_ASSERT(req_off == req_size);
>> + UK_ASSERT(prod <= xsh.buf->req_cons + XENSTORE_RING_SIZE);
>> +
>> + /* Remote must see entire message before updating indexes */
>> + wmb();
>> +
>> + xsh.buf->req_prod += req_size;
>> +
>> + /* Send evtchn to notify remote */
>> + rc = notify_remote_via_evtchn(xsh.evtchn);
>> + UK_ASSERT(rc == 0);
>> +}
>> +
>> +struct xsd_sockmsg *xs_msg_reply(enum xsd_sockmsg_type msg_type,
>> + xenbus_transaction_t xbt,
>> + struct xs_req *req, int req_num)
> I would rename "req" to req_vectors, or similar. Because in fact it is
> not a single request, but a bunch of requests.
Fixed.
>> +{
>> + __u32 reqid;
>> + struct reqid_map_value *reqid_map_val;
> And reqid_map_val is what actually is a request. I think the name
> "xs_request" is better here. It is much easier to focus on what is
> important when you read the code, if you don't have to look it up what
> is reqid_map_val supposed to mean
Done.
>> + struct xsd_sockmsg xsd_req;
>> + struct xsd_sockmsg *xsd_rep;
> This is not as important as xs_req and reqid_map_value. I would propose
> to call this 2 guys xsd_req_msg and xsd_rep_msg;
Fixed.
>> +
>> + /* get a request id */
>> + reqid = reqid_map_get_id();
>> + reqid_map_val = &reqid_map.values[reqid];
>> +
>> + xsd_req.type = msg_type;
>> + xsd_req.req_id = reqid;
>> + xsd_req.tx_id = xbt;
>> + xsd_req.len = 0;
>> + for (int i = 0; i < req_num; i++)
>> + xsd_req.len += req[i].len;
>> +
>> + /* send the request */
>> + xs_msg_write(&xsd_req, req);
>> +
>> + /* wait reply */
>> + uk_waitq_wait_event(&reqid_map_val->waitq,
>> + reqid_map_val->reply != NULL);
>> +
>> + xsd_rep = reqid_map_val->reply;
>> + UK_ASSERT(xsd_rep->req_id == reqid);
>> +
>> + /* free request id */
>> + reqid_map_put_id(reqid);
>> +
>> + return xsd_rep;
>> +}
>> +
>> +/* Process an incoming xs reply */
>> +static void process_reply(struct xsd_sockmsg *msg)
>> +{
>> + struct reqid_map_value *req_map_val = &reqid_map.values[msg->req_id];
>> + int msg_size = sizeof(*msg) + msg->len;
>> +
>> + req_map_val->reply = uk_xb_malloc(msg_size);
>> + if (req_map_val->reply == NULL) {
>> + uk_printd(DLVL_ERR,
>> + "No memory available for saving Xenstore reply!");
> I think DLVL_WARN is better here. This case is pretty safe. If nomem
> happens, we are not increasing req_cons, and will try again on the next
> iteration. Hopefully by that time someone will release a bit of memory.
>
> Ideally some sort of rate_limit_print is needed here. But we don't have
> this that yet.
Fixed.
>> + return;
>> + }
>> +
>> + memcpy_from_ring(
>> + xsh.buf->rsp,
>> + req_map_val->reply,
>> + MASK_XENSTORE_IDX(xsh.buf->rsp_cons),
>> + msg_size
>> + );
>> +
> I believe we need a barrier here too
Fixed.
>> + xsh.buf->rsp_cons += msg_size;
>> +
>> + /* notify waiting requester */
>> + uk_waitq_wake_up(&req_map_val->waitq);
> Aren't we supposed to notify XEN about rsp_cons is changed?
Done, I forgot it. I changed the code a bit here; I added the
'xs_msg_read' function which copies the message payload and calls
'process_reply'/'process_watch_event'.
>> +}
>> +
>> +/* Process an incoming xs watch event */
>> +static void process_watch_event(struct xsd_sockmsg *msg)
>> +{
>> + struct xs_watch_event *event;
>> + char *data;
>> + int err;
>> +
>> + event = uk_xb_malloc(sizeof(*event) + msg->len);
>> + if (event == NULL) {
>> + uk_printd(DLVL_ERR,
>> + "No memory available for saving Xenstore watch
>> notification info!");
>> + return;
>> + }
> Same thing about DLVL_WARN
Fixed.
>> +
>> + data = (char *) event + sizeof(*event);
>> +
>> + memcpy_from_ring(
>> + xsh.buf->rsp,
>> + data,
>> + MASK_XENSTORE_IDX(xsh.buf->rsp_cons + sizeof(*msg)),
>> + msg->len
>> + );
> Barrier?
Fixed.
>> + xsh.buf->rsp_cons += sizeof(*msg) + msg->len;
>> +
>> + event->xs.path = data;
>> + event->xs.token = data + strlen(data) + 1;
>> +
>> + err = xs_watch_notify(event);
>> + if (err) {
>> + uk_printd(DLVL_ERR, "Invalid watch event.");
>> + uk_xb_free(event);
>> + }
>> +}
>> +
>> +static void xs_thread_func(void *ign __unused)
>> +{
>> + struct xsd_sockmsg msg;
>> + XENSTORE_RING_IDX prod = xsh.buf->rsp_prod;
>> +
>> + for (;;) {
>> + /* wait for incoming xs response */
>> + uk_waitq_wait_event(&xsh.waitq, prod != xsh.buf->rsp_prod);
>> +
>> + while (1) {
>> + prod = xsh.buf->rsp_prod;
>> +
>> + DBGXB("Rsp_cons %d, rsp_prod %d.\n",
>> + buf->rsp_cons, buf->rsp_prod);
>> +
>> + if (!xs_avail_space_for_read(sizeof(msg)))
>> + break;
>> +
>> + /* Make sure data is read after reading the indexes */
>> + rmb();
>> +
>> + /* copy the message */
>> + memcpy_from_ring(
>> + xsh.buf->rsp,
>> + (char *) &msg,
>> + MASK_XENSTORE_IDX(xsh.buf->rsp_cons),
>> + sizeof(msg)
>> + );
> If msg is contiguous in the buffer, we can avoid one memcpy. Since it is
> a repeated operation, this could save us some cycles. This is fine to
> keep it this way in this series. Maybe put a TODO?
I'm not sure I understand what you mean here. If you're implying we
should copy the whole message (header + payload) in a single
'memcpy_from_ring', then I would say we cannot do that because we need
to check the message length whether it's in the limits. If you're
implying we should call 'memcpy' only once in 'memcpy_from_ring', then
it's fine with me.
>> +
>> + DBGXB("Msg len %d, %d avail, id %d.\n",
>> + msg.len + sizeof(msg),
>> + xsh.buf->rsp_prod - xsh.buf->rsp_cons,
>> + msg.req_id);
>> +
>> + if (!xs_avail_space_for_read(sizeof(msg) + msg.len))
>> + break;
> Isn't memory barrier needed here?
Fixed.
>> +
>> + DBGXB("Message is good.\n");
>> +
>> + if (msg.type == XS_WATCH_EVENT)
>> + process_watch_event(&msg);
>> + else
>> + process_reply(&msg);
>> + }
>> + }
>> +}
>> +
>> +static void xs_evtchn_handler(evtchn_port_t port,
>> + struct __regs *regs __unused, void *ign __unused)
>> +{
>> + UK_ASSERT(xsh.initialized == 1);
>> + UK_ASSERT(xsh.evtchn == port);
>> + uk_waitq_wake_up(&xsh.waitq);
>> +}
>> +
>> +int xs_comms_init(void)
>> +{
>> + struct uk_thread *thread;
>> + evtchn_port_t port;
>> +
>> + UK_ASSERT(xsh.initialized == 0);
> 1) I don't see xsh.initialized is needed at all. There is only one path
> where it can be initialized, and it happens only once.
>
> The only thing - waking up the xs_thread_func. We want xsh.waitq to be
> initialized before first handler happened. Again, a static initializer
> (__WAIT_QUEUE_INITIALIZER) will solve this. Or, unmask event from the
> xs_thread_func.
>
> 2) Even if it was needed, this is a pure sanity check. If this condition
> is not met it is dangerous to keep running. I think UK_CRASH is better
> here
Fixed.
>> +
>> + reqid_map_init(&reqid_map);
>> +
>> + uk_waitq_init(&xsh.waitq);
>> +
>> + thread = uk_thread_create("xenstore", xs_thread_func, NULL);
>> + if (PTRISERR(thread))
>> + return PTR2ERR(thread);
>> +
>> + xsh.thread = thread;
>> +
>> + xsh.evtchn = HYPERVISOR_start_info->store_evtchn;
>> + xsh.buf = mfn_to_virt(HYPERVISOR_start_info->store_mfn);
>> +
>> + port = bind_evtchn(xsh.evtchn, xs_evtchn_handler, NULL);
>> + UK_ASSERT(port == xsh.evtchn);
>> + unmask_evtchn(xsh.evtchn);
>> +
>> + xsh.initialized = 1;
>> +
>> + uk_printd(DLVL_INFO,
>> + "Xenstore connection initialised on port %d, buf %p (mfn
>> %#lx)\n",
>> + port, xsh.buf, HYPERVISOR_start_info->store_mfn);
>> +
>> + return 0;
>> +}
>> +
>> +void xs_comms_fini(void)
>> +{
>> + UK_ASSERT(xsh.initialized == 1);
>> +
>> + mask_evtchn(xsh.evtchn);
>> + unbind_evtchn(xsh.evtchn);
>> +
>> + xsh.buf = NULL;
>> +
>> + /* TODO stop thread, instead of killing it */
>> + uk_thread_destroy(xsh.thread);
>> + xsh.thread = NULL;
>> +
>> + xsh.initialized = 0;
>> +}
>> diff --git a/plat/xen/xenbus/xs_comms.h b/plat/xen/xenbus/xs_comms.h
>> new file mode 100644
>> index 0000000..230c4ad
>> --- /dev/null
>> +++ b/plat/xen/xenbus/xs_comms.h
>> @@ -0,0 +1,75 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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 __XS_COMMS_H__
>> +#define __XS_COMMS_H__
>> +
>> +#include <xen/io/xs_wire.h>
>> +#include <xenbus/xenbus.h>
>> +#include <xenbus/xs.h>
>> +
>> +int xs_comms_init(void);
>> +void xs_comms_fini(void);
>> +
>> +struct xs_req {
>> + const void *data;
>> + unsigned int len;
>> +};
> It would be easier to understand the code if the name of this structure
> had "batch" or "vector" in it. Like xs_req_vector.
Fixed, renamed to xs_iovec. Also, data field is not const anymore.
>> +
>> +/* Helper macro for initializing xs requests from strings */
>> +#define XS_REQ_STR(str) \
>> + ((struct xs_req) { str, strlen(str) })
>> +
>> +/* Helper macro for initializing xs requests from strings
>> + * (w/ null terminator)
>> + */
>> +#define XS_REQ_STR_NULL(str) \
>> + ((struct xs_req) { str, strlen(str) + 1 })
> These two macro are used only in xs.c. Maybe move them there?
Fixed.
>> +
>> +/*
>> + * Sends a message to Xenstore and blocks waiting for a reply.
>> + * The reply is malloc'ed and should be freed by the caller.
>> + *
>> + * @param msg_type Xenstore message type
>> + * @param xbt Xenbus transaction id
>> + * @param req Array of requests
>> + * @param req_num Requests number
>> + * @return On success, returns a malloc'd copy of the reply. On error,
>> returns
>> + * a negative error number which should be checked using PTRISERR.
>> + */
>> +struct xsd_sockmsg *xs_msg_reply(enum xsd_sockmsg_type msg_type,
>> + xenbus_transaction_t xbt,
>> + struct xs_req *req, int req_num);
>> +
>> +#endif /* __XS_COMMS_H__ */
>> diff --git a/plat/xen/xenbus/xs_watch.c b/plat/xen/xenbus/xs_watch.c
>> new file mode 100644
>> index 0000000..912fdd2
>> --- /dev/null
>> +++ b/plat/xen/xenbus/xs_watch.c
>> @@ -0,0 +1,159 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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.
>> + */
>> +/* Internal API for Xenstore watches */
>> +
>> +#include <string.h>
>> +#include <uk/errptr.h>
>> +#include <xenbus/client.h>
>> +#include "xs_watch.h"
>> +
>> +/* Watches list */
>> +static struct xenbus_watch *xs_watch_list;
>> +
>> +static void watch_list_add(struct xs_watch *xsw)
>> +{
>> + xsw->base.next = xs_watch_list;
>> + xs_watch_list = &xsw->base;
>> +}
>> +
>> +static int xs_watch_info_equal(const struct xs_watch_info *xswi,
>> + const char *path, const char *token)
>> +{
>> + return (strcmp(xswi->path, path) == 0 &&
>> + strcmp(xswi->token, token) == 0);
>> +}
>> +
>> +static struct xs_watch *watch_list_remove(const char *path, const char
>> *token)
>> +{
>> + struct xenbus_watch *xbw, **prev_xbw;
>> +
>> + for (prev_xbw = &xs_watch_list, xbw = *prev_xbw;
>> + xbw != NULL;
>> + prev_xbw = &xbw->next, xbw = *prev_xbw) {
>> + struct xs_watch *xsw = (struct xs_watch *) xbw;
>> +
>> + if (xs_watch_info_equal(&xsw->xs, path, token)) {
>> + *prev_xbw = xbw->next;
>> + return xsw;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static struct xs_watch *watch_list_find(const char *path, const char *token)
>> +{
>> + struct xenbus_watch *xbw;
>> +
>> + for (xbw = xs_watch_list; xbw != NULL; xbw = xbw->next) {
>> + struct xs_watch *xsw = (struct xs_watch *) xbw;
> Please use __containerof instead of direct casting.
Right, that should have been there from the begining.
>> +
>> + if (xs_watch_info_equal(&xsw->xs, path, token))
>> + return xsw;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +struct xs_watch *xs_watch_create(const char *path, const char *token,
>> + xenbus_watch_evlist_t *events)
>> +{
>> + struct xs_watch *xsw;
>> + char *tmpstr;
>> + int stringlen;
>> +
>> + UK_ASSERT(path != NULL);
>> + UK_ASSERT(token != NULL);
>> + UK_ASSERT(events != NULL);
>> +
>> + stringlen = strlen(token) + 1 + strlen(path) + 1;
>> +
>> + xsw = uk_xb_malloc(sizeof(*xsw) + stringlen);
>> + if (!xsw)
>> + return ERR2PTR(ENOMEM);
>> +
>> + xsw->base.events = events;
>> +
>> + /* set token */
>> + tmpstr = (char *) (xsw + 1);
>> + strcpy(tmpstr, token);
>> + xsw->xs.token = tmpstr;
>> +
>> + /* set path */
>> + tmpstr += strlen(token) + 1;
>> + strcpy(tmpstr, path);
>> + xsw->xs.path = tmpstr;
>> +
>> + watch_list_add(xsw);
>> +
>> + return xsw;
>> +}
>> +
>> +int xs_watch_destroy(const char *path, const char *token)
>> +{
>> + struct xs_watch *xsw;
>> + int err = 0;
>> +
>> + UK_ASSERT(path != NULL);
>> + UK_ASSERT(token != NULL);
>> +
>> + xsw = watch_list_remove(path, token);
>> + if (xsw)
>> + uk_xb_free(xsw);
>> + else
>> + err = -ENOENT;
>> +
>> + return err;
>> +}
>> +
>> +int xs_watch_notify(struct xs_watch_event *event)
>> +{
>> + struct xs_watch *xsw;
>> + int err;
>> +
>> + UK_ASSERT(event != NULL);
>> +
>> + /* check if we have a local watch for it */
>> + xsw = watch_list_find(event->xs.path, event->xs.token);
>> + if (!xsw) {
>> + uk_printd(DLVL_WARN, "Unexpected watch: token %s, path %s\n",
>> + event->xs.token, event->xs.path);
>> + return -ENOENT;
>> + }
>> +
>> + /* notify the waiting client */
>> + err = xenbus_notify_watch_event(xsw->base.events, &event->base);
>> +
>> + return err;
>> +}
>> diff --git a/plat/xen/xenbus/xs_watch.h b/plat/xen/xenbus/xs_watch.h
>> new file mode 100644
>> index 0000000..77dd575
>> --- /dev/null
>> +++ b/plat/xen/xenbus/xs_watch.h
>> @@ -0,0 +1,91 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary 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.
>> + */
>> +/* Internal API for Xenstore watches */
>> +
>> +#ifndef __XS_WATCH_H__
>> +#define __XS_WATCH_H__
>> +
>> +#include <xenbus/xenbus.h>
>> +
>> +/* Xenstore watch info */
>> +struct xs_watch_info {
>> + /**< Watched Xenstore path */
>> + char *path;
>> + /**< Watch identification token */
>> + char *token;
>> +};
>> +
>> +/* Xenstore watch event */
>> +struct xs_watch_event {
>> + struct xenbus_watch_event base;
>> + struct xs_watch_info xs;
>> +};
>> +
>> +/* Xenstore watch */
>> +struct xs_watch {
>> + struct xenbus_watch base;
>> + struct xs_watch_info xs;
>> +};
>> +
>> +/*
>> + * Create a Xenstore watch associated with a path.
>> + *
>> + * @param path Xenstore path
>> + * @param token Watch identification token
>> + * @param events List of watch events
>> + * @return On success, returns a malloc'd Xenstore watch. On error, returns
>> + * a negative error number which should be checked using PTRISERR.
>> + */
>> +struct xs_watch *xs_watch_create(const char *path, const char *token,
>> + xenbus_watch_evlist_t *events);
>> +
>> +/*
>> + * Destroy a previously created Xenstore watch.
>> + *
>> + * @param path Xenstore path
>> + * @param token Watch identification token
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xs_watch_destroy(const char *path, const char *token);
>> +
>> +/*
>> + * Notifies the watch associated with the incoming event.
>> + *
>> + * @param event Incoming watch event
>> + * @return 0 on success (if there is a watch for the input event token), a
>> + * negative errno value on error.
>> + */
>> +int xs_watch_notify(struct xs_watch_event *event);
>> +
>> +#endif /* __XS_WATCH_H__ */
>> --
>> 2.11.0
>>
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |