* [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
@ 2017-07-19 3:18 Zhongze Liu
2017-07-19 19:24 ` Stefano Stabellini
0 siblings, 1 reply; 6+ messages in thread
From: Zhongze Liu @ 2017-07-19 3:18 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Zhongze Liu, Ian Jackson, xen-devel,
Julien Grall
Add a new struct libxl_static_shm in the libxl IDL for the proposed new xl
config entry 'static_shm' (see [1]), which allow the user to set up shared
memory areas among several VMs for communication.
Add related parsing code to the libxl/libxlu_* family and xl/xl_parse.c
[1]: [RFC v3]Proposal to allow setting up shared memory areas between VMs from xl config file,
https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg01741.html
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
---
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.h | 10 ++
tools/libxl/libxl_types.idl | 52 +++++++++
tools/libxl/libxlu_sshm.c | 274 ++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxlutil.h | 6 +
tools/xl/xl_parse.c | 24 +++-
6 files changed, 366 insertions(+), 2 deletions(-)
create mode 100644 tools/libxl/libxlu_sshm.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 2ffb78f5c4..b7effb188b 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
- libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
+ libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
$(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
$(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cf0f31f68..cf3cbe1ba1 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2228,6 +2228,16 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
const char *command_line, char **output);
+
+/* Functions to stattically set up shared memory regions between two domains
+ * for shm-based communication. */
+
+#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
+
+/* TODO: int libxl_sshm_add(libxl_ctx *ctx, uint32_t domid,
+ * libxl_static_shm *sshm);
+ */
+
#include <libxl_event.h>
#endif /* LIBXL_H */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 8a9849c643..8c68b45add 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -779,6 +779,57 @@ libxl_device_channel = Struct("device_channel", [
])),
])
+# static shared memory cacheability attributes
+libxl_sshm_cacheattr = Enumeration("sshm_cacheattr", [
+ (-1, "UNKNOWN"),
+ (0, "UC"),
+ (1, "WC"), #x86 only
+ (4, "WT"),
+ (5, "WP"), #x86 only
+ (6, "WB"),
+ (7, "SUC"), #x86 only
+ (8, "BUFFERABLE"), #ARM only
+ (9, "WA"), #ARM only
+ ], init_val = "LIBXL_SSHM_CACHEATTR_UNKNOWN")
+
+# static shared memory shareability attributes
+libxl_sshm_shareattr = Enumeration("sshm_shareattr", [
+ (-1, "UNKNOWN"),
+ (0, "NON"),
+ (2, "OUTER"),
+ (3, "INNER"),
+ ], init_val = "LIBXL_SSHM_SHAREATTR_UNKNOWN")
+
+libxl_sshm_prot = Enumeration("sshm_prot", [
+ (-1, "UNKNOWN"),
+ (0, "N"),
+ (1, "R"),
+ (2, "W"),
+ (4, "X"),
+ (3, "RW"),
+ (5, "RX"),
+ (6, "WX"),
+ (7, "RWX"),
+ ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
+
+libxl_sshm_role = Enumeration("sshm_role", [
+ (-1, "UNKNOWN"),
+ (0, "MASTER"),
+ (1, "SLAVE"),
+ ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
+
+libxl_static_shm = Struct("static_shm", [
+ ("id", string),
+ ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+ ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+ ("prot", libxl_sshm_prot),
+ ("arm_shareattr", libxl_sshm_shareattr),
+ ("arm_inner_cacheattr", libxl_sshm_cacheattr),
+ ("arm_outer_cacheattr", libxl_sshm_cacheattr),
+ ("x86_cacheattr", libxl_sshm_cacheattr),
+ ("role", libxl_sshm_role),
+])
+
libxl_domain_config = Struct("domain_config", [
("c_info", libxl_domain_create_info),
("b_info", libxl_domain_build_info),
@@ -797,6 +848,7 @@ libxl_domain_config = Struct("domain_config", [
("channels", Array(libxl_device_channel, "num_channels")),
("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
+ ("sshms", Array(libxl_static_shm, "num_sshms")),
("on_poweroff", libxl_action_on_shutdown),
("on_reboot", libxl_action_on_shutdown),
diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
new file mode 100644
index 0000000000..fcd65af4d9
--- /dev/null
+++ b/tools/libxl/libxlu_sshm.c
@@ -0,0 +1,274 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxlu_internal.h"
+
+#include <ctype.h>
+
+#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
+#define KEY_RE "([_a-zA-Z0-9]+)"
+#define VAL_RE "([^ \t\n,]+)"
+#define EQU_RE PARAM_RE(KEY_RE "\\s*=\\s*" VAL_RE)
+
+#define MASK_4K ((uint64_t)0xfff)
+#define MAX_ID_LEN 128
+#define CACHEATTR_ARM 0
+#define CACHEATTR_X86 1
+
+#define INVAL_ERR(msg, curr_str) do { \
+ xlu__sshm_err(cfg, msg, curr_str); \
+ rc = EINVAL; \
+ goto out; \
+ } while(0)
+
+/* set a member in libxl_static_shm and report an error if it's respecified,
+ * @curr_str indicates the head of the remaining string. */
+#define SET_VAL(var, name, type, value, curr_str) do { \
+ if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) { \
+ INVAL_ERR("\"" name "\" respecified", curr_str); \
+ } \
+ (var) = value; \
+ } while(0)
+
+
+static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
+ const char *curr_str) {
+ fprintf(cfg->report,
+ "%s: config parsing error in shared_memory: %s at '%s'\n",
+ cfg->config_source, msg, curr_str);
+}
+
+static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
+{
+ int rc;
+ libxl_sshm_prot new_prot;
+
+ if (!strcmp(str, "r") || !strcmp(str, "ro")) {
+ new_prot = LIBXL_SSHM_PROT_R;
+ } else if (!strcmp(str, "w") || !strcmp(str, "wo")) {
+ new_prot = LIBXL_SSHM_PROT_W;
+ } else if (!strcmp(str, "x") || !strcmp(str, "xo")) {
+ new_prot = LIBXL_SSHM_PROT_X;
+ } else if (!strcmp(str, "rw")) {
+ new_prot = LIBXL_SSHM_PROT_RW;
+ } else if (!strcmp(str, "rx")) {
+ new_prot = LIBXL_SSHM_PROT_RX;
+ } else if (!strcmp(str, "wx")) {
+ new_prot = LIBXL_SSHM_PROT_WX;
+ } else if (!strcmp(str, "rwx")) {
+ new_prot = LIBXL_SSHM_PROT_RWX;
+ } else if (!strcmp(str, "n")) {
+ new_prot = LIBXL_SSHM_PROT_N;
+ } else {
+ INVAL_ERR("invalid permission flags", str);
+ }
+
+ SET_VAL(*prot, "permission flags", PROT, new_prot, str);
+
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+static int parse_cacheattr(XLU_Config *cfg, char *str, int arch,
+ libxl_sshm_cacheattr *cattr)
+{
+ int rc;
+ libxl_sshm_cacheattr new_cattr;
+
+ if (!strcmp(str, "uc")) {
+ new_cattr = LIBXL_SSHM_CACHEATTR_UC;
+ } else if (!strcmp(str, "wc")) {
+ if (CACHEATTR_X86 != arch) {
+ INVAL_ERR("invalid cacheability attribute", str);
+ }
+ new_cattr = LIBXL_SSHM_CACHEATTR_WC;
+ } else if (!strcmp(str, "wt")) {
+ new_cattr = LIBXL_SSHM_CACHEATTR_WT;
+ } else if (!strcmp(str, "wp")) {
+ if (CACHEATTR_X86 != arch) {
+ INVAL_ERR("invalid cacheability attribute", str);
+ }
+ new_cattr = LIBXL_SSHM_CACHEATTR_WP;
+ } else if (!strcmp(str, "wb")) {
+ new_cattr = LIBXL_SSHM_CACHEATTR_WB;
+ } else if (!strcmp(str, "suc")) {
+ if (CACHEATTR_X86 != arch) {
+ INVAL_ERR("invalid cacheability attribute", str);
+ }
+ new_cattr = LIBXL_SSHM_CACHEATTR_SUC;
+ } else if (!strcmp(str, "bufferable")) {
+ if (CACHEATTR_ARM != arch) {
+ INVAL_ERR("invalid cacheability attribute", str);
+ }
+ new_cattr = LIBXL_SSHM_CACHEATTR_BUFFERABLE;
+ } else if (!strcmp(str, "wa")) {
+ if (CACHEATTR_ARM != arch) {
+ INVAL_ERR("invalid cacheability attribute", str);
+ }
+ new_cattr = LIBXL_SSHM_CACHEATTR_WA;
+ } else {
+ INVAL_ERR("invalid cacheability attribute", str);
+ }
+
+ SET_VAL(*cattr, "cacheability attributes", CACHEATTR, new_cattr, str);
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+static int parse_shareattr(XLU_Config *cfg, char *str,
+ libxl_sshm_shareattr *sattr)
+{
+ int rc;
+ libxl_sshm_shareattr new_sattr;
+
+ if (!strcmp(str, "non")) {
+ new_sattr = LIBXL_SSHM_SHAREATTR_NON;
+ } else if (!strcmp(str, "outer")) {
+ new_sattr = LIBXL_SSHM_SHAREATTR_OUTER;
+ } else if (!strcmp(str, "inner")) {
+ new_sattr = LIBXL_SSHM_SHAREATTR_INNER;
+ } else {
+ INVAL_ERR("invalid arm shareability attribute", str);
+ }
+
+ SET_VAL(*sattr, "arm shareability attributes", SHAREATTR, new_sattr, str);
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+/* handle key = value pairs */
+static int handle_equ(XLU_Config *cfg, char *key, char *val,
+ libxl_static_shm *sshm)
+{
+ int rc;
+
+ if (!strcmp(key, "id")) {
+ if (strlen(val) > MAX_ID_LEN) { INVAL_ERR("id too long", val); }
+ if (sshm->id && !strcmp(sshm->id, val)) {
+ INVAL_ERR("id respecified", val);
+ }
+
+ if (NULL == (sshm->id = strdup(val))) {
+ fprintf(stderr, "sshm parser out of memory\n");
+ rc = ENOMEM;
+ goto out;
+ }
+ } else if (!strcmp(key, "role")) {
+ libxl_sshm_role new_role;
+
+ if (!strcmp("master", val)) {
+ new_role = LIBXL_SSHM_ROLE_MASTER;
+ } else if (!strcmp("slave", val)) {
+ new_role = LIBXL_SSHM_ROLE_SLAVE;
+ } else {
+ INVAL_ERR("invalid role", val);
+ }
+
+ SET_VAL(sshm->role, "role", ROLE, new_role, val);
+
+ } else if (!strcmp(key, "begin") || !strcmp(key, "end")) {
+ char *endptr;
+ int base = 10;
+ uint64_t new_bound;
+
+ /* could be in hex form */
+ if ('0' == val[0] && 'x' == val[1]) { base = 16; }
+ new_bound = strtoull(val, &endptr, base);
+ if (ERANGE == errno || *endptr) {
+ INVAL_ERR("invalid begin/end", val);
+ }
+ if (new_bound & MASK_4K) {
+ INVAL_ERR("begin/end is not a multiple of 4K", val);
+ }
+
+ /* begin or end */
+ if ('b' == key[0]) {
+ SET_VAL(sshm->begin, "beginning address", RANGE, new_bound, val);
+ } else {
+ SET_VAL(sshm->end, "ending address", RANGE, new_bound, val);
+ }
+ } else if (!strcmp(key, "prot")) {
+ rc = parse_prot(cfg, val, &sshm->prot);
+ if (rc) { goto out; }
+ } else if (!strcmp(key, "arm_inner_cacheattr")) {
+ rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
+ &sshm->arm_inner_cacheattr);
+ if (rc) { goto out; }
+ } else if (!strcmp(key, "arm_outer_cacheattr")) {
+ rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
+ &sshm->arm_outer_cacheattr);
+ if (rc) { goto out; }
+ } else if (!strcmp(key, "x86_cacheattr")) {
+ rc = parse_cacheattr(cfg, val, CACHEATTR_X86,
+ &sshm->x86_cacheattr);
+ if (rc) { goto out; }
+ } else if (!strcmp(key, "arm_shareattr")) {
+ rc = parse_shareattr(cfg, val, &sshm->arm_shareattr);
+ if (rc) { goto out; }
+ } else {
+ INVAL_ERR("invalid option", key);
+ }
+
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+ libxl_static_shm *sshm)
+{
+ int rc;
+ regex_t equ_rec;
+ char *buf2 = NULL, *ptr = NULL;
+ regmatch_t pmatch[3];
+
+ rc = regcomp(&equ_rec, EQU_RE, REG_EXTENDED);
+ if (rc) {
+ fprintf(stderr, "sshm parser failed to initialize\n");
+ goto out;
+ }
+
+ if (NULL == (buf2 = ptr = strdup(spec))) {
+ fprintf(stderr, "sshm parser out of memory\n");
+ rc = ENOMEM;
+ goto out;
+ }
+
+ while (true) {
+ if (!*ptr) { break; }
+ if (regexec(&equ_rec, ptr, 3, pmatch, 0)) {
+ INVAL_ERR("unrecognized token", ptr);
+ }
+
+ ptr[pmatch[1].rm_eo] = '\0';
+ ptr[pmatch[2].rm_eo] = '\0';
+ rc = handle_equ(cfg, ptr + pmatch[1].rm_so,
+ ptr + pmatch[2].rm_so, sshm);
+ if (rc) { goto out; }
+
+ ptr += pmatch[0].rm_eo;
+ }
+
+ if (*ptr) { INVAL_ERR("invalid syntax", ptr); }
+
+ rc = 0;
+
+ out:
+ if (buf2) { free(buf2); }
+ regfree(&equ_rec);
+ return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index e81b644c01..ee39cb5bdc 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
libxl_device_nic *nic);
+/*
+ * static shared memory specification parsing
+ */
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+ libxl_static_shm *sshm);
+
#endif /* LIBXLUTIL_H */
/*
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 5c2bf17222..82d955b8b9 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -813,7 +813,7 @@ void parse_config_data(const char *config_source,
long l, vcpus = 0;
XLU_Config *config;
XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
- *usbctrls, *usbdevs, *p9devs;
+ *usbctrls, *usbdevs, *p9devs, *sshms;
XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
*mca_caps;
int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
@@ -1392,6 +1392,28 @@ void parse_config_data(const char *config_source,
}
}
+ if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
+ d_config->num_sshms = 0;
+ d_config->sshms = NULL;
+ while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
+ libxl_static_shm *sshm;
+ char *buf2 = strdup(buf);
+ int ret;
+
+ sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
+ d_config->num_sshms,
+ libxl_static_shm_init);
+ ret = xlu_sshm_parse(config, buf2, sshm);
+ if (ret) {
+ fprintf(stderr,
+ "xl: Invalid argument for static_shm: %s", buf2);
+ exit(EXIT_FAILURE);
+ }
+
+ free(buf2);
+ }
+ }
+
if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
libxl_device_p9 *p9;
char *security_model = NULL;
--
2.13.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
2017-07-19 3:18 [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry Zhongze Liu
@ 2017-07-19 19:24 ` Stefano Stabellini
2017-07-20 1:43 ` Zhongze Liu
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Stabellini @ 2017-07-19 19:24 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, xen-devel
On Wed, 19 Jul 2017, Zhongze Liu wrote:
> Add a new struct libxl_static_shm in the libxl IDL for the proposed new xl
> config entry 'static_shm' (see [1]), which allow the user to set up shared
> memory areas among several VMs for communication.
>
> Add related parsing code to the libxl/libxlu_* family and xl/xl_parse.c
>
> [1]: [RFC v3]Proposal to allow setting up shared memory areas between VMs from xl config file,
> https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg01741.html
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
> ---
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl.h | 10 ++
> tools/libxl/libxl_types.idl | 52 +++++++++
> tools/libxl/libxlu_sshm.c | 274 ++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxlutil.h | 6 +
> tools/xl/xl_parse.c | 24 +++-
> 6 files changed, 366 insertions(+), 2 deletions(-)
> create mode 100644 tools/libxl/libxlu_sshm.c
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 2ffb78f5c4..b7effb188b 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
> AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
> AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
> LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
> - libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
> + libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
> $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
>
> $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 7cf0f31f68..cf3cbe1ba1 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -2228,6 +2228,16 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
> int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
> const char *command_line, char **output);
>
> +
> +/* Functions to stattically set up shared memory regions between two domains
^ statically ^double space
> + * for shm-based communication. */
> +
> +#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
> +
> +/* TODO: int libxl_sshm_add(libxl_ctx *ctx, uint32_t domid,
> + * libxl_static_shm *sshm);
> + */
> +
> #include <libxl_event.h>
>
> #endif /* LIBXL_H */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 8a9849c643..8c68b45add 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -779,6 +779,57 @@ libxl_device_channel = Struct("device_channel", [
> ])),
> ])
>
> +# static shared memory cacheability attributes
> +libxl_sshm_cacheattr = Enumeration("sshm_cacheattr", [
> + (-1, "UNKNOWN"),
> + (0, "UC"),
> + (1, "WC"), #x86 only
> + (4, "WT"),
> + (5, "WP"), #x86 only
> + (6, "WB"),
> + (7, "SUC"), #x86 only
> + (8, "BUFFERABLE"), #ARM only
> + (9, "WA"), #ARM only
> + ], init_val = "LIBXL_SSHM_CACHEATTR_UNKNOWN")
I would only specify UNKNOWN and WB for now.
> +# static shared memory shareability attributes
> +libxl_sshm_shareattr = Enumeration("sshm_shareattr", [
> + (-1, "UNKNOWN"),
> + (0, "NON"),
> + (2, "OUTER"),
> + (3, "INNER"),
> + ], init_val = "LIBXL_SSHM_SHAREATTR_UNKNOWN")
> +
> +libxl_sshm_prot = Enumeration("sshm_prot", [
> + (-1, "UNKNOWN"),
> + (0, "N"),
> + (1, "R"),
> + (2, "W"),
> + (4, "X"),
> + (3, "RW"),
> + (5, "RX"),
> + (6, "WX"),
> + (7, "RWX"),
> + ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
> +
> +libxl_sshm_role = Enumeration("sshm_role", [
> + (-1, "UNKNOWN"),
> + (0, "MASTER"),
> + (1, "SLAVE"),
> + ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
> +
> +libxl_static_shm = Struct("static_shm", [
> + ("id", string),
> + ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> + ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> + ("prot", libxl_sshm_prot),
> + ("arm_shareattr", libxl_sshm_shareattr),
> + ("arm_inner_cacheattr", libxl_sshm_cacheattr),
> + ("arm_outer_cacheattr", libxl_sshm_cacheattr),
I would have a single arm_cacheattr
> + ("x86_cacheattr", libxl_sshm_cacheattr),
> + ("role", libxl_sshm_role),
> +])
> +
> libxl_domain_config = Struct("domain_config", [
> ("c_info", libxl_domain_create_info),
> ("b_info", libxl_domain_build_info),
> @@ -797,6 +848,7 @@ libxl_domain_config = Struct("domain_config", [
> ("channels", Array(libxl_device_channel, "num_channels")),
> ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
> + ("sshms", Array(libxl_static_shm, "num_sshms")),
>
> ("on_poweroff", libxl_action_on_shutdown),
> ("on_reboot", libxl_action_on_shutdown),
> diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
> new file mode 100644
> index 0000000000..fcd65af4d9
> --- /dev/null
> +++ b/tools/libxl/libxlu_sshm.c
> @@ -0,0 +1,274 @@
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxlu_internal.h"
> +
> +#include <ctype.h>
> +
> +#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
> +#define KEY_RE "([_a-zA-Z0-9]+)"
> +#define VAL_RE "([^ \t\n,]+)"
> +#define EQU_RE PARAM_RE(KEY_RE "\\s*=\\s*" VAL_RE)
> +
> +#define MASK_4K ((uint64_t)0xfff)
> +#define MAX_ID_LEN 128
> +#define CACHEATTR_ARM 0
> +#define CACHEATTR_X86 1
> +
> +#define INVAL_ERR(msg, curr_str) do { \
> + xlu__sshm_err(cfg, msg, curr_str); \
> + rc = EINVAL; \
> + goto out; \
> + } while(0)
> +
> +/* set a member in libxl_static_shm and report an error if it's respecified,
> + * @curr_str indicates the head of the remaining string. */
> +#define SET_VAL(var, name, type, value, curr_str) do { \
> + if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) { \
> + INVAL_ERR("\"" name "\" respecified", curr_str); \
> + } \
> + (var) = value; \
> + } while(0)
> +
> +
> +static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
> + const char *curr_str) {
> + fprintf(cfg->report,
> + "%s: config parsing error in shared_memory: %s at '%s'\n",
> + cfg->config_source, msg, curr_str);
> +}
> +
> +static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
> +{
> + int rc;
> + libxl_sshm_prot new_prot;
> +
> + if (!strcmp(str, "r") || !strcmp(str, "ro")) {
> + new_prot = LIBXL_SSHM_PROT_R;
> + } else if (!strcmp(str, "w") || !strcmp(str, "wo")) {
> + new_prot = LIBXL_SSHM_PROT_W;
> + } else if (!strcmp(str, "x") || !strcmp(str, "xo")) {
> + new_prot = LIBXL_SSHM_PROT_X;
> + } else if (!strcmp(str, "rw")) {
> + new_prot = LIBXL_SSHM_PROT_RW;
> + } else if (!strcmp(str, "rx")) {
> + new_prot = LIBXL_SSHM_PROT_RX;
> + } else if (!strcmp(str, "wx")) {
> + new_prot = LIBXL_SSHM_PROT_WX;
> + } else if (!strcmp(str, "rwx")) {
> + new_prot = LIBXL_SSHM_PROT_RWX;
> + } else if (!strcmp(str, "n")) {
> + new_prot = LIBXL_SSHM_PROT_N;
> + } else {
> + INVAL_ERR("invalid permission flags", str);
shouldn't this return an error?
> + }
> +
> + SET_VAL(*prot, "permission flags", PROT, new_prot, str);
> +
> + rc = 0;
> +
> + out:
> + return rc;
> +}
> +
> +static int parse_cacheattr(XLU_Config *cfg, char *str, int arch,
> + libxl_sshm_cacheattr *cattr)
> +{
> + int rc;
> + libxl_sshm_cacheattr new_cattr;
> +
> + if (!strcmp(str, "uc")) {
> + new_cattr = LIBXL_SSHM_CACHEATTR_UC;
> + } else if (!strcmp(str, "wc")) {
> + if (CACHEATTR_X86 != arch) {
> + INVAL_ERR("invalid cacheability attribute", str);
> + }
> + new_cattr = LIBXL_SSHM_CACHEATTR_WC;
> + } else if (!strcmp(str, "wt")) {
> + new_cattr = LIBXL_SSHM_CACHEATTR_WT;
> + } else if (!strcmp(str, "wp")) {
> + if (CACHEATTR_X86 != arch) {
> + INVAL_ERR("invalid cacheability attribute", str);
> + }
> + new_cattr = LIBXL_SSHM_CACHEATTR_WP;
> + } else if (!strcmp(str, "wb")) {
> + new_cattr = LIBXL_SSHM_CACHEATTR_WB;
> + } else if (!strcmp(str, "suc")) {
> + if (CACHEATTR_X86 != arch) {
> + INVAL_ERR("invalid cacheability attribute", str);
> + }
> + new_cattr = LIBXL_SSHM_CACHEATTR_SUC;
> + } else if (!strcmp(str, "bufferable")) {
> + if (CACHEATTR_ARM != arch) {
> + INVAL_ERR("invalid cacheability attribute", str);
> + }
> + new_cattr = LIBXL_SSHM_CACHEATTR_BUFFERABLE;
> + } else if (!strcmp(str, "wa")) {
> + if (CACHEATTR_ARM != arch) {
> + INVAL_ERR("invalid cacheability attribute", str);
> + }
> + new_cattr = LIBXL_SSHM_CACHEATTR_WA;
> + } else {
> + INVAL_ERR("invalid cacheability attribute", str);
shouldn't this return an error?
> + }
I don't know if the other maintainers agree, but I think we should just
check that str is "wb" and fail in all other cases.
> + SET_VAL(*cattr, "cacheability attributes", CACHEATTR, new_cattr, str);
> + rc = 0;
> +
> + out:
> + return rc;
> +}
> +
> +static int parse_shareattr(XLU_Config *cfg, char *str,
> + libxl_sshm_shareattr *sattr)
> +{
> + int rc;
> + libxl_sshm_shareattr new_sattr;
> +
> + if (!strcmp(str, "non")) {
> + new_sattr = LIBXL_SSHM_SHAREATTR_NON;
> + } else if (!strcmp(str, "outer")) {
> + new_sattr = LIBXL_SSHM_SHAREATTR_OUTER;
> + } else if (!strcmp(str, "inner")) {
> + new_sattr = LIBXL_SSHM_SHAREATTR_INNER;
> + } else {
> + INVAL_ERR("invalid arm shareability attribute", str);
shouldn't this return an error?
> + }
> +
> + SET_VAL(*sattr, "arm shareability attributes", SHAREATTR, new_sattr, str);
> + rc = 0;
> +
> + out:
> + return rc;
> +}
> +
> +/* handle key = value pairs */
> +static int handle_equ(XLU_Config *cfg, char *key, char *val,
> + libxl_static_shm *sshm)
> +{
> + int rc;
> +
> + if (!strcmp(key, "id")) {
> + if (strlen(val) > MAX_ID_LEN) { INVAL_ERR("id too long", val); }
> + if (sshm->id && !strcmp(sshm->id, val)) {
> + INVAL_ERR("id respecified", val);
> + }
> +
> + if (NULL == (sshm->id = strdup(val))) {
> + fprintf(stderr, "sshm parser out of memory\n");
> + rc = ENOMEM;
> + goto out;
> + }
> + } else if (!strcmp(key, "role")) {
> + libxl_sshm_role new_role;
> +
> + if (!strcmp("master", val)) {
> + new_role = LIBXL_SSHM_ROLE_MASTER;
> + } else if (!strcmp("slave", val)) {
> + new_role = LIBXL_SSHM_ROLE_SLAVE;
> + } else {
> + INVAL_ERR("invalid role", val);
> + }
> +
> + SET_VAL(sshm->role, "role", ROLE, new_role, val);
> +
> + } else if (!strcmp(key, "begin") || !strcmp(key, "end")) {
> + char *endptr;
> + int base = 10;
> + uint64_t new_bound;
> +
> + /* could be in hex form */
> + if ('0' == val[0] && 'x' == val[1]) { base = 16; }
Shouldn't you check that val is at least 2 in length?
> + new_bound = strtoull(val, &endptr, base);
> + if (ERANGE == errno || *endptr) {
> + INVAL_ERR("invalid begin/end", val);
> + }
> + if (new_bound & MASK_4K) {
> + INVAL_ERR("begin/end is not a multiple of 4K", val);
> + }
> +
> + /* begin or end */
> + if ('b' == key[0]) {
> + SET_VAL(sshm->begin, "beginning address", RANGE, new_bound, val);
> + } else {
> + SET_VAL(sshm->end, "ending address", RANGE, new_bound, val);
> + }
> + } else if (!strcmp(key, "prot")) {
> + rc = parse_prot(cfg, val, &sshm->prot);
> + if (rc) { goto out; }
> + } else if (!strcmp(key, "arm_inner_cacheattr")) {
> + rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
> + &sshm->arm_inner_cacheattr);
> + if (rc) { goto out; }
> + } else if (!strcmp(key, "arm_outer_cacheattr")) {
> + rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
> + &sshm->arm_outer_cacheattr);
> + if (rc) { goto out; }
> + } else if (!strcmp(key, "x86_cacheattr")) {
> + rc = parse_cacheattr(cfg, val, CACHEATTR_X86,
> + &sshm->x86_cacheattr);
> + if (rc) { goto out; }
> + } else if (!strcmp(key, "arm_shareattr")) {
> + rc = parse_shareattr(cfg, val, &sshm->arm_shareattr);
> + if (rc) { goto out; }
> + } else {
> + INVAL_ERR("invalid option", key);
shouldn't this return an error?
> + }
> +
> + rc = 0;
> +
> + out:
> + return rc;
> +}
> +
> +int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
> + libxl_static_shm *sshm)
> +{
> + int rc;
> + regex_t equ_rec;
> + char *buf2 = NULL, *ptr = NULL;
> + regmatch_t pmatch[3];
> +
> + rc = regcomp(&equ_rec, EQU_RE, REG_EXTENDED);
> + if (rc) {
> + fprintf(stderr, "sshm parser failed to initialize\n");
> + goto out;
> + }
> +
> + if (NULL == (buf2 = ptr = strdup(spec))) {
> + fprintf(stderr, "sshm parser out of memory\n");
> + rc = ENOMEM;
> + goto out;
> + }
> +
> + while (true) {
> + if (!*ptr) { break; }
> + if (regexec(&equ_rec, ptr, 3, pmatch, 0)) {
> + INVAL_ERR("unrecognized token", ptr);
> + }
> +
> + ptr[pmatch[1].rm_eo] = '\0';
> + ptr[pmatch[2].rm_eo] = '\0';
> + rc = handle_equ(cfg, ptr + pmatch[1].rm_so,
> + ptr + pmatch[2].rm_so, sshm);
> + if (rc) { goto out; }
> +
> + ptr += pmatch[0].rm_eo;
> + }
> +
> + if (*ptr) { INVAL_ERR("invalid syntax", ptr); }
> +
> + rc = 0;
> +
> + out:
> + if (buf2) { free(buf2); }
> + regfree(&equ_rec);
> + return rc;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> +
> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
> index e81b644c01..ee39cb5bdc 100644
> --- a/tools/libxl/libxlutil.h
> +++ b/tools/libxl/libxlutil.h
> @@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
> int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
> libxl_device_nic *nic);
>
> +/*
> + * static shared memory specification parsing
> + */
> +int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
> + libxl_static_shm *sshm);
> +
> #endif /* LIBXLUTIL_H */
>
> /*
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 5c2bf17222..82d955b8b9 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -813,7 +813,7 @@ void parse_config_data(const char *config_source,
> long l, vcpus = 0;
> XLU_Config *config;
> XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
> - *usbctrls, *usbdevs, *p9devs;
> + *usbctrls, *usbdevs, *p9devs, *sshms;
> XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
> *mca_caps;
> int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
> @@ -1392,6 +1392,28 @@ void parse_config_data(const char *config_source,
> }
> }
>
> + if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
> + d_config->num_sshms = 0;
> + d_config->sshms = NULL;
> + while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
> + libxl_static_shm *sshm;
> + char *buf2 = strdup(buf);
> + int ret;
> +
> + sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
> + d_config->num_sshms,
> + libxl_static_shm_init);
> + ret = xlu_sshm_parse(config, buf2, sshm);
> + if (ret) {
> + fprintf(stderr,
> + "xl: Invalid argument for static_shm: %s", buf2);
> + exit(EXIT_FAILURE);
> + }
> +
> + free(buf2);
> + }
> + }
> +
> if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
> libxl_device_p9 *p9;
> char *security_model = NULL;
> --
> 2.13.3
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
2017-07-19 19:24 ` Stefano Stabellini
@ 2017-07-20 1:43 ` Zhongze Liu
2017-07-20 10:14 ` Zhongze Liu
2017-07-21 0:07 ` Stefano Stabellini
0 siblings, 2 replies; 6+ messages in thread
From: Zhongze Liu @ 2017-07-20 1:43 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, Julien Grall, Wei Liu, Ian Jackson, xen-devel
Hi Stefano,
2017-07-20 3:24 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
> On Wed, 19 Jul 2017, Zhongze Liu wrote:
>> Add a new struct libxl_static_shm in the libxl IDL for the proposed new xl
>> config entry 'static_shm' (see [1]), which allow the user to set up shared
>> memory areas among several VMs for communication.
>>
>> Add related parsing code to the libxl/libxlu_* family and xl/xl_parse.c
>>
>> [1]: [RFC v3]Proposal to allow setting up shared memory areas between VMs from xl config file,
>> https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg01741.html
>>
>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>> ---
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> tools/libxl/Makefile | 2 +-
>> tools/libxl/libxl.h | 10 ++
>> tools/libxl/libxl_types.idl | 52 +++++++++
>> tools/libxl/libxlu_sshm.c | 274 ++++++++++++++++++++++++++++++++++++++++++++
>> tools/libxl/libxlutil.h | 6 +
>> tools/xl/xl_parse.c | 24 +++-
>> 6 files changed, 366 insertions(+), 2 deletions(-)
>> create mode 100644 tools/libxl/libxlu_sshm.c
>>
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 2ffb78f5c4..b7effb188b 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
>> AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
>> AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
>> LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
>> - libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
>> + libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
>> $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
>>
>> $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 7cf0f31f68..cf3cbe1ba1 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -2228,6 +2228,16 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
>> int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
>> const char *command_line, char **output);
>>
>> +
>> +/* Functions to stattically set up shared memory regions between two domains
> ^ statically ^double space
>
Sorry for the typos.
>
>> + * for shm-based communication. */
>> +
>> +#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
>> +
>> +/* TODO: int libxl_sshm_add(libxl_ctx *ctx, uint32_t domid,
>> + * libxl_static_shm *sshm);
>> + */
>> +
>> #include <libxl_event.h>
>>
>> #endif /* LIBXL_H */
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 8a9849c643..8c68b45add 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -779,6 +779,57 @@ libxl_device_channel = Struct("device_channel", [
>> ])),
>> ])
>>
>> +# static shared memory cacheability attributes
>> +libxl_sshm_cacheattr = Enumeration("sshm_cacheattr", [
>> + (-1, "UNKNOWN"),
>> + (0, "UC"),
>> + (1, "WC"), #x86 only
>> + (4, "WT"),
>> + (5, "WP"), #x86 only
>> + (6, "WB"),
>> + (7, "SUC"), #x86 only
>> + (8, "BUFFERABLE"), #ARM only
>> + (9, "WA"), #ARM only
>> + ], init_val = "LIBXL_SSHM_CACHEATTR_UNKNOWN")
>
> I would only specify UNKNOWN and WB for now.
For here and below, I actually want to left the checks for 'not
implemented' errors
to later stages of handling. The typical call flow of xl is like below:
xl --> libxlu_* --> xl --> libxl_* --> hypercalls
I was planning to check for options that are not implemented currently
in the libxl_*.
>
>
>> +# static shared memory shareability attributes
>> +libxl_sshm_shareattr = Enumeration("sshm_shareattr", [
>> + (-1, "UNKNOWN"),
>> + (0, "NON"),
>> + (2, "OUTER"),
>> + (3, "INNER"),
>> + ], init_val = "LIBXL_SSHM_SHAREATTR_UNKNOWN")
>> +
>> +libxl_sshm_prot = Enumeration("sshm_prot", [
>> + (-1, "UNKNOWN"),
>> + (0, "N"),
>> + (1, "R"),
>> + (2, "W"),
>> + (4, "X"),
>> + (3, "RW"),
>> + (5, "RX"),
>> + (6, "WX"),
>> + (7, "RWX"),
>> + ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
>> +
>> +libxl_sshm_role = Enumeration("sshm_role", [
>> + (-1, "UNKNOWN"),
>> + (0, "MASTER"),
>> + (1, "SLAVE"),
>> + ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
>> +
>> +libxl_static_shm = Struct("static_shm", [
>> + ("id", string),
>> + ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
>> + ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
>> + ("prot", libxl_sshm_prot),
>> + ("arm_shareattr", libxl_sshm_shareattr),
>> + ("arm_inner_cacheattr", libxl_sshm_cacheattr),
>> + ("arm_outer_cacheattr", libxl_sshm_cacheattr),
>
> I would have a single arm_cacheattr
Why? Am I supposed to use a '|' to combine inner and outer cacheattrs ?
>
>
>> + ("x86_cacheattr", libxl_sshm_cacheattr),
>> + ("role", libxl_sshm_role),
>> +])
>> +
>> libxl_domain_config = Struct("domain_config", [
>> ("c_info", libxl_domain_create_info),
>> ("b_info", libxl_domain_build_info),
>> @@ -797,6 +848,7 @@ libxl_domain_config = Struct("domain_config", [
>> ("channels", Array(libxl_device_channel, "num_channels")),
>> ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
>> ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
>> + ("sshms", Array(libxl_static_shm, "num_sshms")),
>>
>> ("on_poweroff", libxl_action_on_shutdown),
>> ("on_reboot", libxl_action_on_shutdown),
>> diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
>> new file mode 100644
>> index 0000000000..fcd65af4d9
>> --- /dev/null
>> +++ b/tools/libxl/libxlu_sshm.c
>> @@ -0,0 +1,274 @@
>> +#include "libxl_osdeps.h" /* must come before any other headers */
>> +#include "libxlu_internal.h"
>> +
>> +#include <ctype.h>
>> +
>> +#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
>> +#define KEY_RE "([_a-zA-Z0-9]+)"
>> +#define VAL_RE "([^ \t\n,]+)"
>> +#define EQU_RE PARAM_RE(KEY_RE "\\s*=\\s*" VAL_RE)
>> +
>> +#define MASK_4K ((uint64_t)0xfff)
>> +#define MAX_ID_LEN 128
>> +#define CACHEATTR_ARM 0
>> +#define CACHEATTR_X86 1
>> +
>> +#define INVAL_ERR(msg, curr_str) do { \
>> + xlu__sshm_err(cfg, msg, curr_str); \
>> + rc = EINVAL; \
>> + goto out; \
>> + } while(0)
>> +
>> +/* set a member in libxl_static_shm and report an error if it's respecified,
>> + * @curr_str indicates the head of the remaining string. */
>> +#define SET_VAL(var, name, type, value, curr_str) do { \
>> + if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) { \
>> + INVAL_ERR("\"" name "\" respecified", curr_str); \
>> + } \
>> + (var) = value; \
>> + } while(0)
>> +
>> +
>> +static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
>> + const char *curr_str) {
>> + fprintf(cfg->report,
>> + "%s: config parsing error in shared_memory: %s at '%s'\n",
>> + cfg->config_source, msg, curr_str);
>> +}
>> +
>> +static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
>> +{
>> + int rc;
>> + libxl_sshm_prot new_prot;
>> +
>> + if (!strcmp(str, "r") || !strcmp(str, "ro")) {
>> + new_prot = LIBXL_SSHM_PROT_R;
>> + } else if (!strcmp(str, "w") || !strcmp(str, "wo")) {
>> + new_prot = LIBXL_SSHM_PROT_W;
>> + } else if (!strcmp(str, "x") || !strcmp(str, "xo")) {
>> + new_prot = LIBXL_SSHM_PROT_X;
>> + } else if (!strcmp(str, "rw")) {
>> + new_prot = LIBXL_SSHM_PROT_RW;
>> + } else if (!strcmp(str, "rx")) {
>> + new_prot = LIBXL_SSHM_PROT_RX;
>> + } else if (!strcmp(str, "wx")) {
>> + new_prot = LIBXL_SSHM_PROT_WX;
>> + } else if (!strcmp(str, "rwx")) {
>> + new_prot = LIBXL_SSHM_PROT_RWX;
>> + } else if (!strcmp(str, "n")) {
>> + new_prot = LIBXL_SSHM_PROT_N;
>> + } else {
>> + INVAL_ERR("invalid permission flags", str);
>
> shouldn't this return an error?
This macro does return an error. but it seems that the naming is not
very appropriate. may I should change it to RET_INVAL or something?
>
>
>> + }
>> +
>> + SET_VAL(*prot, "permission flags", PROT, new_prot, str);
>> +
>> + rc = 0;
>> +
>> + out:
>> + return rc;
>> +}
>> +
>> +static int parse_cacheattr(XLU_Config *cfg, char *str, int arch,
>> + libxl_sshm_cacheattr *cattr)
>> +{
>> + int rc;
>> + libxl_sshm_cacheattr new_cattr;
>> +
>> + if (!strcmp(str, "uc")) {
>> + new_cattr = LIBXL_SSHM_CACHEATTR_UC;
>> + } else if (!strcmp(str, "wc")) {
>> + if (CACHEATTR_X86 != arch) {
>> + INVAL_ERR("invalid cacheability attribute", str);
>> + }
>> + new_cattr = LIBXL_SSHM_CACHEATTR_WC;
>> + } else if (!strcmp(str, "wt")) {
>> + new_cattr = LIBXL_SSHM_CACHEATTR_WT;
>> + } else if (!strcmp(str, "wp")) {
>> + if (CACHEATTR_X86 != arch) {
>> + INVAL_ERR("invalid cacheability attribute", str);
>> + }
>> + new_cattr = LIBXL_SSHM_CACHEATTR_WP;
>> + } else if (!strcmp(str, "wb")) {
>> + new_cattr = LIBXL_SSHM_CACHEATTR_WB;
>> + } else if (!strcmp(str, "suc")) {
>> + if (CACHEATTR_X86 != arch) {
>> + INVAL_ERR("invalid cacheability attribute", str);
>> + }
>> + new_cattr = LIBXL_SSHM_CACHEATTR_SUC;
>> + } else if (!strcmp(str, "bufferable")) {
>> + if (CACHEATTR_ARM != arch) {
>> + INVAL_ERR("invalid cacheability attribute", str);
>> + }
>> + new_cattr = LIBXL_SSHM_CACHEATTR_BUFFERABLE;
>> + } else if (!strcmp(str, "wa")) {
>> + if (CACHEATTR_ARM != arch) {
>> + INVAL_ERR("invalid cacheability attribute", str);
>> + }
>> + new_cattr = LIBXL_SSHM_CACHEATTR_WA;
>> + } else {
>> + INVAL_ERR("invalid cacheability attribute", str);
>
> shouldn't this return an error?
>
>
>> + }
>
> I don't know if the other maintainers agree, but I think we should just
> check that str is "wb" and fail in all other cases.
Just as pointed out above, I prefer to implement all the options in this part
of the code, since parsing and actual handling are two somewhat independent
parts. The checks for options that are not implemented could be left to later
stages.
>
>
>> + SET_VAL(*cattr, "cacheability attributes", CACHEATTR, new_cattr, str);
>> + rc = 0;
>> +
>> + out:
>> + return rc;
>> +}
>> +
>> +static int parse_shareattr(XLU_Config *cfg, char *str,
>> + libxl_sshm_shareattr *sattr)
>> +{
>> + int rc;
>> + libxl_sshm_shareattr new_sattr;
>> +
>> + if (!strcmp(str, "non")) {
>> + new_sattr = LIBXL_SSHM_SHAREATTR_NON;
>> + } else if (!strcmp(str, "outer")) {
>> + new_sattr = LIBXL_SSHM_SHAREATTR_OUTER;
>> + } else if (!strcmp(str, "inner")) {
>> + new_sattr = LIBXL_SSHM_SHAREATTR_INNER;
>> + } else {
>> + INVAL_ERR("invalid arm shareability attribute", str);
>
> shouldn't this return an error?
>
>
>> + }
>> +
>> + SET_VAL(*sattr, "arm shareability attributes", SHAREATTR, new_sattr, str);
>> + rc = 0;
>> +
>> + out:
>> + return rc;
>> +}
>> +
>> +/* handle key = value pairs */
>> +static int handle_equ(XLU_Config *cfg, char *key, char *val,
>> + libxl_static_shm *sshm)
>> +{
>> + int rc;
>> +
>> + if (!strcmp(key, "id")) {
>> + if (strlen(val) > MAX_ID_LEN) { INVAL_ERR("id too long", val); }
>> + if (sshm->id && !strcmp(sshm->id, val)) {
>> + INVAL_ERR("id respecified", val);
>> + }
>> +
>> + if (NULL == (sshm->id = strdup(val))) {
>> + fprintf(stderr, "sshm parser out of memory\n");
>> + rc = ENOMEM;
>> + goto out;
>> + }
>> + } else if (!strcmp(key, "role")) {
>> + libxl_sshm_role new_role;
>> +
>> + if (!strcmp("master", val)) {
>> + new_role = LIBXL_SSHM_ROLE_MASTER;
>> + } else if (!strcmp("slave", val)) {
>> + new_role = LIBXL_SSHM_ROLE_SLAVE;
>> + } else {
>> + INVAL_ERR("invalid role", val);
>> + }
>> +
>> + SET_VAL(sshm->role, "role", ROLE, new_role, val);
>> +
>> + } else if (!strcmp(key, "begin") || !strcmp(key, "end")) {
>> + char *endptr;
>> + int base = 10;
>> + uint64_t new_bound;
>> +
>> + /* could be in hex form */
>> + if ('0' == val[0] && 'x' == val[1]) { base = 16; }
>
> Shouldn't you check that val is at least 2 in length?
Yes. Sorry. I will fix this.
>
>
>> + new_bound = strtoull(val, &endptr, base);
>> + if (ERANGE == errno || *endptr) {
>> + INVAL_ERR("invalid begin/end", val);
>> + }
>> + if (new_bound & MASK_4K) {
>> + INVAL_ERR("begin/end is not a multiple of 4K", val);
>> + }
>> +
>> + /* begin or end */
>> + if ('b' == key[0]) {
>> + SET_VAL(sshm->begin, "beginning address", RANGE, new_bound, val);
>> + } else {
>> + SET_VAL(sshm->end, "ending address", RANGE, new_bound, val);
>> + }
>> + } else if (!strcmp(key, "prot")) {
>> + rc = parse_prot(cfg, val, &sshm->prot);
>> + if (rc) { goto out; }
>> + } else if (!strcmp(key, "arm_inner_cacheattr")) {
>> + rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
>> + &sshm->arm_inner_cacheattr);
>> + if (rc) { goto out; }
>> + } else if (!strcmp(key, "arm_outer_cacheattr")) {
>> + rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
>> + &sshm->arm_outer_cacheattr);
>> + if (rc) { goto out; }
>> + } else if (!strcmp(key, "x86_cacheattr")) {
>> + rc = parse_cacheattr(cfg, val, CACHEATTR_X86,
>> + &sshm->x86_cacheattr);
>> + if (rc) { goto out; }
>> + } else if (!strcmp(key, "arm_shareattr")) {
>> + rc = parse_shareattr(cfg, val, &sshm->arm_shareattr);
>> + if (rc) { goto out; }
>> + } else {
>> + INVAL_ERR("invalid option", key);
>
> shouldn't this return an error?
>
>
>> + }
>> +
>> + rc = 0;
>> +
>> + out:
>> + return rc;
>> +}
>> +
>> +int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
>> + libxl_static_shm *sshm)
>> +{
>> + int rc;
>> + regex_t equ_rec;
>> + char *buf2 = NULL, *ptr = NULL;
>> + regmatch_t pmatch[3];
>> +
>> + rc = regcomp(&equ_rec, EQU_RE, REG_EXTENDED);
>> + if (rc) {
>> + fprintf(stderr, "sshm parser failed to initialize\n");
>> + goto out;
>> + }
>> +
>> + if (NULL == (buf2 = ptr = strdup(spec))) {
>> + fprintf(stderr, "sshm parser out of memory\n");
>> + rc = ENOMEM;
>> + goto out;
>> + }
>> +
>> + while (true) {
>> + if (!*ptr) { break; }
>> + if (regexec(&equ_rec, ptr, 3, pmatch, 0)) {
>> + INVAL_ERR("unrecognized token", ptr);
>> + }
>> +
>> + ptr[pmatch[1].rm_eo] = '\0';
>> + ptr[pmatch[2].rm_eo] = '\0';
>> + rc = handle_equ(cfg, ptr + pmatch[1].rm_so,
>> + ptr + pmatch[2].rm_so, sshm);
>> + if (rc) { goto out; }
>> +
>> + ptr += pmatch[0].rm_eo;
>> + }
>> +
>> + if (*ptr) { INVAL_ERR("invalid syntax", ptr); }
>> +
>> + rc = 0;
>> +
>> + out:
>> + if (buf2) { free(buf2); }
>> + regfree(&equ_rec);
>> + return rc;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> +
>> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
>> index e81b644c01..ee39cb5bdc 100644
>> --- a/tools/libxl/libxlutil.h
>> +++ b/tools/libxl/libxlutil.h
>> @@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
>> int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
>> libxl_device_nic *nic);
>>
>> +/*
>> + * static shared memory specification parsing
>> + */
>> +int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
>> + libxl_static_shm *sshm);
>> +
>> #endif /* LIBXLUTIL_H */
>>
>> /*
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 5c2bf17222..82d955b8b9 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -813,7 +813,7 @@ void parse_config_data(const char *config_source,
>> long l, vcpus = 0;
>> XLU_Config *config;
>> XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
>> - *usbctrls, *usbdevs, *p9devs;
>> + *usbctrls, *usbdevs, *p9devs, *sshms;
>> XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
>> *mca_caps;
>> int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
>> @@ -1392,6 +1392,28 @@ void parse_config_data(const char *config_source,
>> }
>> }
>>
>> + if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
>> + d_config->num_sshms = 0;
>> + d_config->sshms = NULL;
>> + while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
>> + libxl_static_shm *sshm;
>> + char *buf2 = strdup(buf);
>> + int ret;
>> +
>> + sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
>> + d_config->num_sshms,
>> + libxl_static_shm_init);
>> + ret = xlu_sshm_parse(config, buf2, sshm);
>> + if (ret) {
>> + fprintf(stderr,
>> + "xl: Invalid argument for static_shm: %s", buf2);
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + free(buf2);
>> + }
>> + }
>> +
>> if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
>> libxl_device_p9 *p9;
>> char *security_model = NULL;
>> --
>> 2.13.3
>>
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
2017-07-20 1:43 ` Zhongze Liu
@ 2017-07-20 10:14 ` Zhongze Liu
2017-07-21 0:10 ` Stefano Stabellini
2017-07-21 0:07 ` Stefano Stabellini
1 sibling, 1 reply; 6+ messages in thread
From: Zhongze Liu @ 2017-07-20 10:14 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, Julien Grall, Wei Liu, Ian Jackson, xen-devel
2017-07-20 9:43 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
> Hi Stefano,
>
> 2017-07-20 3:24 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
>> On Wed, 19 Jul 2017, Zhongze Liu wrote:
>>> Add a new struct libxl_static_shm in the libxl IDL for the proposed new xl
>>> config entry 'static_shm' (see [1]), which allow the user to set up shared
>>> memory areas among several VMs for communication.
>>>
>>> Add related parsing code to the libxl/libxlu_* family and xl/xl_parse.c
>>>
>>> [1]: [RFC v3]Proposal to allow setting up shared memory areas between VMs from xl config file,
>>> https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg01741.html
>>>
>>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>> ---
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: xen-devel@lists.xen.org
>>> ---
>>> tools/libxl/Makefile | 2 +-
>>> tools/libxl/libxl.h | 10 ++
>>> tools/libxl/libxl_types.idl | 52 +++++++++
>>> tools/libxl/libxlu_sshm.c | 274 ++++++++++++++++++++++++++++++++++++++++++++
>>> tools/libxl/libxlutil.h | 6 +
>>> tools/xl/xl_parse.c | 24 +++-
>>> 6 files changed, 366 insertions(+), 2 deletions(-)
>>> create mode 100644 tools/libxl/libxlu_sshm.c
>>>
>>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>>> index 2ffb78f5c4..b7effb188b 100644
>>> --- a/tools/libxl/Makefile
>>> +++ b/tools/libxl/Makefile
>>> @@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
>>> AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
>>> AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
>>> LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
>>> - libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
>>> + libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
>>> $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
>>>
>>> $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
>>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>>> index 7cf0f31f68..cf3cbe1ba1 100644
>>> --- a/tools/libxl/libxl.h
>>> +++ b/tools/libxl/libxl.h
>>> @@ -2228,6 +2228,16 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
>>> int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
>>> const char *command_line, char **output);
>>>
>>> +
>>> +/* Functions to stattically set up shared memory regions between two domains
>> ^ statically ^double space
>>
>
> Sorry for the typos.
>
>>
>>> + * for shm-based communication. */
>>> +
>>> +#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
>>> +
>>> +/* TODO: int libxl_sshm_add(libxl_ctx *ctx, uint32_t domid,
>>> + * libxl_static_shm *sshm);
>>> + */
>>> +
>>> #include <libxl_event.h>
>>>
>>> #endif /* LIBXL_H */
>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>> index 8a9849c643..8c68b45add 100644
>>> --- a/tools/libxl/libxl_types.idl
>>> +++ b/tools/libxl/libxl_types.idl
>>> @@ -779,6 +779,57 @@ libxl_device_channel = Struct("device_channel", [
>>> ])),
>>> ])
>>>
>>> +# static shared memory cacheability attributes
>>> +libxl_sshm_cacheattr = Enumeration("sshm_cacheattr", [
>>> + (-1, "UNKNOWN"),
>>> + (0, "UC"),
>>> + (1, "WC"), #x86 only
>>> + (4, "WT"),
>>> + (5, "WP"), #x86 only
>>> + (6, "WB"),
>>> + (7, "SUC"), #x86 only
>>> + (8, "BUFFERABLE"), #ARM only
>>> + (9, "WA"), #ARM only
>>> + ], init_val = "LIBXL_SSHM_CACHEATTR_UNKNOWN")
>>
>> I would only specify UNKNOWN and WB for now.
>
> For here and below, I actually want to left the checks for 'not
> implemented' errors
> to later stages of handling. The typical call flow of xl is like below:
>
> xl --> libxlu_* --> xl --> libxl_* --> hypercalls
>
> I was planning to check for options that are not implemented currently
> in the libxl_*.
>
>>
>>
>>> +# static shared memory shareability attributes
>>> +libxl_sshm_shareattr = Enumeration("sshm_shareattr", [
>>> + (-1, "UNKNOWN"),
>>> + (0, "NON"),
>>> + (2, "OUTER"),
>>> + (3, "INNER"),
>>> + ], init_val = "LIBXL_SSHM_SHAREATTR_UNKNOWN")
>>> +
>>> +libxl_sshm_prot = Enumeration("sshm_prot", [
>>> + (-1, "UNKNOWN"),
>>> + (0, "N"),
>>> + (1, "R"),
>>> + (2, "W"),
>>> + (4, "X"),
>>> + (3, "RW"),
>>> + (5, "RX"),
>>> + (6, "WX"),
>>> + (7, "RWX"),
>>> + ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
>>> +
>>> +libxl_sshm_role = Enumeration("sshm_role", [
>>> + (-1, "UNKNOWN"),
>>> + (0, "MASTER"),
>>> + (1, "SLAVE"),
>>> + ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
>>> +
>>> +libxl_static_shm = Struct("static_shm", [
>>> + ("id", string),
>>> + ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
>>> + ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
>>> + ("prot", libxl_sshm_prot),
>>> + ("arm_shareattr", libxl_sshm_shareattr),
>>> + ("arm_inner_cacheattr", libxl_sshm_cacheattr),
>>> + ("arm_outer_cacheattr", libxl_sshm_cacheattr),
>>
>> I would have a single arm_cacheattr
>
> Why? Am I supposed to use a '|' to combine inner and outer cacheattrs ?
>
>>
>>
>>> + ("x86_cacheattr", libxl_sshm_cacheattr),
>>> + ("role", libxl_sshm_role),
>>> +])
>>> +
>>> libxl_domain_config = Struct("domain_config", [
>>> ("c_info", libxl_domain_create_info),
>>> ("b_info", libxl_domain_build_info),
>>> @@ -797,6 +848,7 @@ libxl_domain_config = Struct("domain_config", [
>>> ("channels", Array(libxl_device_channel, "num_channels")),
>>> ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
>>> ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
>>> + ("sshms", Array(libxl_static_shm, "num_sshms")),
>>>
>>> ("on_poweroff", libxl_action_on_shutdown),
>>> ("on_reboot", libxl_action_on_shutdown),
>>> diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
>>> new file mode 100644
>>> index 0000000000..fcd65af4d9
>>> --- /dev/null
>>> +++ b/tools/libxl/libxlu_sshm.c
>>> @@ -0,0 +1,274 @@
>>> +#include "libxl_osdeps.h" /* must come before any other headers */
>>> +#include "libxlu_internal.h"
>>> +
>>> +#include <ctype.h>
>>> +
>>> +#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
>>> +#define KEY_RE "([_a-zA-Z0-9]+)"
>>> +#define VAL_RE "([^ \t\n,]+)"
>>> +#define EQU_RE PARAM_RE(KEY_RE "\\s*=\\s*" VAL_RE)
>>> +
>>> +#define MASK_4K ((uint64_t)0xfff)
>>> +#define MAX_ID_LEN 128
>>> +#define CACHEATTR_ARM 0
>>> +#define CACHEATTR_X86 1
>>> +
>>> +#define INVAL_ERR(msg, curr_str) do { \
>>> + xlu__sshm_err(cfg, msg, curr_str); \
>>> + rc = EINVAL; \
>>> + goto out; \
>>> + } while(0)
>>> +
>>> +/* set a member in libxl_static_shm and report an error if it's respecified,
>>> + * @curr_str indicates the head of the remaining string. */
>>> +#define SET_VAL(var, name, type, value, curr_str) do { \
>>> + if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) { \
>>> + INVAL_ERR("\"" name "\" respecified", curr_str); \
>>> + } \
>>> + (var) = value; \
>>> + } while(0)
>>> +
>>> +
>>> +static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
>>> + const char *curr_str) {
>>> + fprintf(cfg->report,
>>> + "%s: config parsing error in shared_memory: %s at '%s'\n",
>>> + cfg->config_source, msg, curr_str);
>>> +}
>>> +
>>> +static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
>>> +{
>>> + int rc;
>>> + libxl_sshm_prot new_prot;
>>> +
>>> + if (!strcmp(str, "r") || !strcmp(str, "ro")) {
>>> + new_prot = LIBXL_SSHM_PROT_R;
>>> + } else if (!strcmp(str, "w") || !strcmp(str, "wo")) {
>>> + new_prot = LIBXL_SSHM_PROT_W;
>>> + } else if (!strcmp(str, "x") || !strcmp(str, "xo")) {
>>> + new_prot = LIBXL_SSHM_PROT_X;
>>> + } else if (!strcmp(str, "rw")) {
>>> + new_prot = LIBXL_SSHM_PROT_RW;
>>> + } else if (!strcmp(str, "rx")) {
>>> + new_prot = LIBXL_SSHM_PROT_RX;
>>> + } else if (!strcmp(str, "wx")) {
>>> + new_prot = LIBXL_SSHM_PROT_WX;
>>> + } else if (!strcmp(str, "rwx")) {
>>> + new_prot = LIBXL_SSHM_PROT_RWX;
>>> + } else if (!strcmp(str, "n")) {
>>> + new_prot = LIBXL_SSHM_PROT_N;
>>> + } else {
>>> + INVAL_ERR("invalid permission flags", str);
>>
>> shouldn't this return an error?
>
> This macro does return an error. but it seems that the naming is not
> very appropriate. may I should change it to RET_INVAL or something?
>
>>
>>
>>> + }
>>> +
>>> + SET_VAL(*prot, "permission flags", PROT, new_prot, str);
>>> +
>>> + rc = 0;
>>> +
>>> + out:
>>> + return rc;
>>> +}
>>> +
>>> +static int parse_cacheattr(XLU_Config *cfg, char *str, int arch,
>>> + libxl_sshm_cacheattr *cattr)
>>> +{
>>> + int rc;
>>> + libxl_sshm_cacheattr new_cattr;
>>> +
>>> + if (!strcmp(str, "uc")) {
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_UC;
>>> + } else if (!strcmp(str, "wc")) {
>>> + if (CACHEATTR_X86 != arch) {
>>> + INVAL_ERR("invalid cacheability attribute", str);
>>> + }
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_WC;
>>> + } else if (!strcmp(str, "wt")) {
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_WT;
>>> + } else if (!strcmp(str, "wp")) {
>>> + if (CACHEATTR_X86 != arch) {
>>> + INVAL_ERR("invalid cacheability attribute", str);
>>> + }
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_WP;
>>> + } else if (!strcmp(str, "wb")) {
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_WB;
>>> + } else if (!strcmp(str, "suc")) {
>>> + if (CACHEATTR_X86 != arch) {
>>> + INVAL_ERR("invalid cacheability attribute", str);
>>> + }
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_SUC;
>>> + } else if (!strcmp(str, "bufferable")) {
>>> + if (CACHEATTR_ARM != arch) {
>>> + INVAL_ERR("invalid cacheability attribute", str);
>>> + }
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_BUFFERABLE;
>>> + } else if (!strcmp(str, "wa")) {
>>> + if (CACHEATTR_ARM != arch) {
>>> + INVAL_ERR("invalid cacheability attribute", str);
>>> + }
>>> + new_cattr = LIBXL_SSHM_CACHEATTR_WA;
>>> + } else {
>>> + INVAL_ERR("invalid cacheability attribute", str);
>>
>> shouldn't this return an error?
>>
>>
>>> + }
>>
>> I don't know if the other maintainers agree, but I think we should just
>> check that str is "wb" and fail in all other cases.
>
> Just as pointed out above, I prefer to implement all the options in this part
> of the code, since parsing and actual handling are two somewhat independent
> parts. The checks for options that are not implemented could be left to later
> stages.
>
>>
>>
>>> + SET_VAL(*cattr, "cacheability attributes", CACHEATTR, new_cattr, str);
>>> + rc = 0;
>>> +
>>> + out:
>>> + return rc;
>>> +}
>>> +
>>> +static int parse_shareattr(XLU_Config *cfg, char *str,
>>> + libxl_sshm_shareattr *sattr)
>>> +{
>>> + int rc;
>>> + libxl_sshm_shareattr new_sattr;
>>> +
>>> + if (!strcmp(str, "non")) {
>>> + new_sattr = LIBXL_SSHM_SHAREATTR_NON;
>>> + } else if (!strcmp(str, "outer")) {
>>> + new_sattr = LIBXL_SSHM_SHAREATTR_OUTER;
>>> + } else if (!strcmp(str, "inner")) {
>>> + new_sattr = LIBXL_SSHM_SHAREATTR_INNER;
>>> + } else {
>>> + INVAL_ERR("invalid arm shareability attribute", str);
>>
>> shouldn't this return an error?
>>
>>
>>> + }
>>> +
>>> + SET_VAL(*sattr, "arm shareability attributes", SHAREATTR, new_sattr, str);
>>> + rc = 0;
>>> +
>>> + out:
>>> + return rc;
>>> +}
>>> +
>>> +/* handle key = value pairs */
>>> +static int handle_equ(XLU_Config *cfg, char *key, char *val,
>>> + libxl_static_shm *sshm)
>>> +{
>>> + int rc;
>>> +
>>> + if (!strcmp(key, "id")) {
>>> + if (strlen(val) > MAX_ID_LEN) { INVAL_ERR("id too long", val); }
>>> + if (sshm->id && !strcmp(sshm->id, val)) {
>>> + INVAL_ERR("id respecified", val);
>>> + }
>>> +
>>> + if (NULL == (sshm->id = strdup(val))) {
>>> + fprintf(stderr, "sshm parser out of memory\n");
>>> + rc = ENOMEM;
>>> + goto out;
>>> + }
>>> + } else if (!strcmp(key, "role")) {
>>> + libxl_sshm_role new_role;
>>> +
>>> + if (!strcmp("master", val)) {
>>> + new_role = LIBXL_SSHM_ROLE_MASTER;
>>> + } else if (!strcmp("slave", val)) {
>>> + new_role = LIBXL_SSHM_ROLE_SLAVE;
>>> + } else {
>>> + INVAL_ERR("invalid role", val);
>>> + }
>>> +
>>> + SET_VAL(sshm->role, "role", ROLE, new_role, val);
>>> +
>>> + } else if (!strcmp(key, "begin") || !strcmp(key, "end")) {
>>> + char *endptr;
>>> + int base = 10;
>>> + uint64_t new_bound;
>>> +
>>> + /* could be in hex form */
>>> + if ('0' == val[0] && 'x' == val[1]) { base = 16; }
>>
>> Shouldn't you check that val is at least 2 in length?
>
> Yes. Sorry. I will fix this.
When I tried to add some length checking here I recalled that I have
thought about this problem already and this isn't going to cause troubles.
Because I've already made both key and val NULL-terminated strings.
If the length is 0, val[0] will be '0' and the && will be short-circuit'ed.
If the length is 1, the second check will fail because val[1] will be '0'.
>
>>
>>
>>> + new_bound = strtoull(val, &endptr, base);
>>> + if (ERANGE == errno || *endptr) {
>>> + INVAL_ERR("invalid begin/end", val);
>>> + }
>>> + if (new_bound & MASK_4K) {
>>> + INVAL_ERR("begin/end is not a multiple of 4K", val);
>>> + }
>>> +
>>> + /* begin or end */
>>> + if ('b' == key[0]) {
>>> + SET_VAL(sshm->begin, "beginning address", RANGE, new_bound, val);
>>> + } else {
>>> + SET_VAL(sshm->end, "ending address", RANGE, new_bound, val);
>>> + }
>>> + } else if (!strcmp(key, "prot")) {
>>> + rc = parse_prot(cfg, val, &sshm->prot);
>>> + if (rc) { goto out; }
>>> + } else if (!strcmp(key, "arm_inner_cacheattr")) {
>>> + rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
>>> + &sshm->arm_inner_cacheattr);
>>> + if (rc) { goto out; }
>>> + } else if (!strcmp(key, "arm_outer_cacheattr")) {
>>> + rc = parse_cacheattr(cfg, val, CACHEATTR_ARM,
>>> + &sshm->arm_outer_cacheattr);
>>> + if (rc) { goto out; }
>>> + } else if (!strcmp(key, "x86_cacheattr")) {
>>> + rc = parse_cacheattr(cfg, val, CACHEATTR_X86,
>>> + &sshm->x86_cacheattr);
>>> + if (rc) { goto out; }
>>> + } else if (!strcmp(key, "arm_shareattr")) {
>>> + rc = parse_shareattr(cfg, val, &sshm->arm_shareattr);
>>> + if (rc) { goto out; }
>>> + } else {
>>> + INVAL_ERR("invalid option", key);
>>
>> shouldn't this return an error?
>>
>>
>>> + }
>>> +
>>> + rc = 0;
>>> +
>>> + out:
>>> + return rc;
>>> +}
>>> +
>>> +int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
>>> + libxl_static_shm *sshm)
>>> +{
>>> + int rc;
>>> + regex_t equ_rec;
>>> + char *buf2 = NULL, *ptr = NULL;
>>> + regmatch_t pmatch[3];
>>> +
>>> + rc = regcomp(&equ_rec, EQU_RE, REG_EXTENDED);
>>> + if (rc) {
>>> + fprintf(stderr, "sshm parser failed to initialize\n");
>>> + goto out;
>>> + }
>>> +
>>> + if (NULL == (buf2 = ptr = strdup(spec))) {
>>> + fprintf(stderr, "sshm parser out of memory\n");
>>> + rc = ENOMEM;
>>> + goto out;
>>> + }
>>> +
>>> + while (true) {
>>> + if (!*ptr) { break; }
>>> + if (regexec(&equ_rec, ptr, 3, pmatch, 0)) {
>>> + INVAL_ERR("unrecognized token", ptr);
>>> + }
>>> +
>>> + ptr[pmatch[1].rm_eo] = '\0';
>>> + ptr[pmatch[2].rm_eo] = '\0';
>>> + rc = handle_equ(cfg, ptr + pmatch[1].rm_so,
>>> + ptr + pmatch[2].rm_so, sshm);
>>> + if (rc) { goto out; }
>>> +
>>> + ptr += pmatch[0].rm_eo;
>>> + }
>>> +
>>> + if (*ptr) { INVAL_ERR("invalid syntax", ptr); }
>>> +
>>> + rc = 0;
>>> +
>>> + out:
>>> + if (buf2) { free(buf2); }
>>> + regfree(&equ_rec);
>>> + return rc;
>>> +}
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> +
>>> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
>>> index e81b644c01..ee39cb5bdc 100644
>>> --- a/tools/libxl/libxlutil.h
>>> +++ b/tools/libxl/libxlutil.h
>>> @@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
>>> int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
>>> libxl_device_nic *nic);
>>>
>>> +/*
>>> + * static shared memory specification parsing
>>> + */
>>> +int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
>>> + libxl_static_shm *sshm);
>>> +
>>> #endif /* LIBXLUTIL_H */
>>>
>>> /*
>>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>>> index 5c2bf17222..82d955b8b9 100644
>>> --- a/tools/xl/xl_parse.c
>>> +++ b/tools/xl/xl_parse.c
>>> @@ -813,7 +813,7 @@ void parse_config_data(const char *config_source,
>>> long l, vcpus = 0;
>>> XLU_Config *config;
>>> XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
>>> - *usbctrls, *usbdevs, *p9devs;
>>> + *usbctrls, *usbdevs, *p9devs, *sshms;
>>> XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
>>> *mca_caps;
>>> int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
>>> @@ -1392,6 +1392,28 @@ void parse_config_data(const char *config_source,
>>> }
>>> }
>>>
>>> + if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
>>> + d_config->num_sshms = 0;
>>> + d_config->sshms = NULL;
>>> + while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
>>> + libxl_static_shm *sshm;
>>> + char *buf2 = strdup(buf);
>>> + int ret;
>>> +
>>> + sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
>>> + d_config->num_sshms,
>>> + libxl_static_shm_init);
>>> + ret = xlu_sshm_parse(config, buf2, sshm);
>>> + if (ret) {
>>> + fprintf(stderr,
>>> + "xl: Invalid argument for static_shm: %s", buf2);
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + free(buf2);
>>> + }
>>> + }
>>> +
>>> if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
>>> libxl_device_p9 *p9;
>>> char *security_model = NULL;
>>> --
>>> 2.13.3
>>>
>
> Cheers,
>
> Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
2017-07-20 1:43 ` Zhongze Liu
2017-07-20 10:14 ` Zhongze Liu
@ 2017-07-21 0:07 ` Stefano Stabellini
1 sibling, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2017-07-21 0:07 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, xen-devel
On Thu, 20 Jul 2017, Zhongze Liu wrote:
> >> #endif /* LIBXL_H */
> >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> index 8a9849c643..8c68b45add 100644
> >> --- a/tools/libxl/libxl_types.idl
> >> +++ b/tools/libxl/libxl_types.idl
> >> @@ -779,6 +779,57 @@ libxl_device_channel = Struct("device_channel", [
> >> ])),
> >> ])
> >>
> >> +# static shared memory cacheability attributes
> >> +libxl_sshm_cacheattr = Enumeration("sshm_cacheattr", [
> >> + (-1, "UNKNOWN"),
> >> + (0, "UC"),
> >> + (1, "WC"), #x86 only
> >> + (4, "WT"),
> >> + (5, "WP"), #x86 only
> >> + (6, "WB"),
> >> + (7, "SUC"), #x86 only
> >> + (8, "BUFFERABLE"), #ARM only
> >> + (9, "WA"), #ARM only
> >> + ], init_val = "LIBXL_SSHM_CACHEATTR_UNKNOWN")
> >
> > I would only specify UNKNOWN and WB for now.
>
> For here and below, I actually want to left the checks for 'not
> implemented' errors
> to later stages of handling. The typical call flow of xl is like below:
>
> xl --> libxlu_* --> xl --> libxl_* --> hypercalls
>
> I was planning to check for options that are not implemented currently
> in the libxl_*.
Why? You can print out "not implemented" for anything that is not "WB",
right?
> >> +# static shared memory shareability attributes
> >> +libxl_sshm_shareattr = Enumeration("sshm_shareattr", [
> >> + (-1, "UNKNOWN"),
> >> + (0, "NON"),
> >> + (2, "OUTER"),
> >> + (3, "INNER"),
> >> + ], init_val = "LIBXL_SSHM_SHAREATTR_UNKNOWN")
> >> +
> >> +libxl_sshm_prot = Enumeration("sshm_prot", [
> >> + (-1, "UNKNOWN"),
> >> + (0, "N"),
> >> + (1, "R"),
> >> + (2, "W"),
> >> + (4, "X"),
> >> + (3, "RW"),
> >> + (5, "RX"),
> >> + (6, "WX"),
> >> + (7, "RWX"),
> >> + ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
> >> +
> >> +libxl_sshm_role = Enumeration("sshm_role", [
> >> + (-1, "UNKNOWN"),
> >> + (0, "MASTER"),
> >> + (1, "SLAVE"),
> >> + ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
> >> +
> >> +libxl_static_shm = Struct("static_shm", [
> >> + ("id", string),
> >> + ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> >> + ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
> >> + ("prot", libxl_sshm_prot),
> >> + ("arm_shareattr", libxl_sshm_shareattr),
> >> + ("arm_inner_cacheattr", libxl_sshm_cacheattr),
> >> + ("arm_outer_cacheattr", libxl_sshm_cacheattr),
> >
> > I would have a single arm_cacheattr
>
> Why? Am I supposed to use a '|' to combine inner and outer cacheattrs ?
Because the cacheattr are the same no matter the inner or outer setting.
I don't see why we should have two different keys (arm_inner_cacheattr
and arm_outer_cacheattr) instead of only one.
> >
> >
> >> + ("x86_cacheattr", libxl_sshm_cacheattr),
> >> + ("role", libxl_sshm_role),
> >> +])
> >> +
> >> libxl_domain_config = Struct("domain_config", [
> >> ("c_info", libxl_domain_create_info),
> >> ("b_info", libxl_domain_build_info),
> >> @@ -797,6 +848,7 @@ libxl_domain_config = Struct("domain_config", [
> >> ("channels", Array(libxl_device_channel, "num_channels")),
> >> ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> >> ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
> >> + ("sshms", Array(libxl_static_shm, "num_sshms")),
> >>
> >> ("on_poweroff", libxl_action_on_shutdown),
> >> ("on_reboot", libxl_action_on_shutdown),
> >> diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
> >> new file mode 100644
> >> index 0000000000..fcd65af4d9
> >> --- /dev/null
> >> +++ b/tools/libxl/libxlu_sshm.c
> >> @@ -0,0 +1,274 @@
> >> +#include "libxl_osdeps.h" /* must come before any other headers */
> >> +#include "libxlu_internal.h"
> >> +
> >> +#include <ctype.h>
> >> +
> >> +#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
> >> +#define KEY_RE "([_a-zA-Z0-9]+)"
> >> +#define VAL_RE "([^ \t\n,]+)"
> >> +#define EQU_RE PARAM_RE(KEY_RE "\\s*=\\s*" VAL_RE)
> >> +
> >> +#define MASK_4K ((uint64_t)0xfff)
> >> +#define MAX_ID_LEN 128
> >> +#define CACHEATTR_ARM 0
> >> +#define CACHEATTR_X86 1
> >> +
> >> +#define INVAL_ERR(msg, curr_str) do { \
> >> + xlu__sshm_err(cfg, msg, curr_str); \
> >> + rc = EINVAL; \
> >> + goto out; \
> >> + } while(0)
> >> +
> >> +/* set a member in libxl_static_shm and report an error if it's respecified,
> >> + * @curr_str indicates the head of the remaining string. */
> >> +#define SET_VAL(var, name, type, value, curr_str) do { \
> >> + if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) { \
> >> + INVAL_ERR("\"" name "\" respecified", curr_str); \
> >> + } \
> >> + (var) = value; \
> >> + } while(0)
> >> +
> >> +
> >> +static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
> >> + const char *curr_str) {
> >> + fprintf(cfg->report,
> >> + "%s: config parsing error in shared_memory: %s at '%s'\n",
> >> + cfg->config_source, msg, curr_str);
> >> +}
> >> +
> >> +static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
> >> +{
> >> + int rc;
> >> + libxl_sshm_prot new_prot;
> >> +
> >> + if (!strcmp(str, "r") || !strcmp(str, "ro")) {
> >> + new_prot = LIBXL_SSHM_PROT_R;
> >> + } else if (!strcmp(str, "w") || !strcmp(str, "wo")) {
> >> + new_prot = LIBXL_SSHM_PROT_W;
> >> + } else if (!strcmp(str, "x") || !strcmp(str, "xo")) {
> >> + new_prot = LIBXL_SSHM_PROT_X;
> >> + } else if (!strcmp(str, "rw")) {
> >> + new_prot = LIBXL_SSHM_PROT_RW;
> >> + } else if (!strcmp(str, "rx")) {
> >> + new_prot = LIBXL_SSHM_PROT_RX;
> >> + } else if (!strcmp(str, "wx")) {
> >> + new_prot = LIBXL_SSHM_PROT_WX;
> >> + } else if (!strcmp(str, "rwx")) {
> >> + new_prot = LIBXL_SSHM_PROT_RWX;
> >> + } else if (!strcmp(str, "n")) {
> >> + new_prot = LIBXL_SSHM_PROT_N;
> >> + } else {
> >> + INVAL_ERR("invalid permission flags", str);
> >
> > shouldn't this return an error?
>
> This macro does return an error. but it seems that the naming is not
> very appropriate. may I should change it to RET_INVAL or something?
Ops, sorry! I didn't realize INVAL_ERR sets rc and even calls goto! I'll
leave this to the tools maintainer, but I wouldn't want to have a macro,
which looks like it's just setting an error message, also do a goto.
> >
> >
> >> + }
> >> +
> >> + SET_VAL(*prot, "permission flags", PROT, new_prot, str);
> >> +
> >> + rc = 0;
> >> +
> >> + out:
> >> + return rc;
> >> +}
> >> +
> >> +static int parse_cacheattr(XLU_Config *cfg, char *str, int arch,
> >> + libxl_sshm_cacheattr *cattr)
> >> +{
> >> + int rc;
> >> + libxl_sshm_cacheattr new_cattr;
> >> +
> >> + if (!strcmp(str, "uc")) {
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_UC;
> >> + } else if (!strcmp(str, "wc")) {
> >> + if (CACHEATTR_X86 != arch) {
> >> + INVAL_ERR("invalid cacheability attribute", str);
> >> + }
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_WC;
> >> + } else if (!strcmp(str, "wt")) {
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_WT;
> >> + } else if (!strcmp(str, "wp")) {
> >> + if (CACHEATTR_X86 != arch) {
> >> + INVAL_ERR("invalid cacheability attribute", str);
> >> + }
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_WP;
> >> + } else if (!strcmp(str, "wb")) {
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_WB;
> >> + } else if (!strcmp(str, "suc")) {
> >> + if (CACHEATTR_X86 != arch) {
> >> + INVAL_ERR("invalid cacheability attribute", str);
> >> + }
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_SUC;
> >> + } else if (!strcmp(str, "bufferable")) {
> >> + if (CACHEATTR_ARM != arch) {
> >> + INVAL_ERR("invalid cacheability attribute", str);
> >> + }
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_BUFFERABLE;
> >> + } else if (!strcmp(str, "wa")) {
> >> + if (CACHEATTR_ARM != arch) {
> >> + INVAL_ERR("invalid cacheability attribute", str);
> >> + }
> >> + new_cattr = LIBXL_SSHM_CACHEATTR_WA;
> >> + } else {
> >> + INVAL_ERR("invalid cacheability attribute", str);
> >
> > shouldn't this return an error?
> >
> >
> >> + }
> >
> > I don't know if the other maintainers agree, but I think we should just
> > check that str is "wb" and fail in all other cases.
>
> Just as pointed out above, I prefer to implement all the options in this part
> of the code, since parsing and actual handling are two somewhat independent
> parts. The checks for options that are not implemented could be left to later
> stages.
It's important that we check now for what is implemented and return
error for anything that is not implemented. How we do that is less
important. However, given that we might change our minds about what
should be the options in the future, I would prefer to only handle
explicitly the one option we intend to implement now.
That said, I would probably wait for a second opinion from one of the
tools maintainers.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
2017-07-20 10:14 ` Zhongze Liu
@ 2017-07-21 0:10 ` Stefano Stabellini
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2017-07-21 0:10 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, xen-devel
On Thu, 20 Jul 2017, Zhongze Liu wrote:
> >>> + } else if (!strcmp(key, "begin") || !strcmp(key, "end")) {
> >>> + char *endptr;
> >>> + int base = 10;
> >>> + uint64_t new_bound;
> >>> +
> >>> + /* could be in hex form */
> >>> + if ('0' == val[0] && 'x' == val[1]) { base = 16; }
> >>
> >> Shouldn't you check that val is at least 2 in length?
> >
> > Yes. Sorry. I will fix this.
>
> When I tried to add some length checking here I recalled that I have
> thought about this problem already and this isn't going to cause troubles.
> Because I've already made both key and val NULL-terminated strings.
> If the length is 0, val[0] will be '0' and the && will be short-circuit'ed.
> If the length is 1, the second check will fail because val[1] will be '0'.
OK. It might be worth adding an in-code comment about it.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-21 0:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-19 3:18 [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry Zhongze Liu
2017-07-19 19:24 ` Stefano Stabellini
2017-07-20 1:43 ` Zhongze Liu
2017-07-20 10:14 ` Zhongze Liu
2017-07-21 0:10 ` Stefano Stabellini
2017-07-21 0:07 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.