linux-kernel - Re: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fae22c97-489b-0312-4357-973a4969780c@huawei.com>
Date:   Tue, 1 Sep 2020 09:57:33 +0800
From:   Qi Liu <liuqi115@...wei.com>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
CC:     <gregkh@...uxfoundation.org>, <suzuki.poulose@....com>,
        <mike.leach@...aro.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>
Subject: Re: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to
 avoid the overflow of HiSilicon ETM

Hi Mathieu,

Thanks for your review.

On 2020/9/1 6:13, Mathieu Poirier wrote:
> Following Al's comment I have the following recommendations...
>
> On Wed, Aug 19, 2020 at 04:06:37PM +0800, Qi Liu wrote:
>> When too much trace information is generated on-chip, the ETM will
>> overflow, and cause data loss. This is a common phenomenon on ETM
>> devices.
>>
>> But sometimes we do not want to lose performance trace data, so we
>> suppress the speed of instructions sent from CPU core to ETM to
>> avoid the overflow of ETM.
>>
>> Signed-off-by: Qi Liu <liuqi115@...wei.com>
>> ---
>>
>> Changes since v1:
>> - ETM on HiSilicon Hip09 platform supports backpressure, so does
>> not need to modify core commit.
>>
>>  drivers/hwtracing/coresight/coresight-etm4x.c | 43 +++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 7797a57..7641f89 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -43,6 +43,10 @@ MODULE_PARM_DESC(boot_enable, "Enable tracing on boot");
>>  #define PARAM_PM_SAVE_NEVER	  1 /* never save any state */
>>  #define PARAM_PM_SAVE_SELF_HOSTED 2 /* save self-hosted state only */
>>
>> +#define CORE_COMMIT_CLEAR	0x3000
>> +#define CORE_COMMIT_SHIFT	12
>> +#define HISI_ETM_AMBA_ID_V1	0x000b6d01
> Do you have a name for the SoC that can be added so that other HiSilicon SoC can
> be added?  I suggest something like:
>
> #define HISI_NAME_CORE_COMMIT_CLEAR
> #define HISI_NAME_CORE_COMMIT_SHIFT
> #define HISI_NAME_ETM_ID
Will fix this next version.
> Moreover I don't see an entry for 0x000b6d01 in the amba id table - is your
> devices upstream?  Needless to day that is mandatory in order to move forward
> with this work.
A patch has been applied to add this ETM id and here is the link:
https://lore.kernel.org/linux-arm-kernel/20200813210000.GO3393195@xps15/

>> +
>>  static int pm_save_enable = PARAM_PM_SAVE_FIRMWARE;
>>  module_param(pm_save_enable, int, 0444);
>>  MODULE_PARM_DESC(pm_save_enable,
>> @@ -104,11 +108,40 @@ struct etm4_enable_arg {
>>  	int rc;
>>  };
>>
>> +static void etm4_cpu_actlr1_cfg(void *info)
>> +{
>> +	struct etm4_enable_arg *arg = (struct etm4_enable_arg *)info;
>> +	u64 val;
>> +
>> +	asm volatile("mrs %0,s3_1_c15_c2_5" : "=r"(val));
>> +	val &= ~CORE_COMMIT_CLEAR;
>> +	val |= arg->rc << CORE_COMMIT_SHIFT;
>> +	asm volatile("msr s3_1_c15_c2_5,%0" : : "r"(val));
>> +}
>> +
>> +static void etm4_config_core_commit(int cpu, int val)
>> +{
>> +	struct etm4_enable_arg arg = {0};
>> +
>> +	arg.rc = val;
>> +	smp_call_function_single(cpu, etm4_cpu_actlr1_cfg, &arg, 1);
>> +}
>> +
>>  static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>>  {
>>  	int i, rc;
>> +	struct amba_device *adev;
>>  	struct etmv4_config *config = &drvdata->config;
>>  	struct device *etm_dev = &drvdata->csdev->dev;
>> +	struct device *dev = drvdata->csdev->dev.parent;
>> +
>> +	adev = container_of(dev, struct amba_device, dev);
>> +	/*
>> +	 * If ETM device is HiSilicon ETM device, reduce the
>> +	 * core-commit to avoid ETM overflow.
>> +	 */
>> +	if (adev->periphid == HISI_ETM_AMBA_ID_V1)
>> +		etm4_config_core_commit(drvdata->cpu, 1);
> I susggest to add a function like etm4_enable_arch_specific() and do the above
> in there.  The same goes for the disable path.
>
> Thanks,
> Mathieu
Thanks, I'll fix this next version : )

Qi
>
>>  	CS_UNLOCK(drvdata->base);
>>
>> @@ -472,10 +505,20 @@ static void etm4_disable_hw(void *info)
>>  {
>>  	u32 control;
>>  	struct etmv4_drvdata *drvdata = info;
>> +	struct device *dev = drvdata->csdev->dev.parent;
>>  	struct etmv4_config *config = &drvdata->config;
>>  	struct device *etm_dev = &drvdata->csdev->dev;
>> +	struct amba_device *adev;
>>  	int i;
>>
>> +	adev = container_of(dev, struct amba_device, dev);
>> +	/*
>> +	 * If ETM device is HiSilicon ETM device, resume the
>> +	 * core-commit after ETM trace is complete.
>> +	 */
>> +	if (adev->periphid == HISI_ETM_AMBA_ID_V1)
>> +		etm4_config_core_commit(drvdata->cpu, 0);
>> +
>>  	CS_UNLOCK(drvdata->base);
>>
>>  	if (!drvdata->skip_power_up) {
>> --
>> 2.8.1
>>
> .
>