Xen project Mailing List

Re: [Minios-devel] [UNIKRAFT PATCH 3/3] plat/xen: Add Xenbus support

To: Costin Lupu <costin.lupu@xxxxxxxxx>, <minios-devel@xxxxxxxxxxxxx>

From: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>

Date: Thu, 26 Jul 2018 15:59:43 +0200

Cc: simon.kuenzer@xxxxxxxxx, sharan.santhanam@xxxxxxxxx

Delivery-date: Thu, 26 Jul 2018 13:59:59 +0000

List-id: Mini-os development list <minios-devel.lists.xenproject.org>

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. Getting back to the actual patch. First things first, Unikraft would not build with this patch if I disable xenbus. 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? :) > + 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. > + > +struct xenbus_watch { > + struct xenbus_watch *next; > + xenbus_watch_evlist_t *events; > +}; Is there a reason why you did not use TAILQ here? > + > + > +/* > + * 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? > + /**< 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. > + > + > +/* 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. > + > +#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. > + * > + * @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 > +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. 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. > + 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; > + } > + > + /* 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? > + 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. > + > +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). I would just iterate while(err == EAGAIN). Do you think it would be correct? > + > + 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? > + 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? > + 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. > + > + > +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 :) > + 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. > + 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), }; > + > +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. 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. 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 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 > + 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. > + > + 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). > + > + 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 > + > + 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 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? > + > +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. > + > +/* > + * 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? > +{ > + 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. Another thing, it would be great if id's were allocated sequentially. This is good from performance and debug point of view. > + > + 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. > +/* 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. > +/* > + * 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? > + 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. > + > + 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. > + > + 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? > + > + 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. > +{ > + __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 > + 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; > + > + /* 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. > + 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 > + 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? > +} > + > +/* 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 > + > + data = (char *) event + sizeof(*event); > + > + memcpy_from_ring( > + xsh.buf->rsp, > + data, > + MASK_XENSTORE_IDX(xsh.buf->rsp_cons + sizeof(*msg)), > + msg->len > + ); Barrier? > + 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? > + > + 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? > + > + 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 > + > + 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. > + > +/* 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? > + > +/* > + * 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. > + > + 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 > -- Yuri Volchkov Software Specialist NEC Europe Ltd Kurfürsten-Anlage 36 D-69115 Heidelberg _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.