[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



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


> + * 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.