Message-ID: <93a2b528-ef4f-6217-e3ed-afd4a8eef179@arm.com>
Date:   Thu, 12 Nov 2020 14:03:47 +0000
From:   Suzuki K Poulose <suzuki.poulose@....com>
To:     Qi Liu <liuqi115@...wei.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Mike Leach <mike.leach@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxarm@...wei.com, Coresight ML <coresight@...ts.linaro.org>
Subject: Re: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to
 avoid the overflow of HiSilicon ETM

On 11/12/20 1:09 PM, Qi Liu wrote:
> 
> 
> On 2020/11/12 1:03, Mathieu Poirier wrote:
>> On Wed, Nov 11, 2020 at 04:58:23PM +0800, Qi Liu wrote:
>>> Hi Mathieu,
>>>
>>> On 2020/9/10 0:26, Mathieu Poirier wrote:
>>>> On Wed, Sep 09, 2020 at 12:30:02PM +0100, Mike Leach wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, 2 Sep 2020 at 11:36, Suzuki K Poulose <suzuki.poulose@....com> wrote:
>>>>>> On 08/27/2020 09:44 PM, Mathieu Poirier wrote:
>>>>>>> Hi Liu,
>>>>>>>
>>>>>>> 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
>>>>>>>> +
>>>>>>>>    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));
>>>>>>
>>>>>> The ID register (S3_1_C15_c2_5) falls into implementation defined space.
>>>>>> See Arm ARM DDI 0487F.a, section "D12.3.2  Reserved encodings for
>>>>>> IMPLEMENTATION DEFINED registers".
>>>>>>
>>>>>> So, please stop calling this "etm4_cpu_actlr1_cfg". Since,
>>>>>> 1) actlr1 is not an architected name for the said encoding
>>>>>> 2) The id register could mean something else on another CPU.
>>>>>>
>>>>>> Rather this should indicate platform/CPU specific. e.g,
>>>>>>
>>>>>> etm4_cpu_hisilicon_config_core_commit()
>>>>>>
>>>>>>
>>>>>>>> +    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);
>>>>>>> Function etm4_enable/disable_hw() are already running on the CPU they are
>>>>>>> supposed to so no need to call smp_call_function_single().
>>>>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>    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)
>>>>>> Please could you move this check to the function above ?
>>>>>> Ideally, it would be good to have something like :
>>>>>>
>>>>>>          etm4_config_impdef_features();
>>>>>>
>>>>>> And :
>>>>>>
>>>>>>          etm4_config_impdef_features(struct etm4_drvdata *drvdata)
>>>>>>          {
>>>>>>                  etm4_cpu_hisilicon_config_core_commit(drvdata);
>>>>>>          }
>>>>>>
>>>>> In addition to the above, Is it worth having this implementation
>>>>> defined code gated in the kernel configuration - like we do for core
>>>>> features sometimes?
>>>>> i,.e.
>>>>> CONFIG_ETM4X_IMPDEF_FEATURE (controls overall impdef support in the driver)
>>>>> and
>>>>> CONFIG_ETM4X_IMPDEF_HISILICON (depends on CONFIG_ETM4X_IMPDEF_FEATURE )
>>>>>
>>>>> This way we keep non ETM architectural code off platforms that cannot
>>>>> use it / test it.
>>>>>
>>>> That's a good idea - they do the same for CPU erratas.
>>>>   
>>> Considering that users sometimes use the same set of code on different platforms, how about
>>> using both CONFIG andperiphid to keep core-commit code off the platforms that do not
>>> need it?
>>> i, .e.
>>> CONFIG_ETM4X_IMPDEF_FEATURE ( controls overall impdef support in the driver )
>>> periphid ( match impdef code with the target platform )
>>>
>>> This way we could keep the same set of code working on different platforms, and it could help to
>>> ensure compatibility.
>>
>> I'm not 100% sure of what you mean by "same set of code working on different
>> platforms"...  Up to know the way we have been handling peripheral IDs has
>> worked quite well and I don't intend on changing it unless there is a really
>> good reason.
>>
> Hi Mathieu,
> 
> Perhaps I didn't make it clear and here is the code to express what I mean:
> 
> #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
> 
> #define HISI_HIP08_AMBA_ID		0x000b6d01
> #define HISI_HIP08_AMBA_MASK 		0xfffff
> static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
> {
>      struct device *dev = drvdata->csdev->dev.parent;
>      struct amba_device *adev = container_of(dev, struct amba_device, dev);

There is no guarantee that this is always an "AMBA" device, with this
patchset (which is still under review). Also, doing this check at
every time we enable/disable the ETM is not idea..

May be we should add a concept of "features" and use a bit mask instead,
which can be set at probe time, where we do have this information.

#define ETM4x_IMPDEF_HISILICON_CORE_COMMIT	0
#define ETM4x_IMPDEF_ARCH_N_FEATS		1

struct etmv4_drvdata {

	...
	DECALRE_BITMAP(arch_features, ETM4x_IMPDEF_ARCH_FEAT_MAX);
}

struct etm4x_arch_feature {
	void (*callback)(struct etmv4_drdvata *, bool enable);
};

static struct etm4x_arch_features[] = {
	[ETM4x_IMPDEF_HISILICON_CORE_COMMIT] = {
		.callback = etm4x_hisilicon_core_commit,
	},
	{}
};

static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
{
	for_each_set_bit(i, &drvdata->arch_features) {
		struct etm4x_arch_feature *ftr = &etm4x_arch_features[i];

		if (ftr->callback)
			ftr->callback(drvdata, true);
	}
}

etm4x_check_arch_features() {
	if (hisilicon_etm4x_match_pid)
		set_bit(drvdata->arch_features, ETM4x_IMPDEF_HISILICON_CORE_COMMIT);
}

etm4x_probe() {

	etm4x_check_arch_features();	
}

Suzuki

Powered by blists - more mailing lists