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

[Will Deacon <will@kernel.org>]

On Sat, Aug 31, 2024 at 12:37:29PM +0100, Leo Yan wrote:
> On 8/30/2024 2:09 PM, Will Deacon wrote:
> 
> [...]
> 
> >>>> @@ -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.
> 
> The current tool works this way to directly decode a data source packet.
> 
> However, as Arm ARM section D17.2.4 "Data Source packet" describes, the loaded
> data source is implementation dependent, the data source payload format also
> is implementation defined.
> 
> We are halfway here in using the LDS bit to determine if the data source is
> implemented. However, we lack information on the data source format
> implementation. As a first step, we can use the LDS bit for sanity checking in
> the tool to detect any potential silicon implementation issues. Once we have
> an architectural definition for the data source format, we can extend the tool
> accordingly.

I don't think we shyould expose UAPI from the driver to detect potential
hardware bugs. Let's add it when we know it's useful for something instead.

> 
> >> 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?
> 
> Yeah, this is what we don't expect - we can verify the implementation based on
> LDS bit.
> 
> E.g. if users ask data source related questions, we can use LDS bit (saved in
> the perf metadata) to confirm the feature has been implemented in a silicon.

What exactly do you mean by this?

As far as I can tell:

  - Data source packets are either present or absent depending on LDS
  - You need CPU-specific information to decode them it they are present

So it's neither necessary nor sufficient to expose the LDS bit to
userspace.

Will