On 2/5/24 11:00 PM, Wonjae Lee wrote:
> On Thu, Feb 01, 2024 at 04:05:06PM -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. Emit a warning if the qos_class
>> token from the root decoder is different than the mem device qos_class
>> token. User parameter options are provided to fail instead of just
>> warning.
>>
>> Reviewed-by: Alison Schofield <[email protected]>
>> Signed-off-by: Dave Jiang <[email protected]>
>> ---
>> Documentation/cxl/cxl-create-region.txt | 9 ++++
>> cxl/region.c | 56 ++++++++++++++++++++++++-
>> 2 files changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/cxl/cxl-create-region.txt
>> b/Documentation/cxl/cxl-create-region.txt
>> index f11a412bddfe..d5e34cf38236 100644
>> --- a/Documentation/cxl/cxl-create-region.txt
>> +++ b/Documentation/cxl/cxl-create-region.txt
>> @@ -105,6 +105,15 @@ include::bus-option.txt[]
>> supplied, the first cross-host bridge (if available), decoder that
>> supports the largest interleave will be chosen.
>>
>> +-e::
>> +--strict::
>> + Enforce strict execution where any potential error will force failure.
>> + For example, if qos_class mismatches region creation will fail.
>> +
>> +-q::
>> +--no-enforce-qos::
>> + Parameter to bypass qos_class mismatch failure. Will only emit warning.
>> +
>> include::human-option.txt[]
>>
>> include::debug-option.txt[]
>> diff --git a/cxl/region.c b/cxl/region.c
>> index 3a762db4800e..f9033fa0afbf 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -32,6 +32,8 @@ static struct region_params {
>> bool force;
>> bool human;
>> bool debug;
>> + bool strict;
>> + bool no_qos;
>> } param = {
>> .ways = INT_MAX,
>> .granularity = INT_MAX,
>> @@ -49,6 +51,8 @@ struct parsed_params {
>> const char **argv;
>> struct cxl_decoder *root_decoder;
>> enum cxl_decoder_mode mode;
>> + bool strict;
>> + bool no_qos;
>> };
>>
>> enum region_actions {
>> @@ -81,7 +85,9 @@ 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('e', "strict", ¶m.strict, "strict execution enforcement"), \
>> +OPT_BOOLEAN('q', "no-enforce-qos", ¶m.no_qos, "no enforce of qos_class")
>>
>> static const struct option create_options[] = {
>> BASE_OPTIONS(),
>> @@ -360,6 +366,9 @@ static int parse_create_options(struct cxl_ctx *ctx, int
>> count,
>> }
>> }
>>
>> + p->strict = param.strict;
>> + p->no_qos = param.no_qos;
>> +
>> return 0;
>>
>> err:
>> @@ -467,6 +476,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,
>> + struct parsed_params *p)
>> +{
>> + int root_qos_class;
>> + int qos_class;
>> + int i;
>> +
>> + 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) {
>> + if (p->strict && !p->no_qos) {
>> + log_err(&rl, "%s QoS Class mismatches %s\n",
>> + cxl_decoder_get_devname(p->root_decoder),
>> + cxl_memdev_get_devname(memdev));
>> +
>> + return -ENXIO;
>> + }
>> +
>> + log_notice(&rl, "%s QoS Class mismatches %s\n",
>> + cxl_decoder_get_devname(p->root_decoder),
>> + cxl_memdev_get_devname(memdev));
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int create_region_validate_config(struct cxl_ctx *ctx,
>> struct parsed_params *p)
>> {
>> @@ -507,6 +559,8 @@ found:
>> return rc;
>>
>> collect_minsize(ctx, p);
>> + create_region_validate_qos_class(ctx, p);
>
> Hello,
>
> IIUC, if the strict option is given and a qos class mismatch occurs,
> the region creation should fail. To do that, shouldn't the return
> value of create_region_validate_qos_class() be handled like below?
You are correct. I'll fix.
>
> diff --git a/cxl/region.c b/cxl/region.c
> index f9033fa..0468f5f 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -559,7 +559,9 @@ found:
> return rc;
>
> collect_minsize(ctx, p);
> - create_region_validate_qos_class(ctx, p);
> + rc = create_region_validate_qos_class(ctx, p);
> + if (rc)
> + return rc;
>
> return 0;
> }
>
> Thanks,
> Wonjae
