Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <a50a7468-923b-fbb2-32b9-7e371710a1d5@arm.com>
Date:   Tue, 12 Sep 2023 18:37:42 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.0
Subject: Re: [PATCH v2 2/2] coresight: Allow guests to be traced when FEAT_TRF
 and VHE are present
To:     James Clark <james.clark@arm.com>, coresight@lists.linaro.org,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
        broonie@kernel.org, Marc Zyngier <maz@kernel.org>
Cc:     maz@kernel.org, Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mike Leach <mike.leach@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        James Morse <james.morse@arm.com>,
        Kristina Martsenko <kristina.martsenko@arm.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Rob Herring <robh@kernel.org>,
        Jintack Lim <jintack.lim@linaro.org>,
        Joey Gouly <joey.gouly@arm.com>, linux-kernel@vger.kernel.org
References: <20230904140705.1620708-1-james.clark@arm.com>
 <20230904140705.1620708-3-james.clark@arm.com>
From:   Suzuki K Poulose <suzuki.poulose@arm.com>
In-Reply-To: <20230904140705.1620708-3-james.clark@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi James


On 04/09/2023 15:07, James Clark wrote:
> Currently the userspace and kernel filters for guests are never set, so
> no trace will be generated for them. Add it by writing to the guest
> filters when exclude_guest isn't set. By writing either E1TRE or E0TRE,
> filtering on either guest kernel or guest userspace is also supported.
> 
> Since TRFCR_EL1 access is trapped, this can't be  modified by the guest.
> 
> This change also brings exclude_host support which is difficult to add
> as a separate commit without excess churn and resulting in no trace at
> all.
> 
> Testing
> =======
> 
> The addresses were counted with the following:
> 
>    $ perf report -D | grep -Eo 'EL2|EL1|EL0' | sort | uniq -c
> 
> Guest kernel only:
> 
>    $ perf record -e cs_etm//Gk -a -- true
>      535 EL1
>        1 EL2
> 
> Guest user only (0 addresses expected because the guest OS hasn't reached
> userspace yet):
> 
>    $ perf record -e cs_etm//Gu -a -- true
> 
> Host kernel only:
> 
>    $  perf record -e cs_etm//Hk -a -- true
>     3501 EL2
> 
> Host userspace only:
> 
>    $  perf record -e cs_etm//Hu -a -- true
>      408 EL0
>        1 EL2
> 
> Reviewed-by: Mark Brown <broonie@kernel.org> (sysreg)
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>   arch/arm64/tools/sysreg                       |  4 ++
>   .../coresight/coresight-etm4x-core.c          | 51 ++++++++++++++++---
>   drivers/hwtracing/coresight/coresight-etm4x.h |  2 +-
>   drivers/hwtracing/coresight/coresight-priv.h  |  3 ++
>   4 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
> index 6ca7db69d6c9..cae9139b6c05 100644
> --- a/arch/arm64/tools/sysreg
> +++ b/arch/arm64/tools/sysreg
> @@ -2531,3 +2531,7 @@ EndSysreg
>   Sysreg	TRFCR_EL2	3	4	1	2	1
>   Fields	TRFCR_EL2
>   EndSysreg
> +
> +Sysreg TRFCR_EL12	3	5	1	2	1
> +Fields	TRFCR_ELx
> +EndSysreg
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 77b0271ce6eb..6c16a14d6fbe 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -274,6 +274,18 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
>   	cpu_prohibit_trace();
>   }
>   
> +static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata)
> +{
> +	u64 trfcr = drvdata->trfcr;
> +
> +	if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
> +		trfcr &= ~TRFCR_ELx_ExTRE;
> +	if (drvdata->config.mode & ETM_MODE_EXCL_USER)
> +		trfcr &= ~TRFCR_ELx_E0TRE;
> +
> +	return trfcr;
> +}
> +
>   /*
>    * etm4x_allow_trace - Allow CPU tracing in the respective ELs,
>    * as configured by the drvdata->config.mode for the current
> @@ -286,18 +298,39 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
>    */
>   static void etm4x_allow_trace(struct etmv4_drvdata *drvdata)
>   {
> -	u64 trfcr = drvdata->trfcr;
> +	u64 trfcr;
>   
>   	/* If the CPU doesn't support FEAT_TRF, nothing to do */
> -	if (!trfcr)
> +	if (!drvdata->trfcr)
>   		return;
>   
> -	if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
> -		trfcr &= ~TRFCR_ELx_ExTRE;
> -	if (drvdata->config.mode & ETM_MODE_EXCL_USER)
> -		trfcr &= ~TRFCR_ELx_E0TRE;
> +	if (drvdata->config.mode & ETM_MODE_EXCL_HOST)
> +		trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
> +	else
> +		trfcr = etm4x_get_kern_user_filter(drvdata);
>   
>   	write_trfcr(trfcr);
> +
> +	/*
> +	 * Filters for EL1 and EL0 (when running a guest) are stored in
> +	 * TRFCR_EL1 so write it there for VHE. For nVHE, the filters in
> +	 * have to be re-applied when switching to the guest instead.
> +	 */
> +	if (!is_kernel_in_hyp_mode())
> +		return;
> +
> +	if (drvdata->config.mode & ETM_MODE_EXCL_GUEST)
> +		trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
> +	else
> +		trfcr = etm4x_get_kern_user_filter(drvdata);
> +
> +	/*
> +	 * TRFCR_EL1 doesn't have CX and TRFCR_EL1.TS has no effect when TS is
> +	 * set in EL2 so mask them out.
> +	 */
> +	trfcr &= ~(TRFCR_ELx_TS_MASK | TRFCR_EL2_CX);
> +
> +	write_sysreg_s(trfcr, SYS_TRFCR_EL12);


While I agree with the intention of the patch, I am not sure if we
should do the TRFCR_EL1 (via TRFCR_EL12) updates from CoreSight driver.

It might be much better to do it from the KVM for both VHE and nVHE.


Marc

What are your thoughts on this ?

Suzuki


>   }
>   
>   #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
> @@ -655,6 +688,12 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>   	if (attr->exclude_user)
>   		config->mode = ETM_MODE_EXCL_USER;
>   
> +	if (attr->exclude_host)
> +		config->mode |= ETM_MODE_EXCL_HOST;
> +
> +	if (attr->exclude_guest)
> +		config->mode |= ETM_MODE_EXCL_GUEST;
> +
>   	/* Always start from the default config */
>   	etm4_set_default_config(config);
>   
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 20e2e4cb7614..3f170599822f 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -841,7 +841,7 @@ enum etm_impdef_type {
>    * @s_ex_level: Secure ELs where tracing is supported.
>    */
>   struct etmv4_config {
> -	u32				mode;
> +	u64				mode;
>   	u32				pe_sel;
>   	u32				cfg;
>   	u32				eventctrl0;
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 767076e07970..727dd27ba800 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -39,6 +39,9 @@
>   
>   #define ETM_MODE_EXCL_KERN	BIT(30)
>   #define ETM_MODE_EXCL_USER	BIT(31)
> +#define ETM_MODE_EXCL_HOST	BIT(32)
> +#define ETM_MODE_EXCL_GUEST	BIT(33)
> +
>   struct cs_pair_attribute {
>   	struct device_attribute attr;
>   	u32 lo_off;