Messages in this thread | | | Subject | Re: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM | From | Suzuki K Poulose <> | Date | Fri, 13 Nov 2020 10:22:39 +0000 |
| |
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 >> >> . >> >
| |