| Message ID | 20210721090706.21523-3-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support ETE decoding | expand |
HI James On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote: > > Currently the architecture is hard coded as ARCH_V8, but with the > introduction of ETE we want to pick ARCH_AA64. And this change is also > applicable to ETM v4.4 onwards as well. > > Signed-off-by: James Clark <james.clark@arm.com> > --- > tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > index 30889a9d0165..5972a8afcc6b 100644 > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params, > return 0; > } > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4 > +#define TRCIDR1_TRCARCHMIN_MASK GENMASK(7, 4) > +#define TRCIDR1_TRCARCHMIN(x) (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT) > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) > +{ > + /* > + * If the ETM trace minor version is 4 or more then we can assume > + * the architecture is ARCH_AA64 rather than just V8 > + */ > + return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8; > +} This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x Probably need to beef up this comment or the function name to emphasise this. Also only true because we don't currently support AArch32 builds of the ETM4.x driver. Regards Mike > + > static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params, > ocsd_etmv4_cfg *config) > { > @@ -140,7 +152,7 @@ static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params, > config->reg_idr11 = 0; > config->reg_idr12 = 0; > config->reg_idr13 = 0; > - config->arch_ver = ARCH_V8; > + config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1); > config->core_prof = profile_CortexA; > } > > -- > 2.28.0 >
On Thu, Jul 22, 2021 at 12:10:35PM +0100, Mike Leach wrote: > HI James > > On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote: > > > > Currently the architecture is hard coded as ARCH_V8, but with the > > introduction of ETE we want to pick ARCH_AA64. And this change is also > > applicable to ETM v4.4 onwards as well. > > > > Signed-off-by: James Clark <james.clark@arm.com> > > --- > > tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > index 30889a9d0165..5972a8afcc6b 100644 > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params, > > return 0; > > } > > > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4 > > +#define TRCIDR1_TRCARCHMIN_MASK GENMASK(7, 4) > > +#define TRCIDR1_TRCARCHMIN(x) (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT) > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) > > +{ > > + /* > > + * If the ETM trace minor version is 4 or more then we can assume > > + * the architecture is ARCH_AA64 rather than just V8 > > + */ > > + return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8; > > +} > > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x > Probably need to beef up this comment or the function name to emphasise this. Yeah, I think it's good to change the function name. Eventually, this function should only be used for ETM4.x and ETE. Another minor comment is: can we refine the arch version number, e.g. change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version. And a nitpick: how about to change OpenCSD macro "ARCH_V8r3" to "ARCH_V8R3" and assign it for ETMv4.3 IPs. Thanks, Leo
Hi Leo, On Sat, 31 Jul 2021 at 07:03, Leo Yan <leo.yan@linaro.org> wrote: > > On Thu, Jul 22, 2021 at 12:10:35PM +0100, Mike Leach wrote: > > HI James > > > > On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote: > > > > > > Currently the architecture is hard coded as ARCH_V8, but with the > > > introduction of ETE we want to pick ARCH_AA64. And this change is also > > > applicable to ETM v4.4 onwards as well. > > > > > > Signed-off-by: James Clark <james.clark@arm.com> > > > --- > > > tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > > index 30889a9d0165..5972a8afcc6b 100644 > > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > > @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params, > > > return 0; > > > } > > > > > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4 > > > +#define TRCIDR1_TRCARCHMIN_MASK GENMASK(7, 4) > > > +#define TRCIDR1_TRCARCHMIN(x) (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT) > > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) > > > +{ > > > + /* > > > + * If the ETM trace minor version is 4 or more then we can assume > > > + * the architecture is ARCH_AA64 rather than just V8 > > > + */ > > > + return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8; > > > +} > > > > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x > > Probably need to beef up this comment or the function name to emphasise this. > > Yeah, I think it's good to change the function name. Eventually, this > function should only be used for ETM4.x and ETE. > > Another minor comment is: can we refine the arch version number, e.g. > change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or > "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version. > The purpose of these macros is to inform the decoder of the architecture of the PE - not the version of the ETM. These OpenCSD macros are defined by the library headers (ocsd_if_types.h) and not the perf headers. These have been published as the API / ABI for OpenCSD and as such changing them affects all OpenCSD clients, not just perf. This PE architecture version is used along with the core profile to determine which instructions are valid waypoint instructions to associate with atom elements when walking the program image during trace decode. From v8.3 onwards we moved away from filtering on specific architecture versions. This was due to two factors:- 1. The architectural rules now allow architectural features for one increment e.g. Arch 8.4, to be backported into the previous increment - e,g, 8.3, which made this filtering more difficult to track. 2. After discussion with the PE architects it was clear that instructions in a later architect version would not re-use older opcodes from a previous one and be nop / invalid in the earlier architectures. (certainly in the scope of AA64). Therefore the policy in the decoder is to check for all the instructions we know about for the latest version of architecture, even if we could be decoding an earlier architecture version. This means we may check for a few more opcodes than necessary for earlier version of the architecture, but the overall decode is more robust and easier to maintain. Therefore for any AA64 core beyond v8.3 - it is safe to use the ARCH_AA64 PE architecture version and the decoder will handle it. Regards Mike > And a nitpick: how about to change OpenCSD macro "ARCH_V8r3" to > "ARCH_V8R3" and assign it for ETMv4.3 IPs. > > Thanks, > Leo
Hi Mike, On Mon, Aug 02, 2021 at 03:04:14PM +0100, Mike Leach wrote: [...] > > > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4 > > > > +#define TRCIDR1_TRCARCHMIN_MASK GENMASK(7, 4) > > > > +#define TRCIDR1_TRCARCHMIN(x) (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT) > > > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) > > > > +{ > > > > + /* > > > > + * If the ETM trace minor version is 4 or more then we can assume > > > > + * the architecture is ARCH_AA64 rather than just V8 > > > > + */ > > > > + return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8; > > > > +} > > > > > > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x > > > Probably need to beef up this comment or the function name to emphasise this. > > > > Yeah, I think it's good to change the function name. Eventually, this > > function should only be used for ETM4.x and ETE. > > > > Another minor comment is: can we refine the arch version number, e.g. > > change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or > > "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version. > > > > The purpose of these macros is to inform the decoder of the > architecture of the PE - not the version of the ETM. > > These OpenCSD macros are defined by the library headers > (ocsd_if_types.h) and not the perf headers. > These have been published as the API / ABI for OpenCSD and as such > changing them affects all OpenCSD clients, not just perf. I understand these macros are defined in OpenCSD lib as APIs, since I saw these macros have not been widely used in perf tool (e.g. ARCH_AA64), so this is why I think it's good to take chance to refine the naming conventions. > This PE architecture version is used along with the core profile to > determine which instructions are valid waypoint instructions to > associate with atom elements when walking the program image during > trace decode. > > From v8.3 onwards we moved away from filtering on specific > architecture versions. This was due to two factors:- > 1. The architectural rules now allow architectural features for one > increment e.g. Arch 8.4, to be backported into the previous increment > - e,g, 8.3, which made this filtering more difficult to track. > 2. After discussion with the PE architects it was clear that > instructions in a later architect version would not re-use older > opcodes from a previous one and be nop / invalid in the earlier > architectures. (certainly in the scope of AA64). Therefore > the policy in the decoder is to check for all the instructions we know > about for the latest version of architecture, even if we could be > decoding an earlier architecture version. This means we may check for > a few more opcodes than necessary for earlier version of the > architecture, but the overall decode is more robust and easier to > maintain. > > Therefore for any AA64 core beyond v8.3 - it is safe to use the > ARCH_AA64 PE architecture version and the decoder will handle it. I have no objection for current approach; but two things can cause confusions and it might be difficult for maintenance: - The first thing is now we base on the bit fields TRCIDR1::TRCARCHMIN to decide the PE architecture version. In the ETMv4 spec, TRCIDR1::TRCARCHMIN is defined as the trace unit minor version, so essentially it's a minor version number for tracer (ETM) but not the PE architecture number. But now we are using it to decide the PE architecture number (8.3, 8.4, etc...). - The second thing is the macros' naming convention. E.g. "AA64" gives me an impression it is a general naming "Arm Arch 64" for all Arm 64-bit CPUs, it's something like an abbreviation for "aarch64"; so seems to me it doesn't show any meaningful info for PE's architecture version number. This is why I proposed to use more explict macro definition for architectures (e.g. ARCH_V8R3, ARCH_V8R4, ARCH_V9R0, etc). If we really want to use ARCH_AA64, it's better to give some comments in the code. Thanks a lot for shared the background info. Leo
Hi Leo, On Mon, 2 Aug 2021 at 16:04, Leo Yan <leo.yan@linaro.org> wrote: > > Hi Mike, > > On Mon, Aug 02, 2021 at 03:04:14PM +0100, Mike Leach wrote: > > [...] > > > > > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4 > > > > > +#define TRCIDR1_TRCARCHMIN_MASK GENMASK(7, 4) > > > > > +#define TRCIDR1_TRCARCHMIN(x) (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT) > > > > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) > > > > > +{ > > > > > + /* > > > > > + * If the ETM trace minor version is 4 or more then we can assume > > > > > + * the architecture is ARCH_AA64 rather than just V8 > > > > > + */ > > > > > + return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8; > > > > > +} > > > > > > > > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x > > > > Probably need to beef up this comment or the function name to emphasise this. > > > > > > Yeah, I think it's good to change the function name. Eventually, this > > > function should only be used for ETM4.x and ETE. > > > > > > Another minor comment is: can we refine the arch version number, e.g. > > > change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or > > > "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version. > > > > > > > The purpose of these macros is to inform the decoder of the > > architecture of the PE - not the version of the ETM. > > > > These OpenCSD macros are defined by the library headers > > (ocsd_if_types.h) and not the perf headers. > > These have been published as the API / ABI for OpenCSD and as such > > changing them affects all OpenCSD clients, not just perf. > > I understand these macros are defined in OpenCSD lib as APIs, since I > saw these macros have not been widely used in perf tool (e.g. > ARCH_AA64), so this is why I think it's good to take chance to refine > the naming conventions. > The macros are used in other tools - so changing now affects those too. Not something I am prepared to do without good reason. > > This PE architecture version is used along with the core profile to > > determine which instructions are valid waypoint instructions to > > associate with atom elements when walking the program image during > > trace decode. > > > > From v8.3 onwards we moved away from filtering on specific > > architecture versions. This was due to two factors:- > > 1. The architectural rules now allow architectural features for one > > increment e.g. Arch 8.4, to be backported into the previous increment > > - e,g, 8.3, which made this filtering more difficult to track. > > 2. After discussion with the PE architects it was clear that > > instructions in a later architect version would not re-use older > > opcodes from a previous one and be nop / invalid in the earlier > > architectures. (certainly in the scope of AA64). Therefore > > the policy in the decoder is to check for all the instructions we know > > about for the latest version of architecture, even if we could be > > decoding an earlier architecture version. This means we may check for > > a few more opcodes than necessary for earlier version of the > > architecture, but the overall decode is more robust and easier to > > maintain. > > > > Therefore for any AA64 core beyond v8.3 - it is safe to use the > > ARCH_AA64 PE architecture version and the decoder will handle it. > > I have no objection for current approach; but two things can cause > confusions and it might be difficult for maintenance: > > - The first thing is now we base on the bit fields TRCIDR1::TRCARCHMIN > to decide the PE architecture version. In the ETMv4 spec, > TRCIDR1::TRCARCHMIN is defined as the trace unit minor version, > so essentially it's a minor version number for tracer (ETM) but not > the PE architecture number. But now we are using it to decide the > PE architecture number (8.3, 8.4, etc...). > This is a slight weakness in the implementation of perf. Ideally one does need to establish the architecture version of the PE - but perf /cs-etm is using an assumption regarding the profile and version of the core, according to the ETM / ETE versiom. That said - the ETM / ETE version numbers do have a strong relationship with PE architecture version numbers, so this assumption holds for the current supported devices. > - The second thing is the macros' naming convention. > E.g. "AA64" gives me an impression it is a general naming "Arm Arch 64" > for all Arm 64-bit CPUs, it's something like an abbreviation for > "aarch64"; so seems to me it doesn't show any meaningful info for PE's > architecture version number. This is why I proposed to use more > explict macro definition for architectures (e.g. ARCH_V8R3, ARCH_V8R4, > ARCH_V9R0, etc). > For modern cores it is sufficient for the decoder to know the profile and that it is aarch 64 - so yes the macro is simply saying this a general AA64 core. The macros for earlier versions are a little more specific as certain filtering is used according to the version of the PE. ARCH_V8R4, ARCH_V9R0 etc would have no significance to the decoder and would not be useful. If we get to the stage where we need more specific PE architecture versions - then these can be added as required. Using the ARCH_AA64 macro means that we do not have to update the API for every version update of the architecture, and there are no changes required to the perf / cs-etm handling. > If we really want to use ARCH_AA64, it's better to give some comments in > the code. > There are comments in the OpenCSD headers, though additional ones in the perf / cs-etm handling soruce code could be added. Regards Mike > Thanks a lot for shared the background info. > > Leo
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index 30889a9d0165..5972a8afcc6b 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params, return 0; } +#define TRCIDR1_TRCARCHMIN_SHIFT 4 +#define TRCIDR1_TRCARCHMIN_MASK GENMASK(7, 4) +#define TRCIDR1_TRCARCHMIN(x) (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT) +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) +{ + /* + * If the ETM trace minor version is 4 or more then we can assume + * the architecture is ARCH_AA64 rather than just V8 + */ + return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8; +} + static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params, ocsd_etmv4_cfg *config) { @@ -140,7 +152,7 @@ static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params, config->reg_idr11 = 0; config->reg_idr12 = 0; config->reg_idr13 = 0; - config->arch_ver = ARCH_V8; + config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1); config->core_prof = profile_CortexA; }
Currently the architecture is hard coded as ARCH_V8, but with the introduction of ETE we want to pick ARCH_AA64. And this change is also applicable to ETM v4.4 onwards as well. Signed-off-by: James Clark <james.clark@arm.com> --- tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)