[RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Arnaldo Carvalho de Melo]

To get the changes in:

  0ce85db6c2141b7f ("arm64: cputype: Add Neoverse-V3 definitions")
  02a0a04676fa7796 ("arm64: cputype: Add Cortex-X4 definitions")
  f4d9d9dcc70b96b5 ("arm64: Add Neoverse-V2 part")

That makes this perf source code to be rebuilt:

  CC      /tmp/build/perf-tools/util/arm-spe.o

The changes in the above patch add MIDR_NEOVERSE_V[23] and
MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
for later when this is all tested on those machines?

  static const struct midr_range neoverse_spe[] = {
          MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
          MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N2),
          MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V1),
          {},
  };

That addresses this perf build warning:

  Warning: Kernel ABI header differences:
    diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Besar Wicaksono <bwicaksono@nvidia.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Will Deacon <will@kernel.org>
Link: https://lore.kernel.org/lkml/
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/arch/arm64/include/asm/cputype.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/arch/arm64/include/asm/cputype.h b/tools/arch/arm64/include/asm/cputype.h
index 52f076afeb96006c..7b32b99023a21d3a 100644
--- a/tools/arch/arm64/include/asm/cputype.h
+++ b/tools/arch/arm64/include/asm/cputype.h
@@ -86,6 +86,9 @@
 #define ARM_CPU_PART_CORTEX_X2		0xD48
 #define ARM_CPU_PART_NEOVERSE_N2	0xD49
 #define ARM_CPU_PART_CORTEX_A78C	0xD4B
+#define ARM_CPU_PART_NEOVERSE_V2	0xD4F
+#define ARM_CPU_PART_CORTEX_X4		0xD82
+#define ARM_CPU_PART_NEOVERSE_V3	0xD84
 
 #define APM_CPU_PART_XGENE		0x000
 #define APM_CPU_VAR_POTENZA		0x00
@@ -159,6 +162,9 @@
 #define MIDR_CORTEX_X2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_X2)
 #define MIDR_NEOVERSE_N2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_N2)
 #define MIDR_CORTEX_A78C	MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A78C)
+#define MIDR_NEOVERSE_V2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_V2)
+#define MIDR_CORTEX_X4 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_X4)
+#define MIDR_NEOVERSE_V3 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_V3)
 #define MIDR_THUNDERX	MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
 #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
 #define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX)
-- 
2.44.0

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Mark Rutland]

Hi Arnaldo,

On Mon, Jun 03, 2024 at 03:33:07PM -0300, Arnaldo Carvalho de Melo wrote:
> To get the changes in:
> 
>   0ce85db6c2141b7f ("arm64: cputype: Add Neoverse-V3 definitions")
>   02a0a04676fa7796 ("arm64: cputype: Add Cortex-X4 definitions")
>   f4d9d9dcc70b96b5 ("arm64: Add Neoverse-V2 part")

As a heads-up, there are likely to be a couple more updates here
shortly:

  https://lore.kernel.org/linux-arm-kernel/20240603111812.1514101-1-mark.rutland@arm.com/

> That makes this perf source code to be rebuilt:
> 
>   CC      /tmp/build/perf-tools/util/arm-spe.o
> 
> The changes in the above patch add MIDR_NEOVERSE_V[23] and
> MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
> and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
> for later when this is all tested on those machines?

Hmm... looking at where that was added this is somewhat misnamed, this
is really saying that these cores use the same IMPLEMENTATION DEFINED
encoding of the source field. That's not really a property of Neoverse
specifically, and I'm not sure what Arm's policy is here going forwards.

We should probably rename that to something like
common_data_source_encoding, with a big comment about exactly what it
implies.

I would not touch this for now -- someone would have to go audit the
TRMs to check that those other cores have the same encoding, and I think
it'd be better to do that as a follow-up.

The relevant commit was:

  4e6430cbb1a9f1dc ("perf arm-spe: Use SPE data source for neoverse cores")

Mark.

>   static const struct midr_range neoverse_spe[] = {
>           MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
>           MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N2),
>           MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V1),
>           {},
>   };
> 
> That addresses this perf build warning:
> 
>   Warning: Kernel ABI header differences:
>     diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Besar Wicaksono <bwicaksono@nvidia.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Link: https://lore.kernel.org/lkml/
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/arch/arm64/include/asm/cputype.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/arch/arm64/include/asm/cputype.h b/tools/arch/arm64/include/asm/cputype.h
> index 52f076afeb96006c..7b32b99023a21d3a 100644
> --- a/tools/arch/arm64/include/asm/cputype.h
> +++ b/tools/arch/arm64/include/asm/cputype.h
> @@ -86,6 +86,9 @@
>  #define ARM_CPU_PART_CORTEX_X2		0xD48
>  #define ARM_CPU_PART_NEOVERSE_N2	0xD49
>  #define ARM_CPU_PART_CORTEX_A78C	0xD4B
> +#define ARM_CPU_PART_NEOVERSE_V2	0xD4F
> +#define ARM_CPU_PART_CORTEX_X4		0xD82
> +#define ARM_CPU_PART_NEOVERSE_V3	0xD84
>  
>  #define APM_CPU_PART_XGENE		0x000
>  #define APM_CPU_VAR_POTENZA		0x00
> @@ -159,6 +162,9 @@
>  #define MIDR_CORTEX_X2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_X2)
>  #define MIDR_NEOVERSE_N2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_N2)
>  #define MIDR_CORTEX_A78C	MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A78C)
> +#define MIDR_NEOVERSE_V2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_V2)
> +#define MIDR_CORTEX_X4 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_X4)
> +#define MIDR_NEOVERSE_V3 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_V3)
>  #define MIDR_THUNDERX	MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
>  #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
>  #define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX)
> -- 
> 2.44.0
> 

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Arnaldo Carvalho de Melo]

On Tue, Jun 04, 2024 at 10:11:22AM +0100, Mark Rutland wrote:
> Hi Arnaldo,
> 
> On Mon, Jun 03, 2024 at 03:33:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > To get the changes in:
> > 
> >   0ce85db6c2141b7f ("arm64: cputype: Add Neoverse-V3 definitions")
> >   02a0a04676fa7796 ("arm64: cputype: Add Cortex-X4 definitions")
> >   f4d9d9dcc70b96b5 ("arm64: Add Neoverse-V2 part")
> 
> As a heads-up, there are likely to be a couple more updates here
> shortly:
> 
>   https://lore.kernel.org/linux-arm-kernel/20240603111812.1514101-1-mark.rutland@arm.com/
> 
> > That makes this perf source code to be rebuilt:
> > 
> >   CC      /tmp/build/perf-tools/util/arm-spe.o
> > 
> > The changes in the above patch add MIDR_NEOVERSE_V[23] and
> > MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
> > and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
> > for later when this is all tested on those machines?
> 
> Hmm... looking at where that was added this is somewhat misnamed, this
> is really saying that these cores use the same IMPLEMENTATION DEFINED
> encoding of the source field. That's not really a property of Neoverse
> specifically, and I'm not sure what Arm's policy is here going forwards.
> 
> We should probably rename that to something like
> common_data_source_encoding, with a big comment about exactly what it
> implies.
> 
> I would not touch this for now -- someone would have to go audit the

Ok, you mean not touch tools/perf/util/arm-spe.c, right, can I just go
ahead and update the copy of that header so that we have a clean (of
build warnings) build?

Thanks for checking!

- Arnaldo

> TRMs to check that those other cores have the same encoding, and I think
> it'd be better to do that as a follow-up.
> 
> The relevant commit was:
> 
>   4e6430cbb1a9f1dc ("perf arm-spe: Use SPE data source for neoverse cores")
> 
> Mark.
> 
> >   static const struct midr_range neoverse_spe[] = {
> >           MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
> >           MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N2),
> >           MIDR_ALL_VERSIONS(MIDR_NEOVERSE_V1),
> >           {},
> >   };
> > 
> > That addresses this perf build warning:
> > 
> >   Warning: Kernel ABI header differences:
> >     diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Besar Wicaksono <bwicaksono@nvidia.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> > Link: https://lore.kernel.org/lkml/
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/arch/arm64/include/asm/cputype.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tools/arch/arm64/include/asm/cputype.h b/tools/arch/arm64/include/asm/cputype.h
> > index 52f076afeb96006c..7b32b99023a21d3a 100644
> > --- a/tools/arch/arm64/include/asm/cputype.h
> > +++ b/tools/arch/arm64/include/asm/cputype.h
> > @@ -86,6 +86,9 @@
> >  #define ARM_CPU_PART_CORTEX_X2		0xD48
> >  #define ARM_CPU_PART_NEOVERSE_N2	0xD49
> >  #define ARM_CPU_PART_CORTEX_A78C	0xD4B
> > +#define ARM_CPU_PART_NEOVERSE_V2	0xD4F
> > +#define ARM_CPU_PART_CORTEX_X4		0xD82
> > +#define ARM_CPU_PART_NEOVERSE_V3	0xD84
> >  
> >  #define APM_CPU_PART_XGENE		0x000
> >  #define APM_CPU_VAR_POTENZA		0x00
> > @@ -159,6 +162,9 @@
> >  #define MIDR_CORTEX_X2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_X2)
> >  #define MIDR_NEOVERSE_N2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_N2)
> >  #define MIDR_CORTEX_A78C	MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A78C)
> > +#define MIDR_NEOVERSE_V2 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_V2)
> > +#define MIDR_CORTEX_X4 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_X4)
> > +#define MIDR_NEOVERSE_V3 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_V3)
> >  #define MIDR_THUNDERX	MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
> >  #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
> >  #define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX)
> > -- 
> > 2.44.0
> > 

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Mark Rutland]

On Tue, Jun 04, 2024 at 10:53:38AM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Jun 04, 2024 at 10:11:22AM +0100, Mark Rutland wrote:
> > On Mon, Jun 03, 2024 at 03:33:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > > The changes in the above patch add MIDR_NEOVERSE_V[23] and
> > > MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
> > > and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
> > > for later when this is all tested on those machines?
> > 
> > Hmm... looking at where that was added this is somewhat misnamed, this
> > is really saying that these cores use the same IMPLEMENTATION DEFINED
> > encoding of the source field. That's not really a property of Neoverse
> > specifically, and I'm not sure what Arm's policy is here going forwards.
> > 
> > We should probably rename that to something like
> > common_data_source_encoding, with a big comment about exactly what it
> > implies.
> > 
> > I would not touch this for now -- someone would have to go audit the
> 
> Ok, you mean not touch tools/perf/util/arm-spe.c, right, can I just go
> ahead and update the copy of that header so that we have a clean (of
> build warnings) build?

Yes: update the header, but leave arm-spe.c unchanged. Sorry for not
being clear!

It'd be nice if we could update the commit message to note that we're
deliberately leaving that as-is.

Either way:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Leo Yan]

On 6/4/24 10:11, Mark Rutland wrote:
> Hi Arnaldo,
> 
> On Mon, Jun 03, 2024 at 03:33:07PM -0300, Arnaldo Carvalho de Melo wrote:
>> To get the changes in:
>>
>>    0ce85db6c2141b7f ("arm64: cputype: Add Neoverse-V3 definitions")
>>    02a0a04676fa7796 ("arm64: cputype: Add Cortex-X4 definitions")
>>    f4d9d9dcc70b96b5 ("arm64: Add Neoverse-V2 part")
> 
> As a heads-up, there are likely to be a couple more updates here
> shortly:
> 
>    https://lore.kernel.org/linux-arm-kernel/20240603111812.1514101-1-mark.rutland@arm.com/
> 
>> That makes this perf source code to be rebuilt:
>>
>>    CC      /tmp/build/perf-tools/util/arm-spe.o
>>
>> The changes in the above patch add MIDR_NEOVERSE_V[23] and
>> MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
>> and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
>> for later when this is all tested on those machines?
> 
> Hmm... looking at where that was added this is somewhat misnamed, this
> is really saying that these cores use the same IMPLEMENTATION DEFINED
> encoding of the source field. That's not really a property of Neoverse
> specifically, and I'm not sure what Arm's policy is here going forwards.
> 
> We should probably rename that to something like
> common_data_source_encoding, with a big comment about exactly what it
> implies.

Agreed.

I went through Neoverse-V2/V3/V4, Cortex-X4, Cortex-X4, Cortex-A720, and 
Cortex-X925, all of them have the same definition for data source packet 
format. It makes sense to change name to Neoverse specific to a more 
general name.

> I would not touch this for now -- someone would have to go audit the
> TRMs to check that those other cores have the same encoding, and I think
> it'd be better to do that as a follow-up.

So far, it's fine to not change the file util/arm-spe.c.

Now more and more Arm CPUs support the data source in SPE and share the 
same data source format. It's not scalable for us to adding every CPU 
variant into the file util/arm-spe.c.

I would like to expose the PMSIDR_EL1.LDS bit (Data source indicator for 
sampled load instructions) via the 'cap' folder, and then we can save 
this info into the perf meta data during record phase.  In the perf 
report, we can parse the meta data and if the PMSIDR_EL1.LDS bit is set, 
the tool will parse the data source packet based on the common format.

Thanks,
Leo

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Mark Rutland]

On Tue, Jun 04, 2024 at 06:14:22PM +0100, Leo Yan wrote:
> On 6/4/24 10:11, Mark Rutland wrote:
> > Hi Arnaldo,
> > 
> > On Mon, Jun 03, 2024 at 03:33:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > > To get the changes in:
> > > 
> > >    0ce85db6c2141b7f ("arm64: cputype: Add Neoverse-V3 definitions")
> > >    02a0a04676fa7796 ("arm64: cputype: Add Cortex-X4 definitions")
> > >    f4d9d9dcc70b96b5 ("arm64: Add Neoverse-V2 part")
> > 
> > As a heads-up, there are likely to be a couple more updates here
> > shortly:
> > 
> >    https://lore.kernel.org/linux-arm-kernel/20240603111812.1514101-1-mark.rutland@arm.com/
> > 
> > > That makes this perf source code to be rebuilt:
> > > 
> > >    CC      /tmp/build/perf-tools/util/arm-spe.o
> > > 
> > > The changes in the above patch add MIDR_NEOVERSE_V[23] and
> > > MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
> > > and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
> > > for later when this is all tested on those machines?
> > 
> > Hmm... looking at where that was added this is somewhat misnamed, this
> > is really saying that these cores use the same IMPLEMENTATION DEFINED
> > encoding of the source field. That's not really a property of Neoverse
> > specifically, and I'm not sure what Arm's policy is here going forwards.
> > 
> > We should probably rename that to something like
> > common_data_source_encoding, with a big comment about exactly what it
> > implies.
> 
> Agreed.
> 
> I went through Neoverse-V2/V3/V4, Cortex-X4, Cortex-X4, Cortex-A720, and
> Cortex-X925, all of them have the same definition for data source packet
> format. It makes sense to change name to Neoverse specific to a more general
> name.

Cool.

> > I would not touch this for now -- someone would have to go audit the
> > TRMs to check that those other cores have the same encoding, and I think
> > it'd be better to do that as a follow-up.
> 
> So far, it's fine to not change the file util/arm-spe.c.
> 
> Now more and more Arm CPUs support the data source in SPE and share the same
> data source format. It's not scalable for us to adding every CPU variant
> into the file util/arm-spe.c.
> 
> I would like to expose the PMSIDR_EL1.LDS bit (Data source indicator for
> sampled load instructions) via the 'cap' folder, and then we can save this
> info into the perf meta data during record phase.

I'd be happy to expose fields from PMSIDR_EL1.

> In the perf report, we can parse the meta data and if the
> PMSIDR_EL1.LDS bit is set, the tool will parse the data source packet
> based on the common format.

I don't believe that's right.

PMSIDR_EL1.LDS indicates that the loaded data source field is
implemented, but even when it is implemented, the format is
IMPLEMENTATION DEFINED.

Today, Arm Ltd implementations happen to share a format, but that isn't
implied by PMSIDR_EL1.LDS, and there's no guarantee that future CPUs
will all use the same format.

For the moment we'll have to keep adding to this list.

Mark.

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Arnaldo Carvalho de Melo]

On Tue, Jun 04, 2024 at 03:34:36PM +0100, Mark Rutland wrote:
> On Tue, Jun 04, 2024 at 10:53:38AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, Jun 04, 2024 at 10:11:22AM +0100, Mark Rutland wrote:
> > > On Mon, Jun 03, 2024 at 03:33:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > The changes in the above patch add MIDR_NEOVERSE_V[23] and
> > > > MIDR_NEOVERSE_V1 is used in arm-spe.c, so probably we need to add those
> > > > and perhaps MIDR_CORTEX_X4 to that array? Or maybe we need to leave this
> > > > for later when this is all tested on those machines?
> > > 
> > > Hmm... looking at where that was added this is somewhat misnamed, this
> > > is really saying that these cores use the same IMPLEMENTATION DEFINED
> > > encoding of the source field. That's not really a property of Neoverse
> > > specifically, and I'm not sure what Arm's policy is here going forwards.
> > > 
> > > We should probably rename that to something like
> > > common_data_source_encoding, with a big comment about exactly what it
> > > implies.
> > > 
> > > I would not touch this for now -- someone would have to go audit the
> > 
> > Ok, you mean not touch tools/perf/util/arm-spe.c, right, can I just go
> > ahead and update the copy of that header so that we have a clean (of
> > build warnings) build?
> 
> Yes: update the header, but leave arm-spe.c unchanged. Sorry for not
> being clear!

np
 
> It'd be nice if we could update the commit message to note that we're
> deliberately leaving that as-is.

There is a link tag to this thread and I'll update the message removing
my questions and adding your recommendation.

> Either way:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks!

- Arnaldo

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Leo Yan]

On 6/4/24 19:55, Mark Rutland wrote:

[...]

>> Now more and more Arm CPUs support the data source in SPE and share the same
>> data source format. It's not scalable for us to adding every CPU variant
>> into the file util/arm-spe.c.
>>
>> I would like to expose the PMSIDR_EL1.LDS bit (Data source indicator for
>> sampled load instructions) via the 'cap' folder, and then we can save this
>> info into the perf meta data during record phase.
> 
> I'd be happy to expose fields from PMSIDR_EL1.
> 
>> In the perf report, we can parse the meta data and if the
>> PMSIDR_EL1.LDS bit is set, the tool will parse the data source packet
>> based on the common format.
> 
> I don't believe that's right.
> 
> PMSIDR_EL1.LDS indicates that the loaded data source field is
> implemented, but even when it is implemented, the format is
> IMPLEMENTATION DEFINED.

Thanks for correction. PMSIDR_EL1.LDS bit is necessary but not 
sufficient for using the common data source format.

> Today, Arm Ltd implementations happen to share a format, but that isn't
> implied by PMSIDR_EL1.LDS, and there's no guarantee that future CPUs
> will all use the same format.
> 
> For the moment we'll have to keep adding to this list.

I would like to use an opposite way - we can only maintain CPU variants 
with special data source format, otherwise, all other CPUs use the 
common format.

Thanks,
Leo

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Mark Rutland]

On Tue, Jun 04, 2024 at 09:01:41PM +0100, Leo Yan wrote:
> On 6/4/24 19:55, Mark Rutland wrote:
> 
> [...]
> 
> > > Now more and more Arm CPUs support the data source in SPE and share the same
> > > data source format. It's not scalable for us to adding every CPU variant
> > > into the file util/arm-spe.c.
> > > 
> > > I would like to expose the PMSIDR_EL1.LDS bit (Data source indicator for
> > > sampled load instructions) via the 'cap' folder, and then we can save this
> > > info into the perf meta data during record phase.
> > 
> > I'd be happy to expose fields from PMSIDR_EL1.
> > 
> > > In the perf report, we can parse the meta data and if the
> > > PMSIDR_EL1.LDS bit is set, the tool will parse the data source packet
> > > based on the common format.
> > 
> > I don't believe that's right.
> > 
> > PMSIDR_EL1.LDS indicates that the loaded data source field is
> > implemented, but even when it is implemented, the format is
> > IMPLEMENTATION DEFINED.
> 
> Thanks for correction. PMSIDR_EL1.LDS bit is necessary but not sufficient
> for using the common data source format.
> 
> > Today, Arm Ltd implementations happen to share a format, but that isn't
> > implied by PMSIDR_EL1.LDS, and there's no guarantee that future CPUs
> > will all use the same format.
> > 
> > For the moment we'll have to keep adding to this list.
> 
> I would like to use an opposite way - we can only maintain CPU variants with
> special data source format, otherwise, all other CPUs use the common format.

I think that's not a good idea.

Today, Arm Ltd CPUs happen to share *a* common format, but that's likely
to change at some point in future, and CPUs from other vendors are
likely to use different formats.

Assuming any format by default means that when CPUs with different
formats are released, we'll produce incorrect results for those CPU by
default, we'll need to update tables to exclude those CPUs, and we'll
probably want to backport that exclusion to minimize the risk of users
getting incorrect/misleading results.

While the current situation isn't nice, I think the alternative is
worse -- it will confuse and anger users.

I think we need to talk with the Arm architects to see if they can
define some discovery mechanism for the data source format.

Mark.

RE: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Besar Wicaksono]

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Wednesday, June 5, 2024 4:32 PM
> To: Leo Yan <leo.yan@arm.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>; Besar Wicaksono
> <bwicaksono@nvidia.com>; Will Deacon <will@kernel.org>; Adrian Hunter
> <adrian.hunter@intel.com>; Ian Rogers <irogers@google.com>; Jiri Olsa
> <jolsa@kernel.org>; Kan Liang <kan.liang@linux.intel.com>; Namhyung Kim
> <namhyung@kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-perf-users@vger.kernel.org; James Clark
> <james.clark@arm.com>
> Subject: Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h
> with the kernel sources
> 
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Jun 04, 2024 at 09:01:41PM +0100, Leo Yan wrote:
> > On 6/4/24 19:55, Mark Rutland wrote:
> >
> > [...]
> >
> > > > Now more and more Arm CPUs support the data source in SPE and share
> the same
> > > > data source format. It's not scalable for us to adding every CPU variant
> > > > into the file util/arm-spe.c.
> > > >
> > > > I would like to expose the PMSIDR_EL1.LDS bit (Data source indicator for
> > > > sampled load instructions) via the 'cap' folder, and then we can save this
> > > > info into the perf meta data during record phase.
> > >
> > > I'd be happy to expose fields from PMSIDR_EL1.
> > >
> > > > In the perf report, we can parse the meta data and if the
> > > > PMSIDR_EL1.LDS bit is set, the tool will parse the data source packet
> > > > based on the common format.
> > >
> > > I don't believe that's right.
> > >
> > > PMSIDR_EL1.LDS indicates that the loaded data source field is
> > > implemented, but even when it is implemented, the format is
> > > IMPLEMENTATION DEFINED.
> >
> > Thanks for correction. PMSIDR_EL1.LDS bit is necessary but not sufficient
> > for using the common data source format.
> >
> > > Today, Arm Ltd implementations happen to share a format, but that isn't
> > > implied by PMSIDR_EL1.LDS, and there's no guarantee that future CPUs
> > > will all use the same format.
> > >
> > > For the moment we'll have to keep adding to this list.

I apologize I didn't follow. Do you mean adding new CPU identifier to neoverse_spe[]
list is still the way to go for now?

> >
> > I would like to use an opposite way - we can only maintain CPU variants with
> > special data source format, otherwise, all other CPUs use the common format.
> 
> I think that's not a good idea.
> 
> Today, Arm Ltd CPUs happen to share *a* common format, but that's likely
> to change at some point in future, and CPUs from other vendors are
> likely to use different formats.
> 
> Assuming any format by default means that when CPUs with different
> formats are released, we'll produce incorrect results for those CPU by
> default, we'll need to update tables to exclude those CPUs, and we'll
> probably want to backport that exclusion to minimize the risk of users
> getting incorrect/misleading results.
> 
> While the current situation isn't nice, I think the alternative is
> worse -- it will confuse and anger users.
> 
> I think we need to talk with the Arm architects to see if they can
> define some discovery mechanism for the data source format.

Is there a follow-up on this from Arm?

Thanks,
Besar

Re: [RFC/PATCH 1/1] tools headers arm64: Sync arm64's cputype.h with the kernel sources

[Leo Yan]

Hi Besar,

On 8/8/24 05:37, Besar Wicaksono wrote:

[...]

>>>> Today, Arm Ltd implementations happen to share a format, but that isn't
>>>> implied by PMSIDR_EL1.LDS, and there's no guarantee that future CPUs
>>>> will all use the same format.
>>>>
>>>> For the moment we'll have to keep adding to this list.
> 
> I apologize I didn't follow. Do you mean adding new CPU identifier to neoverse_spe[]
> list is still the way to go for now?

Yes, the conclusion is we still need to maintain the CPU ID list for Arm SPE
data source packets.

>>> I would like to use an opposite way - we can only maintain CPU variants with
>>> special data source format, otherwise, all other CPUs use the common format.
>>
>> I think that's not a good idea.
>>
>> Today, Arm Ltd CPUs happen to share *a* common format, but that's likely
>> to change at some point in future, and CPUs from other vendors are
>> likely to use different formats.
>>
>> Assuming any format by default means that when CPUs with different
>> formats are released, we'll produce incorrect results for those CPU by
>> default, we'll need to update tables to exclude those CPUs, and we'll
>> probably want to backport that exclusion to minimize the risk of users
>> getting incorrect/misleading results.
>>
>> While the current situation isn't nice, I think the alternative is
>> worse -- it will confuse and anger users.
>>
>> I think we need to talk with the Arm architects to see if they can
>> define some discovery mechanism for the data source format.
> 
> Is there a follow-up on this from Arm?

I am working on this. The plan is to add meta data support so we can have more
info about Arm SPE features (e.g. data source packet is supported). Then in
the perf tool we need to consolidate to support not only Neoverse core.

It is deferred as I need to firstly resolve the multiple Arm SPE devices, then
I will work on the meta data things and consolidate data source packet (and
include your patches [1]).

Thanks,
Leo

[1] https://lore.kernel.org/linux-perf-users/20240109192310.16234-1-bwicaksono@nvidia.com/