Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210110224850.1880240-1-suzuki.poulose@arm.com> <20210110224850.1880240-29-suzuki.poulose@arm.com>
In-Reply-To: <20210110224850.1880240-29-suzuki.poulose@arm.com>
From:   Mike Leach <mike.leach@linaro.org>
Date:   Fri, 12 Feb 2021 10:34:57 +0000
Message-ID: <CAJ9a7ViwNA__+5f7-DehM6OtGer3DjdZfWsgHc9EOcr5-vQXgA@mail.gmail.com>
Subject: Re: [PATCH v7 28/28] coresight: Add support for v8.4 SelfHosted tracing
To:     Suzuki K Poulose <suzuki.poulose@arm.com>
Cc:     linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Coresight ML <coresight@lists.linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Leo Yan <leo.yan@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jonathan Zhou <jonathan.zhouwen@huawei.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mathieu, Suzuki,

Sorry for the really late response on this patch, but I noticed a
problem while doing a review of the ETE / TRBE set. (TRBE specs
mention TRFCR_ELx, so I was confirming a couple of things).

On Sun, 10 Jan 2021 at 22:49, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> From: Jonathan Zhou <jonathan.zhouwen@huawei.com>
>
> v8.4 tracing extensions added support for trace filtering controlled
> by TRFCR_ELx. This must be programmed to allow tracing at EL1/EL2 and
> EL0. The timestamp used is the virtual time. Also enable CONTEXIDR_EL2
> tracing if we are running the kernel at EL2.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Will Deacon <will@kernel.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com>
> [ Move the trace filtering setup etm_init_arch_data() and
>  clean ups]
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  .../coresight/coresight-etm4x-core.c          | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 3d3165dd09d4..18c1a80abab8 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -859,6 +859,30 @@ static bool etm4_init_csdev_access(struct etmv4_drvdata *drvdata,
>         return false;
>  }
>
> +static void cpu_enable_tracing(void)
> +{
> +       u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
> +       u64 trfcr;
> +
> +       if (!cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_TRACE_FILT_SHIFT))
> +               return;
> +
> +       /*
> +        * If the CPU supports v8.4 SelfHosted Tracing, enable
> +        * tracing at the kernel EL and EL0, forcing to use the
> +        * virtual time as the timestamp.
> +        */
> +       trfcr = (TRFCR_ELx_TS_VIRTUAL |
> +                TRFCR_ELx_ExTRE |
> +                TRFCR_ELx_E0TRE);
> +
> +       /* If we are running at EL2, allow tracing the CONTEXTIDR_EL2. */
> +       if (is_kernel_in_hyp_mode())
> +               trfcr |= TRFCR_EL2_CX;
> +

This is wrong - CX bit is present on TRFCR_EL2, not TRFCR_EL1.
Moreover, TRFCR_EL2 has a separate enables for tracing at EL0 and EL2.

Secondly - is this correct in principal?  Should the driver not be
reading the access it is permitted by the kernel, rather than giving
itself unfettered access to trace where it wants to.
Surely TRFCR_ELx  levels should be chosen in KConfig  and then should
be set up in kernel initialisation?

Regards

Mike


> +       write_sysreg_s(trfcr, SYS_TRFCR_EL1);
> +}
> +
>  static void etm4_init_arch_data(void *info)
>  {
>         u32 etmidr0;
> @@ -1044,6 +1068,7 @@ static void etm4_init_arch_data(void *info)
>         /* NUMCNTR, bits[30:28] number of counters available for tracing */
>         drvdata->nr_cntr = BMVAL(etmidr5, 28, 30);
>         etm4_cs_lock(drvdata, csa);
> +       cpu_enable_tracing();
>  }
>
>  static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config)
> --
> 2.24.1
>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK