Message-ID: <9cb1ef5e-eb61-4177-a039-4049850196a5@linaro.org>
Date: Tue, 3 Dec 2024 14:25:12 +0000
From: James Clark <james.clark@...aro.org>
To: Mike Leach <mike.leach@...aro.org>, coresight@...ts.linaro.org,
 Suzuki K Poulose <Suzuki.Poulose@....com>, Leo Yan <leo.yan@....com>
Cc: "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 8/9] coresight: tools: Add configuration table test
 tools



On 27/11/2024 1:42 pm, Mike Leach wrote:
> Add an example config table generator to test loading configuration
> tables.
> 
> Provides a table buffer writer function that can be re-used in other
> userspace programs.
> 
> Table write format matches that expected by the corresponding reader
> in the configfs driver code.
> 
> Generates tables and outputs in form of binary files.
> 
> Add a config table file reader and printer. Takes in config table files
> and prints the contents. Uses table reader source from kernel driver.
> 
> Signed-off-by: Mike Leach <mike.leach@...aro.org>
> ---
>   MAINTAINERS                                   |   1 +
>   .../coresight/coresight-config-table.h        |   5 +

Hi Mike,

Isn't there some convention about maintaining a copy of kernel headers 
in the tools? Especially as you wouldn't rebuild the tools after 
updating the kernel headers so breakages might go unnoticed.

[...]

> +
> +/*
> + * sets of presets leaves strobing window constant while varying period to allow
> + * experimentation with mark / space ratios for various workloads
> + */
> +static u64 afdo_set_a_presets[AFDO_NR_PRESETS][AFDO_NR_PARAM_SUM] = {
> +	{ 2000, 100 },
> +	{ 2000, 1000 },
> +	{ 2000, 5000 },
> +	{ 2000, 10000 },
> +	{ 4000, 100 },

The comment above here looks like its for example1, this one does vary 
the window size.

Probably only example2 is enough, I assumed they were different but 
example2 is basically the same as example1 with an extra preset list. We 
could comment that the second preset list is optional and delete 
example1. Saves people reading more and wondering what the difference is.

I tried to make an example that doesn't use an existing feature by 
reacreating afdo from scratch which I thought would be a good example. 
It's pasted at the end. I had to copy paste the ETMv4 macros and 
constants though, I couldn't include them in the userspace generator 
because of this error:

   coresight-config.h:10:10: fatal error: linux/coresight.h: No such 
file
   or directory
    10 | #include <linux/coresight.h>

I also got this error when loading it:

Unable to handle kernel NULL pointer dereference at virtual address 
0000000000000008

cscfg_reset_feat (drivers/hwtracing/coresight/coresight-config.c:64 
coresight-config.c:124) coresight
cscfg_load_config_sets 
(drivers/hwtracing/coresight/coresight-syscfg.c:217 
coresight-syscfg.c:262 coresight-syscfg.c:492 coresight-syscfg.c:610) 
coresight
cscfg_dyn_load_cfg_table 
(drivers/hwtracing/coresight/coresight-syscfg-configfs.c:294) coresight
cscfg_cfg_load_table_write 
(drivers/hwtracing/coresight/coresight-syscfg-configfs.c:799) coresight
configfs_release_bin_file (fs/configfs/file.c:415)
__fput (fs/file_table.c:432)
__fput_sync (fs/file_table.c:517)
__arm64_sys_close (fs/open.c:1568 fs/open.c:1550 fs/open.c:1550)
invoke_syscall (arch/arm64/kernel/syscall.c:? 
arch/arm64/kernel/syscall.c:49)
el0_svc_common (include/linux/thread_info.h:127 
arch/arm64/kernel/syscall.c:140)
do_el0_svc (arch/arm64/kernel/syscall.c:152)


So I'm wondering if we can do the same thing by setting values via 
individual files rather than one binary blob which would avoid some of 
these issues. From what I understand the feature params can already be 
set directly this way via 
/sys/kernel/config/cs-syscfg/features/strobing/params/period/value

We'd have to add a way to add new configs with a mkdir, then the same
things can be configured without needing an additional tool. Links 
between features and configs can be done with symlinks which is also 
mentioned in the configfs docs.

Something like this would be a bit like the current version:

  # ls /config/cs-syscfg
  configurations
  features

  # mkdir /config/cs-syscfg/features/my_config
  # ls /config/cs-syscfg/features/my_config
  description
  matches
  regs_desc
  params

  # mkdir /config/cs-syscfg/features/my_config/regs_desc/TRCRSCTLRn0
  # ls /config/cs-syscfg/features/my_config/TRCRSCTLRn0
  type
  offset
  val
  mask

But another way could be to enumerate all possible regs for each device. 
This would remove the need to have all the #defines in whatever tool is
making the config (avoiding the #include issue from above):

  # mkdir /config/cs-syscfg/features/my_config
  # ls /config/cs-syscfg/features/my_config/regs_desc

  regs_desc is initially empty, but specify what device it's for to make
  all the properties appear (or the mkdir could be done in an etmv4
  folder):

  # echo "SRC_ETM4" > matches
  # ls /config/cs-syscfg/features/my_config/regs_desc
  TRCRSCTLRn0
  TRCRSCTLRn1
  TRCRSCTLRn2
  ... etc ...

  Now type and offset don't need to be set:

  # ls /config/cs-syscfg/features/my_config/regs_desc/TRCRSCTLRn0
  val
  mask
  save

Don't we already have the full list of parameters in 
etm4_cfg_map_reg_offset(), so we can expose this to users via configfs 
directly rather than needing any tooling. And doesn't any new device 
that's supported by the config mechanism need to know about all its 
parameters, so it wouldn't remove any flexibility?

Powered by blists - more mailing lists