Resctrl's architecture code gets to specify a function pointer that is
used when parsing schema entries. This is expected to be one of two
helpers from the filesystem code.

Setting this function pointer allows the architecture code to change
the ABI resctrl presents to user-space, and forces resctrl to expose
these helpers.

Instead, add a schema format enum to choose which schema parser to
use. This allows the helpers to be made static and the structs used
for passing arguments moved out of shared headers.

Signed-off-by: James Morse <james.morse@arm.com>
Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
Changes since v4:
 * Creation of the enum moves into this patch - review tags not picked up.
 * Removed some whitespace.

Changes since v3:
 * Removed a spurious semicolon

Changes since v2:
 * This patch is new
---
 arch/x86/kernel/cpu/resctrl/core.c        |  8 +++---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 32 +++++++++++++++++++----
 arch/x86/kernel/cpu/resctrl/internal.h    | 10 -------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  2 +-
 include/linux/resctrl.h                   | 18 +++++++++----
 5 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index a508433ff354..0a05df02d2ed 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -72,7 +72,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.mon_scope		= RESCTRL_L3_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_L3),
 			.mon_domains		= mon_domain_init(RDT_RESOURCE_L3),
-			.parse_ctrlval		= parse_cbm,
+			.schema_fmt		= RESCTRL_SCHEMA_BITMAP,
 			.format_str		= "%d=%0*x",
 		},
 		.msr_base		= MSR_IA32_L3_CBM_BASE,
@@ -85,7 +85,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "L2",
 			.ctrl_scope		= RESCTRL_L2_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_L2),
-			.parse_ctrlval		= parse_cbm,
+			.schema_fmt		= RESCTRL_SCHEMA_BITMAP,
 			.format_str		= "%d=%0*x",
 		},
 		.msr_base		= MSR_IA32_L2_CBM_BASE,
@@ -98,7 +98,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "MB",
 			.ctrl_scope		= RESCTRL_L3_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_MBA),
-			.parse_ctrlval		= parse_bw,
+			.schema_fmt		= RESCTRL_SCHEMA_RANGE,
 			.format_str		= "%d=%*u",
 		},
 	},
@@ -109,7 +109,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "SMBA",
 			.ctrl_scope		= RESCTRL_L3_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_SMBA),
-			.parse_ctrlval		= parse_bw,
+			.schema_fmt		= RESCTRL_SCHEMA_RANGE,
 			.format_str		= "%d=%*u",
 		},
 	},
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index e078bfe3840d..a042e234f4f8 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -23,6 +23,15 @@
 
 #include "internal.h"
 
+struct rdt_parse_data {
+	struct rdtgroup		*rdtgrp;
+	char			*buf;
+};
+
+typedef int (ctrlval_parser_t)(struct rdt_parse_data *data,
+			       struct resctrl_schema *s,
+			       struct rdt_ctrl_domain *d);
+
 /*
  * Check whether MBA bandwidth percentage value is correct. The value is
  * checked against the minimum and max bandwidth values specified by the
@@ -59,8 +68,8 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	return true;
 }
 
-int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
-	     struct rdt_ctrl_domain *d)
+static int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
+		    struct rdt_ctrl_domain *d)
 {
 	struct resctrl_staged_config *cfg;
 	u32 closid = data->rdtgrp->closid;
@@ -138,8 +147,8 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
  * Read one cache bit mask (hex). Check that it is valid for the current
  * resource type.
  */
-int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
-	      struct rdt_ctrl_domain *d)
+static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
+		     struct rdt_ctrl_domain *d)
 {
 	struct rdtgroup *rdtgrp = data->rdtgrp;
 	struct resctrl_staged_config *cfg;
@@ -195,6 +204,18 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 	return 0;
 }
 
+static ctrlval_parser_t *get_parser(struct rdt_resource *r)
+{
+	switch (r->schema_fmt) {
+	case RESCTRL_SCHEMA_BITMAP:
+		return &parse_cbm;
+	case RESCTRL_SCHEMA_RANGE:
+		return &parse_bw;
+	}
+
+	return NULL;
+}
+
 /*
  * For each domain in this resource we expect to find a series of:
  *	id=mask
@@ -204,6 +225,7 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 static int parse_line(char *line, struct resctrl_schema *s,
 		      struct rdtgroup *rdtgrp)
 {
+	ctrlval_parser_t *parse_ctrlval = get_parser(s->res);
 	enum resctrl_conf_type t = s->conf_type;
 	struct resctrl_staged_config *cfg;
 	struct rdt_resource *r = s->res;
@@ -235,7 +257,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
 		if (d->hdr.id == dom_id) {
 			data.buf = dom;
 			data.rdtgrp = rdtgrp;
-			if (r->parse_ctrlval(&data, s, d))
+			if (parse_ctrlval(&data, s, d))
 				return -EINVAL;
 			if (rdtgrp->mode ==  RDT_MODE_PSEUDO_LOCKSETUP) {
 				cfg = &d->staged_config[t];
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index b5a34a3fa599..ffcade365070 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -457,11 +457,6 @@ static inline bool is_mbm_event(int e)
 		e <= QOS_L3_MBM_LOCAL_EVENT_ID);
 }
 
-struct rdt_parse_data {
-	struct rdtgroup		*rdtgrp;
-	char			*buf;
-};
-
 /**
  * struct rdt_hw_resource - arch private attributes of a resctrl resource
  * @r_resctrl:		Attributes of the resource used directly by resctrl.
@@ -498,11 +493,6 @@ static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r
 	return container_of(r, struct rdt_hw_resource, r_resctrl);
 }
 
-int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
-	      struct rdt_ctrl_domain *d);
-int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
-	     struct rdt_ctrl_domain *d);
-
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_hw_resource rdt_resources_all[];
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2abe17574407..11153271cbdc 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2201,7 +2201,7 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 	}
 
 	for_each_mon_capable_rdt_resource(r) {
-		fflags =  fflags_from_resource(r) | RFTYPE_MON_INFO;
+		fflags = fflags_from_resource(r) | RFTYPE_MON_INFO;
 		sprintf(name, "%s_MON", r->name);
 		ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
 		if (ret)
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 496ddcaa4ecf..54ec87339038 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -183,7 +183,6 @@ struct resctrl_membw {
 	u32				*mb_map;
 };
 
-struct rdt_parse_data;
 struct resctrl_schema;
 
 enum resctrl_scope {
@@ -192,6 +191,17 @@ enum resctrl_scope {
 	RESCTRL_L3_NODE,
 };
 
+/**
+ * enum resctrl_schema_fmt - The format user-space provides for a schema.
+ * @RESCTRL_SCHEMA_BITMAP:	The schema is a bitmap in hex.
+ * @RESCTRL_SCHEMA_RANGE:	The schema is a number, either a percentage
+ *				or a MBps value.
+ */
+enum resctrl_schema_fmt {
+	RESCTRL_SCHEMA_BITMAP,
+	RESCTRL_SCHEMA_RANGE,
+};
+
 /**
  * struct rdt_resource - attributes of a resctrl resource
  * @rid:		The index of the resource
@@ -208,7 +218,7 @@ enum resctrl_scope {
  * @data_width:		Character width of data when displaying
  * @default_ctrl:	Specifies default cache cbm or memory B/W percent.
  * @format_str:		Per resource format string to show domain value
- * @parse_ctrlval:	Per resource function pointer to parse control values
+ * @schema_fmt:	Which format string and parser is used for this schema.
  * @evt_list:		List of monitoring events
  * @cdp_capable:	Is the CDP feature available on this resource
  */
@@ -227,9 +237,7 @@ struct rdt_resource {
 	int			data_width;
 	u32			default_ctrl;
 	const char		*format_str;
-	int			(*parse_ctrlval)(struct rdt_parse_data *data,
-						 struct resctrl_schema *s,
-						 struct rdt_ctrl_domain *d);
+	enum resctrl_schema_fmt	schema_fmt;
 	struct list_head	evt_list;
 	bool			cdp_capable;
 };
-- 
2.39.2

Hi James,

On 10/4/24 11:03 AM, James Morse wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 2abe17574407..11153271cbdc 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2201,7 +2201,7 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
>  	}
>  
>  	for_each_mon_capable_rdt_resource(r) {
> -		fflags =  fflags_from_resource(r) | RFTYPE_MON_INFO;
> +		fflags = fflags_from_resource(r) | RFTYPE_MON_INFO;
>  		sprintf(name, "%s_MON", r->name);
>  		ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
>  		if (ret)

Stray hunk.

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 496ddcaa4ecf..54ec87339038 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -183,7 +183,6 @@ struct resctrl_membw {
>  	u32				*mb_map;
>  };
>  
> -struct rdt_parse_data;
>  struct resctrl_schema;
>  
>  enum resctrl_scope {
> @@ -192,6 +191,17 @@ enum resctrl_scope {
>  	RESCTRL_L3_NODE,
>  };
>  
> +/**
> + * enum resctrl_schema_fmt - The format user-space provides for a schema.
> + * @RESCTRL_SCHEMA_BITMAP:	The schema is a bitmap in hex.
> + * @RESCTRL_SCHEMA_RANGE:	The schema is a number, either a percentage
> + *				or a MBps value.

The description of RESCTRL_SCHEMA_RANGE appears to aim to be specific. Considering this
it should also include the "multiples of one eighth GB/s" input option used on
AMD systems.
The software controller is the only user of actual bandwidth and for its
use it should be "MiBps".


> + */
> +enum resctrl_schema_fmt {
> +	RESCTRL_SCHEMA_BITMAP,
> +	RESCTRL_SCHEMA_RANGE,
> +};
> +
>  /**
>   * struct rdt_resource - attributes of a resctrl resource
>   * @rid:		The index of the resource
> @@ -208,7 +218,7 @@ enum resctrl_scope {
>   * @data_width:		Character width of data when displaying
>   * @default_ctrl:	Specifies default cache cbm or memory B/W percent.
>   * @format_str:		Per resource format string to show domain value
> - * @parse_ctrlval:	Per resource function pointer to parse control values
> + * @schema_fmt:	Which format string and parser is used for this schema.

Please fix alignment.

>   * @evt_list:		List of monitoring events
>   * @cdp_capable:	Is the CDP feature available on this resource
>   */
> @@ -227,9 +237,7 @@ struct rdt_resource {
>  	int			data_width;
>  	u32			default_ctrl;
>  	const char		*format_str;
> -	int			(*parse_ctrlval)(struct rdt_parse_data *data,
> -						 struct resctrl_schema *s,
> -						 struct rdt_ctrl_domain *d);
> +	enum resctrl_schema_fmt	schema_fmt;
>  	struct list_head	evt_list;
>  	bool			cdp_capable;
>  };

Reinette

On Fri, Oct 04, 2024 at 06:03:11PM +0000, James Morse wrote:
> +static ctrlval_parser_t *get_parser(struct rdt_resource *r)
> +{
> +	switch (r->schema_fmt) {
> +	case RESCTRL_SCHEMA_BITMAP:
> +		return &parse_cbm;
> +	case RESCTRL_SCHEMA_RANGE:
> +		return &parse_bw;
> +	}
> +
> +	return NULL;
> +}

Is it really worth making this a helper function? It's only
used once.

> +
>  /*
>   * For each domain in this resource we expect to find a series of:
>   *	id=mask
> @@ -204,6 +225,7 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>  static int parse_line(char *line, struct resctrl_schema *s,
>  		      struct rdtgroup *rdtgrp)
>  {
> +	ctrlval_parser_t *parse_ctrlval = get_parser(s->res);

No check to see if get_parser() returned NULL.

>  	enum resctrl_conf_type t = s->conf_type;
>  	struct resctrl_staged_config *cfg;
>  	struct rdt_resource *r = s->res;
> @@ -235,7 +257,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
>  		if (d->hdr.id == dom_id) {
>  			data.buf = dom;
>  			data.rdtgrp = rdtgrp;
> -			if (r->parse_ctrlval(&data, s, d))
> +			if (parse_ctrlval(&data, s, d))
>  				return -EINVAL;

Without the helper this could be:

			switch (r->schema_fmt) {
			case RESCTRL_SCHEMA_BITMAP:
				if (parse_cbm(&data, s, d))
					return -EINVAL;
				break;
			case RESCTRL_SCHEMA_RANGE:
				if (parse_bw(&data, s, d))
					return -EINVAL;
				break;
			default:
				WARN_ON_ONCE(1);
				return -EINVAL;
			}

-Tony

Hi Tony,

On 16/10/2024 00:15, Tony Luck wrote:
> On Fri, Oct 04, 2024 at 06:03:11PM +0000, James Morse wrote:
>> +static ctrlval_parser_t *get_parser(struct rdt_resource *r)
>> +{
>> +	switch (r->schema_fmt) {
>> +	case RESCTRL_SCHEMA_BITMAP:
>> +		return &parse_cbm;
>> +	case RESCTRL_SCHEMA_RANGE:
>> +		return &parse_bw;
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> Is it really worth making this a helper function? It's only
> used once.

Moved. This was just to avoid bloating the caller with boiler-plate.


>> +
>>  /*
>>   * For each domain in this resource we expect to find a series of:
>>   *	id=mask
>> @@ -204,6 +225,7 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>>  static int parse_line(char *line, struct resctrl_schema *s,
>>  		      struct rdtgroup *rdtgrp)
>>  {
>> +	ctrlval_parser_t *parse_ctrlval = get_parser(s->res);
> 
> No check to see if get_parser() returned NULL.

No - but you must have passed it a non-existant enum value for that to happen, so we're
already in memory corruption territory. (I probably should have made get_parser()
WARN_ON_ONCE() when returning NULL)


>>  	enum resctrl_conf_type t = s->conf_type;
>>  	struct resctrl_staged_config *cfg;
>>  	struct rdt_resource *r = s->res;
>> @@ -235,7 +257,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
>>  		if (d->hdr.id == dom_id) {
>>  			data.buf = dom;
>>  			data.rdtgrp = rdtgrp;
>> -			if (r->parse_ctrlval(&data, s, d))
>> +			if (parse_ctrlval(&data, s, d))
>>  				return -EINVAL;
> 
> Without the helper this could be:
> 
> 			switch (r->schema_fmt) {
> 			case RESCTRL_SCHEMA_BITMAP:
> 				if (parse_cbm(&data, s, d))
> 					return -EINVAL;
> 				break;
> 			case RESCTRL_SCHEMA_RANGE:
> 				if (parse_bw(&data, s, d))
> 					return -EINVAL;
> 				break;
> 			default:
> 				WARN_ON_ONCE(1);
> 				return -EINVAL;
> 			}

I'd prefer the switch statement to have no default so that it triggers a compiler warning
when future enum entries are added. This way the compiler can find cases where a new
schema format missed a bit - it doesn't need booting the result on hardware to trigger a
warning.

To avoid 'break' in a loop not breaking out of the loop, and to avoid bloating the loop
I've kept the function pointer so the non-existant enum case is handled with the rest of
the errors at the top of the function:
|        /* Walking r->domains, ensure it can't race with cpuhp */
|        lockdep_assert_cpus_held();
|
|        switch (r->schema_fmt) {
|        case RESCTRL_SCHEMA_BITMAP:
|                parse_ctrlval = &parse_cbm;
|                break;
|        case RESCTRL_SCHEMA_RANGE:
|                parse_ctrlval = &parse_bw;
|                break;
|        }
|
|        if (WARN_ON_ONCE(!parse_ctrlval))
|                return -EINVAL;
|
|        if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&


Thanks,

James