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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 11/13/20 10:18 AM, Qi Liu wrote:
> 
> 
> On 2020/11/12 22:03, Suzuki K Poulose wrote:
>> 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@arm.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@huawei.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
> 
> Hi Suzuki,
> 
> Add a check in probe is a good idea, and perhaps we could also add CONFIG_ETM4X_IMPDEF_FEATURE
> here, like:
>   etm4x_probe() {
>   ...
>   #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>       etm4x_check_arch_features();
>   #endif

Please make this unconditional and define the function conditionally, as below.


	etm4x_check_arch_features();
>   }

and :

#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
static void etm4x_check_arch_features(struct etmv4_drvdata *drvdata, struct device *dev)
{

}

static void etm4x_enable_arch_specific(...)
{
.... do arch specific setup ...
}

#else
static inline void etm4x_check_arch_features(..)
{}
static void etm4x_enable_arch_specific(...)
{}
#endif


> 
> Thanks
> Qi
>>
>> .
>>
>