ETM v4.4 onwards adds support for system instruction access
to the ETM. Detect the support on an ETM and switch to using the
mode when available.
Signed-off-by: Suzuki K Poulose <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 39 +++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 4bc2f15b6332..dc537b5612eb 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -675,6 +675,37 @@ static const struct coresight_ops etm4_cs_ops = {
.source_ops = &etm4_source_ops,
};
+static inline bool cpu_supports_sysreg_trace(void)
+{
+ u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
+
+ return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
+}
+
+static bool etm_init_sysreg_access(struct etmv4_drvdata *drvdata,
+ struct csdev_access *csa)
+{
+ u32 devarch;
+
+ if (!cpu_supports_sysreg_trace())
+ return false;
+
+ /*
+ * ETMs implementing sysreg access must implement TRCDEVARCH.
+ */
+ devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);
+ if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH)
+ return false;
+ *csa = (struct csdev_access) {
+ .io_mem = false,
+ .read = etm4x_sysreg_read,
+ .write = etm4x_sysreg_write,
+ };
+
+ drvdata->arch = etm_devarch_to_arch(devarch);
+ return true;
+}
+
static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
struct csdev_access *csa)
{
@@ -705,9 +736,17 @@ static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
struct csdev_access *csa)
{
+ /*
+ * Always choose the memory mapped io, if there is
+ * a memory map to prevent sysreg access on broken
+ * systems.
+ */
if (drvdata->base)
return etm_init_iomem_access(drvdata, csa);
+ if (etm_init_sysreg_access(drvdata, csa))
+ return true;
+
return false;
}
--
2.24.1
On Wed, Oct 28, 2020 at 10:09:42PM +0000, Suzuki K Poulose wrote:
> ETM v4.4 onwards adds support for system instruction access
> to the ETM. Detect the support on an ETM and switch to using the
> mode when available.
>
> Signed-off-by: Suzuki K Poulose <[email protected]>
> ---
> .../coresight/coresight-etm4x-core.c | 39 +++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 4bc2f15b6332..dc537b5612eb 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -675,6 +675,37 @@ static const struct coresight_ops etm4_cs_ops = {
> .source_ops = &etm4_source_ops,
> };
>
> +static inline bool cpu_supports_sysreg_trace(void)
> +{
> + u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> +
> + return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
I would do:
return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) == 1;
Because any other value than '1' are reserved.
> +}
> +
> +static bool etm_init_sysreg_access(struct etmv4_drvdata *drvdata,
> + struct csdev_access *csa)
> +{
> + u32 devarch;
> +
> + if (!cpu_supports_sysreg_trace())
> + return false;
> +
> + /*
> + * ETMs implementing sysreg access must implement TRCDEVARCH.
> + */
> + devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);
> + if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH)
> + return false;
> + *csa = (struct csdev_access) {
> + .io_mem = false,
> + .read = etm4x_sysreg_read,
> + .write = etm4x_sysreg_write,
> + };
> +
> + drvdata->arch = etm_devarch_to_arch(devarch);
> + return true;
> +}
> +
> static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
> struct csdev_access *csa)
> {
> @@ -705,9 +736,17 @@ static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
> static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
> struct csdev_access *csa)
> {
> + /*
> + * Always choose the memory mapped io, if there is
> + * a memory map to prevent sysreg access on broken
> + * systems.
> + */
> if (drvdata->base)
> return etm_init_iomem_access(drvdata, csa);
>
> + if (etm_init_sysreg_access(drvdata, csa))
> + return true;
> +
> return false;
With the above:
Reviewed-by: Mathieu Poirier <[email protected]>
> }
>
> --
> 2.24.1
>
On 11/9/20 8:22 PM, Mathieu Poirier wrote:
> On Wed, Oct 28, 2020 at 10:09:42PM +0000, Suzuki K Poulose wrote:
>> ETM v4.4 onwards adds support for system instruction access
>> to the ETM. Detect the support on an ETM and switch to using the
>> mode when available.
>>
>> Signed-off-by: Suzuki K Poulose <[email protected]>
>> ---
>> .../coresight/coresight-etm4x-core.c | 39 +++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 4bc2f15b6332..dc537b5612eb 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -675,6 +675,37 @@ static const struct coresight_ops etm4_cs_ops = {
>> .source_ops = &etm4_source_ops,
>> };
>>
>> +static inline bool cpu_supports_sysreg_trace(void)
>> +{
>> + u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
>> +
>> + return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
>
> I would do:
>
> return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) == 1;
>
> Because any other value than '1' are reserved.
Correct. However, this is something we follow for all ID features
in the arm64 kernel and is clarified in the Arm ARM (ARM DDI 0487F.a) :
"D13.1.3 Principles of the ID scheme for fields in ID registers"
Which guarantees that a (field > n) implies, everything that field == n
is implied. (Well there are exceptions listed in the section, but
TRACEVER is not one of those). So this should cover an old kernel
running on a future CPU, using the features that it understands.
(See feature_matches() in arch/arm64/kernel/cpufeature.c, which is
the fundamental logic to detect a feature).
Please let me know if you're OK with the justification.
Thanks for the review.
Suzuki
On Tue, Nov 10, 2020 at 09:31:42AM +0000, Suzuki K Poulose wrote:
> On 11/9/20 8:22 PM, Mathieu Poirier wrote:
> > On Wed, Oct 28, 2020 at 10:09:42PM +0000, Suzuki K Poulose wrote:
> > > ETM v4.4 onwards adds support for system instruction access
> > > to the ETM. Detect the support on an ETM and switch to using the
> > > mode when available.
> > >
> > > Signed-off-by: Suzuki K Poulose <[email protected]>
> > > ---
> > > .../coresight/coresight-etm4x-core.c | 39 +++++++++++++++++++
> > > 1 file changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > index 4bc2f15b6332..dc537b5612eb 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > @@ -675,6 +675,37 @@ static const struct coresight_ops etm4_cs_ops = {
> > > .source_ops = &etm4_source_ops,
> > > };
> > > +static inline bool cpu_supports_sysreg_trace(void)
> > > +{
> > > + u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> > > +
> > > + return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) > 0;
> >
> > I would do:
> >
> > return ((dfr0 >> ID_AA64DFR0_TRACEVER_SHIFT) & 0xfUL) == 1;
> >
> > Because any other value than '1' are reserved.
>
> Correct. However, this is something we follow for all ID features
> in the arm64 kernel and is clarified in the Arm ARM (ARM DDI 0487F.a) :
>
> "D13.1.3 Principles of the ID scheme for fields in ID registers"
>
> Which guarantees that a (field > n) implies, everything that field == n
> is implied. (Well there are exceptions listed in the section, but
> TRACEVER is not one of those). So this should cover an old kernel
> running on a future CPU, using the features that it understands.
> (See feature_matches() in arch/arm64/kernel/cpufeature.c, which is
> the fundamental logic to detect a feature).
>
While I haven't found anything conclusive in cpufeature.c, the documentation is
clear on the fact that versions are incremental and build on top of previous
ones. We can proceed with the current implementation.
> Please let me know if you're OK with the justification.
>
> Thanks for the review.
>
> Suzuki