On 2/23/24 3:48 PM, Verma, Vishal L wrote:
> On Thu, 2024-02-08 at 13:11 -0700, Dave Jiang wrote:
>> The CFMWS provides a QTG ID. The kernel driver creates a root decoder that
>> represents the CFMWS. A qos_class attribute is exported via sysfs for the
>> root
>> decoder.
>>
>> One or more qos_class tokens are retrieved via QTG ID _DSM from the ACPI0017
>> device for a CXL memory device. The input for the _DSM is the read and write
>> latency and bandwidth for the path between the device and the CPU. The
>> numbers are constructed by the kernel driver for the _DSM input. When a
>> device is probed, QoS class tokens are retrieved. This is useful for a
>> hot-plugged CXL memory device that does not have regions created.
>>
>> Add a QoS check during region creation. If --enforce-qos/-Q is set and
>> the qos_class mismatches, the region creation will fail.
>>
>> Reviewed-by: Alison Schofield <[email protected]>
>> Signed-off-by: Dave Jiang <[email protected]>
>> ---
>> v7:
>> - Add qos_class_mismatched to region for cxl list (Vishal)
>> - Add create_region -Q check (Vishal)
>> ---
>> Documentation/cxl/cxl-create-region.txt | 6 +++
>> cxl/json.c | 6 +++
>> cxl/lib/libcxl.c | 11 +++++
>> cxl/lib/libcxl.sym | 2 +
>> cxl/lib/private.h | 1 +
>> cxl/libcxl.h | 2 +
>> cxl/region.c | 56 ++++++++++++++++++++++++-
>> 7 files changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/cxl/cxl-create-region.txt
>> b/Documentation/cxl/cxl-create-region.txt
>> index f11a412bddfe..b244af60b8a6 100644
>> --- a/Documentation/cxl/cxl-create-region.txt
>> +++ b/Documentation/cxl/cxl-create-region.txt
>> @@ -105,6 +105,12 @@ include::bus-option.txt[]
>> supplied, the first cross-host bridge (if available), decoder that
>> supports the largest interleave will be chosen.
>>
>> +-Q::
>> +--enforce-qos::
>> + Parameter to enforce qos_class mismatch failure. Region create operation
>> + will fail of the qos_class of the root decoder and one of the memdev
>> that
>> + backs the region mismatches.
>> +
>> include::human-option.txt[]
>>
>> include::debug-option.txt[]
>> diff --git a/cxl/json.c b/cxl/json.c
>> index c8bd8c27447a..27cbacc84f3a 100644
>> --- a/cxl/json.c
>> +++ b/cxl/json.c
>> @@ -1238,6 +1238,12 @@ struct json_object *util_cxl_region_to_json(struct
>> cxl_region *region,
>> }
>> }
>>
>> + if (cxl_region_qos_class_mismatched(region)) {
>> + jobj = json_object_new_boolean(true);
>> + if (jobj)
>> + json_object_object_add(jregion, "qos_class_mismatched",
>> jobj);
>> + }
>> +
>> json_object_set_userdata(jregion, region, NULL);
>>
>>
>> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
>> index 6c293f1dfc91..3461c4de2097 100644
>> --- a/cxl/lib/libcxl.c
>> +++ b/cxl/lib/libcxl.c
>> @@ -414,6 +414,17 @@ CXL_EXPORT int cxl_region_is_enabled(struct cxl_region
>> *region)
>> return is_enabled(path);
>> }
>>
>> +CXL_EXPORT void cxl_region_qos_class_mismatched_set(struct cxl_region
>> *region,
>> + bool mismatched)
>> +{
>> + region->qos_mismatched = mismatched;
>> +}
>
> This should be called cxl_region_set_qos_class_mismatched() at a
> minimum, but..
>
>> +
>> +CXL_EXPORT bool cxl_region_qos_class_mismatched(struct cxl_region *region)
>> +{
>> + return region->qos_mismatched;
>> +}
>
> .. I think libcxl always perform its own qos mismatch checking when
> this is called and return appropriately, instead of relying on a user-
> set flag.
Ok. I'll add internal compare for libcxl instead and remove the flag.
>
> Actually I don't see this interface getting called anywhere. Was there
> a patch to cxl_region_to_json() that got dropped?
It's the first code chunk above.
>
>> +
>> CXL_EXPORT int cxl_region_disable(struct cxl_region *region)
>> {
>> const char *devname = cxl_region_get_devname(region);
>> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
>> index 465c78dc6c70..47a9c3cafc71 100644
>> --- a/cxl/lib/libcxl.sym
>> +++ b/cxl/lib/libcxl.sym
>> @@ -285,4 +285,6 @@ global:
>> cxl_root_decoder_get_qos_class;
>> cxl_memdev_get_pmem_qos_class;
>> cxl_memdev_get_ram_qos_class;
>> + cxl_region_qos_class_mismatched_set;
>> + cxl_region_qos_class_mismatched;
>> } LIBCXL_7;
>> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
>> index 07dc8c784f1d..88448d82d53f 100644
>> --- a/cxl/lib/private.h
>> +++ b/cxl/lib/private.h
>> @@ -174,6 +174,7 @@ struct cxl_region {
>> struct daxctl_region *dax_region;
>> struct kmod_module *module;
>> struct list_head mappings;
>> + bool qos_mismatched;
>> };
>>
>> struct cxl_memdev_mapping {
>> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
>> index a180f01cb05e..7795496cdbbd 100644
>> --- a/cxl/libcxl.h
>> +++ b/cxl/libcxl.h
>> @@ -335,6 +335,8 @@ int cxl_region_clear_target(struct cxl_region *region,
>> int position);
>> int cxl_region_clear_all_targets(struct cxl_region *region);
>> int cxl_region_decode_commit(struct cxl_region *region);
>> int cxl_region_decode_reset(struct cxl_region *region);
>> +void cxl_region_qos_class_mismatched_set(struct cxl_region *region, bool
>> mismatched);
>> +bool cxl_region_qos_class_mismatched(struct cxl_region *region);
>>
>> #define cxl_region_foreach(decoder, region)
>> \
>> for (region = cxl_region_get_first(decoder); region != NULL; \
>> diff --git a/cxl/region.c b/cxl/region.c
>> index 3a762db4800e..76df177ef246 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -32,6 +32,7 @@ static struct region_params {
>> bool force;
>> bool human;
>> bool debug;
>> + bool qos_enforce;
>> } param = {
>> .ways = INT_MAX,
>> .granularity = INT_MAX,
>> @@ -49,6 +50,8 @@ struct parsed_params {
>> const char **argv;
>> struct cxl_decoder *root_decoder;
>> enum cxl_decoder_mode mode;
>> + bool qos_enforce;
>> + bool qos_mismatched;
>> };
>>
>> enum region_actions {
>> @@ -81,7 +84,8 @@ OPT_STRING('U', "uuid", ¶m.uuid, \
>> "region uuid", "uuid for the new region (default: autogenerate)"), \
>> OPT_BOOLEAN('m', "memdevs", ¶m.memdevs, \
>> "non-option arguments are memdevs"), \
>> -OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number formats")
>> +OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number
>> formats"), \
>> +OPT_BOOLEAN('Q', "enforce-qos", ¶m.qos_enforce, "enforce of qos_class
>> matching")
>>
>> static const struct option create_options[] = {
>> BASE_OPTIONS(),
>> @@ -360,6 +364,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int
>> count,
>> }
>> }
>>
>> + p->qos_enforce = param.qos_enforce;
>> +
>> return 0;
>>
>> err:
>> @@ -467,6 +473,49 @@ static void set_type_from_decoder(struct cxl_ctx *ctx,
>> struct parsed_params *p)
>> p->mode = CXL_DECODER_MODE_PMEM;
>> }
>>
>> +static int create_region_validate_qos_class(struct cxl_ctx *ctx,
>
> ctx is never used, can be removed.
ok
>
>> + struct parsed_params *p)
>> +{
>> + int root_qos_class;
>> + int qos_class;
>> + int i;
>> +
>> + if (!p->qos_enforce)
>> + return 0;
>> +
>> + root_qos_class = cxl_root_decoder_get_qos_class(p->root_decoder);
>> + if (root_qos_class == CXL_QOS_CLASS_NONE)
>> + return 0;
>> +
>> + for (i = 0; i < p->ways; i++) {
>> + struct json_object *jobj =
>> + json_object_array_get_idx(p->memdevs, i);
>> + struct cxl_memdev *memdev = json_object_get_userdata(jobj);
>> +
>> + if (p->mode == CXL_DECODER_MODE_RAM)
>> + qos_class = cxl_memdev_get_ram_qos_class(memdev);
>> + else
>> + qos_class = cxl_memdev_get_pmem_qos_class(memdev);
>> +
>> + /* No qos_class entries. Possibly no kernel support */
>> + if (qos_class == CXL_QOS_CLASS_NONE)
>> + break;
>> +
>> + if (qos_class != root_qos_class) {
>> + p->qos_mismatched = true;
>> + if (p->qos_enforce) {
>> + log_err(&rl, "%s QoS Class mismatches %s\n",
>> +
>> cxl_decoder_get_devname(p->root_decoder),
>> + cxl_memdev_get_devname(memdev));
>> +
>> + return -ENXIO;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int create_region_validate_config(struct cxl_ctx *ctx,
>> struct parsed_params *p)
>> {
>> @@ -507,6 +556,10 @@ found:
>> return rc;
>>
>> collect_minsize(ctx, p);
>> + rc = create_region_validate_qos_class(ctx, p);
>> + if (rc)
>> + return rc;
>> +
>
> Maybe this call can be moved into the existing validate_decoder() check
> since?
ok
>
>> return 0;
>> }
>>
>> @@ -654,6 +707,7 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>> return -EOPNOTSUPP;
>> }
>>
>> + cxl_region_qos_class_mismatched_set(region, p->qos_mismatched);
>> devname = cxl_region_get_devname(region);
>>
>> rc = cxl_region_determine_granularity(region, p);
>
> I think as a future enhancement, it might be nice to add
> cxl_filter_walk() smarts to allow it to filter memdevs based on
> qos_class. That way, when cxl create-region is called without any
> memdev arguments (i.e. it is free to select memdevs), collect_memdevs()
> can ask for memdevs that match the qos_class, and see if those can
> satisfy the interleave requirements if --enforce-qos is used.
