| Message ID | 166777846906.1238089.13466320510516058152.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl-cli test and usability updates | expand |
On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote: > > The core of 'cxl list' knows, among other things, how to filter memdevs by > > their connectivity to a root decoder, enabled status, and how to identify > > memdevs by name, id, serial number. Use the fact that the json-c object > > array returned by cxl_filter_walk() also includes the corresponding libcxl > > objects to populate and validate the memdev target list for 'cxl > > create-region'. > > > > With this in place a default set of memdev targets can be derived from the > > specified root decoder, and the connectivity is validated by the same logic > > that prepares the hierarchical json topology. The argument list becomes > > as tolerant of different id formats as 'cxl list'. For example "mem9" and > > "9" are equivalent. > > > > Comma separated lists are also allowed, e.g. "mem9,mem10". However the > > sorting of memdevs by filter position falls back to the 'cxl list' order in > > that case. In other words: > > > > arg order region position > > mem9 mem10 => [0]: mem9 [1]: mem10 > > mem10 mem9 => [0]: mem10 [1]: mem9 > > mem9,mem10 => [0]: mem9 [1]: mem10 > > mem10,mem9 => [0]: mem9 [1]: mem10 Hm, this does create an awkward situation where cxl create-region -m mem10 mem9 (same as first arg order above) can behave differently from cxl create-region -m "mem10 mem9" (behaves like 4th arg order above) i.e. if the args happen to get quoted into a single space separated list, then it switches back to cxl-list ordering as "mem10 mem9" gets treated as a single filter string. I wonder if we should add another step after collect_memdevs() (or change memdev_sort()) to return the arg order by default, so: mem9 mem10 => [0]: mem9 [1]: mem10 mem10 mem9 => [0]: mem10 [1]: mem9 mem9,mem10 => [0]: mem9 [1]: mem10 mem10,mem9 => [0]: mem10 [1]: mem9 "mem9 mem10" => [0]: mem9 [1]: mem10 "mem10 mem9" => [0]: mem10 [1]: mem9 i.e. region positioning stays consistent no matter what combination of field separators, or quoting, or word splitting ends up happening. Then (future patches) add a --relax-ordering or --allow-reordering option that can fix up the order for creating a region successfully with the given set. With this, the only two options create-region has are either try the user-specified order exactly, or reorder to something that wil be successful. The cxl-list default order doesn't seem as a useful 'mode' to have as it can be hit-or-miss. > > > > Note that 'cxl list' order groups memdevs by port, later this will need to > > augmented with a sort implementation that orders memdevs by a topology > > compatible decode order. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > cxl/region.c | 274 +++++++++++++++++++++++++++++++++++++--------------------- > > 1 file changed, 175 insertions(+), 99 deletions(-) > > > > diff --git a/cxl/region.c b/cxl/region.c > > index c6d7d1a973a8..e47709754447 100644 > > --- a/cxl/region.c > > +++ b/cxl/region.c > > @@ -40,8 +40,10 @@ struct parsed_params { > > u64 ep_min_size; > > int ways; > > int granularity; > > - const char **targets; > > - int num_targets; > > + struct json_object *memdevs; > > + int num_memdevs; > > + int argc; > > + const char **argv; > > struct cxl_decoder *root_decoder; > > enum cxl_decoder_mode mode; > > }; > > @@ -99,16 +101,148 @@ static const struct option destroy_options[] = { > > OPT_END(), > > }; > > > > -static int parse_create_options(int argc, const char **argv, > > - struct parsed_params *p) > > +/* > > + * Convert an array of strings into a single comma-separated-value > > + * string that can be passed as a single 'filter' string to > > + * cxl_filter_walk() > > + */ > > +static const char *to_csv(int count, const char **strings) > > { > > + ssize_t len = count + 1, cursor = 0; > > + char *csv; > > int i; > > > > + if (!count) > > + return NULL; > > + > > + for (i = 0; i < count; i++) > > + len += strlen(strings[i]); > > + csv = calloc(1, len); > > + if (!csv) > > + return NULL; > > + for (i = 0; i < count; i++) { > > + cursor += snprintf(csv + cursor, len - cursor, "%s%s", > > + strings[i], i + 1 < count ? "," : ""); > > + if (cursor >= len) { > > + csv[len] = 0; > > + break; > > + } > > + } > > + return csv; > > +} > > + > > +static struct sort_context { > > + int count; > > + const char **sort; > > +} sort_context; > > + > > +static int memdev_filter_pos(struct json_object *jobj, int count, const char **sort) > > +{ > > + struct cxl_memdev *memdev = json_object_get_userdata(jobj); > > + int pos; > > + > > + for (pos = 0; pos < count; pos++) > > + if (util_cxl_memdev_filter(memdev, sort[pos], NULL)) > > + return pos; > > + return count; > > +} > > + > > +static int memdev_sort(const void *a, const void *b) > > +{ > > + int a_pos, b_pos, count = sort_context.count; > > + const char **sort = sort_context.sort; > > + struct json_object **a_obj, **b_obj; > > + > > + a_obj = (struct json_object **) a; > > + b_obj = (struct json_object **) b; > > + > > + a_pos = memdev_filter_pos(*a_obj, count, sort); > > + b_pos = memdev_filter_pos(*b_obj, count, sort); > > + > > + return a_pos - b_pos; > > +} > > + > > +static struct json_object *collect_memdevs(struct cxl_ctx *ctx, > > + const char *decoder, int count, > > + const char **mems) > > +{ > > + const char *csv = to_csv(count, mems); > > + struct cxl_filter_params filter_params = { > > + .decoder_filter = decoder, > > + .memdevs = true, > > + .memdev_filter = csv, > > + }; > > + struct json_object *jmemdevs; > > + > > + jmemdevs = cxl_filter_walk(ctx, &filter_params); > > + > > + if (!jmemdevs) { > > + log_err(&rl, "failed to retrieve memdevs\n"); > > + goto out; > > + } > > + > > + if (json_object_array_length(jmemdevs) == 0) { > > + log_err(&rl, > > + "no active memdevs found: decoder: %s filter: %s\n", > > + decoder, csv ? csv : "none"); > > + json_object_put(jmemdevs); > > + jmemdevs = NULL; > > + goto out; > > + } > > + > > + sort_context = (struct sort_context){ > > + .count = count, > > + .sort = mems, > > + }; > > + json_object_array_sort(jmemdevs, memdev_sort); > > + > > +out: > > + free((void *)csv); > > + return jmemdevs; > > +} > > + > > +static bool validate_ways(struct parsed_params *p, int count) > > +{ > > + /* > > + * Validate interleave ways against targets found in the topology. If > > + * the targets were specified, then non-default param.ways must equal > > + * that number of targets. > > + */ > > + if (p->ways > p->num_memdevs || (count && p->ways != p->num_memdevs)) { This falls over when doing something like cxl create-region -m mem0,mem1 as @count ends up being '1' from the single filter spec passed in. I think doing a 'count = p->num_memdevs' (below) should fix it - we don't care about how many separate filter specs were passed in, once they have been combined, and the resulting memdevfs collected. > > + log_err(&rl, > > + "Interleave ways %d is %s than number of memdevs %s: %d\n", > > + p->ways, p->ways > p->num_memdevs ? "greater" : "less", > > + count ? "specified" : "found", p->num_memdevs); > > + return false; > > + } > > + return true; > > +} > > + > > +static int parse_create_options(struct cxl_ctx *ctx, int count, > > + const char **mems, struct parsed_params *p) > > +{ > > if (!param.root_decoder) { > > log_err(&rl, "no root decoder specified\n"); > > return -EINVAL; > > } > > > > + /* > > + * For all practical purposes, -m is the default target type, but > > + * hold off on actively making that decision until a second target > > + * option is available. > > + */ > > + if (!param.memdevs) { > > + log_err(&rl, > > + "must specify option for target object types (-m)\n"); > > + return -EINVAL; > > + } > > + > > + /* Collect the valid memdevs relative to the given root decoder */ > > + p->memdevs = collect_memdevs(ctx, param.root_decoder, count, mems); > > + if (!p->memdevs) > > + return -ENXIO; > > + p->num_memdevs = json_object_array_length(p->memdevs); Per the above comment, either count = p->num_memdevs; here, or alternatively, the interleave ways validation shouldn't use @count anymore. > > + > > if (param.type) { > > p->mode = cxl_decoder_mode_from_ident(param.type); > > if (p->mode == CXL_DECODER_MODE_NONE) { <snip> > > > > @@ -664,8 +738,10 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, > > if (rc) > > return rc; > > > > - if (action == ACTION_CREATE) > > - return create_region(ctx, count, p); > > + if (action == ACTION_CREATE) { > > + rc = create_region(ctx, count, p); > > + json_object_put(p->memdevs); Should this dereference happen just before returning from create_region() since that is where cxl_filter_walk() was called from, and is also where p->memdevs is last used. > > + } > > > > cxl_bus_foreach(ctx, bus) { > > struct cxl_decoder *decoder; > >
Verma, Vishal L wrote: > On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote: > > > The core of 'cxl list' knows, among other things, how to filter memdevs by > > > their connectivity to a root decoder, enabled status, and how to identify > > > memdevs by name, id, serial number. Use the fact that the json-c object > > > array returned by cxl_filter_walk() also includes the corresponding libcxl > > > objects to populate and validate the memdev target list for 'cxl > > > create-region'. > > > > > > With this in place a default set of memdev targets can be derived from the > > > specified root decoder, and the connectivity is validated by the same logic > > > that prepares the hierarchical json topology. The argument list becomes > > > as tolerant of different id formats as 'cxl list'. For example "mem9" and > > > "9" are equivalent. > > > > > > Comma separated lists are also allowed, e.g. "mem9,mem10". However the > > > sorting of memdevs by filter position falls back to the 'cxl list' order in > > > that case. In other words: > > > > > > arg order region position > > > mem9 mem10 => [0]: mem9 [1]: mem10 > > > mem10 mem9 => [0]: mem10 [1]: mem9 > > > mem9,mem10 => [0]: mem9 [1]: mem10 > > > mem10,mem9 => [0]: mem9 [1]: mem10 > > Hm, this does create an awkward situation where > > cxl create-region -m mem10 mem9 (same as first arg order above) > > can behave differently from > > cxl create-region -m "mem10 mem9" (behaves like 4th arg order above) > > i.e. if the args happen to get quoted into a single space separated > list, then it switches back to cxl-list ordering as "mem10 mem9" gets > treated as a single filter string. > > I wonder if we should add another step after collect_memdevs() (or > change memdev_sort()) to return the arg order by default, so: > > mem9 mem10 => [0]: mem9 [1]: mem10 > mem10 mem9 => [0]: mem10 [1]: mem9 > mem9,mem10 => [0]: mem9 [1]: mem10 > mem10,mem9 => [0]: mem10 [1]: mem9 > "mem9 mem10" => [0]: mem9 [1]: mem10 > "mem10 mem9" => [0]: mem10 [1]: mem9 > > i.e. region positioning stays consistent no matter what combination of > field separators, or quoting, or word splitting ends up happening. > > Then (future patches) add a --relax-ordering or --allow-reordering > option that can fix up the order for creating a region successfully > with the given set. > > With this, the only two options create-region has are either try the > user-specified order exactly, or reorder to something that wil be > successful. The cxl-list default order doesn't seem as a useful 'mode' > to have as it can be hit-or-miss. Yeah, that timebomb is probably not something I should leave ticking. Fixed up now, but with the 'fun' of C string manipulation. > > > > > > Note that 'cxl list' order groups memdevs by port, later this will need to > > > augmented with a sort implementation that orders memdevs by a topology > > > compatible decode order. > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > cxl/region.c | 274 +++++++++++++++++++++++++++++++++++++--------------------- > > > 1 file changed, 175 insertions(+), 99 deletions(-) > > > > > > diff --git a/cxl/region.c b/cxl/region.c > > > index c6d7d1a973a8..e47709754447 100644 > > > --- a/cxl/region.c > > > +++ b/cxl/region.c > > > @@ -40,8 +40,10 @@ struct parsed_params { > > > u64 ep_min_size; > > > int ways; > > > int granularity; > > > - const char **targets; > > > - int num_targets; > > > + struct json_object *memdevs; > > > + int num_memdevs; > > > + int argc; > > > + const char **argv; > > > struct cxl_decoder *root_decoder; > > > enum cxl_decoder_mode mode; > > > }; > > > @@ -99,16 +101,148 @@ static const struct option destroy_options[] = { > > > OPT_END(), > > > }; > > > > > > -static int parse_create_options(int argc, const char **argv, > > > - struct parsed_params *p) > > > +/* > > > + * Convert an array of strings into a single comma-separated-value > > > + * string that can be passed as a single 'filter' string to > > > + * cxl_filter_walk() > > > + */ > > > +static const char *to_csv(int count, const char **strings) > > > { > > > + ssize_t len = count + 1, cursor = 0; > > > + char *csv; > > > int i; > > > > > > + if (!count) > > > + return NULL; > > > + > > > + for (i = 0; i < count; i++) > > > + len += strlen(strings[i]); > > > + csv = calloc(1, len); > > > + if (!csv) > > > + return NULL; > > > + for (i = 0; i < count; i++) { > > > + cursor += snprintf(csv + cursor, len - cursor, "%s%s", > > > + strings[i], i + 1 < count ? "," : ""); > > > + if (cursor >= len) { > > > + csv[len] = 0; > > > + break; > > > + } > > > + } > > > + return csv; > > > +} > > > + > > > +static struct sort_context { > > > + int count; > > > + const char **sort; > > > +} sort_context; > > > + > > > +static int memdev_filter_pos(struct json_object *jobj, int count, const char **sort) > > > +{ > > > + struct cxl_memdev *memdev = json_object_get_userdata(jobj); > > > + int pos; > > > + > > > + for (pos = 0; pos < count; pos++) > > > + if (util_cxl_memdev_filter(memdev, sort[pos], NULL)) > > > + return pos; > > > + return count; > > > +} > > > + > > > +static int memdev_sort(const void *a, const void *b) > > > +{ > > > + int a_pos, b_pos, count = sort_context.count; > > > + const char **sort = sort_context.sort; > > > + struct json_object **a_obj, **b_obj; > > > + > > > + a_obj = (struct json_object **) a; > > > + b_obj = (struct json_object **) b; > > > + > > > + a_pos = memdev_filter_pos(*a_obj, count, sort); > > > + b_pos = memdev_filter_pos(*b_obj, count, sort); > > > + > > > + return a_pos - b_pos; > > > +} > > > + > > > +static struct json_object *collect_memdevs(struct cxl_ctx *ctx, > > > + const char *decoder, int count, > > > + const char **mems) > > > +{ > > > + const char *csv = to_csv(count, mems); > > > + struct cxl_filter_params filter_params = { > > > + .decoder_filter = decoder, > > > + .memdevs = true, > > > + .memdev_filter = csv, > > > + }; > > > + struct json_object *jmemdevs; > > > + > > > + jmemdevs = cxl_filter_walk(ctx, &filter_params); > > > + > > > + if (!jmemdevs) { > > > + log_err(&rl, "failed to retrieve memdevs\n"); > > > + goto out; > > > + } > > > + > > > + if (json_object_array_length(jmemdevs) == 0) { > > > + log_err(&rl, > > > + "no active memdevs found: decoder: %s filter: %s\n", > > > + decoder, csv ? csv : "none"); > > > + json_object_put(jmemdevs); > > > + jmemdevs = NULL; > > > + goto out; > > > + } > > > + > > > + sort_context = (struct sort_context){ > > > + .count = count, > > > + .sort = mems, > > > + }; > > > + json_object_array_sort(jmemdevs, memdev_sort); > > > + > > > +out: > > > + free((void *)csv); > > > + return jmemdevs; > > > +} > > > + > > > +static bool validate_ways(struct parsed_params *p, int count) > > > +{ > > > + /* > > > + * Validate interleave ways against targets found in the topology. If > > > + * the targets were specified, then non-default param.ways must equal > > > + * that number of targets. > > > + */ > > > + if (p->ways > p->num_memdevs || (count && p->ways != p->num_memdevs)) { > > This falls over when doing something like > > cxl create-region -m mem0,mem1 > > as @count ends up being '1' from the single filter spec passed in. > > I think doing a 'count = p->num_memdevs' (below) should fix it - we > don't care about how many separate filter specs were passed in, once > they have been combined, and the resulting memdevfs collected. I think it matters because if someone typos something like "mem8, mem8, mem10" we want the tool to validate with @count at 3 even though num_memdevs would only return 2 in that case. I am solving this by turning the @count argument into an in/out that gets updated to be the number of individual elements of a target list. Where @count is 4 for this target list on entry: "mem1,mem2" mem3 mem4 "mem5 mem6" ...and 6 on return from collect_memdevs(). > > > > + log_err(&rl, > > > + "Interleave ways %d is %s than number of memdevs %s: %d\n", > > > + p->ways, p->ways > p->num_memdevs ? "greater" : "less", > > > + count ? "specified" : "found", p->num_memdevs); > > > + return false; > > > + } > > > + return true; > > > +} > > > + > > > +static int parse_create_options(struct cxl_ctx *ctx, int count, > > > + const char **mems, struct parsed_params *p) > > > +{ > > > if (!param.root_decoder) { > > > log_err(&rl, "no root decoder specified\n"); > > > return -EINVAL; > > > } > > > > > > + /* > > > + * For all practical purposes, -m is the default target type, but > > > + * hold off on actively making that decision until a second target > > > + * option is available. > > > + */ > > > + if (!param.memdevs) { > > > + log_err(&rl, > > > + "must specify option for target object types (-m)\n"); > > > + return -EINVAL; > > > + } > > > + > > > + /* Collect the valid memdevs relative to the given root decoder */ > > > + p->memdevs = collect_memdevs(ctx, param.root_decoder, count, mems); > > > + if (!p->memdevs) > > > + return -ENXIO; > > > + p->num_memdevs = json_object_array_length(p->memdevs); > > Per the above comment, either > > count = p->num_memdevs; > > here, or alternatively, the interleave ways validation shouldn't use > @count anymore. > > > > + > > > if (param.type) { > > > p->mode = cxl_decoder_mode_from_ident(param.type); > > > if (p->mode == CXL_DECODER_MODE_NONE) { > > <snip> > > > > > > > @@ -664,8 +738,10 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, > > > if (rc) > > > return rc; > > > > > > - if (action == ACTION_CREATE) > > > - return create_region(ctx, count, p); > > > + if (action == ACTION_CREATE) { > > > + rc = create_region(ctx, count, p); > > > + json_object_put(p->memdevs); > > Should this dereference happen just before returning from > create_region() since that is where cxl_filter_walk() was called from, > and is also where p->memdevs is last used. Yeah, not sure why I had it outside.
diff --git a/cxl/region.c b/cxl/region.c index c6d7d1a973a8..e47709754447 100644 --- a/cxl/region.c +++ b/cxl/region.c @@ -40,8 +40,10 @@ struct parsed_params { u64 ep_min_size; int ways; int granularity; - const char **targets; - int num_targets; + struct json_object *memdevs; + int num_memdevs; + int argc; + const char **argv; struct cxl_decoder *root_decoder; enum cxl_decoder_mode mode; }; @@ -99,16 +101,148 @@ static const struct option destroy_options[] = { OPT_END(), }; -static int parse_create_options(int argc, const char **argv, - struct parsed_params *p) +/* + * Convert an array of strings into a single comma-separated-value + * string that can be passed as a single 'filter' string to + * cxl_filter_walk() + */ +static const char *to_csv(int count, const char **strings) { + ssize_t len = count + 1, cursor = 0; + char *csv; int i; + if (!count) + return NULL; + + for (i = 0; i < count; i++) + len += strlen(strings[i]); + csv = calloc(1, len); + if (!csv) + return NULL; + for (i = 0; i < count; i++) { + cursor += snprintf(csv + cursor, len - cursor, "%s%s", + strings[i], i + 1 < count ? "," : ""); + if (cursor >= len) { + csv[len] = 0; + break; + } + } + return csv; +} + +static struct sort_context { + int count; + const char **sort; +} sort_context; + +static int memdev_filter_pos(struct json_object *jobj, int count, const char **sort) +{ + struct cxl_memdev *memdev = json_object_get_userdata(jobj); + int pos; + + for (pos = 0; pos < count; pos++) + if (util_cxl_memdev_filter(memdev, sort[pos], NULL)) + return pos; + return count; +} + +static int memdev_sort(const void *a, const void *b) +{ + int a_pos, b_pos, count = sort_context.count; + const char **sort = sort_context.sort; + struct json_object **a_obj, **b_obj; + + a_obj = (struct json_object **) a; + b_obj = (struct json_object **) b; + + a_pos = memdev_filter_pos(*a_obj, count, sort); + b_pos = memdev_filter_pos(*b_obj, count, sort); + + return a_pos - b_pos; +} + +static struct json_object *collect_memdevs(struct cxl_ctx *ctx, + const char *decoder, int count, + const char **mems) +{ + const char *csv = to_csv(count, mems); + struct cxl_filter_params filter_params = { + .decoder_filter = decoder, + .memdevs = true, + .memdev_filter = csv, + }; + struct json_object *jmemdevs; + + jmemdevs = cxl_filter_walk(ctx, &filter_params); + + if (!jmemdevs) { + log_err(&rl, "failed to retrieve memdevs\n"); + goto out; + } + + if (json_object_array_length(jmemdevs) == 0) { + log_err(&rl, + "no active memdevs found: decoder: %s filter: %s\n", + decoder, csv ? csv : "none"); + json_object_put(jmemdevs); + jmemdevs = NULL; + goto out; + } + + sort_context = (struct sort_context){ + .count = count, + .sort = mems, + }; + json_object_array_sort(jmemdevs, memdev_sort); + +out: + free((void *)csv); + return jmemdevs; +} + +static bool validate_ways(struct parsed_params *p, int count) +{ + /* + * Validate interleave ways against targets found in the topology. If + * the targets were specified, then non-default param.ways must equal + * that number of targets. + */ + if (p->ways > p->num_memdevs || (count && p->ways != p->num_memdevs)) { + log_err(&rl, + "Interleave ways %d is %s than number of memdevs %s: %d\n", + p->ways, p->ways > p->num_memdevs ? "greater" : "less", + count ? "specified" : "found", p->num_memdevs); + return false; + } + return true; +} + +static int parse_create_options(struct cxl_ctx *ctx, int count, + const char **mems, struct parsed_params *p) +{ if (!param.root_decoder) { log_err(&rl, "no root decoder specified\n"); return -EINVAL; } + /* + * For all practical purposes, -m is the default target type, but + * hold off on actively making that decision until a second target + * option is available. + */ + if (!param.memdevs) { + log_err(&rl, + "must specify option for target object types (-m)\n"); + return -EINVAL; + } + + /* Collect the valid memdevs relative to the given root decoder */ + p->memdevs = collect_memdevs(ctx, param.root_decoder, count, mems); + if (!p->memdevs) + return -ENXIO; + p->num_memdevs = json_object_array_length(p->memdevs); + if (param.type) { p->mode = cxl_decoder_mode_from_ident(param.type); if (p->mode == CXL_DECODER_MODE_NONE) { @@ -132,8 +266,12 @@ static int parse_create_options(int argc, const char **argv, return -EINVAL; } else if (param.ways < INT_MAX) { p->ways = param.ways; - } else if (argc) { - p->ways = argc; + if (!validate_ways(p, count)) + return -EINVAL; + } else if (count) { + p->ways = count; + if (!validate_ways(p, count)) + return -EINVAL; } else { log_err(&rl, "couldn't determine interleave ways from options or arguments\n"); @@ -149,19 +287,6 @@ static int parse_create_options(int argc, const char **argv, p->granularity = param.granularity; } - if (argc > p->ways) { - for (i = p->ways; i < argc; i++) - log_err(&rl, "extra argument: %s\n", p->targets[i]); - return -EINVAL; - } - - if (argc < p->ways) { - log_err(&rl, - "too few target arguments (%d) for interleave ways (%u)\n", - argc, p->ways); - return -EINVAL; - } - if (p->size && p->ways) { if (p->size % p->ways) { log_err(&rl, @@ -171,17 +296,6 @@ static int parse_create_options(int argc, const char **argv, } } - /* - * For all practical purposes, -m is the default target type, but - * hold off on actively making that decision until a second target - * option is available. - */ - if (!param.memdevs) { - log_err(&rl, - "must specify option for target object types (-m)\n"); - return -EINVAL; - } - return 0; } @@ -196,8 +310,8 @@ static int parse_region_options(int argc, const char **argv, }; argc = parse_options(argc, argv, options, u, 0); - p->targets = argv; - p->num_targets = argc; + p->argc = argc; + p->argv = argv; if (param.debug) { cxl_set_log_priority(ctx, LOG_DEBUG); @@ -207,62 +321,27 @@ static int parse_region_options(int argc, const char **argv, switch(action) { case ACTION_CREATE: - return parse_create_options(argc, argv, p); + return parse_create_options(ctx, argc, argv, p); default: return 0; } } -/** - * validate_memdev() - match memdev with the target provided, - * and determine its size contribution - * @memdev: cxl_memdev being tested for a match against the named target - * @target: target memdev - * @p: params structure - * - * This is called for each memdev in the system, and only returns 'true' if - * the memdev name matches the target argument being tested. Additionally, - * it sets an ep_min_size attribute that always contains the size of the - * smallest target in the provided list. This is used during the automatic - * size determination later, to ensure that all targets contribute equally - * to the region in case of unevenly sized memdevs. - */ -static bool validate_memdev(struct cxl_memdev *memdev, const char *target, - struct parsed_params *p) +static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p) { - const char *devname = cxl_memdev_get_devname(memdev); - u64 size; - - if (strcmp(devname, target) != 0) - return false; - - size = cxl_memdev_get_pmem_size(memdev); - if (!p->ep_min_size) - p->ep_min_size = size; - else - p->ep_min_size = min(p->ep_min_size, size); - - return true; -} - -static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p) -{ - int i, matched = 0; + int i; for (i = 0; i < p->ways; i++) { - struct cxl_memdev *memdev; + struct json_object *jobj = + json_object_array_get_idx(p->memdevs, i); + struct cxl_memdev *memdev = json_object_get_userdata(jobj); + u64 size = cxl_memdev_get_pmem_size(memdev); - cxl_memdev_foreach(ctx, memdev) - if (validate_memdev(memdev, p->targets[i], p)) - matched++; + if (!p->ep_min_size) + p->ep_min_size = size; + else + p->ep_min_size = min(p->ep_min_size, size); } - if (matched != p->ways) { - log_err(&rl, - "one or more memdevs not found in CXL topology\n"); - return -ENXIO; - } - - return 0; } static int validate_decoder(struct cxl_decoder *decoder, @@ -330,26 +409,18 @@ found: if (rc) return rc; - return validate_config_memdevs(ctx, p); + collect_minsize(ctx, p); + return 0; } -static struct cxl_decoder * -cxl_memdev_target_find_decoder(struct cxl_ctx *ctx, const char *memdev_name) +static struct cxl_decoder *cxl_memdev_find_decoder(struct cxl_memdev *memdev) { - struct cxl_endpoint *ep = NULL; + const char *memdev_name = cxl_memdev_get_devname(memdev); struct cxl_decoder *decoder; - struct cxl_memdev *memdev; + struct cxl_endpoint *ep; struct cxl_port *port; - cxl_memdev_foreach(ctx, memdev) { - const char *devname = cxl_memdev_get_devname(memdev); - - if (strcmp(devname, memdev_name) != 0) - continue; - - ep = cxl_memdev_get_endpoint(memdev); - } - + ep = cxl_memdev_get_endpoint(memdev); if (!ep) { log_err(&rl, "could not get an endpoint for %s\n", memdev_name); @@ -488,9 +559,12 @@ static int create_region(struct cxl_ctx *ctx, int *count, try(cxl_region, set_size, region, size); for (i = 0; i < p->ways; i++) { - struct cxl_decoder *ep_decoder = NULL; + struct cxl_decoder *ep_decoder; + struct json_object *jobj = + json_object_array_get_idx(p->memdevs, i); + struct cxl_memdev *memdev = json_object_get_userdata(jobj); - ep_decoder = cxl_memdev_target_find_decoder(ctx, p->targets[i]); + ep_decoder = cxl_memdev_find_decoder(memdev); if (!ep_decoder) { rc = -ENXIO; goto err_delete; @@ -508,7 +582,7 @@ static int create_region(struct cxl_ctx *ctx, int *count, rc = cxl_region_set_target(region, i, ep_decoder); if (rc) { log_err(&rl, "%s: failed to set target%d to %s\n", - devname, i, p->targets[i]); + devname, i, cxl_memdev_get_devname(memdev)); goto err_delete; } } @@ -630,8 +704,8 @@ static int decoder_region_action(struct parsed_params *p, cxl_region_foreach_safe (decoder, region, _r) { int i, match = 0; - for (i = 0; i < p->num_targets; i++) { - if (util_cxl_region_filter(region, p->targets[i])) { + for (i = 0; i < p->argc; i++) { + if (util_cxl_region_filter(region, p->argv[i])) { match = 1; break; } @@ -664,8 +738,10 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, if (rc) return rc; - if (action == ACTION_CREATE) - return create_region(ctx, count, p); + if (action == ACTION_CREATE) { + rc = create_region(ctx, count, p); + json_object_put(p->memdevs); + } cxl_bus_foreach(ctx, bus) { struct cxl_decoder *decoder;
The core of 'cxl list' knows, among other things, how to filter memdevs by their connectivity to a root decoder, enabled status, and how to identify memdevs by name, id, serial number. Use the fact that the json-c object array returned by cxl_filter_walk() also includes the corresponding libcxl objects to populate and validate the memdev target list for 'cxl create-region'. With this in place a default set of memdev targets can be derived from the specified root decoder, and the connectivity is validated by the same logic that prepares the hierarchical json topology. The argument list becomes as tolerant of different id formats as 'cxl list'. For example "mem9" and "9" are equivalent. Comma separated lists are also allowed, e.g. "mem9,mem10". However the sorting of memdevs by filter position falls back to the 'cxl list' order in that case. In other words: arg order region position mem9 mem10 => [0]: mem9 [1]: mem10 mem10 mem9 => [0]: mem10 [1]: mem9 mem9,mem10 => [0]: mem9 [1]: mem10 mem10,mem9 => [0]: mem9 [1]: mem10 Note that 'cxl list' order groups memdevs by port, later this will need to augmented with a sort implementation that orders memdevs by a topology compatible decode order. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- cxl/region.c | 274 +++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 175 insertions(+), 99 deletions(-)