|
[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 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |