[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
2017-07-20 9:43 GMT+08:00 Zhongze Liu <blackskygg@xxxxxxxxx>: > 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. 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |