Re: [PATCH v1 1/9] perf: arm_spe: Introduce 'lds' capacity

On Fri, Aug 30, 2024 at 01:12:33PM +0100, Leo Yan wrote:

On 8/30/24 11:38, Will Deacon wrote:

quoted

On Tue, Aug 27, 2024 at 05:44:09PM +0100, Leo Yan wrote:

quoted

This commit adds a new entry 'lds' in the capacity folder. 'lds' stands
for "loaded data source". When its value is 1, it indicates the data
source implemented, and data source packets will be recorded in the
trace data.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
  drivers/perf/arm_spe_pmu.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 9100d82bfabc..81c1e7627721 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c

@@ -110,6 +110,7 @@ enum arm_spe_pmu_buf_fault_action {
  /* This sysfs gunk was really good fun to write. */
  enum arm_spe_pmu_capabilities {
       SPE_PMU_CAP_ARCH_INST = 0,
+     SPE_PMU_CAP_LDS,
       SPE_PMU_CAP_ERND,
       SPE_PMU_CAP_FEAT_MAX,
       SPE_PMU_CAP_CNT_SZ = SPE_PMU_CAP_FEAT_MAX,

@@ -118,6 +119,7 @@ enum arm_spe_pmu_capabilities {

  static int arm_spe_pmu_feat_caps[SPE_PMU_CAP_FEAT_MAX] = {
       [SPE_PMU_CAP_ARCH_INST] = SPE_PMU_FEAT_ARCH_INST,
+     [SPE_PMU_CAP_LDS]       = SPE_PMU_FEAT_LDS,
       [SPE_PMU_CAP_ERND]      = SPE_PMU_FEAT_ERND,
  };

@@ -160,6 +162,7 @@ static ssize_t arm_spe_pmu_cap_show(struct device *dev,

  static struct attribute *arm_spe_pmu_cap_attr[] = {
       SPE_CAP_EXT_ATTR_ENTRY(arch_inst, SPE_PMU_CAP_ARCH_INST),
+     SPE_CAP_EXT_ATTR_ENTRY(lds, SPE_PMU_CAP_LDS),
       SPE_CAP_EXT_ATTR_ENTRY(ernd, SPE_PMU_CAP_ERND),
       SPE_CAP_EXT_ATTR_ENTRY(count_size, SPE_PMU_CAP_CNT_SZ),
       SPE_CAP_EXT_ATTR_ENTRY(min_interval, SPE_PMU_CAP_MIN_IVAL),

What will userspace do with this? I don't think you can turn LDS on/off,
so either you'll get the data source packet or you won't.

Yes, LDS bit does not work as a switch.

The tool in the userspace will record the LDS bit into the metadata. During
decoding phase, it reads out the LDS from metadata. Based on it, the perf
tool can know if the data source is supported or not, if yes then decode the
data source packet.

Why not just decode a data source packet when you see it? i.e. assume LDS
is always set.

Another point is how to decide the data source packet format. Now we maintain
a CPU list for tracking CPU variants which support data source trace. For long
term, I would like the tool can based on hardware feature (e.g. a ID register
in Arm SPE) to decide the data source format, so far it is absent. This is why
LDS bit + CPU list is a more reliable way. See some discussion [1].

Huh. Why would you have a CPU in the list if it _doesn't_ have LDS? If
we have to resort to per-CPU decoding, then that's even more of a reason
not to have the LDS cap imo.

Will