Message-ID: <5e89efa7-6780-404b-977f-76e96602e298@linaro.org>
Date: Wed, 18 Dec 2024 16:07:39 +0000
From: James Clark <james.clark@...aro.org>
To: Mike Leach <mike.leach@...aro.org>
Cc: coresight@...ts.linaro.org, Suzuki K Poulose <Suzuki.Poulose@....com>,
 Leo Yan <leo.yan@....com>,
 "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 12/12/2024 12:03 pm, Mike Leach wrote:
> Hi James
> 
> On Tue, 3 Dec 2024 at 14:25, James Clark <james.clark@...aro.org> wrote:
>>
>>
>>
>> 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.
>>
> 
> Not sure - perf keeps a copy and has a check script on build to ensure
> the copy matches the kernel version.
> Keeping two copies of the same thing always strikes me as poor
> practice, so I went for copying over.
> 
> Both methods risk breakages when something in the kernel changes.
> 

You might want to check tools/include/uapi/README which goes over it in 
more detail. I don't think it claims that it's perfect, but just better 
than alternatives. It would make sense to follow it for consistency more 
than anything.

>> [...]
>>
>>> +
>>> +/*
>>> + * 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.
>>
> 
> Yes - perhaps both examples were not necessary - but the point was you
> can have two configs in a single loadable table.
> 
>> 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:
>>
> 
> The updated ETM config set that is to follow addresses this issue -
> the macros are split off into a separate file  (and adds in a whole
> lot of validation - ensuring a configuration cannot specify and
> allocate more resources than are available.)
> This is also the reason that the examples provided were very simple.
> More complex ones are to follow!
> 
> This set was focused on loading tables so that the next patchsets
> dealing with resource validating ETM configs and CTI configs had an
> easy to use test platform.
> 
> 
>>     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)
>>
> 
> I'll go back to my tests to check I was testing what I thought I was -
> and didn't accidentally change something after testing and before
> sending the patchset.
> 
>>
>> 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
>>
> 
> This is precisely what I wanted to avoid. Doing it this way is both
> time consuming for the user and ends up reproducing most of the sysfs
> files in configfs.
> 

I thought I'd suggest the alternative in case Christoph's previous 
comment about not loading a binary blob was a hard nack. I understand 
your counter points about why the blob is better automatically, but I 
don't think any of them are completely unsolvable or blocking with the 
alternate implementation.

For the point about being time consuming, personally I found it quite 
time consuming to use the compiler and write in C. Rather than a pasted 
bash script that just lists each config element:

   echo "12" > /config/cs-syscfg/features/my_config/TRCRSCTLRn0
   echo "11" > /config/cs-syscfg/features/my_config/TRCRSCTLRn1
   ... etc ...

> Parameters are there to allow a limited amount of relevent runtime
> adjustment. Parameter values may also only populate part of a hw
> register depending on use case.
> 
 > The resource approach allows us to define certain bitfields e.g.> 
Branch broadcast - as user configurable - but omit anything users are
> not allowed to meddle with.
> 

This sounds like a mask file along side each register could be used to 
determine which bits to populate for a specific use case. And there 
could be kernel and user provided masks to limit which bits are written.

> The table load is atomic - it is validated and succeeds or fails
> completely. The load mechanism prevents the new configuration from
> becoming visible until it is loaded. The configfs per file method has
> an issue in deciding when the configuration is "complete". If you look
> in the configfs docs/source there was at one time  a  "commit"
> methodology proposed, but this never came to fruition.
> 

Yeah I was going to suggest some kind of "commit" file or "active" flag. 
Or by creating a symlink into an "active" folder at which point the 
config becomes write only (or writes are cached until the next use of 
the config which Suzuki suggested for another sysfs change on the list 
at the moment).

For validation, each field would be validated on write whether the 
config is already active or not. So presumably a script writing a list 
of config values would fail before reaching the "activation" or "commit" 
step.

> Table load  re-uses the same mechanisms as the built-in and loadable
> module methods for adding configurations, but without the need for
> re-compiling the kernel / compiling against a specific kernel -
> meaning loadable tables are dependent only on the hardware available,
 > not the kernel version.

Isn't this the same as with the file approach? Even with the binary blob 
you can't configure a device that the kernel doesn't know about. And 
plain text files can be written to and reconfigured without recompiling 
or using a specific kernel version.

> Adding a per file option means that the new
> configurations would be different from the current load methods
> upstream
> 

This is true, but this binary blob compiler also felt quite different, 
and I wasn't able to blindly copy paste the existing autofdo config into 
it. It needed just as much work to get it to compile as it would have 
been to write the equivalent bash script echoing the config values.

 > - and appear differently in configfs.

I don't think that my suggestion requires that it appears differently at 
the point of use though. After a successful activation the configs could 
appear for use in the same way as the existing module loaded configs.

With all of that said both mechanisms accomplish the same thing, so I 
can't really say to definitely not to it the binary way. Just that it 
feels a bit unusual. Apart from the ACPI example, which is more of a 
firmware thing than something that's controlled by a user at runtime, 
I'm not sure if there is an example of a configfs interface like this?

James

>> 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?
>>
> 
> Mike
> 
> 
>>
> 
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

Powered by blists - more mailing lists