Re: [PATCH v3 1/5] coresight: configfs: Add in functionality for load via configfs

Good day,

I have resumed working on this patchset. More comments below as I went back while looking at other patches...

On Thu, Apr 14, 2022 at 07:44:53AM +0100, Mike Leach wrote:

Add in functionality to allow load via configfs.

define a binary file format and provide a reader for that format that will create and populate configuration and feature structures use by the driver infrastructure.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/Makefile | 2 +- .../coresight/coresight-config-file.c | 432 ++++++++++++++++++ .../coresight/coresight-config-file.h | 117 +++++ .../hwtracing/coresight/coresight-config.h | 15 + .../hwtracing/coresight/coresight-syscfg.h | 1 + 5 files changed, 566 insertions(+), 1 deletion(-) create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index b6c4a48140ec..5de2bb79f4ac 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ coresight-sysfs.o coresight-syscfg.o coresight-config.o \ coresight-cfg-preload.o coresight-cfg-afdo.o \

• coresight-syscfg-configfs.o
  

• coresight-syscfg-configfs.o coresight-config-file.o
  

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c new file mode 100644 index 000000000000..3fd001938324 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config-file.c @@ -0,0 +1,432 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.

I can't remember if I commented on that before but this should likely be 2022, and for all the other files introduced by this patchset.

• • Author: Mike Leach mike.leach@linaro.org
• */

+#include "coresight-config.h" +#include "coresight-config-file.h" +#include "coresight-syscfg.h"

+#define cscfg_extract_u64(val64) { \

• val64 = *(u64 *)(buffer + used); \
• used += sizeof(u64); \
• }

+#define cscfg_extract_u32(val32) { \

• val32 = *(u32 *)(buffer + used); \
• used += sizeof(u32); \
• }

+#define cscfg_extract_u16(val16) { \

• val16 = *(u16 *)(buffer + used); \
• used += sizeof(u16); \
• }

+#define cscfg_extract_u8(val8) { \

• val8 = *(buffer + used); \
• used++; \
• }

+static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,

• 	       struct cscfg_file_header *hdr)
  

+{

• /* file header always at the start of the buffer */
• int used = 0;
• if (buflen < sizeof(struct cscfg_file_header))

• return -EINVAL;
  

• cscfg_extract_u32(hdr->magic_version);
• if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)

• return -EINVAL;
  

• cscfg_extract_u16(hdr->length);
• if (hdr->length > buflen)

• return -EINVAL;
  

• cscfg_extract_u16(hdr->nr_features);
• *buf_used = used;
• return 0;

+}

+static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,

• 		    struct cscfg_file_elem_header *elem_hdr)
  

+{

• int used = *buf_used;
• if ((buflen - used) < (sizeof(u16) + sizeof(u8)))

• return -EINVAL;
  

• /* read length and check enough buffer remains for this element */
• elem_hdr->elem_length = *(u16 *)(buffer + used);
• if ((buflen - used) < elem_hdr->elem_length)

• return -EINVAL;
  

• /* don't use extract fn as we update used _after_ the comparison */
• used += sizeof(u16);
• /* read type and validate */
• cscfg_extract_u8(elem_hdr->elem_type);
• if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||

•    (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
  

• return -EINVAL;
  

• *buf_used = used;
• return 0;

+}

+static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,

• 		    struct cscfg_file_elem_str *elem_str)
  

+{

• int used = *buf_used;
• if ((buflen - used) < sizeof(u16))

• return -EINVAL;
  

• cscfg_extract_u16(elem_str->str_len);
• if ((buflen - used) < elem_str->str_len)

• return -EINVAL;
  

• /* check for 0 termination */
• if (buffer[elem_str->str_len - 1] != 0)

• return -EINVAL;
  

• elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
• used += elem_str->str_len;
• *buf_used = used;
• return 0;

+}

+static int cscfg_file_alloc_desc_arrays(struct cscfg_fs_load_descs *desc_arrays,

• 			int nr_features)
  

+{

• /* arrays are 0 terminated - max of 1 config & nr_features features */
• desc_arrays->config_descs = devm_kcalloc(cscfg_device(), 2,

• 				 sizeof(struct cscfg_config_desc *),
  

• 				 GFP_KERNEL);
  

• if (!desc_arrays->config_descs)

• return -ENOMEM;
  

• desc_arrays->feat_descs = devm_kcalloc(cscfg_device(), nr_features + 1,

• 			       sizeof(struct cscfg_feature_desc *),
  

• 			       GFP_KERNEL);
  

• if (!desc_arrays->feat_descs)

• return -ENOMEM;
  

• return 0;

+}

+static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,

• 		       struct cscfg_fs_load_descs *desc_arrays)
  

+{

• struct cscfg_file_elem_header elem_hdr;
• struct cscfg_file_elem_str elem_str;
• struct cscfg_config_desc *config_desc;
• int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
• int err = 0;
• u64 *presets;
• /*

• * read the header - if not config, then don't update buf_used
  

• * pointer on return
  

• */
  

• err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
• if (err)

• return err;
  

• if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)

• return 0;
  

• /* we have a config - allocate the descriptor */
• config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),

• 		   GFP_KERNEL);
  

• if (!config_desc)

• return -ENOMEM;
  

• /* read the name string */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
• if (err)

• return err;
  

• config_desc->name = elem_str.str;
• /* read the description string */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
• if (err)

• return err;
  

• config_desc->description = elem_str.str;
• /* read in some values */
• if ((buflen - used) < sizeof(u64))

• return -EINVAL;
  

• cscfg_extract_u16(config_desc->nr_presets);
• cscfg_extract_u32(config_desc->nr_total_params);
• cscfg_extract_u16(config_desc->nr_feat_refs);
• /* read the array of 64bit presets if present */
• nr_preset_vals = config_desc->nr_total_params * config_desc->nr_presets;
• if (nr_preset_vals) {

• presets = devm_kcalloc(cscfg_device(), nr_preset_vals,
  

• 		       sizeof(u64), GFP_KERNEL);
  

• if (!presets)
  

• 	return -ENOMEM;
  

• nr_preset_bytes = sizeof(u64) * nr_preset_vals;
  

• if ((buflen - used) < nr_preset_bytes)
  

• 	return -EINVAL;
  

• memcpy(presets, (buffer + used), nr_preset_bytes);
  

• config_desc->presets = presets;
  

• used += nr_preset_bytes;
  

• }
• /* read the array of feature names referenced by the config */
• if (config_desc->nr_feat_refs) {

• config_desc->feat_ref_names = devm_kcalloc(cscfg_device(),
  

• 					   config_desc->nr_feat_refs,
  

• 					   sizeof(char *),
  

• 					   GFP_KERNEL);
  

• if (!config_desc->feat_ref_names)
  

• 	return -ENOMEM;
  

• for (i = 0; i < config_desc->nr_feat_refs; i++) {
  

• 	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
  

• 	if (err)
  

• 		return err;
  

• 	config_desc->feat_ref_names[i] = elem_str.str;
  

• }
  

• }
• desc_arrays->config_descs[0] = config_desc;
• *buf_used = used;
• return 0;

+}

+/* just read the config name - if there is a config at this position */ +static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,

• 			    struct cscfg_file_elem_str *elem_str)
  

+{

• struct cscfg_file_elem_header elem_hdr;
• int used = *buf_used;
• int err;
• elem_str->str_len = 0;
• /*

• * read the header - if not config, then don't update buf_used
  

• * pointer on return
  

• */
  

• err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
• if (err)

• return err;
  

• if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)

• return 0;
  

• /* read the name string */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
• if (err)

• return err;
  

• *buf_used = used;
• return 0;

+}

+static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,

• 		      struct cscfg_parameter_desc *param_desc)
  

+{

• struct cscfg_file_elem_str elem_str;
• int err = 0, used = *buf_used;
• /* parameter name */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
• if (err)

• return err;
  

• param_desc->name = elem_str.str;
• /* parameter value */
• if ((buflen - used) < sizeof(u64))

• return -EINVAL;
  

• cscfg_extract_u64(param_desc->value);
• *buf_used = used;
• return err;

+}

+static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,

• 			struct cscfg_fs_load_descs *desc_arrays,
  

• 			const int feat_idx)
  

+{

• struct cscfg_file_elem_header elem_hdr;
• struct cscfg_file_elem_str elem_str;
• struct cscfg_feature_desc *feat_desc;
• struct cscfg_regval_desc *p_reg_desc;
• int used = *buf_used, err, i, nr_regs_bytes;
• u32 val32;
• /* allocate the feature descriptor object */
• feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),

• 		 GFP_KERNEL);
  

• if (!feat_desc)

• return -ENOMEM;
  

• /* read and check the element header */
• err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
• if (err)

• return err;
  

• if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)

• return -EINVAL;
  

• /* read the feature name */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
• if (err)

• return err;
  

• feat_desc->name = elem_str.str;
• /* read the description string */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
• if (err)

• return err;
  

• feat_desc->description = elem_str.str;
• /*

• * read in some values
  

• * [u32 value: match_flags]
  

• * [u16 value: nr_regs]	    - number of registers.
  

• * [u16 value: nr_params]   - number of parameters.
  

• */
  

• cscfg_extract_u32(feat_desc->match_flags);
• cscfg_extract_u16(feat_desc->nr_regs);
• cscfg_extract_u16(feat_desc->nr_params);
• /* register descriptors - 32 bit + 64 bit value */
• if (feat_desc->nr_regs) {

• nr_regs_bytes = ((sizeof(u32) + sizeof(u64)) * feat_desc->nr_regs);
  

• if ((buflen - used) < nr_regs_bytes)
  

• 	return -EINVAL;
  

• feat_desc->regs_desc = devm_kcalloc(cscfg_device(),
  

• 				    feat_desc->nr_regs,
  

• 				    sizeof(struct cscfg_regval_desc),
  

• 				    GFP_KERNEL);
  

• if (!feat_desc->regs_desc)
  

• 	return -ENOMEM;
  

• for (i = 0; i < feat_desc->nr_regs; i++) {
  

• 	cscfg_extract_u32(val32);
  

• 	p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
  

• 	CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
  

• 	cscfg_extract_u64(feat_desc->regs_desc[i].val64);
  

• }
  

• }
• /* parameter descriptors - string + 64 bit value */
• if (feat_desc->nr_params) {

• feat_desc->params_desc = devm_kcalloc(cscfg_device(),
  

• 				      feat_desc->nr_params,
  

• 				      sizeof(struct cscfg_parameter_desc),
  

• 				      GFP_KERNEL);
  

• if (!feat_desc->params_desc)
  

• 	return -ENOMEM;
  

• for (i = 0; i < feat_desc->nr_params; i++) {
  

• 	err = cscfg_file_read_elem_param(buffer, buflen, &used,
  

• 					 &feat_desc->params_desc[i]);
  

• 	if (err)
  

• 		return err;
  

• }
  

• }
• desc_arrays->feat_descs[feat_idx] = feat_desc;
• *buf_used = used;
• return 0;

+}

+/* just read the feature name - if there is a feature at this position */ +static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,

• 			  struct cscfg_file_elem_str *elem_str)
  

+{

• struct cscfg_file_elem_header elem_hdr;
• int used = *buf_used;
• int err;
• elem_str->str_len = 0;
• /*

• * read the header - if not config, then don't update buf_used
  

• * pointer on return
  

• */
  

• err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
• if (err)

• return err;
  

• if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)

• return -EINVAL;
  

• /* read the feature name */
• err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
• if (err)

• return err;
  

• *buf_used = used;
• return 0;

+}

+/*

• • Read a buffer and create the configuration and feature
• • descriptors to load into the cscfg system
• */

+int cscfg_file_read_buffer(const u8 *buffer, const int buflen,

• 	   struct cscfg_fs_load_descs *desc_arrays)
  

+{

• struct cscfg_file_header hdr;
• int used = 0, err, i;
• /* read in the file header */
• err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
• if (err)

• return err;
  

• /* allocate the memory for the descriptor pointer arrays */
• err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
• if (err)

• return err;
  

• /* read elements */
• /* first element could be a config so check */
• err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
• if (err)

• return err;
  

• /* now read and populate all the feature descriptors */
• for (i = 0; i < hdr.nr_features; i++) {

• err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
  

• if (err)
  

• 	return err;
  

• }
• return used;

+}

+int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,

• 		      const char **name)
  

+{

• struct cscfg_file_header hdr;
• struct cscfg_file_elem_str elem_str;
• int used = 0, err = 0;
• *name = NULL;
• /* read in the file header */
• err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
• if (err)

• return err;
  

• err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
• if (err)

• return err;
  

• /* no config string - get first feature name */
• if (!elem_str.str_len) {

• err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
  

• if (err)
  

• 	return err;
  

• }
• if (elem_str.str_len)

• *name = elem_str.str;
  

• return err;

+} diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h new file mode 100644 index 000000000000..591f4c2c4be9 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config-file.h @@ -0,0 +1,117 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H +#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H

+/*

• • Structures to represent configuration descriptors in a memory buffer
• • to serialise to and from files
• • File structure - for loading a configuration + features
• • from configfs.
• • [cscfg_file_header] - mandatory
• • [CONFIG_ELEM] - optional - only one permitted,
• • [FEATURE_ELEM] * [cscfg_file_header.nr_features]

• • 	- optional - file valid with config only.
    

• • Invalid file if no config or features.
• • File structure for [CONFIG_ELEM]:
• • [cscfg_file_elem_header] - header length value to end of feature strings.
• • [cscfg_file_elem_str] - name of the configuration
• • [cscfg_file_elem_str] - description of configuration
• • [u16 value - nr_presets]
• • [u32 value - nr_total_params]
• • [u16 value - nr_feat_refs]
• • [u64 values] * (nr_presets * nr_total_params)
• • [cscfg_file_elem_str] * nr_feat_refs
• • Only one configuration per file.
• • File structure for a [FEATURE_ELEM]
• • [cscfg_file_elem_header] - header length is total bytes to end of param structures.
• • [cscfg_file_elem_str] - feature name.
• • [cscfg_file_elem_str] - feature description.
• • [u32 value: match_flags]
• • [u16 value: nr_regs] - number of registers.
• • [u16 value: nr_params] - number of parameters.
• • [cscfg_regval_desc struct] * nr_regs
• • [PARAM_ELEM] * nr_params
• • File structure for [PARAM_ELEM]
• • [cscfg_file_elem_str] - parameter name.
• • [u64 value: param_value] - initial value.
• */

+/* major element types - configurations and features */

+#define CSCFG_FILE_ELEM_TYPE_FEAT 0x1 +#define CSCFG_FILE_ELEM_TYPE_CFG 0x2

+#define CSCFG_FILE_MAGIC_VERSION 0xC5CF0001

+#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \

• { \
• p_desc->type = (val32 >> 24) & 0xFF; \
• p_desc->offset = (val32 >> 12) & 0xFFF; \
• p_desc->hw_info = val32 & 0xFFF; \
• }

+#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \

• { \
• val32 = p_desc->hw_info & 0xFFF; \
• val32 |= ((p_desc->offset & 0xFFF) << 12); \
• val32 |= ((p_desc->type & 0xFF) << 24); \
• }

+/* binary attributes in configfs need a max size - declare a value for this. */ +#define CSCFG_FILE_MAXSIZE 16384

s/16384/SZ_16K

Also, please enhance the comment to explain the 16K limit.

+/* limit string sizes */ +#define CSCFG_FILE_STR_MAXSIZE 1024

s/1024/SZ_1K

Thanks, Mathieu

+/**

• • file header.
• • @magic_version: magic number / version for file/buffer format.
• • @length : total length of all data in the buffer.
• • @nr_features : total number of features in the buffer.
• */

+struct cscfg_file_header {

• u32 magic_version;
• u16 length;
• u16 nr_features;

+};

+/**

• • element header
• • @elem_length: total length of this element
• • @elem_type : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
• */

+struct cscfg_file_elem_header {

• u16 elem_length;
• u8 elem_type;

+};

+/**

• • string file element.
• • @str_len: length of string buffer including 0 terminator
• • @str : string buffer - 0 terminated.
• */

+struct cscfg_file_elem_str {

• u16 str_len;
• char *str;

+};

+#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */ diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 2e1670523461..9cd3c26ce023 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -85,6 +85,21 @@ struct cscfg_regval_desc { }; }; +/**

• • Dynamically loaded descriptor arrays loaded via configfs.
• • For builtin or module loaded configurations / features these are
• • statically defined at compile time. For configfs we create the arrays
• • dynamically so need a structure to handle this.
• • @config_descs: array of config descriptor pointers.
• • @feat_descs: array of feature descriptor pointers.
• */

+struct cscfg_fs_load_descs {

• struct cscfg_config_desc **config_descs;
• struct cscfg_feature_desc **feat_descs;

+};

/**

• Device feature descriptor - combination of registers and parameters to
• program a device to implement a specific complex function.

diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h index 9106ffab4833..6a6e33585be9 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.h +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -66,6 +66,7 @@ struct cscfg_registered_csdev { enum cscfg_load_owner_type { CSCFG_OWNER_PRELOAD, CSCFG_OWNER_MODULE,

• CSCFG_OWNER_CONFIGFS,

}; /** -- 2.17.1