|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH] tools/libxl : add struct and parsing utils for the 'static_shm' xl config entry
Hi Stefano, 2017-07-20 3:24 GMT+08:00 Stefano Stabellini <sstabellini@xxxxxxxxxx>: > 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@xxxxxxxxx> >> --- >> Cc: Wei Liu <wei.liu2@xxxxxxxxxx> >> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx> >> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx> >> Cc: Julien Grall <julien.grall@xxxxxxx> >> Cc: xen-devel@xxxxxxxxxxxxx >> --- >> 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |